Skip to content

Commit

Permalink
feat(eslint-plugin): [no-unnecessary-condition][strict-boolean-expres…
Browse files Browse the repository at this point in the history
…sions] add option to make the rules error on files without `strictNullChecks` turned on (#2345)
  • Loading branch information
bradzacher committed Aug 29, 2020
1 parent 58b1c2d commit 9273441
Show file tree
Hide file tree
Showing 9 changed files with 263 additions and 43 deletions.
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 @@ -285,7 +319,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 @@ -490,6 +491,22 @@ declare const unknownTyped: unknown;
if (!(booleanTyped || unknownTyped)) {
}
`,
{
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 @@ -1461,5 +1478,27 @@ if (!speech) {
`,
errors: [ruleError(7, 6, 'never')],
},
{
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'),
},
},
],
});

0 comments on commit 9273441

Please sign in to comment.