From 3a187cafb7302a3c05de0e6a236dd142a5e2d741 Mon Sep 17 00:00:00 2001 From: YeonJuan Date: Mon, 13 Jul 2020 12:53:16 +0900 Subject: [PATCH] fix(eslint-plugin): [no-unnecessary-condition] handle computed member access (#2288) --- .../src/rules/no-unnecessary-condition.ts | 33 +++- .../rules/no-unnecessary-condition.test.ts | 171 ++++++++++++++++++ 2 files changed, 203 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 1147957a20c..0103919e14a 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -24,6 +24,7 @@ import { isIdentifier, isTypeAnyType, isTypeUnknownType, + getTypeName, } from '../util'; // Truthiness utilities @@ -435,6 +436,33 @@ export default createRule({ return false; } + function isNullablePropertyType( + objType: ts.Type, + propertyType: ts.Type, + ): boolean { + if (propertyType.isUnion()) { + return propertyType.types.some(type => + isNullablePropertyType(objType, type), + ); + } + if (propertyType.isNumberLiteral() || propertyType.isStringLiteral()) { + const propType = checker.getTypeOfPropertyOfType( + objType, + propertyType.value.toString(), + ); + if (propType) { + return isNullableType(propType, { allowUndefined: true }); + } + } + const typeName = getTypeName(checker, propertyType); + return !!( + (typeName === 'string' && + checker.getIndexInfoOfType(objType, ts.IndexKind.String)) || + (typeName === 'number' && + checker.getIndexInfoOfType(objType, ts.IndexKind.Number)) + ); + } + // Checks whether a member expression is nullable or not regardless of it's previous node. // Example: // ``` @@ -450,10 +478,13 @@ export default createRule({ const property = node.property; if (prevType.isUnion() && isIdentifier(property)) { const isOwnNullable = prevType.types.some(type => { + if (node.computed) { + const propertyType = getNodeType(node.property); + return isNullablePropertyType(type, propertyType); + } const propType = checker.getTypeOfPropertyOfType(type, property.name); return propType && isNullableType(propType, { allowUndefined: true }); }); - return ( !isOwnNullable && isNullableType(prevType, { allowUndefined: true }) ); 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 81bc96874b8..ce65e717fa0 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -398,6 +398,53 @@ type Bar = { baz: null | string | { qux: string } }; declare const foo: { fooOrBar: Foo | Bar } | null; foo?.fooOrBar?.baz?.qux; `, + ` +type Foo = { [key: string]: string } | null; +declare const foo: Foo; + +const key = '1'; +foo?.[key]?.trim(); + `, + ` +type Foo = { [key: string]: string; foo: 'foo'; bar: 'bar' } | null; +type Key = 'bar' | 'foo'; +declare const foo: Foo; +declare const key: Key; + +foo?.[key].trim(); + `, + ` +interface Outer { + inner?: { + [key: string]: string | undefined; + }; +} + +function Foo(outer: Outer, key: string): number | undefined { + return outer.inner?.[key]?.charCodeAt(0); +} + `, + ` +interface Outer { + inner?: { + [key: string]: string | undefined; + bar: 'bar'; + }; +} +type Foo = 'foo'; + +function Foo(outer: Outer, key: Foo): number | undefined { + return outer.inner?.[key]?.charCodeAt(0); +} + `, + ` +type Foo = { [key: string]: string; foo: 'foo'; bar: 'bar' } | null; +type Key = 'bar' | 'foo' | 'baz'; +declare const foo: Foo; +declare const key: Key; + +foo?.[key]?.trim(); + `, ], invalid: [ // Ensure that it's checking in all the right places @@ -1196,5 +1243,129 @@ foo?.fooOrBar.baz?.qux; }, ], }, + { + code: ` +type Foo = { [key: string]: string; foo: 'foo'; bar: 'bar' } | null; +type Key = 'bar' | 'foo'; +declare const foo: Foo; +declare const key: Key; + +foo?.[key]?.trim(); + `, + output: ` +type Foo = { [key: string]: string; foo: 'foo'; bar: 'bar' } | null; +type Key = 'bar' | 'foo'; +declare const foo: Foo; +declare const key: Key; + +foo?.[key].trim(); + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 7, + endLine: 7, + column: 11, + endColumn: 13, + }, + ], + }, + { + code: ` +type Foo = { [key: string]: string; foo: 'foo'; bar: 'bar' } | null; +declare const foo: Foo; +const key = 'bar'; +foo?.[key]?.trim(); + `, + output: ` +type Foo = { [key: string]: string; foo: 'foo'; bar: 'bar' } | null; +declare const foo: Foo; +const key = 'bar'; +foo?.[key].trim(); + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 5, + endLine: 5, + column: 11, + endColumn: 13, + }, + ], + }, + { + code: ` +interface Outer { + inner?: { + [key: string]: string | undefined; + bar: 'bar'; + }; +} + +export function test(outer: Outer): number | undefined { + const key = 'bar'; + return outer.inner?.[key]?.charCodeAt(0); +} + `, + output: ` +interface Outer { + inner?: { + [key: string]: string | undefined; + bar: 'bar'; + }; +} + +export function test(outer: Outer): number | undefined { + const key = 'bar'; + return outer.inner?.[key].charCodeAt(0); +} + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 11, + endLine: 11, + column: 28, + endColumn: 30, + }, + ], + }, + { + code: ` +interface Outer { + inner?: { + [key: string]: string | undefined; + bar: 'bar'; + }; +} +type Bar = 'bar'; + +function Foo(outer: Outer, key: Bar): number | undefined { + return outer.inner?.[key]?.charCodeAt(0); +} + `, + output: ` +interface Outer { + inner?: { + [key: string]: string | undefined; + bar: 'bar'; + }; +} +type Bar = 'bar'; + +function Foo(outer: Outer, key: Bar): number | undefined { + return outer.inner?.[key].charCodeAt(0); +} + `, + errors: [ + { + messageId: 'neverOptionalChain', + line: 11, + endLine: 11, + column: 28, + endColumn: 30, + }, + ], + }, ], });