8000 feat(eslint-plugin): [no-unnec-cond] array predicate callbacks (#1206) · lionelB/typescript-eslint@f7ad716 · GitHub
[go: up one dir, main page]
Skip to content

Commit f7ad716

Browse filesBrowse files
Retsambradzacher
Retsam
and
bradzacher
committed
feat(eslint-plugin): [no-unnec-cond] array predicate callbacks (typescript-eslint#1206)
Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
1 parent 1898bdf commit f7ad716

File tree

4 files changed

+230
-1
lines changed

4 files changed

+230
-1
lines changed

‎packages/eslint-plugin/docs/rules/no-unnecessary-condition.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,19 @@ for (; true; ) {}
7474
do {} while (true);
7575
```
7676

77+
- `checkArrayPredicates` (default: `false`) - if set checks that the return value from certain array method callbacks (`filter`, `find`, `some`, `every`) is necessarily conditional.
78+
79+
```ts
80+
// Valid: numbers can be truthy or falsy.
81+
[0, 1, 2, 3].filter(t => t);
82+
83+
// Invalid: arrays are always falsy.
84+
[
85+
[1, 2],
86+
[3, 4],
87+
].filter(t => t);
88+
```
89+
7790
## When Not To Use It
7891

7992
The main downside to using this rule is the need for type information.

‎packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
isFalsyType,
1111
isBooleanLiteralType,
1212
isLiteralType,
13+
getCallSignaturesOfType,
1314
} from 'tsutils';
1415
import {
1516
createRule,
@@ - 8000 60,12 +61,15 @@ export type Options = [
6061
{
6162
allowConstantLoopConditions?: boolean;
6263
ignoreRhs?: boolean;
64+
checkArrayPredicates?: boolean;
6365
},
6466
];
6567

6668
export type MessageId =
6769
| 'alwaysTruthy'
6870
| 'alwaysFalsy'
71+
| 'alwaysTruthyFunc'
72+
| 'alwaysFalsyFunc'
6973
| 'neverNullish'
7074
| 'alwaysNullish'
7175
| 'literalBooleanExpression'
@@ -92,6 +96,9 @@ export default createRule<Options, MessageId>({
9296
ignoreRhs: {
9397
type: 'boolean',
9498
},
99+
checkArrayPredicates: {
100+
type: 'boolean',
101+
},
95102
},
96103
additionalProperties: false,
97104
},
@@ -100,6 +107,10 @@ export default createRule<Options, MessageId>({
100107
messages: {
101108
alwaysTruthy: 'Unnecessary conditional, value is always truthy.',
102109
alwaysFalsy: 'Unnecessary conditional, value is always falsy.',
110+
alwaysTruthyFunc:
111+
'This callback should return a conditional, but return is always truthy',
112+
alwaysFalsyFunc:
113+
'This callback should return a conditional, but return is always falsy',
103114
neverNullish:
104115
'Unnecessary conditional, expected left-hand side of `??` operator to be possibly null or undefined.',
105116
alwaysNullish:
@@ -114,9 +125,13 @@ export default createRule<Options, MessageId>({
114125
{
115126
allowConstantLoopConditions: false,
116127
ignoreRhs: false,
128+
checkArrayPredicates: false,
117129
},
118130
],
119-
create(context, [{ allowConstantLoopConditions, ignoreRhs }]) {
131+
create(
132+
context,
133+
[{ allowConstantLoopConditions, checkArrayPredicates, ignoreRhs }],
134+
) {
120135
const service = getParserServices(context);
121136
const checker = service.program.getTypeChecker();
122137
const sourceCode = context.getSourceCode();
@@ -126,6 +141,11 @@ export default createRule<Options, MessageId>({
126141
return getConstrainedTypeAtLocation(checker, tsNode);
127142
}
128143

144+
function nodeIsArrayType(node: TSESTree.Node): boolean {
145+
const nodeType = getNodeType(node);
146+
return checker.isArrayType(nodeType) || checker.isTupleType(nodeType);
147+
}
148+
129149
/**
130150
* Checks if a conditional node is necessary:
131151
* if the type of the node is always true or always false, it's not necessary.
@@ -270,6 +290,77 @@ export default createRule<Options, MessageId>({
270290
checkNode(node.test);
271291
}
272292

293+
const ARRAY_PREDICATE_FUNCTIONS = new Set([
294+
'filter',
295+
'find',
296+
'some',
297+
'every',
298+
]);
299+
function shouldCheckCallback(node: TSESTree.CallExpression): boolean {
300+
const { callee } = node;
301+
return (
302+
// option is on
303+
!!checkArrayPredicates &&
304+
// looks like `something.filter` or `something.find`
305+
callee.type === AST_NODE_TYPES.MemberExpression &&
306+
callee.property.type === AST_NODE_TYPES.Identifier &&
307+
ARRAY_PREDICATE_FUNCTIONS.has(callee.property.name) &&
308+
// and the left-hand side is an array, according to the types
309+
nodeIsArrayType(callee.object)
310+
);
311+
}
312+
function checkCallExpression(node: TSESTree.CallExpression): void {
313+
const {
314+
arguments: [callback],
315+
} = node;
316+
if (callback && shouldCheckCallback(node)) {
317+
// Inline defined functions
318+
if (
319+
(callback.type === AST_NODE_TYPES.ArrowFunctionExpression ||
320+
callback.type === AST_NODE_TYPES.FunctionExpression) &&
321+
callback.body
322+
) {
323+
// Two special cases, where we can directly check the node that's returned:
324+
// () => something
325+
if (callback.body.type !== AST_NODE_TYPES.BlockStatement) {
326+
return checkNode(callback.body);
327+
}
328+
// () => { return something; }
329+
const callbackBody = callback.body.body;
330+
if (
331+
callbackBody.length === 1 &&
332+
callbackBody[0].type === AST_NODE_TYPES.ReturnStatement &&
333+
callbackBody[0].argument
334+
) {
335+
return checkNode(callbackBody[0].argument);
336+
}
337+
// Potential enhancement: could use code-path analysis to check
338+
// any function with a single return statement
339+
// (Value to complexity ratio is dubious however)
340+
}
341+
// Otherwise just do type analysis on the function as a whole.
342+
const returnTypes = getCallSignaturesOfType(
343+
getNodeType(callback),
344+
).map(sig => sig.getReturnType());
345+
/* istanbul ignore if */ if (returnTypes.length === 0) {
346+
// Not a callable function
347+
return;
348+
}
349+
if (!returnTypes.some(isPossiblyFalsy)) {
350+
return context.report({
351+
node: callback,
352+
messageId: 'alwaysTruthyFunc',
353+
});
354+
}
355+
if (!returnTypes.some(isPossiblyTruthy)) {
356+
return context.report({
357+
node: callback,
358+
messageId: 'alwaysFalsyFunc',
359+
});
360+
}
361+
}
362+
}
363+
273364
function checkOptionalChain(
274365
node: TSESTree.OptionalMemberExpression | TSESTree.OptionalCallExpression,
275366
beforeOperator: TSESTree.Node,
@@ -323,6 +414,7 @@ export default createRule<Options, MessageId>({
323414

324415
return {
325416
BinaryExpression: checkIfBinaryExpressionIsNecessaryConditional,
417+
CallExpression: checkCallExpression,
326418
ConditionalExpression: checkIfTestExpressionIsNecessaryConditional,
327419
DoWhileStatement: checkIfLoopIsNecessaryConditional,
328420
ForStatement: checkIfLoopIsNecessaryConditional,

‎packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,52 @@ function test<T>(t: T | []) {
8888
function test(a: string) {
8989
return a === "a"
9090
}`,
91+
92+
/**
93+
* Predicate functions
94+
**/
95+
// valid, with the flag off
96+
`
97+
[1,3,5].filter(() => true);
98+
[1,2,3].find(() => false);
99+
function truthy() {
100+
return [];
101+
}
102+
function falsy() {}
103+
[1,3,5].filter(truthy);
104+
[1,2,3].find(falsy);
105+
`,
106+
{
107+
options: [{ checkArrayPredicates: true }],
108+
code: `
109+
// with literal arrow function
110+
[0,1,2].filter(x => x);
111+
112+
// filter with named function
113+
function length(x: string) {
114+
return x.length;
115+
}
116+
["a", "b", ""].filter(length);
117+
118+
// with non-literal array
119+
function nonEmptyStrings(x: string[]) {
120+
return x.filter(length);
121+
}
122+
`,
123+
},
124+
// Ignores non-array methods of the same name
125+
{
126+
options: [{ checkArrayPredicates: true }],
127+
code: `
128+
const notArray = {
129+
filter: (func: () => boolean) => func(),
130+
find: (func: () => boolean) => func(),
131+
};
132+
notArray.filter(() => true);
133+
notArray.find(() => true);
134+
`,
135+
},
136+
91137
// Nullish coalescing operator
92138
`
93139
function test(a: string | null) {
@@ -289,6 +335,63 @@ function test(a: never) {
289335
errors: [ruleError(3, 10, 'never')],
290336
},
291337

338+
// Predicate functions
339+
{
340+
options: [{ checkArrayPredicates: true }],
341+
code: `
342+
[1,3,5].filter(() => true);
343+
[1,2,3].find(() => { return false; });
344+
345+
// with non-literal array
346+
function nothing(x: string[]) {
347+
return x.filter(() => false);
348+
}
349+
// with readonly array
350+
function nothing2(x: readonly string[]) {
351+
return x.filter(() => false);
352+
}
353+
// with tuple
354+
function nothing3(x: [string, string]) {
355+
return x.filter(() => false);
356+
}
357+
`,
358+
errors: [
359+
ruleError(2, 22, 'alwaysTruthy'),
360+
ruleError(3, 29, 'alwaysFalsy'),
361+
ruleError(7, 25, 'alwaysFalsy'),
362+
ruleError(11, 25, 'alwaysFalsy'),
363+
ruleError(15, 25, 'alwaysFalsy'),
364+
],
365+
},
366+
{
367+
options: [{ checkArrayPredicates: true }],
368+
code: `
369+
function truthy() {
370+
return [];
371+
}
372+
function falsy() {}
373+
[1,3,5].filter(truthy);
374+
[1,2,3].find(falsy);
375+
`,
376+
errors: [
377+
ruleError(6, 16, 'alwaysTruthyFunc'),
378+
ruleError(7, 14, 'alwaysFalsyFunc'),
379+
],
380+
},
381+
// Supports generics
382+
// TODO: fix this
383+
// {
384+
// options: [{ checkArrayPredicates: true }],
385+
// code: `
386+
// const isTruthy = <T>(t: T) => T;
387+
// // Valid: numbers can be truthy or falsy (0).
388+
// [0,1,2,3].filter(isTruthy);
389+
// // Invalid: arrays are always falsy.
390+
// [[1,2], [3,4]].filter(isTruthy);
391+
// `,
392+
// errors: [ruleError(6, 23, 'alwaysTruthyFunc')],
393+
// },
394+
292395
// Still errors on in the expected locations when ignoring RHS
293396
{
294397
options: [{ ignoreRhs: true }],

‎packages/eslint-plugin/typings/typescript.d.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import { TypeChecker, Type } from 'typescript';
2+
3+
declare module 'typescript' {
4+
interface TypeChecker {
5+
// internal TS APIs
6+
7+
/**
8+
* @returns `true` if the given type is an array type:
9+
* - Array<foo>
10+
* - ReadonlyArray<foo>
11+
* - foo[]
12+
* - readonly foo[]
13+
*/
14+
isArrayType(type: Type): boolean;
15+
/**
16+
* @returns `true` if the given type is a tuple type:
17+
* - [foo]
18+
*/
19+
isTupleType(type: Type): boolean;
20+
}
21+
}

0 commit comments

Comments
 (0)
0