Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(eslint-plugin): [no-unnecessary-condition][strict-boolean-expressions] add option to make the rules error on files without strictNullChecks turned on #2345

Merged
merged 1 commit into from Aug 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 25 additions & 3 deletions packages/eslint-plugin/docs/rules/no-unnecessary-condition.md
Expand Up @@ -62,18 +62,40 @@ function bar(arg?: string | null) {

## Options

Accepts an object with the following options:
```ts
type Options = {
// if true, the rule will ignore constant loop conditions
allowConstantLoopConditions?: boolean;
// if true, the rule will not error when running with a tsconfig that has strictNullChecks turned **off**
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing?: boolean;
};

const defaultOptions: Options = {
allowConstantLoopConditions: false,
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: false,
};
```

- `allowConstantLoopConditions` (default `false`) - allows constant expressions in loops.
### `allowConstantLoopConditions`

Example of correct code for when `allowConstantLoopConditions` is `true`:
Example of correct code for `{ allowConstantLoopConditions: true }`:

```ts
while (true) {}
for (; true; ) {}
do {} while (true);
```

### `allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing`

If this is set to `false`, then the rule will error on every file whose `tsconfig.json` does _not_ have the `strictNullChecks` compiler option (or `strict`) set to `true`.

Without `strictNullChecks`, TypeScript essentially erases `undefined` and `null` from the types. This means when this rule inspects the types from a variable, **it will not be able to tell that the variable might be `null` or `undefined`**, which essentially makes this rule useless.

You should be using `strictNullChecks` to ensure complete type-safety in your codebase.

If for some reason you cannot turn on `strictNullChecks`, but still want to use this rule - you can use this option to allow it - but know that the behavior of this rule is _undefined_ with the compiler option turned off. We will not accept bug reports if you are using this option.

## When Not To Use It

The main downside to using this rule is the need for type information.
Expand Down
110 changes: 75 additions & 35 deletions packages/eslint-plugin/docs/rules/strict-boolean-expressions.md
Expand Up @@ -79,41 +79,81 @@ const foo = (arg: any) => (Boolean(arg) ? 1 : 0);

## Options

Options may be provided as an object with:

- `allowString` (`true` by default) -
Allows `string` in a boolean context.
This is safe because strings have only one falsy value (`""`).
Set this to `false` if you prefer the explicit `str != ""` or `str.length > 0` style.

- `allowNumber` (`true` by default) -
Allows `number` in a boolean context.
This is safe because numbers have only two falsy values (`0` and `NaN`).
Set this to `false` if you prefer the explicit `num != 0` and `!Number.isNaN(num)` style.

- `allowNullableObject` (`true` by default) -
Allows `object | function | symbol | null | undefined` in a boolean context.
This is safe because objects, functions and symbols don't have falsy values.
Set this to `false` if you prefer the explicit `obj != null` style.

- `allowNullableBoolean` (`false` by default) -
Allows `boolean | null | undefined` in a boolean context.
This is unsafe because nullable booleans can be either `false` or nullish.
Set this to `false` if you want to enforce explicit `bool ?? false` or `bool ?? true` style.
Set this to `true` if you don't mind implicitly treating false the same as a nullish value.

- `allowNullableString` (`false` by default) -
Allows `string | null | undefined` in a boolean context.
This is unsafe because nullable strings can be either an empty string or nullish.
Set this to `true` if you don't mind implicitly treating an empty string the same as a nullish value.

- `allowNullableNumber` (`false` by default) -
Allows `number | null | undefined` in a boolean context.
This is unsafe because nullable numbers can be either a falsy number or nullish.
Set this to `true` if you don't mind implicitly treating zero or NaN the same as a nullish value.

- `allowAny` (`false` by default) -
Allows `any` in a boolean context.
```ts
type Options = {
allowString?: boolean;
allowNumber?: boolean;
allowNullableObject?: boolean;
allowNullableBoolean?: boolean;
allowNullableString?: boolean;
allowNullableNumber?: boolean;
allowAny?: boolean;
};

const defaultOptions: Options = {
allowString: true,
allowNumber: true,
allowNullableObject: true,
allowNullableBoolean: false,
allowNullableString: false,
allowNullableNumber: false,
allowAny: false,
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: false,
};
```

### `allowString`

Allows `string` in a boolean context.
This is safe because strings have only one falsy value (`""`).
Set this to `false` if you prefer the explicit `str != ""` or `str.length > 0` style.

### `allowNumber`

Allows `number` in a boolean context.
This is safe because numbers have only two falsy values (`0` and `NaN`).
Set this to `false` if you prefer the explicit `num != 0` and `!Number.isNaN(num)` style.

### `allowNullableObject`

Allows `object | function | symbol | null | undefined` in a boolean context.
This is safe because objects, functions and symbols don't have falsy values.
Set this to `false` if you prefer the explicit `obj != null` style.

### `allowNullableBoolean`

Allows `boolean | null | undefined` in a boolean context.
This is unsafe because nullable booleans can be either `false` or nullish.
Set this to `false` if you want to enforce explicit `bool ?? false` or `bool ?? true` style.
Set this to `true` if you don't mind implicitly treating false the same as a nullish value.

### `allowNullableString`

Allows `string | null | undefined` in a boolean context.
This is unsafe because nullable strings can be either an empty string or nullish.
Set this to `true` if you don't mind implicitly treating an empty string the same as a nullish value.

### `allowNullableNumber`

Allows `number | null | undefined` in a boolean context.
This is unsafe because nullable numbers can be either a falsy number or nullish.
Set this to `true` if you don't mind implicitly treating zero or NaN the same as a nullish value.

### `allowAny`

Allows `any` in a boolean context.
This is unsafe for obvious reasons.
Set this to `true` at your own risk.

### `allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing`

If this is set to `false`, then the rule will error on every file whose `tsconfig.json` does _not_ have the `strictNullChecks` compiler option (or `strict`) set to `true`.

Without `strictNullChecks`, TypeScript essentially erases `undefined` and `null` from the types. This means when this rule inspects the types from a variable, **it will not be able to tell that the variable might be `null` or `undefined`**, which essentially makes this rule a lot less useful.

You should be using `strictNullChecks` to ensure complete type-safety in your codebase.

If for some reason you cannot turn on `strictNullChecks`, but still want to use this rule - you can use this option to allow it - but know that the behavior of this rule is _undefined_ with the compiler option turned off. We will not accept bug reports if you are using this option.

## Related To

Expand Down
40 changes: 37 additions & 3 deletions packages/eslint-plugin/src/rules/no-unnecessary-condition.ts
Expand Up @@ -65,6 +65,7 @@ const isLiteral = (type: ts.Type): boolean =>
export type Options = [
{
allowConstantLoopConditions?: boolean;
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing?: boolean;
},
];

Expand All @@ -78,7 +79,9 @@ export type MessageId =
| 'literalBooleanExpression'
| 'noOverlapBooleanExpression'
| 'never'
| 'neverOptionalChain';
| 'neverOptionalChain'
| 'noStrictNullCheck';

export default createRule<Options, MessageId>({
name: 'no-unnecessary-condition',
meta: {
Expand All @@ -97,6 +100,9 @@ export default createRule<Options, MessageId>({
allowConstantLoopConditions: {
type: 'boolean',
},
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: {
type: 'boolean',
},
},
additionalProperties: false,
},
Expand All @@ -119,18 +125,46 @@ export default createRule<Options, MessageId>({
'Unnecessary conditional, the types have no overlap',
never: 'Unnecessary conditional, value is `never`',
neverOptionalChain: 'Unnecessary optional chain on a non-nullish value',
noStrictNullCheck:
'This rule requires the `strictNullChecks` compiler option to be turned on to function correctly.',
},
},
defaultOptions: [
{
allowConstantLoopConditions: false,
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: false,
},
],
create(context, [{ allowConstantLoopConditions }]) {
create(
context,
[
{
allowConstantLoopConditions,
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing,
},
],
) {
const service = getParserServices(context);
const checker = service.program.getTypeChecker();
const sourceCode = context.getSourceCode();
const compilerOptions = service.program.getCompilerOptions();
const isStrictNullChecks = isStrictCompilerOptionEnabled(
compilerOptions,
'strictNullChecks',
);

if (
!isStrictNullChecks &&
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing !== true
) {
context.report({
loc: {
start: { line: 0, column: 0 },
end: { line: 0, column: 0 },
},
messageId: 'noStrictNullCheck',
});
}

function getNodeType(node: TSESTree.Expression): ts.Type {
const tsNode = service.esTreeNodeToTSNodeMap.get(node);
Expand Down Expand Up @@ -263,7 +297,7 @@ export default createRule<Options, MessageId>({
return;
}
// Workaround for https://github.com/microsoft/TypeScript/issues/37160
if (isStrictCompilerOptionEnabled(compilerOptions, 'strictNullChecks')) {
if (isStrictNullChecks) {
const UNDEFINED = ts.TypeFlags.Undefined;
const NULL = ts.TypeFlags.Null;
const isComparable = (type: ts.Type, flag: ts.TypeFlags): boolean => {
Expand Down
32 changes: 31 additions & 1 deletion packages/eslint-plugin/src/rules/strict-boolean-expressions.ts
Expand Up @@ -15,6 +15,7 @@ export type Options = [
allowNullableString?: boolean;
allowNullableNumber?: boolean;
allowAny?: boolean;
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing?: boolean;
},
];

Expand All @@ -28,7 +29,8 @@ export type MessageId =
| 'conditionErrorNumber'
| 'conditionErrorNullableNumber'
| 'conditionErrorObject'
| 'conditionErrorNullableObject';
| 'conditionErrorNullableObject'
| 'noStrictNullCheck';

export default util.createRule<Options, MessageId>({
name: 'strict-boolean-expressions',
Expand All @@ -51,6 +53,9 @@ export default util.createRule<Options, MessageId>({
allowNullableString: { type: 'boolean' },
allowNullableNumber: { type: 'boolean' },
allowAny: { type: 'boolean' },
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: {
type: 'boolean',
},
},
additionalProperties: false,
},
Expand Down Expand Up @@ -86,18 +91,43 @@ export default util.createRule<Options, MessageId>({
conditionErrorNullableObject:
'Unexpected nullable object value in conditional. ' +
'An explicit null check is required.',
noStrictNullCheck:
'This rule requires the `strictNullChecks` compiler option to be turned on to function correctly.',
},
},
defaultOptions: [
{
allowString: true,
allowNumber: true,
allowNullableObject: true,
allowNullableBoolean: false,
allowNullableString: false,
allowNullableNumber: false,
allowAny: false,
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: false,
},
],
create(context, [options]) {
const service = util.getParserServices(context);
const checker = service.program.getTypeChecker();
const compilerOptions = service.program.getCompilerOptions();
const isStrictNullChecks = tsutils.isStrictCompilerOptionEnabled(
compilerOptions,
'strictNullChecks',
);

if (
!isStrictNullChecks &&
options.allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing !== true
) {
context.report({
loc: {
start: { line: 0, column: 0 },
end: { line: 0, column: 0 },
},
messageId: 'noStrictNullCheck',
});
}

const checkedNodes = new Set<TSESTree.Node>();

Expand Down
Empty file.
Empty file.
15 changes: 15 additions & 0 deletions packages/eslint-plugin/tests/fixtures/unstrict/tsconfig.json
@@ -0,0 +1,15 @@
{
"compilerOptions": {
"jsx": "preserve",
"target": "es5",
"module": "commonjs",
"strict": false,
"esModuleInterop": true,
"lib": ["es2015", "es2017", "esnext"],
"experimentalDecorators": true
},
"include": [
"file.ts",
"react.tsx"
]
}
Expand Up @@ -2,6 +2,7 @@ import {
TestCaseError,
InvalidTestCase,
} from '@typescript-eslint/experimental-utils/dist/ts-eslint';
import * as path from 'path';
import rule, {
Options,
MessageId,
Expand Down Expand Up @@ -445,6 +446,22 @@ declare const key: Key;

foo?.[key]?.trim();
`,
{
code: `
declare const x: string[] | null;
// eslint-disable-next-line
if (x) {
}
`,
options: [
{
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: true,
},
],
parserOptions: {
tsconfigRootDir: path.join(rootPath, 'unstrict'),
},
},
],
invalid: [
// Ensure that it's checking in all the right places
Expand Down Expand Up @@ -1367,5 +1384,27 @@ function Foo(outer: Outer, key: Bar): number | undefined {
},
],
},
{
code: `
declare const x: string[] | null;
if (x) {
}
`,
errors: [
{
messageId: 'noStrictNullCheck',
line: 0,
column: 1,
},
{
messageId: 'alwaysTruthy',
line: 3,
column: 5,
},
],
parserOptions: {
tsconfigRootDir: path.join(rootPath, 'unstrict'),
},
},
],
});