From 95525e01bf2bb050e3cd63919b4a7adab84614f6 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Fri, 4 Oct 2024 21:34:43 +0300 Subject: [PATCH 01/41] initial implementation --- .../src/rules/strict-boolean-expressions.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 7adbad1e525..5dfe1e385f0 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -8,9 +8,11 @@ import * as ts from 'typescript'; import { createRule, + forEachReturnStatement, getConstrainedTypeAtLocation, getParserServices, getWrappingFixer, + isArrayMethodCallWithPredicate, isTypeArrayTypeOrUnionOfArrayTypes, } from '../util'; import { findTruthinessAssertedArgument } from '../util/assertionFunctionUtils'; @@ -272,6 +274,36 @@ export default createRule({ if (assertedArgument != null) { traverseNode(assertedArgument, true); } + if (isArrayMethodCallWithPredicate(context, services, node)) { + const predicate = node.arguments[0]; + + // Shorthand arrow function expression + if ( + predicate.type === AST_NODE_TYPES.ArrowFunctionExpression && + predicate.body.type !== AST_NODE_TYPES.BlockStatement + ) { + traverseNode(predicate.body, true); + } + + // Any other function expression with a block body + if ( + (predicate.type === AST_NODE_TYPES.FunctionExpression || + predicate.type === AST_NODE_TYPES.ArrowFunctionExpression) && + predicate.body.type === AST_NODE_TYPES.BlockStatement + ) { + const tsBody = services.esTreeNodeToTSNodeMap.get(predicate.body); + + forEachReturnStatement(tsBody, tsStatement => { + const statement = services.tsNodeToESTreeNodeMap.get(tsStatement); + if ( + statement.type === AST_NODE_TYPES.ReturnStatement && + statement.argument + ) { + traverseNode(statement.argument, true); + } + }); + } + } } /** From e6dd55ca045a87bbd15f312e1b9b96918ee94b23 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Fri, 4 Oct 2024 22:43:19 +0300 Subject: [PATCH 02/41] tests --- .../rules/strict-boolean-expressions.test.ts | 335 ++++++++++++++++++ 1 file changed, 335 insertions(+) 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 50395575a81..3d215cc0090 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -51,6 +51,17 @@ declare const x: never; if (x) { } `, + ` +[true, false].some(function (x) { + return x && 1; +}); + `, + ` +[true, false].filter(x => { + return x; +}); + `, + '[true, false].some(x => x && 1);', // string in boolean context ` @@ -67,6 +78,17 @@ if (x) { `, '(x: string) => !x;', '(x: T) => (x ? 1 : 0);', + ` +['1', '2', '3'].every(function (x) { + return x; +}); + `, + ` +[].every(() => { + return ''; +}); + `, + "['1', '2', '3'].every(x => x);", // number in boolean context ` @@ -83,6 +105,17 @@ if (x) { `, '(x: bigint) => !x;', '(x: T) => (x ? 1 : 0);', + ` +[1, 2, 3].every(function (x) { + return x; +}); + `, + ` +[1, 2, 3].some(x => { + return !x; +}); + `, + '[].find(() => 1);', // nullable object in boolean context ` @@ -92,6 +125,17 @@ if (x) { `, '(x?: { a: any }) => !x;', '(x: T) => (x ? 1 : 0);', + ` +[{}, null, {}].every(function (x) { + return x; +}); + `, + ` +[{}, null, {}].every(x => { + return x ? 1 : 0; +}); + `, + '[{}, null, null].findIndex(x => !x);', // nullable boolean in boolean context { @@ -114,6 +158,19 @@ if (x) { (x: T) => (x ? 1 : 0); `, }, + { + options: [{ allowNullableBoolean: true }], + code: ` + declare const x: (boolean | null)[]; + x.filter(function (t) { + return t; + }); + x.filter(t => { + return t ? 1 : 0; + }); + x.filter(t => !t); + `, + }, // nullable string in boolean context { @@ -136,6 +193,19 @@ if (x) { (x: T) => (x ? 1 : 0); `, }, + { + options: [{ allowNullableString: true }], + code: ` + declare const x: (string | null)[]; + x.findLast(function (t) { + return t; + }); + x.findLast(t => { + return !t; + }); + x.findLast(t => (t ? 1 : 0)); + `, + }, // nullable number in boolean context { @@ -158,6 +228,19 @@ if (x) { (x: T) => (x ? 1 : 0); `, }, + { + options: [{ allowNullableNumber: true }], + code: ` + declare const x: (number | null)[]; + x.findLast(function (t) { + return t; + }); + x.findLast(t => { + return !t; + }); + x.findLast(t => (t ? 1 : 0)); + `, + }, // any in boolean context { @@ -180,6 +263,19 @@ if (x) { (x: T) => (x ? 1 : 0); `, }, + { + options: [{ allowAny: true }], + code: ` + declare const x: any[]; + x.filter(function (t) { + return t; + }); + x.find(t => { + return !t; + }); + x.some(t => (t ? 1 : 0)); + `, + }, // logical operator { @@ -218,6 +314,18 @@ if (x) { 0 || false || '' ? null : {}; `, }, + { + options: [{ allowString: true, allowNumber: true }], + code: ` + [0, false, '', null, {}].filter(function (x) { + return x ? 1 : 0; + }); + [0, false, '', null, {}].filter(x => { + return !x; + }); + [0, false, '', null, {}].filter(x => x); + `, + }, // nullable enum in boolean context { @@ -284,6 +392,23 @@ if (x) { `, options: [{ allowNullableEnum: true }], }, + { + code: ` + enum ExampleEnum { + This = 'one', + That = 'two', + } + declare const x: (ExampleEnum | null)[]; + x.some(function (t) { + return t; + }); + x.every(t => { + return !t; + }); + x.find(t => t); + `, + options: [{ allowNullableEnum: true }], + }, // nullable mixed enum in boolean context { @@ -521,6 +646,17 @@ assert(nullableString); break; } `, + ` + const x = (t: string | null) => t; + ['', null].filter(x); + `, + ` + ['one', 'two'].filter(x => { + if (x.length > 2) { + return true; + } + }); + `, ], invalid: [ @@ -1303,6 +1439,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } declare const x: string; if (x) {} (x: string) => (!x); (x: T) => x ? 1 : 0; + ['foo', 'bar'].filter(x => x); `, errors: [ { @@ -1400,6 +1537,25 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, + { + messageId: 'conditionErrorString', + line: 7, + column: 36, + suggestions: [ + { + messageId: 'conditionFixCompareStringLength', + output: ` ['foo', 'bar'].filter(x => x.length > 0);`, + }, + { + messageId: 'conditionFixCompareEmptyString', + output: ` ['foo', 'bar'].filter(x => x !== "");`, + }, + { + messageId: 'conditionFixCastBoolean', + output: ` ['foo', 'bar'].filter(x => Boolean(x));`, + }, + ], + }, ], }), @@ -1414,6 +1570,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } (x: T) => (x) ? 1 : 0; ![]["length"]; // doesn't count as array.length when computed declare const a: any[] & { notLength: number }; if (a.notLength) {} + [1, 2, 3].some(x => { return x; }); `, errors: [ { @@ -1553,6 +1710,27 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, + { + messageId: 'conditionErrorNumber', + line: 9, + column: 38, + suggestions: [ + { + messageId: 'conditionFixCompareZero', + // TODO: fix compare zero suggestion for bigint + output: ` [1, 2, 3].some(x => { return x !== 0; });`, + }, + { + // TODO: remove check NaN suggestion for bigint + messageId: 'conditionFixCompareNaN', + output: ` [1, 2, 3].some(x => { return !Number.isNaN(x); });`, + }, + { + messageId: 'conditionFixCastBoolean', + output: ` [1, 2, 3].some(x => { return Boolean(x); });`, + }, + ], + }, ], }), @@ -1563,16 +1741,19 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } if (![].length) {} (a: number[]) => a.length && "..." (...a: T) => a.length || "empty"; + [[1], [2]].find(x => x.length); `, errors: [ { messageId: 'conditionErrorNumber', line: 2, column: 6 }, { messageId: 'conditionErrorNumber', line: 3, column: 26 }, { messageId: 'conditionErrorNumber', line: 4, column: 43 }, + { messageId: 'conditionErrorNumber', line: 5, column: 30 }, ], output: ` if ([].length === 0) {} (a: number[]) => (a.length > 0) && "..." (...a: T) => (a.length > 0) || "empty"; + [[1], [2]].find(x => x.length > 0); `, }), @@ -1583,11 +1764,13 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } declare const x: string | number; if (x) {} (x: bigint | string) => !x; (x: T) => x ? 1 : 0; + [1, 'two'].findIndex(function (x) { return x; }); `, errors: [ { messageId: 'conditionErrorOther', line: 2, column: 39 }, { messageId: 'conditionErrorOther', line: 3, column: 34 }, { messageId: 'conditionErrorOther', line: 4, column: 55 }, + { messageId: 'conditionErrorOther', line: 5, column: 52 }, ], }), @@ -1598,6 +1781,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } declare const x: boolean | null; if (x) {} (x?: boolean) => !x; (x: T) => x ? 1 : 0; + [null, true].every(x => x); `, errors: [ { @@ -1645,6 +1829,21 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, + { + messageId: 'conditionErrorNullableBoolean', + line: 5, + column: 33, + suggestions: [ + { + messageId: 'conditionFixDefaultFalse', + output: ` [null, true].every(x => x ?? false);`, + }, + { + messageId: 'conditionFixCompareTrue', + output: ` [null, true].every(x => x === true);`, + }, + ], + }, ], }), @@ -1655,6 +1854,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } declare const x: object | null; if (x) {} (x?: { a: number }) => !x; (x: T) => x ? 1 : 0; + [{}, null].findIndex(x => { return x; }); `, errors: [ { @@ -1690,6 +1890,18 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, + { + messageId: 'conditionErrorNullableObject', + line: 5, + column: 44, + suggestions: [ + { + messageId: 'conditionFixCompareNullish', + output: + ' [{}, null].findIndex(x => { return x != null; });', + }, + ], + }, ], }), @@ -1700,6 +1912,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } (x?: string) => !x; (x: T) => x ? 1 : 0; function foo(x: '' | 'bar' | null) { if (!x) {} } + ['foo', null].some(x => x); `, errors: [ { @@ -1784,6 +1997,25 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, + { + messageId: 'conditionErrorNullableString', + line: 6, + column: 33, + suggestions: [ + { + messageId: 'conditionFixCompareNullish', + output: " ['foo', null].some(x => x != null);", + }, + { + messageId: 'conditionFixDefaultEmptyString', + output: ' [\'foo\', null].some(x => x ?? "");', + }, + { + messageId: 'conditionFixCastBoolean', + output: " ['foo', null].some(x => Boolean(x));", + }, + ], + }, ], }), @@ -1794,6 +2026,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } (x?: number) => !x; (x: T) => x ? 1 : 0; function foo(x: 0 | 1 | null) { if (!x) {} } + [5, null].findLastIndex(x => x); `, errors: [ { @@ -1878,6 +2111,25 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, + { + messageId: 'conditionErrorNullableNumber', + line: 6, + column: 38, + suggestions: [ + { + messageId: 'conditionFixCompareNullish', + output: ' [5, null].findLastIndex(x => x != null);', + }, + { + messageId: 'conditionFixDefaultZero', + output: ' [5, null].findLastIndex(x => x ?? 0);', + }, + { + messageId: 'conditionFixCastBoolean', + output: ' [5, null].findLastIndex(x => Boolean(x));', + }, + ], + }, ], }), @@ -1892,6 +2144,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum) { } + [theEnum].filter(x => x); `, errors: [ { @@ -1901,6 +2154,13 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } endLine: 7, endColumn: 20, }, + { + line: 9, + column: 31, + messageId: 'conditionErrorNullableEnum', + endLine: 9, + endColumn: 32, + }, ], output: ` enum ExampleEnum { @@ -1910,6 +2170,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum != null) { } + [theEnum].filter(x => x != null); `, }, { @@ -1922,6 +2183,9 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (!theEnum) { } + [theEnum].filter(function (x) { + return !x; + }); `, errors: [ { @@ -1931,6 +2195,13 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } endLine: 7, endColumn: 21, }, + { + line: 10, + column: 19, + messageId: 'conditionErrorNullableEnum', + endLine: 10, + endColumn: 20, + }, ], output: ` enum ExampleEnum { @@ -1940,6 +2211,9 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum == null) { } + [theEnum].filter(function (x) { + return x == null; + }); `, }, { @@ -1952,6 +2226,9 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (!theEnum) { } + [theEnum].filter(x => { + return !x; + }); `, errors: [ { @@ -1961,6 +2238,13 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } endLine: 7, endColumn: 21, }, + { + line: 10, + column: 19, + messageId: 'conditionErrorNullableEnum', + endLine: 10, + endColumn: 20, + }, ], output: ` enum ExampleEnum { @@ -1970,6 +2254,9 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum == null) { } + [theEnum].filter(x => { + return x == null; + }); `, }, { @@ -1982,6 +2269,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (!theEnum) { } + [theEnum].every(x => !x); `, errors: [ { @@ -1991,6 +2279,13 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } endLine: 7, endColumn: 21, }, + { + line: 9, + column: 31, + messageId: 'conditionErrorNullableEnum', + endLine: 9, + endColumn: 32, + }, ], output: ` enum ExampleEnum { @@ -2000,6 +2295,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum == null) { } + [theEnum].every(x => x == null); `, }, { @@ -2012,6 +2308,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (!theEnum) { } + [theEnum].filter(x => !x); `, errors: [ { @@ -2021,6 +2318,13 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } endLine: 7, endColumn: 21, }, + { + line: 9, + column: 32, + messageId: 'conditionErrorNullableEnum', + endLine: 9, + endColumn: 33, + }, ], output: ` enum ExampleEnum { @@ -2030,6 +2334,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum == null) { } + [theEnum].filter(x => x == null); `, }, { @@ -2042,6 +2347,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (!theEnum) { } + [theEnum].filter(x => !x); `, errors: [ { @@ -2051,6 +2357,13 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } endLine: 7, endColumn: 21, }, + { + line: 9, + column: 32, + messageId: 'conditionErrorNullableEnum', + endLine: 9, + endColumn: 33, + }, ], output: ` enum ExampleEnum { @@ -2060,6 +2373,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum == null) { } + [theEnum].filter(x => x == null); `, }, { @@ -2072,6 +2386,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (!theEnum) { } + [theEnum].find(x => !x); `, errors: [ { @@ -2081,6 +2396,13 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } endLine: 7, endColumn: 21, }, + { + line: 9, + column: 30, + messageId: 'conditionErrorNullableEnum', + endLine: 9, + endColumn: 31, + }, ], output: ` enum ExampleEnum { @@ -2090,6 +2412,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum == null) { } + [theEnum].find(x => x == null); `, }, @@ -2210,6 +2533,7 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } x => !x; (x: T) => x ? 1 : 0; (x: T) => x ? 1 : 0; + [x].filter(t => t); `, errors: [ { @@ -2256,6 +2580,17 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, + { + messageId: 'conditionErrorAny', + line: 6, + column: 25, + suggestions: [ + { + messageId: 'conditionFixCastBoolean', + output: ' [x].filter(t => Boolean(t));', + }, + ], + }, ], }), From 1a898fe3d70d76d659ca3a0d862d35b53e4e2b43 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Sat, 5 Oct 2024 08:49:23 +0300 Subject: [PATCH 03/41] refactor --- .../src/rules/strict-boolean-expressions.ts | 60 +++++++++++-------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 5dfe1e385f0..afc146ce494 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -269,39 +269,47 @@ export default createRule({ traverseNode(node.right, isCondition); } + function traverseArrayPredicateReturnStatements( + node: TSESTree.CallExpressionArgument, + ): void { + // Shorthand arrow function expression + if ( + node.type === AST_NODE_TYPES.ArrowFunctionExpression && + node.body.type !== AST_NODE_TYPES.BlockStatement + ) { + traverseNode(node.body, true); + } + + // Any other function expression with a block body + else if ( + (node.type === AST_NODE_TYPES.FunctionExpression || + node.type === AST_NODE_TYPES.ArrowFunctionExpression) && + node.body.type === AST_NODE_TYPES.BlockStatement + ) { + const tsBody = services.esTreeNodeToTSNodeMap.get(node.body); + + forEachReturnStatement(tsBody, tsStatement => { + const statement = services.tsNodeToESTreeNodeMap.get(tsStatement); + if ( + statement.type === AST_NODE_TYPES.ReturnStatement && + statement.argument + ) { + traverseNode(statement.argument, true); + } + }); + } + } + function traverseCallExpression(node: TSESTree.CallExpression): void { const assertedArgument = findTruthinessAssertedArgument(services, node); if (assertedArgument != null) { traverseNode(assertedArgument, true); } if (isArrayMethodCallWithPredicate(context, services, node)) { - const predicate = node.arguments[0]; - - // Shorthand arrow function expression - if ( - predicate.type === AST_NODE_TYPES.ArrowFunctionExpression && - predicate.body.type !== AST_NODE_TYPES.BlockStatement - ) { - traverseNode(predicate.body, true); - } + const predicate = node.arguments.at(0); - // Any other function expression with a block body - if ( - (predicate.type === AST_NODE_TYPES.FunctionExpression || - predicate.type === AST_NODE_TYPES.ArrowFunctionExpression) && - predicate.body.type === AST_NODE_TYPES.BlockStatement - ) { - const tsBody = services.esTreeNodeToTSNodeMap.get(predicate.body); - - forEachReturnStatement(tsBody, tsStatement => { - const statement = services.tsNodeToESTreeNodeMap.get(tsStatement); - if ( - statement.type === AST_NODE_TYPES.ReturnStatement && - statement.argument - ) { - traverseNode(statement.argument, true); - } - }); + if (predicate != null) { + traverseArrayPredicateReturnStatements(predicate); } } } From a16f46e1dd90407664a6dbc2ca45118c0760ea09 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Sat, 5 Oct 2024 09:16:01 +0300 Subject: [PATCH 04/41] some more tests --- .../rules/strict-boolean-expressions.test.ts | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+) 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 3d215cc0090..55e8e2d9093 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -2703,6 +2703,107 @@ if (x) { }, ], }, + // multi line predicate function + { + code: ` +['one', null].filter(x => { + if (Math.random() > 0.5) { + return x; + } + + return !x; +}); + `, + output: null, + errors: [ + { + messageId: 'conditionErrorNullableString', + line: 4, + column: 12, + suggestions: [ + { + messageId: 'conditionFixCompareNullish', + output: ` +['one', null].filter(x => { + if (Math.random() > 0.5) { + return x != null; + } + + return !x; +}); + `, + }, + { + messageId: 'conditionFixDefaultEmptyString', + output: ` +['one', null].filter(x => { + if (Math.random() > 0.5) { + return x ?? ""; + } + + return !x; +}); + `, + }, + { + messageId: 'conditionFixCastBoolean', + output: ` +['one', null].filter(x => { + if (Math.random() > 0.5) { + return Boolean(x); + } + + return !x; +}); + `, + }, + ], + }, + { + messageId: 'conditionErrorNullableString', + line: 7, + column: 11, + suggestions: [ + { + messageId: 'conditionFixCompareNullish', + output: ` +['one', null].filter(x => { + if (Math.random() > 0.5) { + return x; + } + + return x == null; +}); + `, + }, + { + messageId: 'conditionFixDefaultEmptyString', + output: ` +['one', null].filter(x => { + if (Math.random() > 0.5) { + return x; + } + + return !(x ?? ""); +}); + `, + }, + { + messageId: 'conditionFixCastBoolean', + output: ` +['one', null].filter(x => { + if (Math.random() > 0.5) { + return x; + } + + return !Boolean(x); +}); + `, + }, + ], + }, + ], + }, { code: ` declare function assert(x: unknown): asserts x; From 057a68e3a8872f5b63901317f63d72dac56d46e4 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Sat, 5 Oct 2024 09:24:27 +0300 Subject: [PATCH 05/41] update docs --- .../eslint-plugin/docs/rules/strict-boolean-expressions.mdx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/eslint-plugin/docs/rules/strict-boolean-expressions.mdx b/packages/eslint-plugin/docs/rules/strict-boolean-expressions.mdx index 820d4d188d4..343d315813e 100644 --- a/packages/eslint-plugin/docs/rules/strict-boolean-expressions.mdx +++ b/packages/eslint-plugin/docs/rules/strict-boolean-expressions.mdx @@ -22,6 +22,7 @@ The following nodes are considered boolean expressions and their type is checked - Right-hand side operand is ignored when it's not a descendant of another boolean expression. This is to allow usage of boolean operators for their short-circuiting behavior. - Asserted argument of an assertion function (`assert(arg)`). +- Return type of array predicate functions such as `filter()`, `some()`, etc. ## Examples @@ -61,6 +62,9 @@ while (obj) { declare function assert(value: unknown): asserts value; let maybeString = Math.random() > 0.5 ? '' : undefined; assert(maybeString); + +// array predicates return statements are boolean contexts. +['one', null].filter(x => x); ``` From d2ea1ab2078b7dae1185511c595b49accb810252 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Sat, 5 Oct 2024 09:26:30 +0300 Subject: [PATCH 06/41] update snapshots --- .../strict-boolean-expressions.shot | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/strict-boolean-expressions.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/strict-boolean-expressions.shot index a62ee559e04..e5ca1573a0f 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/strict-boolean-expressions.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/strict-boolean-expressions.shot @@ -41,6 +41,10 @@ declare function assert(value: unknown): asserts value; let maybeString = Math.random() > 0.5 ? '' : undefined; assert(maybeString); ~~~~~~~~~~~ Unexpected nullable string value in conditional. Please handle the nullish/empty cases explicitly. + +// array predicates return statements are boolean contexts. +['one', null].filter(x => x); + ~ Unexpected nullable string value in conditional. Please handle the nullish/empty cases explicitly. " `; From d641dc9699255d0b966f10d5d498dde77539155c Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Fri, 11 Oct 2024 10:58:01 +0300 Subject: [PATCH 07/41] handle implicit or explicit undefined return types --- .../src/rules/strict-boolean-expressions.ts | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 67e1f5fff19..3519236ceda 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -55,7 +55,8 @@ export type MessageId = | 'conditionFixDefaultEmptyString' | 'conditionFixDefaultFalse' | 'conditionFixDefaultZero' - | 'noStrictNullCheck'; + | 'noStrictNullCheck' + | 'predicateReturnsUndefined'; export default createRule({ name: 'strict-boolean-expressions', @@ -126,6 +127,8 @@ export default createRule({ 'Explicitly treat nullish value the same as 0 (`value ?? 0`)', noStrictNullCheck: 'This rule requires the `strictNullChecks` compiler option to be turned on to function correctly.', + predicateReturnsUndefined: + 'Unexpected `undefined` return value from predicate function', }, schema: [ { @@ -310,11 +313,42 @@ export default createRule({ const predicate = node.arguments.at(0); if (predicate != null) { + const type = checker.getApparentType( + services.getTypeAtLocation(predicate), + ); + + const returnTypeIncludesUndefined = tsutils + .unionTypeParts(type) + .some(t => isFunctionReturningUndefined(t)); + + if (returnTypeIncludesUndefined) { + context.report({ + node: predicate, + messageId: 'predicateReturnsUndefined', + }); + } + traverseArrayPredicateReturnStatements(predicate); } } } + function isFunctionReturningUndefined(type: ts.Type): boolean { + for (const signature of type.getCallSignatures()) { + const returnType = signature.getReturnType(); + + if ( + tsutils + .unionTypeParts(returnType) + .some(t => tsutils.isTypeFlagSet(t, ts.TypeFlags.Undefined)) + ) { + return true; + } + } + + return false; + } + /** * Inspects any node. * From 8d997d1dbf1d3ad130b808eabc15532e67cc4e73 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Fri, 11 Oct 2024 11:28:58 +0300 Subject: [PATCH 08/41] refactor and update tests --- .../src/rules/strict-boolean-expressions.ts | 13 +- .../rules/strict-boolean-expressions.test.ts | 111 ++++++++++++++++-- 2 files changed, 108 insertions(+), 16 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 3519236ceda..1b7b060d8b4 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -128,7 +128,7 @@ export default createRule({ noStrictNullCheck: 'This rule requires the `strictNullChecks` compiler option to be turned on to function correctly.', predicateReturnsUndefined: - 'Unexpected `undefined` return value from predicate function', + 'Unexpected `undefined` or `void` return value from predicate function', }, schema: [ { @@ -319,7 +319,7 @@ export default createRule({ const returnTypeIncludesUndefined = tsutils .unionTypeParts(type) - .some(t => isFunctionReturningUndefined(t)); + .some(t => isFunctionReturningUndefinedOrVoid(t)); if (returnTypeIncludesUndefined) { context.report({ @@ -333,14 +333,19 @@ export default createRule({ } } - function isFunctionReturningUndefined(type: ts.Type): boolean { + function isFunctionReturningUndefinedOrVoid(type: ts.Type): boolean { for (const signature of type.getCallSignatures()) { const returnType = signature.getReturnType(); if ( tsutils .unionTypeParts(returnType) - .some(t => tsutils.isTypeFlagSet(t, ts.TypeFlags.Undefined)) + .some(t => + tsutils.isTypeFlagSet( + t, + ts.TypeFlags.Undefined | ts.TypeFlags.Void, + ), + ) ) { return true; } 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 df6669f1839..a49d76d6aeb 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -164,10 +164,6 @@ if (x) { x.filter(function (t) { return t; }); - x.filter(t => { - return t ? 1 : 0; - }); - x.filter(t => !t); `, options: [{ allowNullableBoolean: true }], }, @@ -650,13 +646,6 @@ assert(nullableString); const x = (t: string | null) => t; ['', null].filter(x); `, - ` - ['one', 'two'].filter(x => { - if (x.length > 2) { - return true; - } - }); - `, ], invalid: [ @@ -1438,7 +1427,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } declare const x: string; if (x) {} (x: string) => (!x); (x: T) => x ? 1 : 0; - ['foo', 'bar'].filter(x => x); `, errors: [ { @@ -2804,6 +2792,105 @@ if (x) { ], output: null, }, + // implicitly return `undefined` + { + code: ` +['one', 'two'].filter(x => { + if (x.length > 2) { + return true; + } +}); + `, + errors: [ + { + column: 23, + endColumn: 2, + endLine: 6, + line: 2, + messageId: 'predicateReturnsUndefined', + }, + ], + }, + // implicitly return `undefined` with a return type annotation + { + code: ` +['one', 'two'].filter((x): boolean | undefined => { + if (x.length > 2) { + return true; + } +}); + `, + errors: [ + { + column: 23, + endColumn: 2, + endLine: 6, + line: 2, + messageId: 'predicateReturnsUndefined', + }, + ], + }, + // explicitly returning `undefined` + { + code: ` +['one', 'two'].filter(x => { + return undefined; +}); + `, + errors: [ + { + column: 23, + endColumn: 2, + endLine: 4, + line: 2, + messageId: 'predicateReturnsUndefined', + }, + { + column: 10, + endColumn: 19, + endLine: 3, + line: 3, + messageId: 'conditionErrorNullish', + }, + ], + }, + // `void` returning functions + { + code: ` +['one', 'two'].filter(x => { + return; +}); + `, + errors: [ + { + column: 23, + endColumn: 2, + endLine: 4, + line: 2, + messageId: 'predicateReturnsUndefined', + }, + ], + }, + { + code: ` +['one', null].filter(x => { + if (Math.random() > 0.5) { + return; + } + + return x != null; +}); + `, + errors: [ + { + column: 22, + endColumn: 2, + endLine: 8, + line: 2, + messageId: 'predicateReturnsUndefined', + }, + ], + }, { code: ` declare function assert(x: unknown): asserts x; From 47c167c119ced42804e038931f92b2f03a83563f Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Fri, 11 Oct 2024 11:29:53 +0300 Subject: [PATCH 09/41] remove unnecessary union check --- .../eslint-plugin/src/rules/strict-boolean-expressions.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 1b7b060d8b4..96e470a24f8 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -317,9 +317,8 @@ export default createRule({ services.getTypeAtLocation(predicate), ); - const returnTypeIncludesUndefined = tsutils - .unionTypeParts(type) - .some(t => isFunctionReturningUndefinedOrVoid(t)); + const returnTypeIncludesUndefined = + isFunctionReturningUndefinedOrVoid(type); if (returnTypeIncludesUndefined) { context.report({ From 90f9ba8229aade823243a09c2fbdc1c4771c8986 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Fri, 11 Oct 2024 11:30:16 +0300 Subject: [PATCH 10/41] inline check --- .../eslint-plugin/src/rules/strict-boolean-expressions.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 96e470a24f8..a285ff620a3 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -317,10 +317,7 @@ export default createRule({ services.getTypeAtLocation(predicate), ); - const returnTypeIncludesUndefined = - isFunctionReturningUndefinedOrVoid(type); - - if (returnTypeIncludesUndefined) { + if (isFunctionReturningUndefinedOrVoid(type)) { context.report({ node: predicate, messageId: 'predicateReturnsUndefined', From 42a13368df04cca97c88ed69c836010e8d223502 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Fri, 11 Oct 2024 13:21:16 +0300 Subject: [PATCH 11/41] cover empty return staetments --- .../src/rules/strict-boolean-expressions.ts | 65 ++++++++++++++----- .../rules/strict-boolean-expressions.test.ts | 29 ++++----- 2 files changed, 58 insertions(+), 36 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index a285ff620a3..1770bffa1c9 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -56,6 +56,7 @@ export type MessageId = | 'conditionFixDefaultFalse' | 'conditionFixDefaultZero' | 'noStrictNullCheck' + | 'predicateReturnsEmptyExpression' | 'predicateReturnsUndefined'; export default createRule({ @@ -127,8 +128,10 @@ export default createRule({ 'Explicitly treat nullish value the same as 0 (`value ?? 0`)', noStrictNullCheck: 'This rule requires the `strictNullChecks` compiler option to be turned on to function correctly.', + predicateReturnsEmptyExpression: + 'Unexpected empty return from predicate function', predicateReturnsUndefined: - 'Unexpected `undefined` or `void` return value from predicate function', + 'Unexpected `undefined` return value from predicate function', }, schema: [ { @@ -273,14 +276,23 @@ export default createRule({ traverseNode(node.right, isCondition); } + /** + * Inspects return statements of predicate functions. + * + * Returns a boolean representing whether or not the predicate + * has at least one return statement. + */ function traverseArrayPredicateReturnStatements( node: TSESTree.CallExpressionArgument, - ): void { + ): boolean { + let hasReturnStatement = false; + // Shorthand arrow function expression if ( node.type === AST_NODE_TYPES.ArrowFunctionExpression && node.body.type !== AST_NODE_TYPES.BlockStatement ) { + hasReturnStatement = true; traverseNode(node.body, true); } @@ -293,15 +305,24 @@ export default createRule({ const tsBody = services.esTreeNodeToTSNodeMap.get(node.body); forEachReturnStatement(tsBody, tsStatement => { + hasReturnStatement = true; + const statement = services.tsNodeToESTreeNodeMap.get(tsStatement); - if ( - statement.type === AST_NODE_TYPES.ReturnStatement && - statement.argument - ) { - traverseNode(statement.argument, true); + + if (statement.type === AST_NODE_TYPES.ReturnStatement) { + if (statement.argument) { + return traverseNode(statement.argument, true); + } + + context.report({ + node: statement, + messageId: 'predicateReturnsEmptyExpression', + }); } }); } + + return hasReturnStatement; } function traverseCallExpression(node: TSESTree.CallExpression): void { @@ -313,35 +334,43 @@ export default createRule({ const predicate = node.arguments.at(0); if (predicate != null) { + const hasReturnStatement = + traverseArrayPredicateReturnStatements(predicate); + + // Empty functions has their return type as `void` rather than `undefined` + if ( + !hasReturnStatement && + (predicate.type === AST_NODE_TYPES.FunctionExpression || + predicate.type === AST_NODE_TYPES.ArrowFunctionExpression) + ) { + return context.report({ + node: predicate, + messageId: 'predicateReturnsUndefined', + }); + } + const type = checker.getApparentType( services.getTypeAtLocation(predicate), ); - if (isFunctionReturningUndefinedOrVoid(type)) { - context.report({ + if (isFunctionReturningUndefined(type)) { + return context.report({ node: predicate, messageId: 'predicateReturnsUndefined', }); } - - traverseArrayPredicateReturnStatements(predicate); } } } - function isFunctionReturningUndefinedOrVoid(type: ts.Type): boolean { + function isFunctionReturningUndefined(type: ts.Type): boolean { for (const signature of type.getCallSignatures()) { const returnType = signature.getReturnType(); if ( tsutils .unionTypeParts(returnType) - .some(t => - tsutils.isTypeFlagSet( - t, - ts.TypeFlags.Undefined | ts.TypeFlags.Void, - ), - ) + .some(t => tsutils.isTypeFlagSet(t, ts.TypeFlags.Undefined)) ) { return true; } 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 a49d76d6aeb..fd4396cd5b1 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -2838,13 +2838,6 @@ if (x) { }); `, errors: [ - { - column: 23, - endColumn: 2, - endLine: 4, - line: 2, - messageId: 'predicateReturnsUndefined', - }, { column: 10, endColumn: 19, @@ -2854,7 +2847,7 @@ if (x) { }, ], }, - // `void` returning functions + // empty return statements { code: ` ['one', 'two'].filter(x => { @@ -2863,11 +2856,11 @@ if (x) { `, errors: [ { - column: 23, - endColumn: 2, - endLine: 4, - line: 2, - messageId: 'predicateReturnsUndefined', + column: 3, + endColumn: 10, + endLine: 3, + line: 3, + messageId: 'predicateReturnsEmptyExpression', }, ], }, @@ -2883,11 +2876,11 @@ if (x) { `, errors: [ { - column: 22, - endColumn: 2, - endLine: 8, - line: 2, - messageId: 'predicateReturnsUndefined', + column: 5, + endColumn: 12, + endLine: 4, + line: 4, + messageId: 'predicateReturnsEmptyExpression', }, ], }, From 655e52cabd67b0f541246a595d609ccc532fc6e7 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Fri, 11 Oct 2024 14:01:55 +0300 Subject: [PATCH 12/41] report a function as returning undefined only if no other isssues were found --- .../src/rules/strict-boolean-expressions.ts | 88 ++++++++++++------- .../rules/strict-boolean-expressions.test.ts | 18 ++++ 2 files changed, 72 insertions(+), 34 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 1770bffa1c9..ac48a7db497 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -268,12 +268,14 @@ export default createRule({ function traverseLogicalExpression( node: TSESTree.LogicalExpression, isCondition = false, - ): void { + ): boolean { // left argument is always treated as a condition - traverseNode(node.left, true); + const isLeftReported = traverseNode(node.left, true); // if the logical expression is used for control flow, // then its right argument is used for its side effects only - traverseNode(node.right, isCondition); + const isRightReported = traverseNode(node.right, isCondition); + + return isLeftReported || isRightReported; } /** @@ -284,8 +286,9 @@ export default createRule({ */ function traverseArrayPredicateReturnStatements( node: TSESTree.CallExpressionArgument, - ): boolean { + ): { hasReported: boolean; hasReturnStatement: boolean } { let hasReturnStatement = false; + let hasReported = false; // Shorthand arrow function expression if ( @@ -293,7 +296,7 @@ export default createRule({ node.body.type !== AST_NODE_TYPES.BlockStatement ) { hasReturnStatement = true; - traverseNode(node.body, true); + hasReported = traverseNode(node.body, true); } // Any other function expression with a block body @@ -311,18 +314,20 @@ export default createRule({ if (statement.type === AST_NODE_TYPES.ReturnStatement) { if (statement.argument) { - return traverseNode(statement.argument, true); - } + hasReported ||= traverseNode(statement.argument, true); + } else { + hasReported = true; - context.report({ - node: statement, - messageId: 'predicateReturnsEmptyExpression', - }); + context.report({ + node: statement, + messageId: 'predicateReturnsEmptyExpression', + }); + } } }); } - return hasReturnStatement; + return { hasReported, hasReturnStatement }; } function traverseCallExpression(node: TSESTree.CallExpression): void { @@ -334,9 +339,14 @@ export default createRule({ const predicate = node.arguments.at(0); if (predicate != null) { - const hasReturnStatement = + const { hasReported, hasReturnStatement } = traverseArrayPredicateReturnStatements(predicate); + // Don't report a function as returning `undefined` unless every + if (hasReported) { + return; + } + // Empty functions has their return type as `void` rather than `undefined` if ( !hasReturnStatement && @@ -353,6 +363,7 @@ export default createRule({ services.getTypeAtLocation(predicate), ); + // if (isFunctionReturningUndefined(type)) { return context.report({ node: predicate, @@ -390,10 +401,10 @@ export default createRule({ function traverseNode( node: TSESTree.Expression, isCondition: boolean, - ): void { + ): boolean { // prevent checking the same node multiple times if (traversedNodes.has(node)) { - return; + return false; } traversedNodes.add(node); @@ -402,23 +413,22 @@ export default createRule({ node.type === AST_NODE_TYPES.LogicalExpression && node.operator !== '??' ) { - traverseLogicalExpression(node, isCondition); - return; + return traverseLogicalExpression(node, isCondition); } // skip if node is not a condition if (!isCondition) { - return; + return false; } - checkNode(node); + return checkNode(node); } /** * This function does the actual type check on a node. * It analyzes the type of a node and checks if it is allowed in a boolean context. */ - function checkNode(node: TSESTree.Expression): void { + function checkNode(node: TSESTree.Expression): boolean { const type = getConstrainedTypeAtLocation(services, node); const types = inspectVariantTypes(tsutils.unionTypeParts(type)); @@ -429,25 +439,25 @@ export default createRule({ // boolean if (is('boolean') || is('truthy boolean')) { // boolean is always okay - return; + return false; } // never if (is('never')) { // never is always okay - return; + return false; } // nullish if (is('nullish')) { // condition is always false context.report({ node, messageId: 'conditionErrorNullish' }); - return; + return true; } // Known edge case: boolean `true` and nullish values are always valid boolean expressions if (is('nullish', 'truthy boolean')) { - return; + return false; } // nullable boolean @@ -503,8 +513,9 @@ export default createRule({ ], }); } + return true; } - return; + return false; } // Known edge case: truthy primitives and nullish values are always valid boolean expressions @@ -512,7 +523,7 @@ export default createRule({ (options.allowNumber && is('nullish', 'truthy number')) || (options.allowString && is('nullish', 'truthy string')) ) { - return; + return false; } // string @@ -586,8 +597,9 @@ export default createRule({ ], }); } + return true; } - return; + return false; } // nullable string @@ -660,8 +672,9 @@ export default createRule({ ], }); } + return true; } - return; + return false; } // number @@ -762,8 +775,9 @@ export default createRule({ ], }); } + return true; } - return; + return false; } // nullable number @@ -836,15 +850,16 @@ export default createRule({ ], }); } + return true; } - return; + return false; } // object if (is('object')) { // condition is always true context.report({ node, messageId: 'conditionErrorObject' }); - return; + return true; } // nullable object @@ -884,8 +899,9 @@ export default createRule({ ], }); } + return true; } - return; + return false; } // nullable enum @@ -923,8 +939,9 @@ export default createRule({ }), }); } + return true; } - return; + return false; } // any @@ -944,12 +961,15 @@ export default createRule({ }, ], }); + return true; } - return; + return false; } // other context.report({ node, messageId: 'conditionErrorOther' }); + + return true; } /** The types we care about */ 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 fd4396cd5b1..60973644ee1 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -2847,6 +2847,24 @@ if (x) { }, ], }, + { + code: ` +['one', 'two'].filter(x => { + if (Math.random() > 0.5) { + return undefined; + } +}); + `, + errors: [ + { + column: 12, + endColumn: 21, + endLine: 4, + line: 4, + messageId: 'conditionErrorNullish', + }, + ], + }, // empty return statements { code: ` From b8643c9539a9b761929b42ff81281d5446d8d3cf Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Fri, 11 Oct 2024 14:05:37 +0300 Subject: [PATCH 13/41] update comments --- .../src/rules/strict-boolean-expressions.ts | 10 +++++----- .../tests/rules/strict-boolean-expressions.test.ts | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index ac48a7db497..d3a3625f7d0 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -280,9 +280,6 @@ export default createRule({ /** * Inspects return statements of predicate functions. - * - * Returns a boolean representing whether or not the predicate - * has at least one return statement. */ function traverseArrayPredicateReturnStatements( node: TSESTree.CallExpressionArgument, @@ -342,7 +339,8 @@ export default createRule({ const { hasReported, hasReturnStatement } = traverseArrayPredicateReturnStatements(predicate); - // Don't report a function as returning `undefined` unless every + // Don't report a function as implicitly returning `undefined` only + // when no other issues were found with it. if (hasReported) { return; } @@ -363,7 +361,7 @@ export default createRule({ services.getTypeAtLocation(predicate), ); - // + // Report a function as implicitly returning `undefined` if (isFunctionReturningUndefined(type)) { return context.report({ node: predicate, @@ -427,6 +425,8 @@ export default createRule({ /** * This function does the actual type check on a node. * It analyzes the type of a node and checks if it is allowed in a boolean context. + * + * @returns true if it reported an issue, false otherwise. */ function checkNode(node: TSESTree.Expression): boolean { const type = getConstrainedTypeAtLocation(services, node); 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 60973644ee1..2cfab318107 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -2811,6 +2811,20 @@ if (x) { }, ], }, + { + code: ` +['one', 'two'].filter(x => {}); + `, + errors: [ + { + column: 23, + endColumn: 30, + endLine: 2, + line: 2, + messageId: 'predicateReturnsUndefined', + }, + ], + }, // implicitly return `undefined` with a return type annotation { code: ` From c665d825342641aeeecac5ecaa4213a6972ed45a Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Fri, 11 Oct 2024 19:02:24 +0300 Subject: [PATCH 14/41] add test --- .../rules/strict-boolean-expressions.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) 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 2cfab318107..01a561b29d0 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -646,6 +646,22 @@ assert(nullableString); const x = (t: string | null) => t; ['', null].filter(x); `, + ` + [1, 2].filter(x => { + if (x === 1) { + return true; + } + + throw new Error('oops'); + }); + `, + ` + [1, 2].filter(x => { + if (typeof x === 'number') { + return true; + } + }); + `, ], invalid: [ From b3dd8becb3930ab06282845f32f1eb163d186ef2 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Fri, 11 Oct 2024 21:03:24 +0300 Subject: [PATCH 15/41] remove unnecessary test --- .../tests/rules/strict-boolean-expressions.test.ts | 7 ------- 1 file changed, 7 deletions(-) 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 01a561b29d0..8f6ef4f78c8 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -655,13 +655,6 @@ assert(nullableString); throw new Error('oops'); }); `, - ` - [1, 2].filter(x => { - if (typeof x === 'number') { - return true; - } - }); - `, ], invalid: [ From 569138041b9d73b73d5e60242a977b02e4072803 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Wed, 30 Oct 2024 21:26:44 +0200 Subject: [PATCH 16/41] update code to match function's inferred return type rather than each return statement individually --- .../src/rules/strict-boolean-expressions.ts | 257 +++--- .../rules/strict-boolean-expressions.test.ts | 820 ++++++------------ 2 files changed, 366 insertions(+), 711 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 2897ff1ceee..46b1a4e87e3 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -1,5 +1,6 @@ import type { ParserServicesWithTypeInformation, + TSESLint, TSESTree, } from '@typescript-eslint/utils'; @@ -9,12 +10,12 @@ import * as ts from 'typescript'; import { createRule, - forEachReturnStatement, getConstrainedTypeAtLocation, getParserServices, getWrappingFixer, isArrayMethodCallWithPredicate, isTypeArrayTypeOrUnionOfArrayTypes, + nullThrows, } from '../util'; import { findTruthinessAssertedArgument } from '../util/assertionFunctionUtils'; @@ -55,9 +56,10 @@ export type MessageId = | 'conditionFixDefaultEmptyString' | 'conditionFixDefaultFalse' | 'conditionFixDefaultZero' + | 'explicitBooleanReturnType' | 'noStrictNullCheck' - | 'predicateReturnsEmptyExpression' - | 'predicateReturnsUndefined'; + | 'predicateCannotBeAsync' + | 'predicateReturnsNonBoolean'; export default createRule({ name: 'strict-boolean-expressions', @@ -126,12 +128,13 @@ export default createRule({ 'Explicitly treat nullish value the same as false (`value ?? false`)', conditionFixDefaultZero: 'Explicitly treat nullish value the same as 0 (`value ?? 0`)', + explicitBooleanReturnType: + 'Explicitly return a boolean expression for type accuracy.', noStrictNullCheck: 'This rule requires the `strictNullChecks` compiler option to be turned on to function correctly.', - predicateReturnsEmptyExpression: - 'Unexpected empty return from predicate function', - predicateReturnsUndefined: - 'Unexpected `undefined` return value from predicate function', + predicateCannotBeAsync: + "Predicate function should not be 'async'; expected a boolean return type.", + predicateReturnsNonBoolean: 'Predicate function should return a boolean.', }, schema: [ { @@ -240,6 +243,27 @@ export default createRule({ | TSESTree.IfStatement | TSESTree.WhileStatement; + function isFunctionReturningBoolean(type: ts.Type): boolean { + for (const signature of type.getCallSignatures()) { + const returnType = signature.getReturnType(); + + if ( + tsutils + .unionTypeParts(returnType) + .every(t => + tsutils.isTypeFlagSet( + t, + ts.TypeFlags.Boolean | ts.TypeFlags.BooleanLiteral, + ), + ) + ) { + return true; + } + } + + return false; + } + /** * Inspects condition of a test expression. (`if`, `while`, `for`, etc.) */ @@ -270,63 +294,12 @@ export default createRule({ function traverseLogicalExpression( node: TSESTree.LogicalExpression, isCondition = false, - ): boolean { + ): void { // left argument is always treated as a condition - const isLeftReported = traverseNode(node.left, true); + traverseNode(node.left, true); // if the logical expression is used for control flow, // then its right argument is used for its side effects only - const isRightReported = traverseNode(node.right, isCondition); - - return isLeftReported || isRightReported; - } - - /** - * Inspects return statements of predicate functions. - */ - function traverseArrayPredicateReturnStatements( - node: TSESTree.CallExpressionArgument, - ): { hasReported: boolean; hasReturnStatement: boolean } { - let hasReturnStatement = false; - let hasReported = false; - - // Shorthand arrow function expression - if ( - node.type === AST_NODE_TYPES.ArrowFunctionExpression && - node.body.type !== AST_NODE_TYPES.BlockStatement - ) { - hasReturnStatement = true; - hasReported = traverseNode(node.body, true); - } - - // Any other function expression with a block body - else if ( - (node.type === AST_NODE_TYPES.FunctionExpression || - node.type === AST_NODE_TYPES.ArrowFunctionExpression) && - node.body.type === AST_NODE_TYPES.BlockStatement - ) { - const tsBody = services.esTreeNodeToTSNodeMap.get(node.body); - - forEachReturnStatement(tsBody, tsStatement => { - hasReturnStatement = true; - - const statement = services.tsNodeToESTreeNodeMap.get(tsStatement); - - if (statement.type === AST_NODE_TYPES.ReturnStatement) { - if (statement.argument) { - hasReported ||= traverseNode(statement.argument, true); - } else { - hasReported = true; - - context.report({ - node: statement, - messageId: 'predicateReturnsEmptyExpression', - }); - } - } - }); - } - - return { hasReported, hasReturnStatement }; + traverseNode(node.right, isCondition); } function traverseCallExpression(node: TSESTree.CallExpression): void { @@ -337,57 +310,84 @@ export default createRule({ if (isArrayMethodCallWithPredicate(context, services, node)) { const predicate = node.arguments.at(0); - if (predicate != null) { - const { hasReported, hasReturnStatement } = - traverseArrayPredicateReturnStatements(predicate); + if (!predicate) { + return; + } - // Don't report a function as implicitly returning `undefined` only - // when no other issues were found with it. - if (hasReported) { - return; - } + // custom-tailored message for accidental `async` function expressions + if ( + (predicate.type === AST_NODE_TYPES.FunctionExpression || + predicate.type === AST_NODE_TYPES.ArrowFunctionExpression) && + predicate.async + ) { + return context.report({ + node: predicate, + messageId: 'predicateCannotBeAsync', + }); + } + + const type = checker.getApparentType( + services.getTypeAtLocation(predicate), + ); + + if (!isFunctionReturningBoolean(type)) { + const suggest: TSESLint.ReportSuggestionArray = []; - // Empty functions has their return type as `void` rather than `undefined` if ( - !hasReturnStatement && (predicate.type === AST_NODE_TYPES.FunctionExpression || - predicate.type === AST_NODE_TYPES.ArrowFunctionExpression) + predicate.type === AST_NODE_TYPES.ArrowFunctionExpression) && + !predicate.returnType ) { - return context.report({ - node: predicate, - messageId: 'predicateReturnsUndefined', - }); - } - - const type = checker.getApparentType( - services.getTypeAtLocation(predicate), - ); - - // Report a function as implicitly returning `undefined` - if (isFunctionReturningUndefined(type)) { - return context.report({ - node: predicate, - messageId: 'predicateReturnsUndefined', + suggest.push({ + messageId: 'explicitBooleanReturnType', + fix: fixer => { + const lastParam = predicate.params.at(-1); + + // zero parameters + if (!lastParam) { + const closingBracket = nullThrows( + context.sourceCode.getFirstToken( + predicate, + token => token.value === ')', + ), + 'function expression with no arguments must have a closing parenthesis.', + ); + + return fixer.insertTextAfterRange( + closingBracket.range, + ': boolean', + ); + } + + const closingBracket = + context.sourceCode.getTokenAfter(lastParam); + + // one or more parameters wrapped with parens + if (closingBracket?.value === ')') { + return fixer.insertTextAfterRange( + closingBracket.range, + ': boolean', + ); + } + + // one param not wrapped with parens + const parameterText = context.sourceCode.getText(lastParam); + + return fixer.replaceText( + predicate.params[0], + `(${parameterText}): boolean`, + ); + }, }); } - } - } - } - - function isFunctionReturningUndefined(type: ts.Type): boolean { - for (const signature of type.getCallSignatures()) { - const returnType = signature.getReturnType(); - if ( - tsutils - .unionTypeParts(returnType) - .some(t => tsutils.isTypeFlagSet(t, ts.TypeFlags.Undefined)) - ) { - return true; + return context.report({ + node: predicate, + messageId: 'predicateReturnsNonBoolean', + suggest, + }); } } - - return false; } /** @@ -401,10 +401,10 @@ export default createRule({ function traverseNode( node: TSESTree.Expression, isCondition: boolean, - ): boolean { + ): void { // prevent checking the same node multiple times if (traversedNodes.has(node)) { - return false; + return; } traversedNodes.add(node); @@ -413,24 +413,23 @@ export default createRule({ node.type === AST_NODE_TYPES.LogicalExpression && node.operator !== '??' ) { - return traverseLogicalExpression(node, isCondition); + traverseLogicalExpression(node, isCondition); + return; } // skip if node is not a condition if (!isCondition) { - return false; + return; } - return checkNode(node); + checkNode(node); } /** * This function does the actual type check on a node. * It analyzes the type of a node and checks if it is allowed in a boolean context. - * - * @returns true if it reported an issue, false otherwise. */ - function checkNode(node: TSESTree.Expression): boolean { + function checkNode(node: TSESTree.Expression): void { const type = getConstrainedTypeAtLocation(services, node); const types = inspectVariantTypes(tsutils.unionTypeParts(type)); @@ -441,25 +440,25 @@ export default createRule({ // boolean if (is('boolean') || is('truthy boolean')) { // boolean is always okay - return false; + return; } // never if (is('never')) { // never is always okay - return false; + return; } // nullish if (is('nullish')) { // condition is always false context.report({ node, messageId: 'conditionErrorNullish' }); - return true; + return; } // Known edge case: boolean `true` and nullish values are always valid boolean expressions if (is('nullish', 'truthy boolean')) { - return false; + return; } // nullable boolean @@ -515,9 +514,8 @@ export default createRule({ ], }); } - return true; } - return false; + return; } // Known edge case: truthy primitives and nullish values are always valid boolean expressions @@ -525,7 +523,7 @@ export default createRule({ (options.allowNumber && is('nullish', 'truthy number')) || (options.allowString && is('nullish', 'truthy string')) ) { - return false; + return; } // string @@ -599,9 +597,8 @@ export default createRule({ ], }); } - return true; } - return false; + return; } // nullable string @@ -674,9 +671,8 @@ export default createRule({ ], }); } - return true; } - return false; + return; } // number @@ -777,9 +773,8 @@ export default createRule({ ], }); } - return true; } - return false; + return; } // nullable number @@ -852,16 +847,15 @@ export default createRule({ ], }); } - return true; } - return false; + return; } // object if (is('object')) { // condition is always true context.report({ node, messageId: 'conditionErrorObject' }); - return true; + return; } // nullable object @@ -901,9 +895,8 @@ export default createRule({ ], }); } - return true; } - return false; + return; } // nullable enum @@ -941,9 +934,8 @@ export default createRule({ }), }); } - return true; } - return false; + return; } // any @@ -963,15 +955,12 @@ export default createRule({ }, ], }); - return true; } - return false; + return; } // other context.report({ node, messageId: 'conditionErrorOther' }); - - return true; } /** The types we care about */ 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 8f6ef4f78c8..a49b328498e 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -51,17 +51,6 @@ declare const x: never; if (x) { } `, - ` -[true, false].some(function (x) { - return x && 1; -}); - `, - ` -[true, false].filter(x => { - return x; -}); - `, - '[true, false].some(x => x && 1);', // string in boolean context ` @@ -78,17 +67,6 @@ if (x) { `, '(x: string) => !x;', '(x: T) => (x ? 1 : 0);', - ` -['1', '2', '3'].every(function (x) { - return x; -}); - `, - ` -[].every(() => { - return ''; -}); - `, - "['1', '2', '3'].every(x => x);", // number in boolean context ` @@ -105,17 +83,6 @@ if (x) { `, '(x: bigint) => !x;', '(x: T) => (x ? 1 : 0);', - ` -[1, 2, 3].every(function (x) { - return x; -}); - `, - ` -[1, 2, 3].some(x => { - return !x; -}); - `, - '[].find(() => 1);', // nullable object in boolean context ` @@ -125,17 +92,6 @@ if (x) { `, '(x?: { a: any }) => !x;', '(x: T) => (x ? 1 : 0);', - ` -[{}, null, {}].every(function (x) { - return x; -}); - `, - ` -[{}, null, {}].every(x => { - return x ? 1 : 0; -}); - `, - '[{}, null, null].findIndex(x => !x);', // nullable boolean in boolean context { @@ -158,15 +114,6 @@ if (x) { `, options: [{ allowNullableBoolean: true }], }, - { - code: ` - declare const x: (boolean | null)[]; - x.filter(function (t) { - return t; - }); - `, - options: [{ allowNullableBoolean: true }], - }, // nullable string in boolean context { @@ -189,19 +136,6 @@ if (x) { `, options: [{ allowNullableString: true }], }, - { - code: ` - declare const x: (string | null)[]; - x.findLast(function (t) { - return t; - }); - x.findLast(t => { - return !t; - }); - x.findLast(t => (t ? 1 : 0)); - `, - options: [{ allowNullableString: true }], - }, // nullable number in boolean context { @@ -224,19 +158,6 @@ if (x) { `, options: [{ allowNullableNumber: true }], }, - { - code: ` - declare const x: (number | null)[]; - x.findLast(function (t) { - return t; - }); - x.findLast(t => { - return !t; - }); - x.findLast(t => (t ? 1 : 0)); - `, - options: [{ allowNullableNumber: true }], - }, // any in boolean context { @@ -259,19 +180,6 @@ if (x) { `, options: [{ allowAny: true }], }, - { - code: ` - declare const x: any[]; - x.filter(function (t) { - return t; - }); - x.find(t => { - return !t; - }); - x.some(t => (t ? 1 : 0)); - `, - options: [{ allowAny: true }], - }, // logical operator { @@ -310,18 +218,6 @@ if (x) { `, options: [{ allowNumber: true, allowString: true }], }, - { - code: ` - [0, false, '', null, {}].filter(function (x) { - return x ? 1 : 0; - }); - [0, false, '', null, {}].filter(x => { - return !x; - }); - [0, false, '', null, {}].filter(x => x); - `, - options: [{ allowNumber: true, allowString: true }], - }, // nullable enum in boolean context { @@ -388,23 +284,6 @@ if (x) { `, options: [{ allowNullableEnum: true }], }, - { - code: ` - enum ExampleEnum { - This = 'one', - That = 'two', - } - declare const x: (ExampleEnum | null)[]; - x.some(function (t) { - return t; - }); - x.every(t => { - return !t; - }); - x.find(t => t); - `, - options: [{ allowNullableEnum: true }], - }, // nullable mixed enum in boolean context { @@ -643,17 +522,45 @@ assert(nullableString); } `, ` - const x = (t: string | null) => t; - ['', null].filter(x); +[true, false].some(function (x) { + return x; +}); + `, + ` +[1, null].filter(function (x) { + return x != null; +}); `, ` - [1, 2].filter(x => { - if (x === 1) { - return true; - } +['one', 'two', ''].filter(function (x) { + return !!x; +}); + `, + ` +['one', 'two', ''].filter(function (x): boolean { + return !!x; +}); + `, + // implicit return statements should be warned by TS's `noImplicitReturns` + ` +['one', 'two', ''].filter(function (x): boolean { + if (x) { + return true; + } +}); + `, + ` +['one', 'two', ''].filter(function (x): boolean { + if (x) { + return true; + } - throw new Error('oops'); - }); + throw new Error('oops'); +}); + `, + ` +declare const predicate: (string) => boolean; +['one', 'two', ''].filter(predicate); `, ], @@ -1533,25 +1440,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, - { - column: 36, - line: 7, - messageId: 'conditionErrorString', - suggestions: [ - { - messageId: 'conditionFixCompareStringLength', - output: ` ['foo', 'bar'].filter(x => x.length > 0);`, - }, - { - messageId: 'conditionFixCompareEmptyString', - output: ` ['foo', 'bar'].filter(x => x !== "");`, - }, - { - messageId: 'conditionFixCastBoolean', - output: ` ['foo', 'bar'].filter(x => Boolean(x));`, - }, - ], - }, ], options: [{ allowString: false }], }), @@ -1566,7 +1454,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } (x: T) => (x) ? 1 : 0; ![]["length"]; // doesn't count as array.length when computed declare const a: any[] & { notLength: number }; if (a.notLength) {} - [1, 2, 3].some(x => { return x; }); `, errors: [ { @@ -1706,27 +1593,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, - { - column: 38, - line: 9, - messageId: 'conditionErrorNumber', - suggestions: [ - { - messageId: 'conditionFixCompareZero', - // TODO: fix compare zero suggestion for bigint - output: ` [1, 2, 3].some(x => { return x !== 0; });`, - }, - { - // TODO: remove check NaN suggestion for bigint - messageId: 'conditionFixCompareNaN', - output: ` [1, 2, 3].some(x => { return !Number.isNaN(x); });`, - }, - { - messageId: 'conditionFixCastBoolean', - output: ` [1, 2, 3].some(x => { return Boolean(x); });`, - }, - ], - }, ], options: [{ allowNumber: false }], }), @@ -1737,20 +1603,17 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } if (![].length) {} (a: number[]) => a.length && "..." (...a: T) => a.length || "empty"; - [[1], [2]].find(x => x.length); `, errors: [ { column: 6, line: 2, messageId: 'conditionErrorNumber' }, { column: 26, line: 3, messageId: 'conditionErrorNumber' }, { column: 43, line: 4, messageId: 'conditionErrorNumber' }, - { column: 30, line: 5, messageId: 'conditionErrorNumber' }, ], options: [{ allowNumber: false }], output: ` if ([].length === 0) {} (a: number[]) => (a.length > 0) && "..." (...a: T) => (a.length > 0) || "empty"; - [[1], [2]].find(x => x.length > 0); `, }), @@ -1760,13 +1623,11 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } declare const x: string | number; if (x) {} (x: bigint | string) => !x; (x: T) => x ? 1 : 0; - [1, 'two'].findIndex(function (x) { return x; }); `, errors: [ { column: 39, line: 2, messageId: 'conditionErrorOther' }, { column: 34, line: 3, messageId: 'conditionErrorOther' }, { column: 55, line: 4, messageId: 'conditionErrorOther' }, - { column: 52, line: 5, messageId: 'conditionErrorOther' }, ], options: [{ allowNumber: true, allowString: true }], }), @@ -1777,7 +1638,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } declare const x: boolean | null; if (x) {} (x?: boolean) => !x; (x: T) => x ? 1 : 0; - [null, true].every(x => x); `, errors: [ { @@ -1825,21 +1685,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, - { - column: 33, - line: 5, - messageId: 'conditionErrorNullableBoolean', - suggestions: [ - { - messageId: 'conditionFixDefaultFalse', - output: ` [null, true].every(x => x ?? false);`, - }, - { - messageId: 'conditionFixCompareTrue', - output: ` [null, true].every(x => x === true);`, - }, - ], - }, ], options: [{ allowNullableBoolean: false }], }), @@ -1850,7 +1695,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } declare const x: object | null; if (x) {} (x?: { a: number }) => !x; (x: T) => x ? 1 : 0; - [{}, null].findIndex(x => { return x; }); `, errors: [ { @@ -1886,18 +1730,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, - { - column: 44, - line: 5, - messageId: 'conditionErrorNullableObject', - suggestions: [ - { - messageId: 'conditionFixCompareNullish', - output: - ' [{}, null].findIndex(x => { return x != null; });', - }, - ], - }, ], options: [{ allowNullableObject: false }], }), @@ -1909,7 +1741,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } (x?: string) => !x; (x: T) => x ? 1 : 0; function foo(x: '' | 'bar' | null) { if (!x) {} } - ['foo', null].some(x => x); `, errors: [ { @@ -1994,25 +1825,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, - { - column: 33, - line: 6, - messageId: 'conditionErrorNullableString', - suggestions: [ - { - messageId: 'conditionFixCompareNullish', - output: " ['foo', null].some(x => x != null);", - }, - { - messageId: 'conditionFixDefaultEmptyString', - output: ' [\'foo\', null].some(x => x ?? "");', - }, - { - messageId: 'conditionFixCastBoolean', - output: " ['foo', null].some(x => Boolean(x));", - }, - ], - }, ], }), @@ -2023,7 +1835,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } (x?: number) => !x; (x: T) => x ? 1 : 0; function foo(x: 0 | 1 | null) { if (!x) {} } - [5, null].findLastIndex(x => x); `, errors: [ { @@ -2108,25 +1919,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, - { - column: 38, - line: 6, - messageId: 'conditionErrorNullableNumber', - suggestions: [ - { - messageId: 'conditionFixCompareNullish', - output: ' [5, null].findLastIndex(x => x != null);', - }, - { - messageId: 'conditionFixDefaultZero', - output: ' [5, null].findLastIndex(x => x ?? 0);', - }, - { - messageId: 'conditionFixCastBoolean', - output: ' [5, null].findLastIndex(x => Boolean(x));', - }, - ], - }, ], }), @@ -2140,7 +1932,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum) { } - [theEnum].filter(x => x); `, errors: [ { @@ -2150,13 +1941,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } line: 7, messageId: 'conditionErrorNullableEnum', }, - { - column: 31, - endColumn: 32, - endLine: 9, - line: 9, - messageId: 'conditionErrorNullableEnum', - }, ], options: [{ allowNullableEnum: false }], output: ` @@ -2167,7 +1951,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum != null) { } - [theEnum].filter(x => x != null); `, }, { @@ -2179,9 +1962,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (!theEnum) { } - [theEnum].filter(function (x) { - return !x; - }); `, errors: [ { @@ -2191,13 +1971,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } line: 7, messageId: 'conditionErrorNullableEnum', }, - { - column: 19, - endColumn: 20, - endLine: 10, - line: 10, - messageId: 'conditionErrorNullableEnum', - }, ], options: [{ allowNullableEnum: false }], output: ` @@ -2208,9 +1981,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum == null) { } - [theEnum].filter(function (x) { - return x == null; - }); `, }, { @@ -2222,9 +1992,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (!theEnum) { } - [theEnum].filter(x => { - return !x; - }); `, errors: [ { @@ -2234,13 +2001,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } line: 7, messageId: 'conditionErrorNullableEnum', }, - { - column: 19, - endColumn: 20, - endLine: 10, - line: 10, - messageId: 'conditionErrorNullableEnum', - }, ], options: [{ allowNullableEnum: false }], output: ` @@ -2251,9 +2011,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum == null) { } - [theEnum].filter(x => { - return x == null; - }); `, }, { @@ -2265,7 +2022,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (!theEnum) { } - [theEnum].every(x => !x); `, errors: [ { @@ -2275,13 +2031,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } line: 7, messageId: 'conditionErrorNullableEnum', }, - { - column: 31, - endColumn: 32, - endLine: 9, - line: 9, - messageId: 'conditionErrorNullableEnum', - }, ], options: [{ allowNullableEnum: false }], output: ` @@ -2292,7 +2041,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum == null) { } - [theEnum].every(x => x == null); `, }, { @@ -2304,7 +2052,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (!theEnum) { } - [theEnum].filter(x => !x); `, errors: [ { @@ -2314,13 +2061,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } line: 7, messageId: 'conditionErrorNullableEnum', }, - { - column: 32, - endColumn: 33, - endLine: 9, - line: 9, - messageId: 'conditionErrorNullableEnum', - }, ], options: [{ allowNullableEnum: false }], output: ` @@ -2331,7 +2071,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum == null) { } - [theEnum].filter(x => x == null); `, }, { @@ -2343,7 +2082,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (!theEnum) { } - [theEnum].filter(x => !x); `, errors: [ { @@ -2353,13 +2091,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } line: 7, messageId: 'conditionErrorNullableEnum', }, - { - column: 32, - endColumn: 33, - endLine: 9, - line: 9, - messageId: 'conditionErrorNullableEnum', - }, ], options: [{ allowNullableEnum: false }], output: ` @@ -2370,7 +2101,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum == null) { } - [theEnum].filter(x => x == null); `, }, { @@ -2382,7 +2112,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (!theEnum) { } - [theEnum].find(x => !x); `, errors: [ { @@ -2392,13 +2121,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } line: 7, messageId: 'conditionErrorNullableEnum', }, - { - column: 30, - endColumn: 31, - endLine: 9, - line: 9, - messageId: 'conditionErrorNullableEnum', - }, ], options: [{ allowNullableEnum: false }], output: ` @@ -2409,7 +2131,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } const theEnum = Math.random() < 0.3 ? ExampleEnum.This : null; if (theEnum == null) { } - [theEnum].find(x => x == null); `, }, @@ -2530,7 +2251,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } x => !x; (x: T) => x ? 1 : 0; (x: T) => x ? 1 : 0; - [x].filter(t => t); `, errors: [ { @@ -2577,17 +2297,6 @@ if (((Boolean('')) && {}) || (foo && void 0)) { } }, ], }, - { - column: 25, - line: 6, - messageId: 'conditionErrorAny', - suggestions: [ - { - messageId: 'conditionFixCastBoolean', - output: ' [x].filter(t => Boolean(t));', - }, - ], - }, ], }), @@ -2700,282 +2409,57 @@ if (x) { options: [{ allowNullableObject: false }], output: null, }, - // multi line predicate function { code: ` -['one', null].filter(x => { - if (Math.random() > 0.5) { - return x; - } - - return !x; -}); +declare function assert(x: unknown): asserts x; +declare const nullableString: string | null; +assert(nullableString); `, errors: [ { - column: 12, + column: 8, line: 4, messageId: 'conditionErrorNullableString', suggestions: [ { messageId: 'conditionFixCompareNullish', output: ` -['one', null].filter(x => { - if (Math.random() > 0.5) { - return x != null; - } - - return !x; -}); +declare function assert(x: unknown): asserts x; +declare const nullableString: string | null; +assert(nullableString != null); `, }, { messageId: 'conditionFixDefaultEmptyString', output: ` -['one', null].filter(x => { - if (Math.random() > 0.5) { - return x ?? ""; - } - - return !x; -}); +declare function assert(x: unknown): asserts x; +declare const nullableString: string | null; +assert(nullableString ?? ""); `, }, { messageId: 'conditionFixCastBoolean', output: ` -['one', null].filter(x => { - if (Math.random() > 0.5) { - return Boolean(x); - } - - return !x; -}); +declare function assert(x: unknown): asserts x; +declare const nullableString: string | null; +assert(Boolean(nullableString)); `, }, ], }, + ], + output: null, + }, + { + code: ` +declare function assert(a: number, b: unknown): asserts b; +declare const nullableString: string | null; +assert(foo, nullableString); + `, + errors: [ { - column: 11, - line: 7, - messageId: 'conditionErrorNullableString', - suggestions: [ - { - messageId: 'conditionFixCompareNullish', - output: ` -['one', null].filter(x => { - if (Math.random() > 0.5) { - return x; - } - - return x == null; -}); - `, - }, - { - messageId: 'conditionFixDefaultEmptyString', - output: ` -['one', null].filter(x => { - if (Math.random() > 0.5) { - return x; - } - - return !(x ?? ""); -}); - `, - }, - { - messageId: 'conditionFixCastBoolean', - output: ` -['one', null].filter(x => { - if (Math.random() > 0.5) { - return x; - } - - return !Boolean(x); -}); - `, - }, - ], - }, - ], - output: null, - }, - // implicitly return `undefined` - { - code: ` -['one', 'two'].filter(x => { - if (x.length > 2) { - return true; - } -}); - `, - errors: [ - { - column: 23, - endColumn: 2, - endLine: 6, - line: 2, - messageId: 'predicateReturnsUndefined', - }, - ], - }, - { - code: ` -['one', 'two'].filter(x => {}); - `, - errors: [ - { - column: 23, - endColumn: 30, - endLine: 2, - line: 2, - messageId: 'predicateReturnsUndefined', - }, - ], - }, - // implicitly return `undefined` with a return type annotation - { - code: ` -['one', 'two'].filter((x): boolean | undefined => { - if (x.length > 2) { - return true; - } -}); - `, - errors: [ - { - column: 23, - endColumn: 2, - endLine: 6, - line: 2, - messageId: 'predicateReturnsUndefined', - }, - ], - }, - // explicitly returning `undefined` - { - code: ` -['one', 'two'].filter(x => { - return undefined; -}); - `, - errors: [ - { - column: 10, - endColumn: 19, - endLine: 3, - line: 3, - messageId: 'conditionErrorNullish', - }, - ], - }, - { - code: ` -['one', 'two'].filter(x => { - if (Math.random() > 0.5) { - return undefined; - } -}); - `, - errors: [ - { - column: 12, - endColumn: 21, - endLine: 4, - line: 4, - messageId: 'conditionErrorNullish', - }, - ], - }, - // empty return statements - { - code: ` -['one', 'two'].filter(x => { - return; -}); - `, - errors: [ - { - column: 3, - endColumn: 10, - endLine: 3, - line: 3, - messageId: 'predicateReturnsEmptyExpression', - }, - ], - }, - { - code: ` -['one', null].filter(x => { - if (Math.random() > 0.5) { - return; - } - - return x != null; -}); - `, - errors: [ - { - column: 5, - endColumn: 12, - endLine: 4, - line: 4, - messageId: 'predicateReturnsEmptyExpression', - }, - ], - }, - { - code: ` -declare function assert(x: unknown): asserts x; -declare const nullableString: string | null; -assert(nullableString); - `, - errors: [ - { - column: 8, - line: 4, - messageId: 'conditionErrorNullableString', - suggestions: [ - { - messageId: 'conditionFixCompareNullish', - output: ` -declare function assert(x: unknown): asserts x; -declare const nullableString: string | null; -assert(nullableString != null); - `, - }, - { - messageId: 'conditionFixDefaultEmptyString', - output: ` -declare function assert(x: unknown): asserts x; -declare const nullableString: string | null; -assert(nullableString ?? ""); - `, - }, - { - messageId: 'conditionFixCastBoolean', - output: ` -declare function assert(x: unknown): asserts x; -declare const nullableString: string | null; -assert(Boolean(nullableString)); - `, - }, - ], - }, - ], - output: null, - }, - { - code: ` -declare function assert(a: number, b: unknown): asserts b; -declare const nullableString: string | null; -assert(foo, nullableString); - `, - errors: [ - { - column: 13, - line: 4, + column: 13, + line: 4, messageId: 'conditionErrorNullableString', suggestions: [ { @@ -3467,5 +2951,187 @@ assert(Boolean(nullableString)); }, ], }, + { + code: ` +['one', 'two', ''].find(x => { + return x; +}); + `, + errors: [ + { + column: 25, + endColumn: 2, + endLine: 4, + line: 2, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +['one', 'two', ''].find(x => { + return; +}); + `, + errors: [ + { + column: 25, + endColumn: 2, + endLine: 4, + line: 2, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +['one', 'two', ''].findLast(x => { + return undefined; +}); + `, + errors: [ + { + column: 29, + endColumn: 2, + endLine: 4, + line: 2, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +['one', 'two', ''].find(x => { + if (x) { + return true; + } +}); + `, + errors: [ + { + column: 25, + endColumn: 2, + endLine: 6, + line: 2, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +[1, null].every(async x => { + return x != null; +}); + `, + errors: [ + { + column: 17, + data: { + type: 'Promise', + }, + endColumn: 2, + endLine: 4, + line: 2, + messageId: 'predicateCannotBeAsync', + }, + ], + }, + { + code: ` +[1, null].every(async x => { + return true; +}); + `, + errors: [ + { + column: 17, + data: { + type: 'Promise', + }, + endColumn: 2, + endLine: 4, + line: 2, + messageId: 'predicateCannotBeAsync', + }, + ], + }, + { + code: ` +[1, null].every(async x => {}); + `, + errors: [ + { + column: 17, + data: { + type: 'Promise', + }, + endColumn: 30, + endLine: 2, + line: 2, + messageId: 'predicateCannotBeAsync', + }, + ], + }, + { + code: ` +[1, null].every((x): boolean | number => { + return x != null; +}); + `, + errors: [ + { + column: 17, + endColumn: 2, + endLine: 4, + line: 2, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +[1, null].every((x): boolean | undefined => { + return x != null; +}); + `, + errors: [ + { + column: 17, + endColumn: 2, + endLine: 4, + line: 2, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +[1, null].every(x => {}); + `, + errors: [ + { + column: 17, + endColumn: 24, + endLine: 2, + line: 2, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare const predicate: (string) => string; +['one', 'two', ''].findIndex(predicate); + `, + errors: [ + { + column: 30, + endColumn: 39, + endLine: 3, + line: 3, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, ], }); From f71888561f464db00a7b1fa878d1e65c8e6bfdf0 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Sat, 2 Nov 2024 15:41:57 +0200 Subject: [PATCH 17/41] update tests to match the implementation changes --- .../rules/strict-boolean-expressions.test.ts | 174 ++++++++++++++++++ 1 file changed, 174 insertions(+) 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 a49b328498e..40ec605f281 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -2964,6 +2964,16 @@ assert(Boolean(nullableString)); endLine: 4, line: 2, messageId: 'predicateReturnsNonBoolean', + suggestions: [ + { + messageId: 'explicitBooleanReturnType', + output: ` +['one', 'two', ''].find((x): boolean => { + return x; +}); + `, + }, + ], }, ], }, @@ -2980,6 +2990,16 @@ assert(Boolean(nullableString)); endLine: 4, line: 2, messageId: 'predicateReturnsNonBoolean', + suggestions: [ + { + messageId: 'explicitBooleanReturnType', + output: ` +['one', 'two', ''].find((x): boolean => { + return; +}); + `, + }, + ], }, ], }, @@ -2996,6 +3016,16 @@ assert(Boolean(nullableString)); endLine: 4, line: 2, messageId: 'predicateReturnsNonBoolean', + suggestions: [ + { + messageId: 'explicitBooleanReturnType', + output: ` +['one', 'two', ''].findLast((x): boolean => { + return undefined; +}); + `, + }, + ], }, ], }, @@ -3014,6 +3044,18 @@ assert(Boolean(nullableString)); endLine: 6, line: 2, messageId: 'predicateReturnsNonBoolean', + suggestions: [ + { + messageId: 'explicitBooleanReturnType', + output: ` +['one', 'two', ''].find((x): boolean => { + if (x) { + return true; + } +}); + `, + }, + ], }, ], }, @@ -3115,6 +3157,138 @@ assert(Boolean(nullableString)); endLine: 2, line: 2, messageId: 'predicateReturnsNonBoolean', + suggestions: [ + { + messageId: 'explicitBooleanReturnType', + output: ` +[1, null].every((x): boolean => {}); + `, + }, + ], + }, + ], + }, + { + code: ` +[1, null].every((x): boolean | number => {}); + `, + errors: [ + { + column: 17, + endColumn: 44, + endLine: 2, + line: 2, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +[1, null].every((x: number) => {}); + `, + errors: [ + { + column: 17, + endColumn: 34, + endLine: 2, + line: 2, + messageId: 'predicateReturnsNonBoolean', + suggestions: [ + { + messageId: 'explicitBooleanReturnType', + output: ` +[1, null].every((x: number): boolean => {}); + `, + }, + ], + }, + ], + }, + { + code: ` +[1, null].every(() => {}); + `, + errors: [ + { + column: 17, + endColumn: 25, + endLine: 2, + line: 2, + messageId: 'predicateReturnsNonBoolean', + suggestions: [ + { + messageId: 'explicitBooleanReturnType', + output: ` +[1, null].every((): boolean => {}); + `, + }, + ], + }, + ], + }, + { + code: ` +[1, null].every((x, i) => {}); + `, + errors: [ + { + column: 17, + endColumn: 29, + endLine: 2, + line: 2, + messageId: 'predicateReturnsNonBoolean', + suggestions: [ + { + messageId: 'explicitBooleanReturnType', + output: ` +[1, null].every((x, i): boolean => {}); + `, + }, + ], + }, + ], + }, + { + code: ` +[() => {}, null].every((x: () => void) => {}); + `, + errors: [ + { + column: 24, + endColumn: 45, + endLine: 2, + line: 2, + messageId: 'predicateReturnsNonBoolean', + suggestions: [ + { + messageId: 'explicitBooleanReturnType', + output: ` +[() => {}, null].every((x: () => void): boolean => {}); + `, + }, + ], + }, + ], + }, + { + code: ` +[() => {}, null].every(function (x: () => void) {}); + `, + errors: [ + { + column: 24, + endColumn: 51, + endLine: 2, + line: 2, + messageId: 'predicateReturnsNonBoolean', + suggestions: [ + { + messageId: 'explicitBooleanReturnType', + output: ` +[() => {}, null].every(function (x: () => void): boolean {}); + `, + }, + ], }, ], }, From dbc4c7a7ddca1dd1288d4141b542b2e88f2741bb Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Sat, 2 Nov 2024 15:56:39 +0200 Subject: [PATCH 18/41] adjust suggestion message --- packages/eslint-plugin/src/rules/strict-boolean-expressions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 46b1a4e87e3..b1988712dbc 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -129,7 +129,7 @@ export default createRule({ conditionFixDefaultZero: 'Explicitly treat nullish value the same as 0 (`value ?? 0`)', explicitBooleanReturnType: - 'Explicitly return a boolean expression for type accuracy.', + 'Add an explicit boolean return type annotation.', noStrictNullCheck: 'This rule requires the `strictNullChecks` compiler option to be turned on to function correctly.', predicateCannotBeAsync: From f6329dc2fadabcdd8f237556417408a8e67dda85 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Sat, 2 Nov 2024 16:10:27 +0200 Subject: [PATCH 19/41] final adjustments --- .../src/rules/strict-boolean-expressions.ts | 120 +++++++++--------- 1 file changed, 61 insertions(+), 59 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index b1988712dbc..89f4f4cbd25 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -1,6 +1,5 @@ import type { ParserServicesWithTypeInformation, - TSESLint, TSESTree, } from '@typescript-eslint/utils'; @@ -243,7 +242,7 @@ export default createRule({ | TSESTree.IfStatement | TSESTree.WhileStatement; - function isFunctionReturningBoolean(type: ts.Type): boolean { + function isFunctionReturningBooleanType(type: ts.Type): boolean { for (const signature of type.getCallSignatures()) { const returnType = signature.getReturnType(); @@ -264,6 +263,15 @@ export default createRule({ return false; } + function isFunctionExpressionNode( + node: TSESTree.CallExpressionArgument, + ): node is TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression { + return ( + node.type === AST_NODE_TYPES.FunctionExpression || + node.type === AST_NODE_TYPES.ArrowFunctionExpression + ); + } + /** * Inspects condition of a test expression. (`if`, `while`, `for`, etc.) */ @@ -314,12 +322,8 @@ export default createRule({ return; } - // custom-tailored message for accidental `async` function expressions - if ( - (predicate.type === AST_NODE_TYPES.FunctionExpression || - predicate.type === AST_NODE_TYPES.ArrowFunctionExpression) && - predicate.async - ) { + // custom message for accidental `async` function expressions + if (isFunctionExpressionNode(predicate) && predicate.async) { return context.report({ node: predicate, messageId: 'predicateCannotBeAsync', @@ -330,61 +334,59 @@ export default createRule({ services.getTypeAtLocation(predicate), ); - if (!isFunctionReturningBoolean(type)) { - const suggest: TSESLint.ReportSuggestionArray = []; - - if ( - (predicate.type === AST_NODE_TYPES.FunctionExpression || - predicate.type === AST_NODE_TYPES.ArrowFunctionExpression) && - !predicate.returnType - ) { - suggest.push({ - messageId: 'explicitBooleanReturnType', - fix: fixer => { - const lastParam = predicate.params.at(-1); - - // zero parameters - if (!lastParam) { - const closingBracket = nullThrows( - context.sourceCode.getFirstToken( - predicate, - token => token.value === ')', - ), - 'function expression with no arguments must have a closing parenthesis.', - ); - - return fixer.insertTextAfterRange( - closingBracket.range, - ': boolean', - ); - } - - const closingBracket = - context.sourceCode.getTokenAfter(lastParam); - - // one or more parameters wrapped with parens - if (closingBracket?.value === ')') { - return fixer.insertTextAfterRange( - closingBracket.range, - ': boolean', - ); - } - - // one param not wrapped with parens - const parameterText = context.sourceCode.getText(lastParam); - - return fixer.replaceText( - predicate.params[0], - `(${parameterText}): boolean`, - ); - }, - }); - } + if (!isFunctionReturningBooleanType(type)) { + const canFix = + isFunctionExpressionNode(predicate) && !predicate.returnType; return context.report({ node: predicate, messageId: 'predicateReturnsNonBoolean', - suggest, + suggest: canFix + ? [ + { + messageId: 'explicitBooleanReturnType', + fix: fixer => { + const lastParam = predicate.params.at(-1); + + // zero parameters + if (!lastParam) { + const closingBracket = nullThrows( + context.sourceCode.getFirstToken( + predicate, + token => token.value === ')', + ), + 'function expression with no arguments must have a closing parenthesis.', + ); + + return fixer.insertTextAfterRange( + closingBracket.range, + ': boolean', + ); + } + + const closingBracket = + context.sourceCode.getTokenAfter(lastParam); + + // one or more parameters wrapped with parens + if (closingBracket?.value === ')') { + return fixer.insertTextAfterRange( + closingBracket.range, + ': boolean', + ); + } + + // one param not wrapped with parens + const parameterText = + context.sourceCode.getText(lastParam); + + return fixer.replaceText( + predicate.params[0], + `(${parameterText}): boolean`, + ); + }, + }, + ] + : null, }); } } From 3ce22c5c85f40ef355b6a208a38661dfb0dbe1ad Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Sat, 2 Nov 2024 18:30:28 +0200 Subject: [PATCH 20/41] final adjustments #2 --- .../src/rules/strict-boolean-expressions.ts | 136 +++++++++--------- 1 file changed, 71 insertions(+), 65 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 89f4f4cbd25..2b07f808533 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -318,77 +318,83 @@ export default createRule({ if (isArrayMethodCallWithPredicate(context, services, node)) { const predicate = node.arguments.at(0); - if (!predicate) { - return; + if (predicate) { + checkArrayMethodCallPredicate(predicate); } + } + } - // custom message for accidental `async` function expressions - if (isFunctionExpressionNode(predicate) && predicate.async) { - return context.report({ - node: predicate, - messageId: 'predicateCannotBeAsync', - }); - } + /** + * Dedicated function to check array method predicate calls. Reports predicate + * arguments that don't return a boolean value. + * + * Ignores the `allow*` options and requires a boolean value. + */ + function checkArrayMethodCallPredicate( + node: TSESTree.CallExpressionArgument, + ): void { + // custom message for accidental `async` function expressions + if (isFunctionExpressionNode(node) && node.async) { + return context.report({ + node, + messageId: 'predicateCannotBeAsync', + }); + } - const type = checker.getApparentType( - services.getTypeAtLocation(predicate), - ); + const type = checker.getApparentType(services.getTypeAtLocation(node)); + + if (!isFunctionReturningBooleanType(type)) { + const canFix = isFunctionExpressionNode(node) && !node.returnType; - if (!isFunctionReturningBooleanType(type)) { - const canFix = - isFunctionExpressionNode(predicate) && !predicate.returnType; - - return context.report({ - node: predicate, - messageId: 'predicateReturnsNonBoolean', - suggest: canFix - ? [ - { - messageId: 'explicitBooleanReturnType', - fix: fixer => { - const lastParam = predicate.params.at(-1); - - // zero parameters - if (!lastParam) { - const closingBracket = nullThrows( - context.sourceCode.getFirstToken( - predicate, - token => token.value === ')', - ), - 'function expression with no arguments must have a closing parenthesis.', - ); - - return fixer.insertTextAfterRange( - closingBracket.range, - ': boolean', - ); - } - - const closingBracket = - context.sourceCode.getTokenAfter(lastParam); - - // one or more parameters wrapped with parens - if (closingBracket?.value === ')') { - return fixer.insertTextAfterRange( - closingBracket.range, - ': boolean', - ); - } - - // one param not wrapped with parens - const parameterText = - context.sourceCode.getText(lastParam); - - return fixer.replaceText( - predicate.params[0], - `(${parameterText}): boolean`, + return context.report({ + node, + messageId: 'predicateReturnsNonBoolean', + suggest: canFix + ? [ + { + messageId: 'explicitBooleanReturnType', + fix: fixer => { + const lastParam = node.params.at(-1); + + // zero parameters + if (!lastParam) { + const closingBracket = nullThrows( + context.sourceCode.getFirstToken( + node, + token => token.value === ')', + ), + 'function expression with no arguments must have a closing parenthesis.', + ); + + return fixer.insertTextAfterRange( + closingBracket.range, + ': boolean', + ); + } + + const closingBracket = + context.sourceCode.getTokenAfter(lastParam); + + // one or more parameters wrapped with parens + if (closingBracket?.value === ')') { + return fixer.insertTextAfterRange( + closingBracket.range, + ': boolean', ); - }, + } + + // one param not wrapped with parens + const parameterText = context.sourceCode.getText(lastParam); + + return fixer.replaceText( + lastParam, + `(${parameterText}): boolean`, + ); }, - ] - : null, - }); - } + }, + ] + : null, + }); } } From 96ef5c0afda4486a995653712c51b5d7ed3a91ad Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Sat, 2 Nov 2024 18:35:51 +0200 Subject: [PATCH 21/41] test additions --- .../rules/strict-boolean-expressions.test.ts | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) 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 40ec605f281..eac7c11ba16 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -3061,6 +3061,26 @@ assert(Boolean(nullableString)); }, { code: ` +const predicate = (x: string) => { + if (x) { + return true; + } +}; + +['one', 'two', ''].find(predicate); + `, + errors: [ + { + column: 25, + endColumn: 34, + endLine: 8, + line: 8, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` [1, null].every(async x => { return x != null; }); @@ -3080,6 +3100,24 @@ assert(Boolean(nullableString)); }, { code: ` +const predicate = async x => { + return x != null; +}; + +[1, null].every(predicate); + `, + errors: [ + { + column: 17, + endColumn: 26, + endLine: 6, + line: 6, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` [1, null].every(async x => { return true; }); From 403c0119455deb78de5a9fb339c148d0cee9ca38 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Sat, 2 Nov 2024 19:02:58 +0200 Subject: [PATCH 22/41] update snapshots --- .../strict-boolean-expressions.shot | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/strict-boolean-expressions.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/strict-boolean-expressions.shot index e5ca1573a0f..d574832bb3d 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/strict-boolean-expressions.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/strict-boolean-expressions.shot @@ -44,7 +44,7 @@ assert(maybeString); // array predicates return statements are boolean contexts. ['one', null].filter(x => x); - ~ Unexpected nullable string value in conditional. Please handle the nullish/empty cases explicitly. + ~~~~~~ Predicate function should return a boolean. " `; From 339fa3a86724c9b11d64546adc47668d1a2feb80 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Mon, 11 Nov 2024 20:53:41 +0200 Subject: [PATCH 23/41] Update packages/eslint-plugin/docs/rules/strict-boolean-expressions.mdx Co-authored-by: Kirk Waiblinger --- .../eslint-plugin/docs/rules/strict-boolean-expressions.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/strict-boolean-expressions.mdx b/packages/eslint-plugin/docs/rules/strict-boolean-expressions.mdx index 7ee13d86c4d..1dfb27ecc38 100644 --- a/packages/eslint-plugin/docs/rules/strict-boolean-expressions.mdx +++ b/packages/eslint-plugin/docs/rules/strict-boolean-expressions.mdx @@ -63,7 +63,7 @@ declare function assert(value: unknown): asserts value; let maybeString = Math.random() > 0.5 ? '' : undefined; assert(maybeString); -// array predicates return statements are boolean contexts. +// array predicates' return types are boolean contexts. ['one', null].filter(x => x); ``` From 30b3d1cd50c607daa274586b6f1ae4ee1339c8c5 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Wed, 27 Nov 2024 21:38:04 +0200 Subject: [PATCH 24/41] initial implementation of deducing the correct signature for non-function-expressions --- .../src/rules/strict-boolean-expressions.ts | 196 ++++++++++++------ 1 file changed, 131 insertions(+), 65 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 2b07f808533..1ff0576a3c8 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -242,20 +242,18 @@ export default createRule({ | TSESTree.IfStatement | TSESTree.WhileStatement; + function isBooleanType(expressionType: ts.Type): boolean { + return tsutils.isTypeFlagSet( + expressionType, + ts.TypeFlags.Boolean | ts.TypeFlags.BooleanLiteral, + ); + } + function isFunctionReturningBooleanType(type: ts.Type): boolean { for (const signature of type.getCallSignatures()) { const returnType = signature.getReturnType(); - if ( - tsutils - .unionTypeParts(returnType) - .every(t => - tsutils.isTypeFlagSet( - t, - ts.TypeFlags.Boolean | ts.TypeFlags.BooleanLiteral, - ), - ) - ) { + if (tsutils.unionTypeParts(returnType).every(isBooleanType)) { return true; } } @@ -263,6 +261,60 @@ export default createRule({ return false; } + function getMatchingArrayPredicateSignatures( + type: ts.Type, + arrayType: ts.Type, + ): ts.Signature[] { + const signatures: ts.Signature[] = []; + + for (const signature of type.getCallSignatures()) { + const parameters = signature.getParameters(); + + const valueParameter = parameters.at(0); + + if ( + valueParameter && + !checker.isTypeAssignableTo( + arrayType, + checker.getTypeOfSymbol(valueParameter), + ) + ) { + continue; + } + + const indexParameter = parameters.at(1); + + if ( + indexParameter && + !checker.isTypeAssignableTo( + checker.getNumberType(), + checker.getTypeOfSymbol(indexParameter), + ) + ) { + continue; + } + + signatures.push(signature); + } + + return signatures; + } + + function isArrayPredicateFunctionReturningBooleanType( + type: ts.Type, + arrayNode: TSESTree.Expression, + ): boolean { + const [arrayType] = checker.getTypeArguments( + getConstrainedTypeAtLocation(services, arrayNode) as ts.TypeReference, + ); + + const signatures = getMatchingArrayPredicateSignatures(type, arrayType); + + return signatures.every(signature => + tsutils.unionTypeParts(signature.getReturnType()).every(isBooleanType), + ); + } + function isFunctionExpressionNode( node: TSESTree.CallExpressionArgument, ): node is TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression { @@ -317,9 +369,10 @@ export default createRule({ } if (isArrayMethodCallWithPredicate(context, services, node)) { const predicate = node.arguments.at(0); + const arrayNode = (node.callee as TSESTree.MemberExpression).object; if (predicate) { - checkArrayMethodCallPredicate(predicate); + checkArrayMethodCallPredicate(predicate, arrayNode); } } } @@ -331,71 +384,84 @@ export default createRule({ * Ignores the `allow*` options and requires a boolean value. */ function checkArrayMethodCallPredicate( - node: TSESTree.CallExpressionArgument, + predicateNode: TSESTree.CallExpressionArgument, + arrayNode: TSESTree.Expression, ): void { + const isFunctionExpression = isFunctionExpressionNode(predicateNode); + // custom message for accidental `async` function expressions - if (isFunctionExpressionNode(node) && node.async) { + if (isFunctionExpression && predicateNode.async) { return context.report({ - node, + node: predicateNode, messageId: 'predicateCannotBeAsync', }); } - const type = checker.getApparentType(services.getTypeAtLocation(node)); + const type = checker.getApparentType( + services.getTypeAtLocation(predicateNode), + ); - if (!isFunctionReturningBooleanType(type)) { - const canFix = isFunctionExpressionNode(node) && !node.returnType; + if (isFunctionExpression) { + if (isFunctionReturningBooleanType(type)) { + return; + } + } else if ( + isArrayPredicateFunctionReturningBooleanType(type, arrayNode) + ) { + return; + } - return context.report({ - node, - messageId: 'predicateReturnsNonBoolean', - suggest: canFix - ? [ - { - messageId: 'explicitBooleanReturnType', - fix: fixer => { - const lastParam = node.params.at(-1); - - // zero parameters - if (!lastParam) { - const closingBracket = nullThrows( - context.sourceCode.getFirstToken( - node, - token => token.value === ')', - ), - 'function expression with no arguments must have a closing parenthesis.', - ); - - return fixer.insertTextAfterRange( - closingBracket.range, - ': boolean', - ); - } - - const closingBracket = - context.sourceCode.getTokenAfter(lastParam); - - // one or more parameters wrapped with parens - if (closingBracket?.value === ')') { - return fixer.insertTextAfterRange( - closingBracket.range, - ': boolean', - ); - } - - // one param not wrapped with parens - const parameterText = context.sourceCode.getText(lastParam); - - return fixer.replaceText( - lastParam, - `(${parameterText}): boolean`, + const canFix = isFunctionExpression && !predicateNode.returnType; + + return context.report({ + node: predicateNode, + messageId: 'predicateReturnsNonBoolean', + suggest: canFix + ? [ + { + messageId: 'explicitBooleanReturnType', + fix: fixer => { + const lastParam = predicateNode.params.at(-1); + + // zero parameters + if (!lastParam) { + const closingBracket = nullThrows( + context.sourceCode.getFirstToken( + predicateNode, + token => token.value === ')', + ), + 'function expression with no arguments must have a closing parenthesis.', + ); + + return fixer.insertTextAfterRange( + closingBracket.range, + ': boolean', + ); + } + + const closingBracket = + context.sourceCode.getTokenAfter(lastParam); + + // one or more parameters wrapped with parens + if (closingBracket?.value === ')') { + return fixer.insertTextAfterRange( + closingBracket.range, + ': boolean', ); - }, + } + + // one param not wrapped with parens + const parameterText = context.sourceCode.getText(lastParam); + + return fixer.replaceText( + lastParam, + `(${parameterText}): boolean`, + ); }, - ] - : null, - }); - } + }, + ] + : null, + }); } /** From 690fdbc165746f393e95341ee4f61f0e846b43a4 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Wed, 27 Nov 2024 21:43:11 +0200 Subject: [PATCH 25/41] comments --- .../eslint-plugin/src/rules/strict-boolean-expressions.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 1ff0576a3c8..092b3cb8bea 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -261,6 +261,10 @@ export default createRule({ return false; } + /** + * Returns the call signatures that match the type of an array predicate function + * for an array of type `type`. + */ function getMatchingArrayPredicateSignatures( type: ts.Type, arrayType: ts.Type, @@ -272,6 +276,7 @@ export default createRule({ const valueParameter = parameters.at(0); + // value parameter should match the array's type if ( valueParameter && !checker.isTypeAssignableTo( @@ -284,6 +289,7 @@ export default createRule({ const indexParameter = parameters.at(1); + // index parameter should match the number type if ( indexParameter && !checker.isTypeAssignableTo( From d607a005c1aef2aecdd22aa0bf1fa1b36a76fb81 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Wed, 27 Nov 2024 21:57:20 +0200 Subject: [PATCH 26/41] take type constraints into consideration --- .../src/rules/strict-boolean-expressions.ts | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 092b3cb8bea..4366e48ad91 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -261,6 +261,21 @@ export default createRule({ return false; } + function isParameterAssignableTo( + parameter: ts.Symbol, + expected: ts.Type, + ): boolean { + const parameterType = checker.getTypeOfSymbol(parameter); + const constrained = checker.getBaseConstraintOfType(parameterType); + + // treat as if this is `unknown` + if (!constrained) { + return true; + } + + return checker.isTypeAssignableTo(expected, constrained); + } + /** * Returns the call signatures that match the type of an array predicate function * for an array of type `type`. @@ -279,10 +294,7 @@ export default createRule({ // value parameter should match the array's type if ( valueParameter && - !checker.isTypeAssignableTo( - arrayType, - checker.getTypeOfSymbol(valueParameter), - ) + !isParameterAssignableTo(valueParameter, arrayType) ) { continue; } @@ -292,10 +304,7 @@ export default createRule({ // index parameter should match the number type if ( indexParameter && - !checker.isTypeAssignableTo( - checker.getNumberType(), - checker.getTypeOfSymbol(indexParameter), - ) + !isParameterAssignableTo(indexParameter, checker.getNumberType()) ) { continue; } @@ -411,9 +420,9 @@ export default createRule({ if (isFunctionReturningBooleanType(type)) { return; } - } else if ( - isArrayPredicateFunctionReturningBooleanType(type, arrayNode) - ) { + } + // Check only call signatures that match those of an array predicate + else if (isArrayPredicateFunctionReturningBooleanType(type, arrayNode)) { return; } From f5c7300f225374467f8e10eed4e7814a5a37de35 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Wed, 27 Nov 2024 22:48:09 +0200 Subject: [PATCH 27/41] only check type constraints on type parameters --- .../src/rules/strict-boolean-expressions.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 4366e48ad91..913f6e703c4 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -261,14 +261,22 @@ export default createRule({ return false; } + function isTypeParameter(type: ts.Type): boolean { + // https://github.com/JoshuaKGoldberg/ts-api-utils/issues/382 + return (tsutils.isTypeParameter as (type: ts.Type) => boolean)(type); + } + function isParameterAssignableTo( parameter: ts.Symbol, expected: ts.Type, ): boolean { const parameterType = checker.getTypeOfSymbol(parameter); - const constrained = checker.getBaseConstraintOfType(parameterType); - // treat as if this is `unknown` + const constrained = isTypeParameter(parameterType) + ? checker.getBaseConstraintOfType(parameterType) + : parameterType; + + // treat a type parameter without constraint as `unknown` if (!constrained) { return true; } From 287122e1068a12280815a9a44edb8be941bd5214 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Wed, 27 Nov 2024 22:59:48 +0200 Subject: [PATCH 28/41] update tests --- .../rules/strict-boolean-expressions.test.ts | 125 ++++++++++++++++++ 1 file changed, 125 insertions(+) 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 eac7c11ba16..4c6eaffa916 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -562,6 +562,45 @@ assert(nullableString); declare const predicate: (string) => boolean; ['one', 'two', ''].filter(predicate); `, + ` +declare function notNullish(x: T): x is NonNullable; +['one', null].filter(notNullish); + `, + ` +declare function isString(x: string | null): x is string; +['one', null].filter(isString); + `, + ` +declare function f(x: number): string; +declare function f(x: string | null): boolean; + +['one', null].filter(f); + `, + ` +declare function f(x: number): string; +declare function f(x: string | boolean | null): boolean; + +['one', null].filter(f); + `, + ` +declare function f(x: number): string; +declare function f(x: string | boolean | null): boolean; +declare function f(x: string | null): boolean; + +['one', null].filter(f); + `, + ` +declare function f(x: number): string; +declare function f(x: T): boolean; + +['one', null].filter(f); + `, + ` +declare function f(x: number): string; +declare function f(x: T): boolean; + +['one', null].filter(f); + `, ], invalid: [ @@ -3345,5 +3384,91 @@ declare const predicate: (string) => string; }, ], }, + { + code: ` +declare function f(x: number): string; +declare function f(x: string | null): boolean; + +[35].filter(f); + `, + errors: [ + { + column: 13, + endColumn: 14, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: number): string; +declare function f(x: number | boolean): boolean; +declare function f(x: string | null): boolean; + +[35].filter(f); + `, + errors: [ + { + column: 13, + endColumn: 14, + endLine: 6, + line: 6, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: T): string; +declare function f(x: string | null): boolean; + +[35].filter(f); + `, + errors: [ + { + column: 13, + endColumn: 14, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: T): string; +declare function f(x: string | null): boolean; + +[35].filter(f); + `, + errors: [ + { + column: 13, + endColumn: 14, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: T): string; +declare function f(x: string | null): boolean; + +[35].filter(f); + `, + errors: [ + { + column: 13, + endColumn: 14, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, ], }); From 7566d0bf1bf1bf1fe8c72842b39644d0a91a91cf Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Wed, 27 Nov 2024 23:27:24 +0200 Subject: [PATCH 29/41] update index tests --- .../rules/strict-boolean-expressions.test.ts | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) 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 4c6eaffa916..e85bd6f42d4 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -599,6 +599,24 @@ declare function f(x: T): boolean; declare function f(x: number): string; declare function f(x: T): boolean; +['one', null].filter(f); + `, + ` +declare function f(x: number, i: number): string; +declare function f(x: string | null): boolean; + +['one', null].filter(f); + `, + ` +declare function f(x: number, i: number): string; +declare function f(x: string | null, i: number): boolean; + +['one', null].filter(f); + `, + ` +declare function f(x: number): string; +declare function f(x: string | null, i: number | boolean): boolean; + ['one', null].filter(f); `, ], @@ -3458,6 +3476,74 @@ declare function f(x: string | null): boolean; declare function f(x: T): string; declare function f(x: string | null): boolean; +[35].filter(f); + `, + errors: [ + { + column: 13, + endColumn: 14, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: number, i: number): string; +declare function f(x: string | null): boolean; + +[35].filter(f); + `, + errors: [ + { + column: 13, + endColumn: 14, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: number, i: number): string; +declare function f(x: number | null, i: string): boolean; + +[35].filter(f); + `, + errors: [ + { + column: 13, + endColumn: 14, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: number, i: number | boolean): string; +declare function f(x: number | null, i: string): boolean; + +[35].filter(f); + `, + errors: [ + { + column: 13, + endColumn: 14, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: number): string; +declare function f(x: number, i: string): boolean; + [35].filter(f); `, errors: [ From d472c86ce0a8be33b3efc83caeee0264855a538e Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Wed, 27 Nov 2024 23:28:46 +0200 Subject: [PATCH 30/41] update snapshot --- .../strict-boolean-expressions.shot | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/strict-boolean-expressions.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/strict-boolean-expressions.shot index d574832bb3d..bd33701cf5a 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/strict-boolean-expressions.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/strict-boolean-expressions.shot @@ -42,7 +42,7 @@ let maybeString = Math.random() > 0.5 ? '' : undefined; assert(maybeString); ~~~~~~~~~~~ Unexpected nullable string value in conditional. Please handle the nullish/empty cases explicitly. -// array predicates return statements are boolean contexts. +// array predicates' return types are boolean contexts. ['one', null].filter(x => x); ~~~~~~ Predicate function should return a boolean. " From 00b8424849fd7c9e1a4f6052a5813df1e839bf91 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Thu, 28 Nov 2024 09:11:09 +0200 Subject: [PATCH 31/41] simplify code a bit --- .../src/rules/strict-boolean-expressions.ts | 51 ++++++------------- 1 file changed, 16 insertions(+), 35 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 913f6e703c4..7083be2f082 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -249,18 +249,6 @@ export default createRule({ ); } - function isFunctionReturningBooleanType(type: ts.Type): boolean { - for (const signature of type.getCallSignatures()) { - const returnType = signature.getReturnType(); - - if (tsutils.unionTypeParts(returnType).every(isBooleanType)) { - return true; - } - } - - return false; - } - function isTypeParameter(type: ts.Type): boolean { // https://github.com/JoshuaKGoldberg/ts-api-utils/issues/382 return (tsutils.isTypeParameter as (type: ts.Type) => boolean)(type); @@ -290,8 +278,12 @@ export default createRule({ */ function getMatchingArrayPredicateSignatures( type: ts.Type, - arrayType: ts.Type, + arrayNode: TSESTree.Expression, ): ts.Signature[] { + const [arrayType] = checker.getTypeArguments( + getConstrainedTypeAtLocation(services, arrayNode) as ts.TypeReference, + ); + const signatures: ts.Signature[] = []; for (const signature of type.getCallSignatures()) { @@ -323,21 +315,6 @@ export default createRule({ return signatures; } - function isArrayPredicateFunctionReturningBooleanType( - type: ts.Type, - arrayNode: TSESTree.Expression, - ): boolean { - const [arrayType] = checker.getTypeArguments( - getConstrainedTypeAtLocation(services, arrayNode) as ts.TypeReference, - ); - - const signatures = getMatchingArrayPredicateSignatures(type, arrayType); - - return signatures.every(signature => - tsutils.unionTypeParts(signature.getReturnType()).every(isBooleanType), - ); - } - function isFunctionExpressionNode( node: TSESTree.CallExpressionArgument, ): node is TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression { @@ -424,13 +401,17 @@ export default createRule({ services.getTypeAtLocation(predicateNode), ); - if (isFunctionExpression) { - if (isFunctionReturningBooleanType(type)) { - return; - } - } - // Check only call signatures that match those of an array predicate - else if (isArrayPredicateFunctionReturningBooleanType(type, arrayNode)) { + const signatures = isFunctionExpression + ? type.getCallSignatures() + : getMatchingArrayPredicateSignatures(type, arrayNode); + + if ( + signatures.every(signature => + tsutils + .unionTypeParts(signature.getReturnType()) + .every(isBooleanType), + ) + ) { return; } From 5e5973328ee59acf97dbd546cb726c3574a5a2ee Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Tue, 17 Dec 2024 21:08:39 +0200 Subject: [PATCH 32/41] remove overly complex heuristic --- .../src/rules/strict-boolean-expressions.ts | 95 ++----------------- 1 file changed, 10 insertions(+), 85 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 7083be2f082..ad7cb6856aa 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -242,79 +242,6 @@ export default createRule({ | TSESTree.IfStatement | TSESTree.WhileStatement; - function isBooleanType(expressionType: ts.Type): boolean { - return tsutils.isTypeFlagSet( - expressionType, - ts.TypeFlags.Boolean | ts.TypeFlags.BooleanLiteral, - ); - } - - function isTypeParameter(type: ts.Type): boolean { - // https://github.com/JoshuaKGoldberg/ts-api-utils/issues/382 - return (tsutils.isTypeParameter as (type: ts.Type) => boolean)(type); - } - - function isParameterAssignableTo( - parameter: ts.Symbol, - expected: ts.Type, - ): boolean { - const parameterType = checker.getTypeOfSymbol(parameter); - - const constrained = isTypeParameter(parameterType) - ? checker.getBaseConstraintOfType(parameterType) - : parameterType; - - // treat a type parameter without constraint as `unknown` - if (!constrained) { - return true; - } - - return checker.isTypeAssignableTo(expected, constrained); - } - - /** - * Returns the call signatures that match the type of an array predicate function - * for an array of type `type`. - */ - function getMatchingArrayPredicateSignatures( - type: ts.Type, - arrayNode: TSESTree.Expression, - ): ts.Signature[] { - const [arrayType] = checker.getTypeArguments( - getConstrainedTypeAtLocation(services, arrayNode) as ts.TypeReference, - ); - - const signatures: ts.Signature[] = []; - - for (const signature of type.getCallSignatures()) { - const parameters = signature.getParameters(); - - const valueParameter = parameters.at(0); - - // value parameter should match the array's type - if ( - valueParameter && - !isParameterAssignableTo(valueParameter, arrayType) - ) { - continue; - } - - const indexParameter = parameters.at(1); - - // index parameter should match the number type - if ( - indexParameter && - !isParameterAssignableTo(indexParameter, checker.getNumberType()) - ) { - continue; - } - - signatures.push(signature); - } - - return signatures; - } - function isFunctionExpressionNode( node: TSESTree.CallExpressionArgument, ): node is TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression { @@ -369,10 +296,9 @@ export default createRule({ } if (isArrayMethodCallWithPredicate(context, services, node)) { const predicate = node.arguments.at(0); - const arrayNode = (node.callee as TSESTree.MemberExpression).object; if (predicate) { - checkArrayMethodCallPredicate(predicate, arrayNode); + checkArrayMethodCallPredicate(predicate); } } } @@ -385,7 +311,6 @@ export default createRule({ */ function checkArrayMethodCallPredicate( predicateNode: TSESTree.CallExpressionArgument, - arrayNode: TSESTree.Expression, ): void { const isFunctionExpression = isFunctionExpressionNode(predicateNode); @@ -401,9 +326,7 @@ export default createRule({ services.getTypeAtLocation(predicateNode), ); - const signatures = isFunctionExpression - ? type.getCallSignatures() - : getMatchingArrayPredicateSignatures(type, arrayNode); + const signatures = type.getCallSignatures(); if ( signatures.every(signature => @@ -1198,11 +1121,13 @@ function isArrayLengthExpression( function isBrandedBoolean(type: ts.Type): boolean { return ( type.isIntersection() && - type.types.some(childType => - tsutils.isTypeFlagSet( - childType, - ts.TypeFlags.BooleanLiteral | ts.TypeFlags.Boolean, - ), - ) + type.types.some(childType => isBooleanType(childType)) + ); +} + +function isBooleanType(expressionType: ts.Type): boolean { + return tsutils.isTypeFlagSet( + expressionType, + ts.TypeFlags.Boolean | ts.TypeFlags.BooleanLiteral, ); } From d2a254ade938d3cfb402749c2c30a227ca6efc60 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Tue, 17 Dec 2024 21:09:03 +0200 Subject: [PATCH 33/41] use an existing helper rather than implementing one --- .../src/rules/strict-boolean-expressions.ts | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index ad7cb6856aa..2e7138ff530 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -3,7 +3,7 @@ import type { TSESTree, } from '@typescript-eslint/utils'; -import { AST_NODE_TYPES } from '@typescript-eslint/utils'; +import { AST_NODE_TYPES, ASTUtils } from '@typescript-eslint/utils'; import * as tsutils from 'ts-api-utils'; import * as ts from 'typescript'; @@ -242,15 +242,6 @@ export default createRule({ | TSESTree.IfStatement | TSESTree.WhileStatement; - function isFunctionExpressionNode( - node: TSESTree.CallExpressionArgument, - ): node is TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression { - return ( - node.type === AST_NODE_TYPES.FunctionExpression || - node.type === AST_NODE_TYPES.ArrowFunctionExpression - ); - } - /** * Inspects condition of a test expression. (`if`, `while`, `for`, etc.) */ @@ -312,7 +303,7 @@ export default createRule({ function checkArrayMethodCallPredicate( predicateNode: TSESTree.CallExpressionArgument, ): void { - const isFunctionExpression = isFunctionExpressionNode(predicateNode); + const isFunctionExpression = ASTUtils.isFunction(predicateNode); // custom message for accidental `async` function expressions if (isFunctionExpression && predicateNode.async) { From 6b32e50b292988f1c69821c318d7c188d9f3607e Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Tue, 17 Dec 2024 21:22:56 +0200 Subject: [PATCH 34/41] update tests --- .../rules/strict-boolean-expressions.test.ts | 201 +++++++++++++----- 1 file changed, 152 insertions(+), 49 deletions(-) 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 e85bd6f42d4..d5f13e62062 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -570,55 +570,6 @@ declare function notNullish(x: T): x is NonNullable; declare function isString(x: string | null): x is string; ['one', null].filter(isString); `, - ` -declare function f(x: number): string; -declare function f(x: string | null): boolean; - -['one', null].filter(f); - `, - ` -declare function f(x: number): string; -declare function f(x: string | boolean | null): boolean; - -['one', null].filter(f); - `, - ` -declare function f(x: number): string; -declare function f(x: string | boolean | null): boolean; -declare function f(x: string | null): boolean; - -['one', null].filter(f); - `, - ` -declare function f(x: number): string; -declare function f(x: T): boolean; - -['one', null].filter(f); - `, - ` -declare function f(x: number): string; -declare function f(x: T): boolean; - -['one', null].filter(f); - `, - ` -declare function f(x: number, i: number): string; -declare function f(x: string | null): boolean; - -['one', null].filter(f); - `, - ` -declare function f(x: number, i: number): string; -declare function f(x: string | null, i: number): boolean; - -['one', null].filter(f); - `, - ` -declare function f(x: number): string; -declare function f(x: string | null, i: number | boolean): boolean; - -['one', null].filter(f); - `, ], invalid: [ @@ -3556,5 +3507,157 @@ declare function f(x: number, i: string): boolean; }, ], }, + { + code: ` +declare function f(x: number): string; +declare function f(x: string | null): boolean; + +['one', null].filter(f); + `, + errors: [ + { + column: 22, + endColumn: 23, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: number): string; +declare function f(x: string | boolean | null): boolean; + +['one', null].filter(f); + `, + errors: [ + { + column: 22, + endColumn: 23, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: number): string; +declare function f(x: string | boolean | null): boolean; +declare function f(x: string | null): boolean; + +['one', null].filter(f); + `, + errors: [ + { + column: 22, + endColumn: 23, + endLine: 6, + line: 6, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: number): string; +declare function f(x: T): boolean; + +['one', null].filter(f); + `, + errors: [ + { + column: 22, + endColumn: 23, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: number): string; +declare function f(x: T): boolean; + +['one', null].filter(f); + `, + errors: [ + { + column: 22, + endColumn: 23, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: number, i: number): string; +declare function f(x: string | null): boolean; + +['one', null].filter(f); + `, + errors: [ + { + column: 22, + endColumn: 23, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: number, i: number): string; +declare function f(x: string | null, i: number): boolean; + +['one', null].filter(f); + `, + errors: [ + { + column: 22, + endColumn: 23, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +declare function f(x: number): string; +declare function f(x: string | null, i: number | boolean): boolean; + +['one', null].filter(f); + `, + errors: [ + { + column: 22, + endColumn: 23, + endLine: 5, + line: 5, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +function foo(x: number) {} +[1, null].every(foo); + `, + errors: [ + { + column: 17, + endColumn: 20, + endLine: 3, + line: 3, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, ], }); From c97d53c291ba4cd0dbd223adba0049e01f515dc4 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Tue, 17 Dec 2024 21:28:47 +0200 Subject: [PATCH 35/41] Update packages/eslint-plugin/src/rules/strict-boolean-expressions.ts Co-authored-by: Kirk Waiblinger --- packages/eslint-plugin/src/rules/strict-boolean-expressions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 2e7138ff530..f036f9fca4a 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -128,7 +128,7 @@ export default createRule({ conditionFixDefaultZero: 'Explicitly treat nullish value the same as 0 (`value ?? 0`)', explicitBooleanReturnType: - 'Add an explicit boolean return type annotation.', + 'Add an explicit `boolean` return type annotation.', noStrictNullCheck: 'This rule requires the `strictNullChecks` compiler option to be turned on to function correctly.', predicateCannotBeAsync: From ac6be7e80c47ec9b73a610cce4a73f981d33361e Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Tue, 17 Dec 2024 22:01:00 +0200 Subject: [PATCH 36/41] fix codecov --- .../eslint-plugin/src/rules/strict-boolean-expressions.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index f036f9fca4a..5d6979369f8 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -357,11 +357,13 @@ export default createRule({ ); } - const closingBracket = - context.sourceCode.getTokenAfter(lastParam); + const closingBracket = nullThrows( + context.sourceCode.getTokenAfter(lastParam), + 'missing token following function parameters.', + ); // one or more parameters wrapped with parens - if (closingBracket?.value === ')') { + if (closingBracket.value === ')') { return fixer.insertTextAfterRange( closingBracket.range, ': boolean', From e94c0ee860fd5128ad58f4a85a68825410a36050 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Thu, 19 Dec 2024 20:33:34 +0200 Subject: [PATCH 37/41] cleanup old code, support type constraints --- .../src/rules/strict-boolean-expressions.ts | 23 +++++------ .../rules/strict-boolean-expressions.test.ts | 38 +++++++++++++++++++ 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 5d6979369f8..398ad571952 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -313,19 +313,20 @@ export default createRule({ }); } - const type = checker.getApparentType( - services.getTypeAtLocation(predicateNode), - ); + const returnTypes = services + .getTypeAtLocation(predicateNode) + .getCallSignatures() + .map(signature => { + const type = signature.getReturnType(); + + if (tsutils.isTypeParameter(type)) { + return checker.getBaseConstraintOfType(type) ?? type; + } - const signatures = type.getCallSignatures(); + return type; + }); - if ( - signatures.every(signature => - tsutils - .unionTypeParts(signature.getReturnType()) - .every(isBooleanType), - ) - ) { + if (returnTypes.every(returnType => isBooleanType(returnType))) { return; } 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 d5f13e62062..aecbf4bde29 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -570,6 +570,14 @@ declare function notNullish(x: T): x is NonNullable; declare function isString(x: string | null): x is string; ['one', null].filter(isString); `, + ` +declare function isString(x: string | null): T; +['one', null].filter(isString); + `, + ` +declare function makePredicate boolean>(): T; +['one', null].filter(makePredicate()); + `, ], invalid: [ @@ -3647,6 +3655,36 @@ declare function f(x: string | null, i: number | boolean): boolean; { code: ` function foo(x: number) {} +[1, null].every(foo); + `, + errors: [ + { + column: 17, + endColumn: 20, + endLine: 3, + line: 3, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +function foo(x: number): T {} +[1, null].every(foo); + `, + errors: [ + { + column: 17, + endColumn: 20, + endLine: 3, + line: 3, + messageId: 'predicateReturnsNonBoolean', + }, + ], + }, + { + code: ` +function foo(x: number): T {} [1, null].every(foo); `, errors: [ From 601dfbaf88722a59e3d808c431363d0d947b5a3a Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Thu, 19 Dec 2024 21:15:04 +0200 Subject: [PATCH 38/41] refactor fixer --- .../src/rules/strict-boolean-expressions.ts | 52 ++++++++++--------- .../use-unknown-in-catch-callback-variable.ts | 7 ++- packages/eslint-plugin/src/util/misc.ts | 6 +-- 3 files changed, 35 insertions(+), 30 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 398ad571952..53a61cf88cf 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -13,6 +13,7 @@ import { getParserServices, getWrappingFixer, isArrayMethodCallWithPredicate, + isParenlessFunctionExpression, isTypeArrayTypeOrUnionOfArrayTypes, nullThrows, } from '../util'; @@ -340,43 +341,44 @@ export default createRule({ { messageId: 'explicitBooleanReturnType', fix: fixer => { - const lastParam = predicateNode.params.at(-1); + if ( + isParenlessFunctionExpression( + predicateNode, + context.sourceCode, + ) + ) { + return [ + fixer.insertTextBefore(predicateNode.params[0], '('), + fixer.insertTextAfter( + predicateNode.params[0], + '): boolean', + ), + ]; + } - // zero parameters - if (!lastParam) { + if (predicateNode.params.length === 0) { const closingBracket = nullThrows( context.sourceCode.getFirstToken( predicateNode, token => token.value === ')', ), - 'function expression with no arguments must have a closing parenthesis.', + 'function expression has to have a closing parenthesis.', ); - return fixer.insertTextAfterRange( - closingBracket.range, - ': boolean', - ); + return fixer.insertTextAfter(closingBracket, ': boolean'); } - const closingBracket = nullThrows( - context.sourceCode.getTokenAfter(lastParam), - 'missing token following function parameters.', + const lastClosingParenthesis = nullThrows( + context.sourceCode.getTokenAfter( + predicateNode.params[predicateNode.params.length - 1], + token => token.value === ')', + ), + 'function expression has to have a closing parenthesis.', ); - // one or more parameters wrapped with parens - if (closingBracket.value === ')') { - return fixer.insertTextAfterRange( - closingBracket.range, - ': boolean', - ); - } - - // one param not wrapped with parens - const parameterText = context.sourceCode.getText(lastParam); - - return fixer.replaceText( - lastParam, - `(${parameterText}): boolean`, + return fixer.insertTextAfter( + lastClosingParenthesis, + ': boolean', ); }, }, diff --git a/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts b/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts index a7d2c53b1a2..d58fb9ac349 100644 --- a/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts +++ b/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts @@ -9,7 +9,7 @@ import { createRule, getParserServices, getStaticMemberAccessValue, - isParenlessArrowFunction, + isParenlessFunctionExpression, isRestParameterDeclaration, nullThrows, } from '../util'; @@ -155,7 +155,10 @@ export default createRule<[], MessageIds>({ if ( argument.type === AST_NODE_TYPES.ArrowFunctionExpression && - isParenlessArrowFunction(argument, context.sourceCode) + isParenlessFunctionExpression( + argument, + context.sourceCode, + ) ) { return [ fixer.insertTextBefore(catchVariableInner, '('), diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index 4e65d228713..38b8aacf075 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -224,8 +224,8 @@ function isRestParameterDeclaration(decl: ts.Declaration): boolean { return ts.isParameter(decl) && decl.dotDotDotToken != null; } -function isParenlessArrowFunction( - node: TSESTree.ArrowFunctionExpression, +function isParenlessFunctionExpression( + node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, sourceCode: TSESLint.SourceCode, ): boolean { return ( @@ -335,7 +335,7 @@ export { getNameFromMember, getStaticMemberAccessValue, isDefinitionFile, - isParenlessArrowFunction, + isParenlessFunctionExpression, isRestParameterDeclaration, isStaticMemberAccessOfValue, MemberNameType, From 1994ba64838c0260010cb77e5a8eb1282b882f54 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Thu, 19 Dec 2024 21:30:04 +0200 Subject: [PATCH 39/41] remove unnecessary tests --- .../rules/strict-boolean-expressions.test.ts | 434 ++---------------- 1 file changed, 34 insertions(+), 400 deletions(-) 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 aecbf4bde29..4b7d772c8cc 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -527,6 +527,16 @@ assert(nullableString); }); `, ` +[true, false].some(function check(x) { + return x; +}); + `, + ` +[true, false].some(x => { + return x; +}); + `, + ` [1, null].filter(function (x) { return x != null; }); @@ -541,7 +551,6 @@ assert(nullableString); return !!x; }); `, - // implicit return statements should be warned by TS's `noImplicitReturns` ` ['one', 'two', ''].filter(function (x): boolean { if (x) { @@ -567,16 +576,18 @@ declare function notNullish(x: T): x is NonNullable; ['one', null].filter(notNullish); `, ` -declare function isString(x: string | null): x is string; -['one', null].filter(isString); +declare function predicate(x: string | null): x is string; +['one', null].filter(predicate); `, ` -declare function isString(x: string | null): T; -['one', null].filter(isString); +declare function predicate(x: string | null): T; +['one', null].filter(predicate); `, ` -declare function makePredicate boolean>(): T; -['one', null].filter(makePredicate()); +declare function f(x: number): boolean; +declare function f(x: string | null): boolean; + +[35].filter(f); `, ], @@ -3134,42 +3145,6 @@ const predicate = async x => { }, { code: ` -[1, null].every(async x => { - return true; -}); - `, - errors: [ - { - column: 17, - data: { - type: 'Promise', - }, - endColumn: 2, - endLine: 4, - line: 2, - messageId: 'predicateCannotBeAsync', - }, - ], - }, - { - code: ` -[1, null].every(async x => {}); - `, - errors: [ - { - column: 17, - data: { - type: 'Promise', - }, - endColumn: 30, - endLine: 2, - line: 2, - messageId: 'predicateCannotBeAsync', - }, - ], - }, - { - code: ` [1, null].every((x): boolean | number => { return x != null; }); @@ -3200,86 +3175,7 @@ const predicate = async x => { }, ], }, - { - code: ` -[1, null].every(x => {}); - `, - errors: [ - { - column: 17, - endColumn: 24, - endLine: 2, - line: 2, - messageId: 'predicateReturnsNonBoolean', - suggestions: [ - { - messageId: 'explicitBooleanReturnType', - output: ` -[1, null].every((x): boolean => {}); - `, - }, - ], - }, - ], - }, - { - code: ` -[1, null].every((x): boolean | number => {}); - `, - errors: [ - { - column: 17, - endColumn: 44, - endLine: 2, - line: 2, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -[1, null].every((x: number) => {}); - `, - errors: [ - { - column: 17, - endColumn: 34, - endLine: 2, - line: 2, - messageId: 'predicateReturnsNonBoolean', - suggestions: [ - { - messageId: 'explicitBooleanReturnType', - output: ` -[1, null].every((x: number): boolean => {}); - `, - }, - ], - }, - ], - }, - { - code: ` -[1, null].every(() => {}); - `, - errors: [ - { - column: 17, - endColumn: 25, - endLine: 2, - line: 2, - messageId: 'predicateReturnsNonBoolean', - suggestions: [ - { - messageId: 'explicitBooleanReturnType', - output: ` -[1, null].every((): boolean => {}); - `, - }, - ], - }, - ], - }, + // various cases for the suggestion fix { code: ` [1, null].every((x, i) => {}); @@ -3348,19 +3244,27 @@ const predicate = async x => { }, { code: ` -declare const predicate: (string) => string; -['one', 'two', ''].findIndex(predicate); +[() => {}, null].every(() => {}); `, errors: [ { - column: 30, - endColumn: 39, - endLine: 3, - line: 3, + column: 24, + endColumn: 32, + endLine: 2, + line: 2, messageId: 'predicateReturnsNonBoolean', + suggestions: [ + { + messageId: 'explicitBooleanReturnType', + output: ` +[() => {}, null].every((): boolean => {}); + `, + }, + ], }, ], }, + // function overloading { code: ` declare function f(x: number): string; @@ -3396,277 +3300,7 @@ declare function f(x: string | null): boolean; }, ], }, - { - code: ` -declare function f(x: T): string; -declare function f(x: string | null): boolean; - -[35].filter(f); - `, - errors: [ - { - column: 13, - endColumn: 14, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: T): string; -declare function f(x: string | null): boolean; - -[35].filter(f); - `, - errors: [ - { - column: 13, - endColumn: 14, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: T): string; -declare function f(x: string | null): boolean; - -[35].filter(f); - `, - errors: [ - { - column: 13, - endColumn: 14, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: number, i: number): string; -declare function f(x: string | null): boolean; - -[35].filter(f); - `, - errors: [ - { - column: 13, - endColumn: 14, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: number, i: number): string; -declare function f(x: number | null, i: string): boolean; - -[35].filter(f); - `, - errors: [ - { - column: 13, - endColumn: 14, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: number, i: number | boolean): string; -declare function f(x: number | null, i: string): boolean; - -[35].filter(f); - `, - errors: [ - { - column: 13, - endColumn: 14, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: number): string; -declare function f(x: number, i: string): boolean; - -[35].filter(f); - `, - errors: [ - { - column: 13, - endColumn: 14, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: number): string; -declare function f(x: string | null): boolean; - -['one', null].filter(f); - `, - errors: [ - { - column: 22, - endColumn: 23, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: number): string; -declare function f(x: string | boolean | null): boolean; - -['one', null].filter(f); - `, - errors: [ - { - column: 22, - endColumn: 23, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: number): string; -declare function f(x: string | boolean | null): boolean; -declare function f(x: string | null): boolean; - -['one', null].filter(f); - `, - errors: [ - { - column: 22, - endColumn: 23, - endLine: 6, - line: 6, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: number): string; -declare function f(x: T): boolean; - -['one', null].filter(f); - `, - errors: [ - { - column: 22, - endColumn: 23, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: number): string; -declare function f(x: T): boolean; - -['one', null].filter(f); - `, - errors: [ - { - column: 22, - endColumn: 23, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: number, i: number): string; -declare function f(x: string | null): boolean; - -['one', null].filter(f); - `, - errors: [ - { - column: 22, - endColumn: 23, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: number, i: number): string; -declare function f(x: string | null, i: number): boolean; - -['one', null].filter(f); - `, - errors: [ - { - column: 22, - endColumn: 23, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -declare function f(x: number): string; -declare function f(x: string | null, i: number | boolean): boolean; - -['one', null].filter(f); - `, - errors: [ - { - column: 22, - endColumn: 23, - endLine: 5, - line: 5, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, - { - code: ` -function foo(x: number) {} -[1, null].every(foo); - `, - errors: [ - { - column: 17, - endColumn: 20, - endLine: 3, - line: 3, - messageId: 'predicateReturnsNonBoolean', - }, - ], - }, + // type constraints { code: ` function foo(x: number): T {} From 8e54722aef9c603544b5bd463901c22269e0f032 Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Thu, 26 Dec 2024 11:51:18 +0200 Subject: [PATCH 40/41] revert changes to isParenlessArrowFunction --- .../src/rules/strict-boolean-expressions.ts | 9 ++++----- packages/eslint-plugin/src/util/misc.ts | 6 +++--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 1510acb7c8a..216929d6263 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -13,7 +13,7 @@ import { getParserServices, getWrappingFixer, isArrayMethodCallWithPredicate, - isParenlessFunctionExpression, + isParenlessArrowFunction, isTypeArrayTypeOrUnionOfArrayTypes, nullThrows, } from '../util'; @@ -342,10 +342,9 @@ export default createRule({ messageId: 'explicitBooleanReturnType', fix: fixer => { if ( - isParenlessFunctionExpression( - predicateNode, - context.sourceCode, - ) + predicateNode.type === + AST_NODE_TYPES.ArrowFunctionExpression && + isParenlessArrowFunction(predicateNode, context.sourceCode) ) { return [ fixer.insertTextBefore(predicateNode.params[0], '('), diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index d17628097b5..c75a000487f 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -225,8 +225,8 @@ function isRestParameterDeclaration(decl: ts.Declaration): boolean { return ts.isParameter(decl) && decl.dotDotDotToken != null; } -function isParenlessFunctionExpression( - node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, +function isParenlessArrowFunction( + node: TSESTree.ArrowFunctionExpression, sourceCode: TSESLint.SourceCode, ): boolean { return ( @@ -336,7 +336,7 @@ export { getNameFromMember, getStaticMemberAccessValue, isDefinitionFile, - isParenlessFunctionExpression, + isParenlessArrowFunction, isRestParameterDeclaration, isStaticMemberAccessOfValue, MemberNameType, From f17a05ce6d1f67d2cca80e5a016850b480b2425f Mon Sep 17 00:00:00 2001 From: Ronen Amiel Date: Thu, 26 Dec 2024 12:05:32 +0200 Subject: [PATCH 41/41] oops --- .../src/rules/use-unknown-in-catch-callback-variable.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts b/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts index e7a11329fb5..0ee671556d1 100644 --- a/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts +++ b/packages/eslint-plugin/src/rules/use-unknown-in-catch-callback-variable.ts @@ -9,7 +9,7 @@ import { createRule, getParserServices, getStaticMemberAccessValue, - isParenlessFunctionExpression, + isParenlessArrowFunction, isRestParameterDeclaration, nullThrows, } from '../util'; @@ -176,10 +176,7 @@ export default createRule<[], MessageIds>({ if ( argument.type === AST_NODE_TYPES.ArrowFunctionExpression && - isParenlessFunctionExpression( - argument, - context.sourceCode, - ) + isParenlessArrowFunction(argument, context.sourceCode) ) { return [ fixer.insertTextBefore(catchVariableInner, '('),