Skip to content

fix(eslint-plugin): [prefer-readonly-parameter-types] handle class sharp private field and member without throwing error #4343

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Mar 1, 2022
Merged

fix(eslint-plugin): [prefer-readonly-parameter-types] handle class sharp private field and member without throwing error #4343

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
43e34e4
fix: handle class sharp private field and member
lonyele Dec 24, 2021
cf9199f
fix: handle class sharp private field and member
lonyele Dec 24, 2021
915f1b3
Merge branch 'fix/handle-private-identifier' of https://github.com/lo…
lonyele Feb 11, 2022
9a8d4f9
feat: exempt private identifier from the rule
lonyele Feb 12, 2022
2754294
chore: shorten unnecessary logic
lonyele Feb 12, 2022
4ba1c7c
test: add unit test
lonyele Feb 13, 2022
33e4789
test: cover case that crashed before
lonyele Feb 13, 2022
0dc841a
test: add readonly test case
lonyele Feb 27, 2022
3d8acb4
Update packages/type-utils/src/propertyTypes.ts
JoshuaKGoldberg Feb 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/eslint-plugin/tests/rules/prefer-readonly-parameter-types.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: `
Expand Down Expand Up @@ -222,7 +232,6 @@ ruleTester.run('prefer-readonly-parameter-types', rule, {
},
],
},

// parameter properties should work fine
{
code: `
Expand Down
6 changes: 6 additions & 0 deletions packages/type-utils/src/isTypeReadonly.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,12 @@ function isTypeReadonlyObject(
continue;
}

const name = ts.getNameOfDeclaration(property.valueDeclaration);
const isPrivateIdentifier = name && ts.isPrivateIdentifier(name);
if (isPrivateIdentifier) {
continue;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there is no handy function for checking private identifier from ts or tsutils. I borrowed from a typescript's src/compiler/checker.ts that suits this situation.

 function checkIndexConstraintForProperty(type: Type, prop: Symbol, propNameType: Type, propType: Type) {
    const declaration = prop.valueDeclaration;
    const name = getNameOfDeclaration(declaration);
    if (name && isPrivateIdentifier(name)) {
        return;
    }
    ........
}

return Readonlyness.Mutable;
}

Expand Down
17 changes: 16 additions & 1 deletion packages/type-utils/src/propertyTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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('__#');
}