Skip to content
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.

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

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

Merged
Expand Up @@ -169,6 +169,25 @@ 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) {}
`,
},
{
code: `
class HasText {
readonly #text: string;
}

export function onDone(task: HasText): void {}
`,
},
// methods treated as readonly
{
code: `
Expand Down Expand Up @@ -222,7 +241,6 @@ ruleTester.run('prefer-readonly-parameter-types', rule, {
},
],
},

// parameter properties should work fine
{
code: `
Expand Down
5 changes: 5 additions & 0 deletions packages/type-utils/src/isTypeReadonly.ts
Expand Up @@ -157,6 +157,11 @@ function isTypeReadonlyObject(
continue;
}

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

return Readonlyness.Mutable;
}

Expand Down
17 changes: 16 additions & 1 deletion packages/type-utils/src/propertyTypes.ts
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.
lonyele marked this conversation as resolved.
Show resolved Hide resolved
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
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('__#');
}
8 changes: 8 additions & 0 deletions packages/type-utils/tests/isTypeReadonly.test.ts
Expand Up @@ -82,6 +82,14 @@ describe('isTypeReadonly', () => {
['type Test = Readonly<ReadonlySet<string>>;'],
['type Test = Readonly<ReadonlyMap<string, string>>;'],
])('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 { readonly #readonlyPrivateField = "foo"; }'],
['class Foo { #privateField = "foo"; }'],
['class Foo { #privateMember() {}; }'],
])('treat private identifier as readonly', runTests);
});

describe('is not readonly', () => {
Expand Down