From f7b727b399df918b55de5fb4264765d0d2982b9a Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Sun, 31 May 2020 22:36:22 +0900 Subject: [PATCH 1/9] fix(eslint-plugin): [no-unnecessary-condition] improve optional chain handling --- .../src/rules/no-unnecessary-condition.ts | 33 ++- .../rules/no-unnecessary-condition.test.ts | 195 ++++++++++++++++++ 2 files changed, 227 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 5f04366b5fd..e8328f9212a 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -21,6 +21,8 @@ import { nullThrows, NullThrowsReasons, isMemberOrOptionalMemberExpression, + isIdentifier, + isOptionalOptionalCallExpression, } from '../util'; // Truthiness utilities @@ -426,6 +428,33 @@ export default createRule({ return false; } + // Checks whether a node is nullable or not regardless of it's previous node. + // Example: + // ``` + // // 'bar' is nullable if 'foo' is null. + // // but this function checks regardless of 'foo' type, so returns 'true'. + // declare const foo: { bar : { baz: string } } | null + // foo?.bar; + // ``` + function isNullableNodeRegardlessOfPrev( + node: TSESTree.LeftHandSideExpression, + ): boolean { + if (isMemberOrOptionalMemberExpression(node)) { + const prevType = getNodeType(node.object); + const property = node.property; + if (prevType.isUnion() && isIdentifier(property)) { + const [ownPropertyType] = prevType.types + .map(type => checker.getTypeOfPropertyOfType(type, property.name)) + .filter(t => t); + if (ownPropertyType) { + return isNullableType(ownPropertyType, { allowUndefined: true }); + } + } + } + + return isNullableType(getNodeType(node), { allowUndefined: true }); + } + function checkOptionalChain( node: TSESTree.OptionalMemberExpression | TSESTree.OptionalCallExpression, beforeOperator: TSESTree.Node, @@ -446,13 +475,15 @@ export default createRule({ const nodeToCheck = isMemberOrOptionalMemberExpression(node) ? node.object + : isOptionalOptionalCallExpression(node) + ? node.callee : node; const type = getNodeType(nodeToCheck); if ( isTypeFlagSet(type, ts.TypeFlags.Any) || isTypeFlagSet(type, ts.TypeFlags.Unknown) || - isNullableType(type, { allowUndefined: true }) + isNullableNodeRegardlessOfPrev(nodeToCheck) ) { return; } 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 1efb193df42..f55a9f67790 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -329,6 +329,13 @@ let unknownValue: unknown; unknownValue?.(); `, 'const foo = [1, 2, 3][0];', + ` +declare const foo: { bar?: { baz: { c: string } } } | null; +foo?.bar?.baz; + `, + ` +foo?.bar?.baz?.qux; + `, ], invalid: [ // Ensure that it's checking in all the right places @@ -836,5 +843,193 @@ x.a; }, ], }, + { + code: ` +declare const foo: { bar: { baz: { c: string } } } | null; +foo?.bar?.baz; + `, + output: ` +declare const foo: { bar: { baz: { c: string } } } | null; +foo?.bar.baz; + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 3, + endLine: 3, + column: 9, + endColumn: 11, + }, + ], + }, + { + code: ` +declare const foo: { bar?: { baz: { qux: string } } } | null; +foo?.bar?.baz?.qux; + `, + output: ` +declare const foo: { bar?: { baz: { qux: string } } } | null; +foo?.bar?.baz.qux; + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 3, + endLine: 3, + column: 14, + endColumn: 16, + }, + ], + }, + { + code: ` +declare const foo: { bar: { baz: { qux?: () => {} } } } | null; +foo?.bar?.baz?.qux?.(); + `, + output: ` +declare const foo: { bar: { baz: { qux?: () => {} } } } | null; +foo?.bar.baz.qux?.(); + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 3, + endLine: 3, + column: 9, + endColumn: 11, + }, + { + messageId: 'neverOptionalChain', + line: 3, + endLine: 3, + column: 14, + endColumn: 16, + }, + ], + }, + { + code: ` +declare const foo: { bar: { baz: { qux: () => {} } } } | null; +foo?.bar?.baz?.qux?.(); + `, + output: ` +declare const foo: { bar: { baz: { qux: () => {} } } } | null; +foo?.bar.baz.qux(); + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 3, + endLine: 3, + column: 9, + endColumn: 11, + }, + { + messageId: 'neverOptionalChain', + line: 3, + endLine: 3, + column: 14, + endColumn: 16, + }, + { + messageId: 'neverOptionalChain', + line: 3, + endLine: 3, + column: 19, + endColumn: 21, + }, + ], + }, + { + code: ` +type baz = () => { qux: () => {} }; +declare const foo: { bar: { baz: baz } } | null; +foo?.bar?.baz?.().qux?.(); + `, + output: ` +type baz = () => { qux: () => {} }; +declare const foo: { bar: { baz: baz } } | null; +foo?.bar.baz().qux(); + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 4, + endLine: 4, + column: 9, + endColumn: 11, + }, + { + messageId: 'neverOptionalChain', + line: 4, + endLine: 4, + column: 14, + endColumn: 16, + }, + { + messageId: 'neverOptionalChain', + line: 4, + endLine: 4, + column: 22, + endColumn: 24, + }, + ], + }, + { + code: ` +type baz = null | (() => { qux: () => {} }); +declare const foo: { bar: { baz: baz } } | null; +foo?.bar?.baz?.().qux?.(); + `, + output: ` +type baz = null | (() => { qux: () => {} }); +declare const foo: { bar: { baz: baz } } | null; +foo?.bar.baz?.().qux(); + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 4, + endLine: 4, + column: 9, + endColumn: 11, + }, + { + messageId: 'neverOptionalChain', + line: 4, + endLine: 4, + column: 22, + endColumn: 24, + }, + ], + }, + { + code: ` +type baz = null | (() => { qux: () => {} } | null); +declare const foo: { bar: { baz: baz } } | null; +foo?.bar?.baz?.()?.qux?.(); + `, + output: ` +type baz = null | (() => { qux: () => {} } | null); +declare const foo: { bar: { baz: baz } } | null; +foo?.bar.baz?.()?.qux(); + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 4, + endLine: 4, + column: 9, + endColumn: 11, + }, + { + messageId: 'neverOptionalChain', + line: 4, + endLine: 4, + column: 23, + endColumn: 25, + }, + ], + }, ], }); From 8ab4cb5267e4b67a4564bc99b81c7b37f544d8ad Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Sun, 31 May 2020 22:38:42 +0900 Subject: [PATCH 2/9] fix formatting --- .../rules/no-unnecessary-condition.test.ts | 86 +++++++++---------- 1 file changed, 43 insertions(+), 43 deletions(-) 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 f55a9f67790..00f5d3d3820 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -976,60 +976,60 @@ foo?.bar.baz().qux(); ], }, { - code: ` + code: ` type baz = null | (() => { qux: () => {} }); declare const foo: { bar: { baz: baz } } | null; foo?.bar?.baz?.().qux?.(); - `, - output: ` + `, + output: ` type baz = null | (() => { qux: () => {} }); declare const foo: { bar: { baz: baz } } | null; foo?.bar.baz?.().qux(); - `, - errors: [ - { - messageId: 'neverOptionalChain', - line: 4, - endLine: 4, - column: 9, - endColumn: 11, - }, - { - messageId: 'neverOptionalChain', - line: 4, - endLine: 4, - column: 22, - endColumn: 24, - }, - ], - }, - { - code: ` + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 4, + endLine: 4, + column: 9, + endColumn: 11, + }, + { + messageId: 'neverOptionalChain', + line: 4, + endLine: 4, + column: 22, + endColumn: 24, + }, + ], + }, + { + code: ` type baz = null | (() => { qux: () => {} } | null); declare const foo: { bar: { baz: baz } } | null; foo?.bar?.baz?.()?.qux?.(); - `, - output: ` + `, + output: ` type baz = null | (() => { qux: () => {} } | null); declare const foo: { bar: { baz: baz } } | null; foo?.bar.baz?.()?.qux(); - `, - errors: [ - { - messageId: 'neverOptionalChain', - line: 4, - endLine: 4, - column: 9, - endColumn: 11, - }, - { - messageId: 'neverOptionalChain', - line: 4, - endLine: 4, - column: 23, - endColumn: 25, - }, - ], - }, + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 4, + endLine: 4, + column: 9, + endColumn: 11, + }, + { + messageId: 'neverOptionalChain', + line: 4, + endLine: 4, + column: 23, + endColumn: 25, + }, + ], + }, ], }); From ef4ff9453a879890dbefdc25abdc2c4523b6cd6f Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Sun, 31 May 2020 22:54:55 +0900 Subject: [PATCH 3/9] refactor --- packages/eslint-plugin/src/rules/no-unnecessary-condition.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index e8328f9212a..2a61ef11761 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -443,9 +443,10 @@ export default createRule({ const prevType = getNodeType(node.object); const property = node.property; if (prevType.isUnion() && isIdentifier(property)) { - const [ownPropertyType] = prevType.types + const ownPropertyType = prevType.types .map(type => checker.getTypeOfPropertyOfType(type, property.name)) - .filter(t => t); + .find(t => t); + if (ownPropertyType) { return isNullableType(ownPropertyType, { allowUndefined: true }); } From 2a2e9ece753ea1019b2dfa1040229e8a30847397 Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Sun, 31 May 2020 23:26:11 +0900 Subject: [PATCH 4/9] add test case --- .../tests/rules/no-unnecessary-condition.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 00f5d3d3820..f856427d8ae 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -336,6 +336,10 @@ foo?.bar?.baz; ` foo?.bar?.baz?.qux; `, + ` +declare const foo: { bar: { baz: string } }; +foo.bar.qux?.(); + ` ], invalid: [ // Ensure that it's checking in all the right places @@ -1030,6 +1034,6 @@ foo?.bar.baz?.()?.qux(); endColumn: 25, }, ], - }, + } ], }); From 378cae073341e3556e27692b90bd11c8b0043fd7 Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Sun, 31 May 2020 23:26:53 +0900 Subject: [PATCH 5/9] fix lint --- .../eslint-plugin/tests/rules/no-unnecessary-condition.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 f856427d8ae..05c8bdc11a7 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -1034,6 +1034,6 @@ foo?.bar.baz?.()?.qux(); endColumn: 25, }, ], - } + }, ], }); From 2c3c38344ef21e2dd758e33c43cc67d54e2a7ba2 Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Sun, 31 May 2020 23:27:54 +0900 Subject: [PATCH 6/9] trailing comma --- .../eslint-plugin/tests/rules/no-unnecessary-condition.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 05c8bdc11a7..6ec5e7cda53 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -339,7 +339,7 @@ foo?.bar?.baz?.qux; ` declare const foo: { bar: { baz: string } }; foo.bar.qux?.(); - ` + `, ], invalid: [ // Ensure that it's checking in all the right places From 77721a609b4ca86b8705dc1e158d82a1140495a8 Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Sun, 31 May 2020 23:46:28 +0900 Subject: [PATCH 7/9] delete not occurred case --- .../src/rules/no-unnecessary-condition.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 2a61ef11761..39f399ca729 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -22,7 +22,6 @@ import { NullThrowsReasons, isMemberOrOptionalMemberExpression, isIdentifier, - isOptionalOptionalCallExpression, } from '../util'; // Truthiness utilities @@ -474,11 +473,11 @@ export default createRule({ return; } - const nodeToCheck = isMemberOrOptionalMemberExpression(node) - ? node.object - : isOptionalOptionalCallExpression(node) - ? node.callee - : node; + const nodeToCheck = + node.type === AST_NODE_TYPES.OptionalCallExpression + ? node.callee + : node.object; + const type = getNodeType(nodeToCheck); if ( From 07fde54e896123f0ad91213031ad365423464ee6 Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Mon, 1 Jun 2020 01:38:08 +0900 Subject: [PATCH 8/9] refactor --- .../src/rules/no-unnecessary-condition.ts | 52 +++++++++++-------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 39f399ca729..f93317d8bb6 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -427,7 +427,7 @@ export default createRule({ return false; } - // Checks whether a node is nullable or not regardless of it's previous node. + // Checks whether a member expression is nullable or not regardless of it's previous node. // Example: // ``` // // 'bar' is nullable if 'foo' is null. @@ -435,24 +435,38 @@ export default createRule({ // declare const foo: { bar : { baz: string } } | null // foo?.bar; // ``` - function isNullableNodeRegardlessOfPrev( - node: TSESTree.LeftHandSideExpression, + function isNullableOriginFromPrev( + node: TSESTree.MemberExpression | TSESTree.OptionalMemberExpression, ): boolean { - if (isMemberOrOptionalMemberExpression(node)) { - const prevType = getNodeType(node.object); - const property = node.property; - if (prevType.isUnion() && isIdentifier(property)) { - const ownPropertyType = prevType.types - .map(type => checker.getTypeOfPropertyOfType(type, property.name)) - .find(t => t); - - if (ownPropertyType) { - return isNullableType(ownPropertyType, { allowUndefined: true }); - } + const prevType = getNodeType(node.object); + const property = node.property; + if (prevType.isUnion() && isIdentifier(property)) { + const ownPropertyType = prevType.types + .map(type => checker.getTypeOfPropertyOfType(type, property.name)) + .find(t => t); + + if (ownPropertyType) { + return ( + !isNullableType(ownPropertyType, { allowUndefined: true }) && + isNullableType(prevType, { allowUndefined: true }) + ); } } + return false; + } - return isNullableType(getNodeType(node), { allowUndefined: true }); + function isOptionableExpression( + node: TSESTree.LeftHandSideExpression, + ): boolean { + const type = getNodeType(node); + const isOwnNullable = isMemberOrOptionalMemberExpression(node) + ? !isNullableOriginFromPrev(node) + : true; + return ( + isTypeFlagSet(type, ts.TypeFlags.Any) || + isTypeFlagSet(type, ts.TypeFlags.Unknown) || + (isNullableType(type, { allowUndefined: true }) && isOwnNullable) + ); } function checkOptionalChain( @@ -478,13 +492,7 @@ export default createRule({ ? node.callee : node.object; - const type = getNodeType(nodeToCheck); - - if ( - isTypeFlagSet(type, ts.TypeFlags.Any) || - isTypeFlagSet(type, ts.TypeFlags.Unknown) || - isNullableNodeRegardlessOfPrev(nodeToCheck) - ) { + if (isOptionableExpression(nodeToCheck)) { return; } From 16e5a14eba9f65c00fc7fb32409e6be9e28ba3c3 Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Mon, 1 Jun 2020 11:14:15 +0900 Subject: [PATCH 9/9] fix false neg --- .../src/rules/no-unnecessary-condition.ts | 18 +++++------- .../rules/no-unnecessary-condition.test.ts | 29 +++++++++++++++++++ 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index f93317d8bb6..0834c5a3b57 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -441,16 +441,14 @@ export default createRule({ const prevType = getNodeType(node.object); const property = node.property; if (prevType.isUnion() && isIdentifier(property)) { - const ownPropertyType = prevType.types - .map(type => checker.getTypeOfPropertyOfType(type, property.name)) - .find(t => t); - - if (ownPropertyType) { - return ( - !isNullableType(ownPropertyType, { allowUndefined: true }) && - isNullableType(prevType, { allowUndefined: true }) - ); - } + const isOwnNullable = prevType.types.some(type => { + const propType = checker.getTypeOfPropertyOfType(type, property.name); + return propType && isNullableType(propType, { allowUndefined: true }); + }); + + return ( + !isOwnNullable && isNullableType(prevType, { allowUndefined: true }) + ); } return false; } 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 6ec5e7cda53..a823d1ab6f1 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -340,6 +340,12 @@ foo?.bar?.baz?.qux; declare const foo: { bar: { baz: string } }; foo.bar.qux?.(); `, + ` +type Foo = { baz: number } | null; +type Bar = { baz: null | string | { qux: string } }; +declare const foo: { fooOrBar: Foo | Bar } | null; +foo?.fooOrBar?.baz?.qux; + `, ], invalid: [ // Ensure that it's checking in all the right places @@ -1035,5 +1041,28 @@ foo?.bar.baz?.()?.qux(); }, ], }, + { + code: ` +type Foo = { baz: number }; +type Bar = { baz: null | string | { qux: string } }; +declare const foo: { fooOrBar: Foo | Bar } | null; +foo?.fooOrBar?.baz?.qux; + `, + output: ` +type Foo = { baz: number }; +type Bar = { baz: null | string | { qux: string } }; +declare const foo: { fooOrBar: Foo | Bar } | null; +foo?.fooOrBar.baz?.qux; + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 5, + endLine: 5, + column: 14, + endColumn: 16, + }, + ], + }, ], });