From ab8413a20263884ff432fbf32e7e3d663531a8bb Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Tue, 15 Sep 2020 01:33:06 -0400 Subject: [PATCH 1/2] fix(eslint-plugin): added safe getTypeOfPropertyOfType wrapper --- .../src/rules/no-unnecessary-condition.ts | 10 +++++-- packages/eslint-plugin/src/util/index.ts | 1 + .../eslint-plugin/src/util/isTypeReadonly.ts | 9 +++--- .../eslint-plugin/src/util/propertyTypes.ts | 25 +++++++++++++++++ .../prefer-readonly-parameter-types.test.ts | 28 +++++++++++++++++++ 5 files changed, 67 insertions(+), 6 deletions(-) create mode 100644 packages/eslint-plugin/src/util/propertyTypes.ts diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 134cefd76d1..639a3b7c4b8 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 { isTypeAnyType, isTypeUnknownType, getTypeName, + getTypeOfPropertyOfType, } from '../util'; // Truthiness utilities @@ -499,7 +500,8 @@ export default createRule({ ); } if (propertyType.isNumberLiteral() || propertyType.isStringLiteral()) { - const propType = checker.getTypeOfPropertyOfType( + const propType = getTypeOfPropertyOfType( + checker, objType, propertyType.value.toString(), ); @@ -535,7 +537,11 @@ export default createRule({ const propertyType = getNodeType(node.property); return isNullablePropertyType(type, propertyType); } - const propType = checker.getTypeOfPropertyOfType(type, property.name); + const propType = getTypeOfPropertyOfType( + checker, + type, + property.name, + ); return propType && isNullableType(propType, { allowUndefined: true }); }); return ( diff --git a/packages/eslint-plugin/src/util/index.ts b/packages/eslint-plugin/src/util/index.ts index 82828f13893..672f50dc4ff 100644 --- a/packages/eslint-plugin/src/util/index.ts +++ b/packages/eslint-plugin/src/util/index.ts @@ -6,6 +6,7 @@ export * from './isTypeReadonly'; export * from './misc'; export * from './nullThrows'; export * from './objectIterators'; +export * from './propertyTypes'; export * from './types'; // this is done for convenience - saves migrating all of the old rules diff --git a/packages/eslint-plugin/src/util/isTypeReadonly.ts b/packages/eslint-plugin/src/util/isTypeReadonly.ts index da9529e63a9..d1502760942 100644 --- a/packages/eslint-plugin/src/util/isTypeReadonly.ts +++ b/packages/eslint-plugin/src/util/isTypeReadonly.ts @@ -6,7 +6,7 @@ import { isPropertyReadonlyInType, } from 'tsutils'; import * as ts from 'typescript'; -import { nullThrows, NullThrowsReasons } from '.'; +import { getTypeOfPropertyOfType, nullThrows, NullThrowsReasons } from '.'; const enum Readonlyness { /** the type cannot be handled by the function */ @@ -100,9 +100,10 @@ function isTypeReadonlyObject( // as we might be able to bail out early due to a mutable property before // doing this deep, potentially expensive check. for (const property of properties) { - const propertyType = nullThrows( - checker.getTypeOfPropertyOfType(type, property.getName()), - NullThrowsReasons.MissingToken(`property "${property.name}"`, 'type'), + const propertyType = getTypeOfPropertyOfType( + checker, + type, + property.name, ); // handle recursive types. diff --git a/packages/eslint-plugin/src/util/propertyTypes.ts b/packages/eslint-plugin/src/util/propertyTypes.ts new file mode 100644 index 00000000000..f30b33b7fe5 --- /dev/null +++ b/packages/eslint-plugin/src/util/propertyTypes.ts @@ -0,0 +1,25 @@ +import * as ts from 'typescript'; + +import { nullThrows, NullThrowsReasons } from './nullThrows'; + +export function getTypeOfPropertyOfType( + checker: ts.TypeChecker, + type: ts.Type, + propertyName: string, +): ts.Type { + const reason = NullThrowsReasons.MissingToken( + `property "${propertyName}"`, + 'type', + ); + + return propertyName.startsWith('__') + ? checker.getDeclaredTypeOfSymbol( + nullThrows( + type + .getProperties() + .find(property => property.escapedName === propertyName), + reason, + ), + ) + : nullThrows(checker.getTypeOfPropertyOfType(type, propertyName), reason); +} diff --git a/packages/eslint-plugin/tests/rules/prefer-readonly-parameter-types.test.ts b/packages/eslint-plugin/tests/rules/prefer-readonly-parameter-types.test.ts index 3ca6c0201fc..d5408a5d859 100644 --- a/packages/eslint-plugin/tests/rules/prefer-readonly-parameter-types.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-readonly-parameter-types.test.ts @@ -238,6 +238,15 @@ ruleTester.run('prefer-readonly-parameter-types', rule, { } function foo(arg: Readonly) {} `, + ` + const sym = Symbol('sym'); + + interface WithSymbol { + [sym]: number; + } + + const willNotCrash = (foo: Readonly) => {}; + `, ], invalid: [ // arrays @@ -643,5 +652,24 @@ ruleTester.run('prefer-readonly-parameter-types', rule, { }, ], }, + { + code: ` + const sym = Symbol('sym'); + + interface WithSymbol { + [sym]: number; + } + + const willNot = (foo: WithSymbol) => {}; + `, + errors: [ + { + messageId: 'shouldBeReadonly', + line: 8, + column: 26, + endColumn: 41, + }, + ], + }, ], }); From 9bec7d96e20aa2a8d8e104ef4e809fea090be24c Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Tue, 15 Sep 2020 15:06:46 -0400 Subject: [PATCH 2/2] Allow either symbol or name --- .../src/rules/no-unnecessary-condition.ts | 6 +-- .../eslint-plugin/src/util/isTypeReadonly.ts | 7 ++- .../eslint-plugin/src/util/propertyTypes.ts | 45 ++++++++++++------- 3 files changed, 34 insertions(+), 24 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 639a3b7c4b8..3e1ad26bcda 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -24,7 +24,7 @@ import { isTypeAnyType, isTypeUnknownType, getTypeName, - getTypeOfPropertyOfType, + getTypeOfPropertyOfName, } from '../util'; // Truthiness utilities @@ -500,7 +500,7 @@ export default createRule({ ); } if (propertyType.isNumberLiteral() || propertyType.isStringLiteral()) { - const propType = getTypeOfPropertyOfType( + const propType = getTypeOfPropertyOfName( checker, objType, propertyType.value.toString(), @@ -537,7 +537,7 @@ export default createRule({ const propertyType = getNodeType(node.property); return isNullablePropertyType(type, propertyType); } - const propType = getTypeOfPropertyOfType( + const propType = getTypeOfPropertyOfName( checker, type, property.name, diff --git a/packages/eslint-plugin/src/util/isTypeReadonly.ts b/packages/eslint-plugin/src/util/isTypeReadonly.ts index d1502760942..b10f00006ec 100644 --- a/packages/eslint-plugin/src/util/isTypeReadonly.ts +++ b/packages/eslint-plugin/src/util/isTypeReadonly.ts @@ -100,10 +100,9 @@ function isTypeReadonlyObject( // as we might be able to bail out early due to a mutable property before // doing this deep, potentially expensive check. for (const property of properties) { - const propertyType = getTypeOfPropertyOfType( - checker, - type, - property.name, + const propertyType = nullThrows( + getTypeOfPropertyOfType(checker, type, property), + NullThrowsReasons.MissingToken(`property "${property.name}"`, 'type'), ); // handle recursive types. diff --git a/packages/eslint-plugin/src/util/propertyTypes.ts b/packages/eslint-plugin/src/util/propertyTypes.ts index f30b33b7fe5..5e2f1054239 100644 --- a/packages/eslint-plugin/src/util/propertyTypes.ts +++ b/packages/eslint-plugin/src/util/propertyTypes.ts @@ -1,25 +1,36 @@ import * as ts from 'typescript'; -import { nullThrows, NullThrowsReasons } from './nullThrows'; +export function getTypeOfPropertyOfName( + checker: ts.TypeChecker, + type: ts.Type, + name: string, + escapedName?: ts.__String, +): ts.Type | undefined { + // Most names are directly usable in the checker and aren't different from escaped names + if (!escapedName || !name.startsWith('__')) { + return checker.getTypeOfPropertyOfType(type, name); + } + + // Symbolic names may differ in their escaped name compared to their human-readable name + // https://github.com/typescript-eslint/typescript-eslint/issues/2143 + const escapedProperty = type + .getProperties() + .find(property => property.escapedName === escapedName); + + return escapedProperty + ? checker.getDeclaredTypeOfSymbol(escapedProperty) + : undefined; +} export function getTypeOfPropertyOfType( checker: ts.TypeChecker, type: ts.Type, - propertyName: string, -): ts.Type { - const reason = NullThrowsReasons.MissingToken( - `property "${propertyName}"`, - 'type', + property: ts.Symbol, +): ts.Type | undefined { + return getTypeOfPropertyOfName( + checker, + type, + property.getName(), + property.getEscapedName(), ); - - return propertyName.startsWith('__') - ? checker.getDeclaredTypeOfSymbol( - nullThrows( - type - .getProperties() - .find(property => property.escapedName === propertyName), - reason, - ), - ) - : nullThrows(checker.getTypeOfPropertyOfType(type, propertyName), reason); }