From 9273441f7592b52620e10432cb2dd4dc5c3b4db1 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Sun, 16 Aug 2020 12:02:07 -0700 Subject: [PATCH] feat(eslint-plugin): [no-unnecessary-condition][strict-boolean-expressions] add option to make the rules error on files without `strictNullChecks` turned on (#2345) --- .../docs/rules/no-unnecessary-condition.md | 28 ++++- .../docs/rules/strict-boolean-expressions.md | 110 ++++++++++++------ .../src/rules/no-unnecessary-condition.ts | 40 ++++++- .../src/rules/strict-boolean-expressions.ts | 32 ++++- .../tests/fixtures/unstrict/file.ts | 0 .../tests/fixtures/unstrict/react.tsx | 0 .../tests/fixtures/unstrict/tsconfig.json | 15 +++ .../rules/no-unnecessary-condition.test.ts | 39 +++++++ .../rules/strict-boolean-expressions.test.ts | 42 ++++++- 9 files changed, 263 insertions(+), 43 deletions(-) create mode 100644 packages/eslint-plugin/tests/fixtures/unstrict/file.ts create mode 100644 packages/eslint-plugin/tests/fixtures/unstrict/react.tsx create mode 100644 packages/eslint-plugin/tests/fixtures/unstrict/tsconfig.json diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md index 09e6730c3c5..bff0f548f50 100644 --- a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md +++ b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md @@ -62,11 +62,23 @@ 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) {} @@ -74,6 +86,16 @@ 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. diff --git a/packages/eslint-plugin/docs/rules/strict-boolean-expressions.md b/packages/eslint-plugin/docs/rules/strict-boolean-expressions.md index 07284154a31..243d54fda13 100644 --- a/packages/eslint-plugin/docs/rules/strict-boolean-expressions.md +++ b/packages/eslint-plugin/docs/rules/strict-boolean-expressions.md @@ -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 diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 675bf613efb..134cefd76d1 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -65,6 +65,7 @@ const isLiteral = (type: ts.Type): boolean => export type Options = [ { allowConstantLoopConditions?: boolean; + allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing?: boolean; }, ]; @@ -78,7 +79,9 @@ export type MessageId = | 'literalBooleanExpression' | 'noOverlapBooleanExpression' | 'never' - | 'neverOptionalChain'; + | 'neverOptionalChain' + | 'noStrictNullCheck'; + export default createRule({ name: 'no-unnecessary-condition', meta: { @@ -97,6 +100,9 @@ export default createRule({ allowConstantLoopConditions: { type: 'boolean', }, + allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: { + type: 'boolean', + }, }, additionalProperties: false, }, @@ -119,18 +125,46 @@ export default createRule({ '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); @@ -285,7 +319,7 @@ export default createRule({ 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 => { diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 747cbefb7eb..51265feb136 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -15,6 +15,7 @@ export type Options = [ allowNullableString?: boolean; allowNullableNumber?: boolean; allowAny?: boolean; + allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing?: boolean; }, ]; @@ -28,7 +29,8 @@ export type MessageId = | 'conditionErrorNumber' | 'conditionErrorNullableNumber' | 'conditionErrorObject' - | 'conditionErrorNullableObject'; + | 'conditionErrorNullableObject' + | 'noStrictNullCheck'; export default util.createRule({ name: 'strict-boolean-expressions', @@ -51,6 +53,9 @@ export default util.createRule({ allowNullableString: { type: 'boolean' }, allowNullableNumber: { type: 'boolean' }, allowAny: { type: 'boolean' }, + allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: { + type: 'boolean', + }, }, additionalProperties: false, }, @@ -86,6 +91,8 @@ export default util.createRule({ 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: [ @@ -93,11 +100,34 @@ export default util.createRule({ 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(); diff --git a/packages/eslint-plugin/tests/fixtures/unstrict/file.ts b/packages/eslint-plugin/tests/fixtures/unstrict/file.ts new file mode 100644 index 00000000000..e69de29bb2d diff --git a/packages/eslint-plugin/tests/fixtures/unstrict/react.tsx b/packages/eslint-plugin/tests/fixtures/unstrict/react.tsx new file mode 100644 index 00000000000..e69de29bb2d diff --git a/packages/eslint-plugin/tests/fixtures/unstrict/tsconfig.json b/packages/eslint-plugin/tests/fixtures/unstrict/tsconfig.json new file mode 100644 index 00000000000..751747ef2f4 --- /dev/null +++ b/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" + ] +} diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts index afb0b67a246..2afba392673 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -2,6 +2,7 @@ import { TestCaseError, InvalidTestCase, } from '@typescript-eslint/experimental-utils/dist/ts-eslint'; +import * as path from 'path'; import rule, { Options, MessageId, @@ -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 @@ -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'), + }, + }, ], }); diff --git a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts index f72739fb004..0d697ebb905 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -1,3 +1,4 @@ +import * as path from 'path'; import rule, { Options, MessageId, @@ -9,10 +10,11 @@ import { noFormat, } from '../RuleTester'; +const rootPath = getFixturesRootDir(); const ruleTester = new RuleTester({ parser: '@typescript-eslint/parser', parserOptions: { - tsconfigRootDir: getFixturesRootDir(), + tsconfigRootDir: rootPath, project: './tsconfig.json', }, }); @@ -115,6 +117,22 @@ ruleTester.run('strict-boolean-expressions', rule, { (x: T) => x ? 1 : 0; `, }), + { + code: ` +declare const x: string[] | null; +// eslint-disable-next-line +if (x) { +} + `, + options: [ + { + allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: true, + }, + ], + parserOptions: { + tsconfigRootDir: path.join(rootPath, 'unstrict'), + }, + }, ], invalid: [ @@ -313,5 +331,27 @@ ruleTester.run('strict-boolean-expressions', rule, { { messageId: 'conditionErrorAny', line: 4, column: 34 }, ], }), + { + code: ` +declare const x: string[] | null; +if (x) { +} + `, + errors: [ + { + messageId: 'noStrictNullCheck', + line: 0, + column: 1, + }, + { + messageId: 'conditionErrorObject', + line: 3, + column: 5, + }, + ], + parserOptions: { + tsconfigRootDir: path.join(rootPath, 'unstrict'), + }, + }, ], });