Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(eslint-plugin): [no-unnecessary-condition] remove checkArrayPredicates and always check it #1579

Merged
merged 3 commits into from May 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 8 additions & 13 deletions packages/eslint-plugin/docs/rules/no-unnecessary-condition.md
Expand Up @@ -28,6 +28,12 @@ function bar<T>(arg: string) {
// arg can never be nullish, so ?. is unnecessary
return arg?.length;
}

// Checks array predicate return types, where possible
[
[1, 2],
[3, 4],
].filter(t => t); // number[] is always truthy
```

Examples of **correct** code for this rule:
Expand All @@ -50,6 +56,8 @@ function bar(arg?: string | null) {
// Necessary, since arg might be nullish
return arg?.length;
}

[0, 1, 2, 3].filter(t => t); // number can be truthy or falsy
```

## Options
Expand All @@ -74,19 +82,6 @@ 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.
Expand Down
18 changes: 5 additions & 13 deletions packages/eslint-plugin/src/rules/no-unnecessary-condition.ts
Expand Up @@ -64,7 +64,6 @@ const isLiteral = (type: ts.Type): boolean =>
export type Options = [
{
allowConstantLoopConditions?: boolean;
checkArrayPredicates?: boolean;
},
];

Expand Down Expand Up @@ -97,9 +96,6 @@ export default createRule<Options, MessageId>({
allowConstantLoopConditions: {
type: 'boolean',
},
checkArrayPredicates: {
type: 'boolean',
},
},
additionalProperties: false,
},
Expand Down Expand Up @@ -127,10 +123,9 @@ export default createRule<Options, MessageId>({
defaultOptions: [
{
allowConstantLoopConditions: false,
checkArrayPredicates: false,
},
],
create(context, [{ allowConstantLoopConditions, checkArrayPredicates }]) {
create(context, [{ allowConstantLoopConditions }]) {
const service = getParserServices(context);
const checker = service.program.getTypeChecker();
const sourceCode = context.getSourceCode();
Expand Down Expand Up @@ -337,11 +332,9 @@ export default createRule<Options, MessageId>({
'some',
'every',
]);
function shouldCheckCallback(node: TSESTree.CallExpression): boolean {
function isArrayPredicateFunction(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 &&
Expand All @@ -351,10 +344,9 @@ export default createRule<Options, MessageId>({
);
}
function checkCallExpression(node: TSESTree.CallExpression): void {
const {
arguments: [callback],
} = node;
if (callback && shouldCheckCallback(node)) {
// If this is something like arr.filter(x => /*condition*/), check `condition`
if (isArrayPredicateFunction(node) && node.arguments.length) {
bradzacher marked this conversation as resolved.
Show resolved Hide resolved
const callback = node.arguments[0]!;
// Inline defined functions
if (
(callback.type === AST_NODE_TYPES.ArrowFunctionExpression ||
Expand Down
Expand Up @@ -113,20 +113,7 @@ function test(a?: string) {
/**
* 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);

Expand All @@ -140,20 +127,16 @@ function length(x: string) {
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
`
Expand Down Expand Up @@ -470,7 +453,6 @@ function test(a: never) {

// Predicate functions
{
options: [{ checkArrayPredicates: true }],
code: `
[1, 3, 5].filter(() => true);
[1, 2, 3].find(() => {
Expand Down Expand Up @@ -534,7 +516,6 @@ if (arr.filter) {
errors: [ruleError(3, 5, 'alwaysTruthy')],
},
{
options: [{ checkArrayPredicates: true }],
code: `
function truthy() {
return [];
Expand All @@ -551,7 +532,6 @@ function falsy() {}
// Supports generics
// TODO: fix this
// {
// options: [{ checkArrayPredicates: true }],
// code: `
// const isTruthy = <T>(t: T) => T;
// // Valid: numbers can be truthy or falsy (0).
Expand Down