From 43e34e4d93dd8970f0b458d9c9c20c9b39ac66e6 Mon Sep 17 00:00:00 2001 From: lonyele Date: Fri, 24 Dec 2021 16:34:57 +0900 Subject: [PATCH 1/8] fix: handle class sharp private field and member --- .../prefer-readonly-parameter-types.test.ts | 15 +++++++++++++++ packages/type-utils/src/propertyTypes.ts | 17 ++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) 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 be50f096963..e178bafd5ff 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 @@ -222,6 +222,21 @@ ruleTester.run('prefer-readonly-parameter-types', rule, { }, ], }, + // PrivateIdentifier is handled without throwing error. + { + code: ` + class Foo { + readonly #privateField = 'foo'; + #privateMember() {} + } + function foo(arg: Foo) {} + `, + options: [ + { + treatMethodsAsReadonly: true, + }, + ], + }, // parameter properties should work fine { diff --git a/packages/type-utils/src/propertyTypes.ts b/packages/type-utils/src/propertyTypes.ts index 5e2f1054239..09e04b9049d 100644 --- a/packages/type-utils/src/propertyTypes.ts +++ b/packages/type-utils/src/propertyTypes.ts @@ -7,7 +7,7 @@ export function getTypeOfPropertyOfName( 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('__')) { + if (!escapedName || !isSymbol(escapedName)) { return checker.getTypeOfPropertyOfType(type, name); } @@ -34,3 +34,18 @@ export function getTypeOfPropertyOfType( property.getEscapedName(), ); } + +// Symbolic names need to be specially handled because TS api is not sufficient for these cases. +function isSymbol(escapedName: string): boolean { + return isKnownSymbol(escapedName) || isPrivateIdentifierSymbol(escapedName); +} + +// case for escapedName: "__@foo@10", name: "__@foo@10" +function isKnownSymbol(escapedName: string): boolean { + return escapedName.startsWith('__@'); +} + +// case for escapedName: "__#1@#foo", name: "#foo" +function isPrivateIdentifierSymbol(escapedName: string): boolean { + return escapedName.startsWith('__#'); +} From cf9199f9419b155a9e03d7c48163a2c94cc55ddb Mon Sep 17 00:00:00 2001 From: lonyele Date: Fri, 24 Dec 2021 16:34:57 +0900 Subject: [PATCH 2/8] fix: handle class sharp private field and member --- .../prefer-readonly-parameter-types.test.ts | 15 +++++++++++++++ packages/type-utils/src/propertyTypes.ts | 17 ++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) 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 f17a19f1f6d..c8fb885a73e 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 @@ -222,6 +222,21 @@ ruleTester.run('prefer-readonly-parameter-types', rule, { }, ], }, + // PrivateIdentifier is handled without throwing error. + { + code: ` + class Foo { + readonly #privateField = 'foo'; + #privateMember() {} + } + function foo(arg: Foo) {} + `, + options: [ + { + treatMethodsAsReadonly: true, + }, + ], + }, // parameter properties should work fine { diff --git a/packages/type-utils/src/propertyTypes.ts b/packages/type-utils/src/propertyTypes.ts index 5e2f1054239..09e04b9049d 100644 --- a/packages/type-utils/src/propertyTypes.ts +++ b/packages/type-utils/src/propertyTypes.ts @@ -7,7 +7,7 @@ export function getTypeOfPropertyOfName( 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('__')) { + if (!escapedName || !isSymbol(escapedName)) { return checker.getTypeOfPropertyOfType(type, name); } @@ -34,3 +34,18 @@ export function getTypeOfPropertyOfType( property.getEscapedName(), ); } + +// Symbolic names need to be specially handled because TS api is not sufficient for these cases. +function isSymbol(escapedName: string): boolean { + return isKnownSymbol(escapedName) || isPrivateIdentifierSymbol(escapedName); +} + +// case for escapedName: "__@foo@10", name: "__@foo@10" +function isKnownSymbol(escapedName: string): boolean { + return escapedName.startsWith('__@'); +} + +// case for escapedName: "__#1@#foo", name: "#foo" +function isPrivateIdentifierSymbol(escapedName: string): boolean { + return escapedName.startsWith('__#'); +} From 9a8d4f93d68f39db6af1e2c2372da11b9e273e59 Mon Sep 17 00:00:00 2001 From: lonyele Date: Sat, 12 Feb 2022 09:34:18 +0900 Subject: [PATCH 3/8] feat: exempt private identifier from the rule --- .../prefer-readonly-parameter-types.test.ts | 26 +++++++------------ packages/type-utils/src/isTypeReadonly.ts | 6 +++++ 2 files changed, 16 insertions(+), 16 deletions(-) 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 c8fb885a73e..18b3563151c 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 @@ -169,6 +169,16 @@ ruleTester.run('prefer-readonly-parameter-types', rule, { }; function bar(arg: MyType) {} `, + // PrivateIdentifier is exempt from this rule + { + code: ` + class Foo { + #privateField = 'foo'; + #privateMember() {} + } + function foo(arg: Foo) {} + `, + }, // methods treated as readonly { code: ` @@ -222,22 +232,6 @@ ruleTester.run('prefer-readonly-parameter-types', rule, { }, ], }, - // PrivateIdentifier is handled without throwing error. - { - code: ` - class Foo { - readonly #privateField = 'foo'; - #privateMember() {} - } - function foo(arg: Foo) {} - `, - options: [ - { - treatMethodsAsReadonly: true, - }, - ], - }, - // parameter properties should work fine { code: ` diff --git a/packages/type-utils/src/isTypeReadonly.ts b/packages/type-utils/src/isTypeReadonly.ts index 0ec24438943..00128a0ff42 100644 --- a/packages/type-utils/src/isTypeReadonly.ts +++ b/packages/type-utils/src/isTypeReadonly.ts @@ -157,6 +157,12 @@ function isTypeReadonlyObject( continue; } + const name = ts.getNameOfDeclaration(property.valueDeclaration); + const isPrivateIdentifier = name && ts.isPrivateIdentifier(name); + if (isPrivateIdentifier) { + continue; + } + return Readonlyness.Mutable; } From 2754294779f3f950276568e756619c178093d0c2 Mon Sep 17 00:00:00 2001 From: lonyele Date: Sat, 12 Feb 2022 09:47:38 +0900 Subject: [PATCH 4/8] chore: shorten unnecessary logic --- packages/type-utils/src/isTypeReadonly.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/type-utils/src/isTypeReadonly.ts b/packages/type-utils/src/isTypeReadonly.ts index 00128a0ff42..3098df3c435 100644 --- a/packages/type-utils/src/isTypeReadonly.ts +++ b/packages/type-utils/src/isTypeReadonly.ts @@ -158,8 +158,7 @@ function isTypeReadonlyObject( } const name = ts.getNameOfDeclaration(property.valueDeclaration); - const isPrivateIdentifier = name && ts.isPrivateIdentifier(name); - if (isPrivateIdentifier) { + if (name && ts.isPrivateIdentifier(name)) { continue; } From 4ba1c7cac441fcb87b5d55193a35fce55d8d9e73 Mon Sep 17 00:00:00 2001 From: lonyele Date: Sun, 13 Feb 2022 15:43:11 +0900 Subject: [PATCH 5/8] test: add unit test --- packages/type-utils/tests/isTypeReadonly.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/type-utils/tests/isTypeReadonly.test.ts b/packages/type-utils/tests/isTypeReadonly.test.ts index f6f2cebd12b..83c724928cd 100644 --- a/packages/type-utils/tests/isTypeReadonly.test.ts +++ b/packages/type-utils/tests/isTypeReadonly.test.ts @@ -82,6 +82,12 @@ describe('isTypeReadonly', () => { ['type Test = Readonly>;'], ['type Test = Readonly>;'], ])('handles fully readonly sets and maps', runTests); + + // Private Identifier. + // Note: It can't be accessed from outside of class thus exempt from the checks. + it.each([ + ['class Foo { #privateField = "foo"; #privateMember() {}; }'], + ])('treat private identifier as readonly', runTests); }); describe('is not readonly', () => { From 33e478913d528b1eb5432fbe2c67428ae5d0d5fd Mon Sep 17 00:00:00 2001 From: lonyele Date: Sun, 13 Feb 2022 15:55:12 +0900 Subject: [PATCH 6/8] test: cover case that crashed before --- .../tests/rules/prefer-readonly-parameter-types.test.ts | 9 +++++++++ 1 file changed, 9 insertions(+) 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 18b3563151c..eadd526ca09 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 @@ -179,6 +179,15 @@ ruleTester.run('prefer-readonly-parameter-types', rule, { function foo(arg: Foo) {} `, }, + { + code: ` + class HasText { + readonly #text: string; + } + + export function onDone(task: HasText): void {} + `, + }, // methods treated as readonly { code: ` From 0dc841a71dc2ae5c8f1948ad3f1bdcd7018873d8 Mon Sep 17 00:00:00 2001 From: lonyele Date: Sun, 27 Feb 2022 14:16:01 +0900 Subject: [PATCH 7/8] test: add readonly test case --- packages/type-utils/tests/isTypeReadonly.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/type-utils/tests/isTypeReadonly.test.ts b/packages/type-utils/tests/isTypeReadonly.test.ts index 83c724928cd..0cbda39727a 100644 --- a/packages/type-utils/tests/isTypeReadonly.test.ts +++ b/packages/type-utils/tests/isTypeReadonly.test.ts @@ -86,7 +86,9 @@ describe('isTypeReadonly', () => { // Private Identifier. // Note: It can't be accessed from outside of class thus exempt from the checks. it.each([ - ['class Foo { #privateField = "foo"; #privateMember() {}; }'], + ['class Foo { readonly #readonlyPrivateField = "foo"; }'], + ['class Foo { #privateField = "foo"; }'], + ['class Foo { #privateMember() {}; }'], ])('treat private identifier as readonly', runTests); }); From 3d8acb40beb597266d26f0261e01c96d252138f3 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 28 Feb 2022 11:50:07 -0500 Subject: [PATCH 8/8] Update packages/type-utils/src/propertyTypes.ts --- packages/type-utils/src/propertyTypes.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/type-utils/src/propertyTypes.ts b/packages/type-utils/src/propertyTypes.ts index 09e04b9049d..7e064ea0ace 100644 --- a/packages/type-utils/src/propertyTypes.ts +++ b/packages/type-utils/src/propertyTypes.ts @@ -36,6 +36,8 @@ export function getTypeOfPropertyOfType( } // Symbolic names need to be specially handled because TS api is not sufficient for these cases. +// Source based on: +// https://github.com/microsoft/TypeScript/blob/0043abe982aae0d35f8df59f9715be6ada758ff7/src/compiler/utilities.ts#L3388-L3402 function isSymbol(escapedName: string): boolean { return isKnownSymbol(escapedName) || isPrivateIdentifierSymbol(escapedName); }