diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md index ae1e8eb0aa0..f2bea6f4fd8 100644 --- a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md +++ b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md @@ -74,6 +74,19 @@ for (; true; ) {} do {} while (true); ``` +- `checkArrayPredicates` (default: `false`) - if set checks that the return value from certain array method callbacks (`filter`, `find`, `some`, `every`) is necessarily conditional. + +```ts +// Valid: numbers can be truthy or falsy. +[0, 1, 2, 3].filter(t => t); + +// Invalid: arrays are always falsy. +[ + [1, 2], + [3, 4], +].filter(t => t); +``` + ## When Not To Use It The main downside to using this rule is the need for type information. diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index e7f8e030679..043a52304c6 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -10,6 +10,7 @@ import { isFalsyType, isBooleanLiteralType, isLiteralType, + getCallSignaturesOfType, } from 'tsutils'; import { createRule, @@ -60,12 +61,15 @@ export type Options = [ { allowConstantLoopConditions?: boolean; ignoreRhs?: boolean; + checkArrayPredicates?: boolean; }, ]; export type MessageId = | 'alwaysTruthy' | 'alwaysFalsy' + | 'alwaysTruthyFunc' + | 'alwaysFalsyFunc' | 'neverNullish' | 'alwaysNullish' | 'literalBooleanExpression' @@ -92,6 +96,9 @@ export default createRule({ ignoreRhs: { type: 'boolean', }, + checkArrayPredicates: { + type: 'boolean', + }, }, additionalProperties: false, }, @@ -100,6 +107,10 @@ export default createRule({ messages: { alwaysTruthy: 'Unnecessary conditional, value is always truthy.', alwaysFalsy: 'Unnecessary conditional, value is always falsy.', + alwaysTruthyFunc: + 'This callback should return a conditional, but return is always truthy', + alwaysFalsyFunc: + 'This callback should return a conditional, but return is always falsy', neverNullish: 'Unnecessary conditional, expected left-hand side of `??` operator to be possibly null or undefined.', alwaysNullish: @@ -114,9 +125,13 @@ export default createRule({ { allowConstantLoopConditions: false, ignoreRhs: false, + checkArrayPredicates: false, }, ], - create(context, [{ allowConstantLoopConditions, ignoreRhs }]) { + create( + context, + [{ allowConstantLoopConditions, checkArrayPredicates, ignoreRhs }], + ) { const service = getParserServices(context); const checker = service.program.getTypeChecker(); const sourceCode = context.getSourceCode(); @@ -126,6 +141,11 @@ export default createRule({ return getConstrainedTypeAtLocation(checker, tsNode); } + function nodeIsArrayType(node: TSESTree.Node): boolean { + const nodeType = getNodeType(node); + return checker.isArrayType(nodeType) || checker.isTupleType(nodeType); + } + /** * Checks if a conditional node is necessary: * if the type of the node is always true or always false, it's not necessary. @@ -270,6 +290,77 @@ export default createRule({ checkNode(node.test); } + const ARRAY_PREDICATE_FUNCTIONS = new Set([ + 'filter', + 'find', + 'some', + 'every', + ]); + function shouldCheckCallback(node: TSESTree.CallExpression): boolean { + const { callee } = node; + return ( + // option is on + !!checkArrayPredicates && + // looks like `something.filter` or `something.find` + callee.type === AST_NODE_TYPES.MemberExpression && + callee.property.type === AST_NODE_TYPES.Identifier && + ARRAY_PREDICATE_FUNCTIONS.has(callee.property.name) && + // and the left-hand side is an array, according to the types + nodeIsArrayType(callee.object) + ); + } + function checkCallExpression(node: TSESTree.CallExpression): void { + const { + arguments: [callback], + } = node; + if (callback && shouldCheckCallback(node)) { + // Inline defined functions + if ( + (callback.type === AST_NODE_TYPES.ArrowFunctionExpression || + callback.type === AST_NODE_TYPES.FunctionExpression) && + callback.body + ) { + // Two special cases, where we can directly check the node that's returned: + // () => something + if (callback.body.type !== AST_NODE_TYPES.BlockStatement) { + return checkNode(callback.body); + } + // () => { return something; } + const callbackBody = callback.body.body; + if ( + callbackBody.length === 1 && + callbackBody[0].type === AST_NODE_TYPES.ReturnStatement && + callbackBody[0].argument + ) { + return checkNode(callbackBody[0].argument); + } + // Potential enhancement: could use code-path analysis to check + // any function with a single return statement + // (Value to complexity ratio is dubious however) + } + // Otherwise just do type analysis on the function as a whole. + const returnTypes = getCallSignaturesOfType( + getNodeType(callback), + ).map(sig => sig.getReturnType()); + /* istanbul ignore if */ if (returnTypes.length === 0) { + // Not a callable function + return; + } + if (!returnTypes.some(isPossiblyFalsy)) { + return context.report({ + node: callback, + messageId: 'alwaysTruthyFunc', + }); + } + if (!returnTypes.some(isPossiblyTruthy)) { + return context.report({ + node: callback, + messageId: 'alwaysFalsyFunc', + }); + } + } + } + function checkOptionalChain( node: TSESTree.OptionalMemberExpression | TSESTree.OptionalCallExpression, beforeOperator: TSESTree.Node, @@ -323,6 +414,7 @@ export default createRule({ return { BinaryExpression: checkIfBinaryExpressionIsNecessaryConditional, + CallExpression: checkCallExpression, ConditionalExpression: checkIfTestExpressionIsNecessaryConditional, DoWhileStatement: checkIfLoopIsNecessaryConditional, ForStatement: checkIfLoopIsNecessaryConditional, diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts index e2e46696224..1a7abc30f46 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -88,6 +88,52 @@ function test(t: T | []) { function test(a: string) { return a === "a" }`, + + /** + * Predicate functions + **/ + // valid, with the flag off + ` +[1,3,5].filter(() => true); +[1,2,3].find(() => false); +function truthy() { + return []; +} +function falsy() {} +[1,3,5].filter(truthy); +[1,2,3].find(falsy); +`, + { + options: [{ checkArrayPredicates: true }], + code: ` +// with literal arrow function +[0,1,2].filter(x => x); + +// filter with named function +function length(x: string) { + return x.length; +} +["a", "b", ""].filter(length); + +// with non-literal array +function nonEmptyStrings(x: string[]) { + return x.filter(length); +} +`, + }, + // Ignores non-array methods of the same name + { + options: [{ checkArrayPredicates: true }], + code: ` +const notArray = { + filter: (func: () => boolean) => func(), + find: (func: () => boolean) => func(), +}; +notArray.filter(() => true); +notArray.find(() => true); +`, + }, + // Nullish coalescing operator ` function test(a: string | null) { @@ -289,6 +335,63 @@ function test(a: never) { errors: [ruleError(3, 10, 'never')], }, + // Predicate functions + { + options: [{ checkArrayPredicates: true }], + code: ` +[1,3,5].filter(() => true); +[1,2,3].find(() => { return false; }); + +// with non-literal array +function nothing(x: string[]) { + return x.filter(() => false); +} +// with readonly array +function nothing2(x: readonly string[]) { + return x.filter(() => false); +} +// with tuple +function nothing3(x: [string, string]) { + return x.filter(() => false); +} +`, + errors: [ + ruleError(2, 22, 'alwaysTruthy'), + ruleError(3, 29, 'alwaysFalsy'), + ruleError(7, 25, 'alwaysFalsy'), + ruleError(11, 25, 'alwaysFalsy'), + ruleError(15, 25, 'alwaysFalsy'), + ], + }, + { + options: [{ checkArrayPredicates: true }], + code: ` +function truthy() { + return []; +} +function falsy() {} +[1,3,5].filter(truthy); +[1,2,3].find(falsy); +`, + errors: [ + ruleError(6, 16, 'alwaysTruthyFunc'), + ruleError(7, 14, 'alwaysFalsyFunc'), + ], + }, + // Supports generics + // TODO: fix this + // { + // options: [{ checkArrayPredicates: true }], + // code: ` + // const isTruthy = (t: T) => T; + // // Valid: numbers can be truthy or falsy (0). + // [0,1,2,3].filter(isTruthy); + // // Invalid: arrays are always falsy. + // [[1,2], [3,4]].filter(isTruthy); + // `, + // errors: [ruleError(6, 23, 'alwaysTruthyFunc')], + // }, + // Still errors on in the expected locations when ignoring RHS { options: [{ ignoreRhs: true }], diff --git a/packages/eslint-plugin/typings/typescript.d.ts b/packages/eslint-plugin/typings/typescript.d.ts new file mode 100644 index 00000000000..6d9a098b538 --- /dev/null +++ b/packages/eslint-plugin/typings/typescript.d.ts @@ -0,0 +1,21 @@ +import { TypeChecker, Type } from 'typescript'; + +declare module 'typescript' { + interface TypeChecker { + // internal TS APIs + + /** + * @returns `true` if the given type is an array type: + * - Array + * - ReadonlyArray + * - foo[] + * - readonly foo[] + */ + isArrayType(type: Type): boolean; + /** + * @returns `true` if the given type is a tuple type: + * - [foo] + */ + isTupleType(type: Type): boolean; + } +}