-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
Conversation
Thanks for the PR, @lonyele! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
❌ Deploy Preview for typescript-eslint failed. 🔨 Explore the source changes: 3d8acb4 🔍 Inspect the deploy log: https://app.netlify.com/sites/typescript-eslint/deploys/621cfd42804d350008c96c22 |
Codecov Report
@@ Coverage Diff @@
## main #4343 +/- ##
==========================================
+ Coverage 92.49% 92.83% +0.34%
==========================================
Files 346 164 -182
Lines 11696 8490 -3206
Branches 3340 2725 -615
==========================================
- Hits 10818 7882 -2936
+ Misses 608 406 -202
+ Partials 270 202 -68
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something - but I'm not sure how this changes the logic.
Before it checked for __<string>
, now it checks for __@<string>
or __#<string>
. Aren't the new checks covered by the old check?
The code is handling parameter properties but what about some complicated combos of public/private fields and members?
The code should just iterate over the properties of the object. If they can be seen as readonly, then it's readonly - else it's mutable. It depends entirely on what the TS APIs reports when the type is interrogated.
I'd assume that the TS APIs would not report the private/protected members unless they're accessible at that location.
// PrivateIdentifier is handled without throwing error. | ||
{ | ||
code: ` | ||
class Foo { | ||
readonly #privateField = 'foo'; | ||
#privateMember() {} | ||
} | ||
function foo(arg: Foo) {} | ||
`, | ||
options: [ | ||
{ | ||
treatMethodsAsReadonly: true, | ||
}, | ||
], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to double check - this case is ignored when #privateField
is not readonly
as well?
Could you add a test to verify this please? EG this should not error:
{
code: `
class Foo {
#privateField = 'foo';
#privateMember() {}
}
function foo(arg: Foo) {}
`,
options: [
{
treatMethodsAsReadonly: false,
},
],
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the quick answer, it is not. It fails on test. But not throwing an exception.
{
code: `
class Foo {
#privateField = 'foo'; ---> test fails becuase it is not readonly
#privateMember() {}. ---> test fails because "treatMethodsAsReadonly" is set to "false"
}
function foo(arg: Foo) {}
`,
options: [
{
treatMethodsAsReadonly: false,
},
],
},
Additional info is that before the fix, #privateMember
throws an exception when treatMethodsAsReadonly: true
, but no exception and test fails on treatMethodsAsReadonly: false
After the fix, whether private method with treatMethodsAsReadonly
is set true/false or private field with readonly
or not. There is no exception thrown.
Here are some results of after the fix
{
code: `
class Foo {
#privateField1 = 'foo'; ----> test fails becuase it is not readonly
readonly #privateField2 = 'foo'; -----> test passed because it's readonly
private privateField3 = 'foo'; ----> test fails becuase it's not readonly
private readonly privateField4 = 'foo'; ----> test passed because it's readonly
#privateMember1() {} ----> test fails because "treatMethodsAsReadonly" is set false
private #privateMember2() {} ----> same as above
private privateMember3() {} ----> same as above
}
function foo(arg: Foo) {}
`,
options: [
{
treatMethodsAsReadonly: false,
},
],
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting.
For private
fields - we 100% should be treating them as "public" (and thus subject them to readonly
checks), however for #private
fields, they should be exempt from readonly
checks.
The intent behind the rule is that you should not be able to mutate the type at runtime. You can access private
fields via computed member access syntax, but because you cannot access #private
fields at all outside the class - they shouldn't be included in the checks.
(playground)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the playground. I didn't know about that. I just enjoy all your comments on any issues or prs. I think very constructive(very educational to me) :D
please check this commit
The difference is what to check. Changed code use
I just wrote a little bit more explicitly why Why |
sounds good! |
Not sure why my reply to your request on additional test is not showing on this main page. |
wierdly I can't see your reply there either. |
@@ -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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoshuaKGoldberg I also checked your previous pr and this tsutil's handling. I was wondering handling this if statement with using escapedName
is still ok
tsutil handling
|
||
// 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('__#'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took it from typescript codebase
export function getPropertyNameForUniqueESSymbol(symbol: Symbol): __String {
return `__@${getSymbolId(symbol)}@${symbol.escapedName}` as __String;
}
export function getSymbolNameForPrivateIdentifier(containingClassSymbol: Symbol, description: __String): __String {
return `__#${getSymbolId(containingClassSymbol)}@${description}` as __String;
}
export function isKnownSymbol(symbol: Symbol): boolean {
return startsWith(symbol.escapedName as string, "__@");
}
export function isPrivateIdentifierSymbol(symbol: Symbol): boolean {
return startsWith(symbol.escapedName as string, "__#");
}
// PrivateIdentifier is handled without throwing error. | ||
{ | ||
code: ` | ||
class Foo { | ||
readonly #privateField = 'foo'; | ||
#privateMember() {} | ||
} | ||
function foo(arg: Foo) {} | ||
`, | ||
options: [ | ||
{ | ||
treatMethodsAsReadonly: true, | ||
}, | ||
], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the quick answer, it is not. It fails on test. But not throwing an exception.
{
code: `
class Foo {
#privateField = 'foo'; ---> test fails becuase it is not readonly
#privateMember() {}. ---> test fails because "treatMethodsAsReadonly" is set to "false"
}
function foo(arg: Foo) {}
`,
options: [
{
treatMethodsAsReadonly: false,
},
],
},
Additional info is that before the fix, #privateMember
throws an exception when treatMethodsAsReadonly: true
, but no exception and test fails on treatMethodsAsReadonly: false
After the fix, whether private method with treatMethodsAsReadonly
is set true/false or private field with readonly
or not. There is no exception thrown.
Here are some results of after the fix
{
code: `
class Foo {
#privateField1 = 'foo'; ----> test fails becuase it is not readonly
readonly #privateField2 = 'foo'; -----> test passed because it's readonly
private privateField3 = 'foo'; ----> test fails becuase it's not readonly
private readonly privateField4 = 'foo'; ----> test passed because it's readonly
#privateMember1() {} ----> test fails because "treatMethodsAsReadonly" is set false
private #privateMember2() {} ----> same as above
private privateMember3() {} ----> same as above
}
function foo(arg: Foo) {}
`,
options: [
{
treatMethodsAsReadonly: false,
},
],
},
wow... how newbie mistake it is. Actually I didn't even know there is this pending system exists in github. holy cow. though kind of weird that I also checked in incognito mode whether it was actually published... anyway... |
b63de76
to
43e34e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with bradzacher here. Thanks for the discussion + PR so far! ❤️
Would like to see some tests of this behavior too please :)
43e34e4
to
cf9199f
Compare
// PrivateIdentifier is handled without throwing error. | ||
{ | ||
code: ` | ||
class Foo { | ||
readonly #privateField = 'foo'; | ||
#privateMember() {} | ||
} | ||
function foo(arg: Foo) {} | ||
`, | ||
options: [ | ||
{ | ||
treatMethodsAsReadonly: true, | ||
}, | ||
], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the playground. I didn't know about that. I just enjoy all your comments on any issues or prs. I think very constructive(very educational to me) :D
please check this commit
const name = ts.getNameOfDeclaration(property.valueDeclaration); | ||
const isPrivateIdentifier = name && ts.isPrivateIdentifier(name); | ||
if (isPrivateIdentifier) { | ||
continue; | ||
} | ||
|
There was a problem hiding this comment.
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;
}
........
}
@JoshuaKGoldberg Thanks! just to double-check, what does it mean by |
What I mean is, Codecov is pointing out that there is at least one line in I think there are a couple of options you could go with:
Does that help @lonyele? |
@JoshuaKGoldberg Absolutely! thanks for the clear guide! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Source code looks good to me in general (alas, TypeScript API woes...) but missing a crashing test case from the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super, thanks for all your work on this @lonyele! 🙌
Since @bradzacher also reviewed I'll wait a bit to give them a chance to re-review too.
LGTM! Thanks! |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.13.0` -> `5.14.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.13.0/5.14.0) | | [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.13.0` -> `5.14.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.13.0/5.14.0) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@​typescript-eslint/eslint-plugin)</summary> ### [`v5.14.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#​5140-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5130v5140-2022-03-07) [Compare Source](typescript-eslint/typescript-eslint@v5.13.0...v5.14.0) ##### Bug Fixes - **eslint-plugin:** \[naming-convention] cover case that requires quotes ([#​4582](typescript-eslint/typescript-eslint#4582)) ([3ea0947](typescript-eslint/typescript-eslint@3ea0947)) - **eslint-plugin:** \[no-misused-promises] factor thenable returning function overload signatures ([#​4620](typescript-eslint/typescript-eslint#4620)) ([56a09e9](typescript-eslint/typescript-eslint@56a09e9)) - **eslint-plugin:** \[prefer-readonly-parameter-types] handle class sharp private field and member without throwing error ([#​4343](typescript-eslint/typescript-eslint#4343)) ([a65713a](typescript-eslint/typescript-eslint@a65713a)) - **eslint-plugin:** \[return-await] correct autofixer in binary expression ([#​4401](typescript-eslint/typescript-eslint#4401)) ([5fa2fad](typescript-eslint/typescript-eslint@5fa2fad)) ##### Features - **eslint-plugin:** \[no-misused-promises] add granular options within `checksVoidReturns` ([#​4623](typescript-eslint/typescript-eslint#4623)) ([1085177](typescript-eslint/typescript-eslint@1085177)) </details> <details> <summary>typescript-eslint/typescript-eslint (@​typescript-eslint/parser)</summary> ### [`v5.14.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#​5140-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5130v5140-2022-03-07) [Compare Source](typescript-eslint/typescript-eslint@v5.13.0...v5.14.0) **Note:** Version bump only for package [@​typescript-eslint/parser](https://github.com/typescript-eslint/parser) </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <[email protected]> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1201 Reviewed-by: 6543 <[email protected]> Co-authored-by: Calciumdibromid Bot <[email protected]> Co-committed-by: Calciumdibromid Bot <[email protected]>
PR Checklist
Overview
It was my first time diving into typescript code and it was a rabbit hole... I feel like it's a bug from typescript that other symbols have the same
escapedName
andname
likeescapedName: "__@foo@10", name: "__@foo@10"
but this#
private identifiers have differentescapedName
andname
likeescapedName: "__#1@#foo", name: "#foo"
. This is why the code goes intochecker.getTypeOfPropertyOfType(type, name)
and returnundefined
that at the end throws the error.I stepped into
const type = checker.getTypeAtLocation(tsNode)
to find whyescapedName
andname
are different but failed...even going into the endAside from handling this error case from
typescript-eslint
side. I'm curious how this rule should handles the Class type. The code is handling parameter properties but what about some complicated combos of public/private fields and members? I haven't really used to this extend so I'm purely interested in what behavior is expected. I found that making read-only member in class is impossible but private or this# private
is possible which is automatically readonly? more precisely just cannot be accessed on compile(private) or on runtime(# private)