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

Conversation

lonyele
Copy link
Contributor

@lonyele lonyele commented Dec 24, 2021

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 and name like escapedName: "__@foo@10", name: "__@foo@10" but this# private identifiers have different escapedName and name like escapedName: "__#1@#foo", name: "#foo". This is why the code goes into checker.getTypeOfPropertyOfType(type, name) and return undefined that at the end throws the error.

I stepped into const type = checker.getTypeAtLocation(tsNode) to find why escapedName and name are different but failed...even going into the end

Aside 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)

@nx-cloud
Copy link

nx-cloud bot commented Dec 24, 2021

☁️ Nx Cloud Report

CI ran the following commands for commit 3d8acb4. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 48 targets

Sent with 💌 from NxCloud.

@typescript-eslint
Copy link
Contributor

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.

@netlify
Copy link

netlify bot commented Dec 24, 2021

❌ 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
Copy link

codecov bot commented Dec 24, 2021

Codecov Report

Merging #4343 (3d8acb4) into main (d698f6b) will increase coverage by 0.34%.
The diff coverage is 100.00%.

@@            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     
Flag Coverage Δ
unittest 92.83% <100.00%> (+0.34%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/type-utils/src/isTypeReadonly.ts 85.14% <100.00%> (+0.45%) ⬆️
packages/type-utils/src/propertyTypes.ts 90.90% <100.00%> (+3.40%) ⬆️
...ges/eslint-plugin/src/rules/no-misused-promises.ts 98.63% <0.00%> (-1.37%) ⬇️
...ckages/eslint-plugin/src/util/getESLintCoreRule.ts 75.00% <0.00%> (ø)
...plugin/src/rules/explicit-module-boundary-types.ts 91.04% <0.00%> (ø)
...ackages/scope-manager/src/variable/VariableBase.ts
packages/typescript-estree/src/version-check.ts
packages/scope-manager/src/lib/esnext.intl.ts
...kages/scope-manager/src/referencer/ClassVisitor.ts
packages/scope-manager/src/scope/FunctionScope.ts
... and 190 more

@bradzacher bradzacher added the bug Something isn't working label Dec 27, 2021
Copy link
Member

@bradzacher bradzacher left a 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.

Comment on lines 225 to 239
// PrivateIdentifier is handled without throwing error.
{
code: `
class Foo {
readonly #privateField = 'foo';
#privateMember() {}
}
function foo(arg: Foo) {}
`,
options: [
{
treatMethodsAsReadonly: true,
},
],
},
Copy link
Member

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,
        },
      ],
    },

Copy link
Contributor Author

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,
     }, 
   ],
},

Copy link
Member

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)

Copy link
Contributor Author

@lonyele lonyele Feb 12, 2022

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

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Dec 27, 2021
@lonyele
Copy link
Contributor Author

lonyele commented Dec 27, 2021

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 difference is what to check. Changed code use escapedName instead of name. Technically fix can be simple as this.

from previous code 
if (!escapedName || !name.startsWith('__')) { ... }

to fixed code
if (!escapedName || !escapedName.startsWith('__')) { ... }

I just wrote a little bit more explicitly why .startsWith('__') is needed using isKnowSymbol and isPrivateIdentifierSymbol with comments.

Why escapedName should be used instaed of name? because of this case. #private's escapedName and name are different. reference
case for escapedName: "__@foo@10", name: "__@foo@10"
case for escapedName: "__#1@#foo", name: "#foo"

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed awaiting response Issues waiting for a reply from the OP or another party labels Dec 28, 2021
@bradzacher
Copy link
Member

sounds good!
just a response for my above query about the tests and we're good to go

@lonyele
Copy link
Contributor Author

lonyele commented Dec 28, 2021

Not sure why my reply to your request on additional test is not showing on this main page.
But i actually replied it, please go to the Files Changed section

@bradzacher
Copy link
Member

wierdly I can't see your reply there either.
Could you please try replying again?
You might need to finish "reviewing" the PR to make the comment public.

@@ -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)) {
Copy link
Contributor Author

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

Comment on lines 37 to 51

// 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('__#');
}
Copy link
Contributor Author

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, "__#");
}

Comment on lines 225 to 239
// PrivateIdentifier is handled without throwing error.
{
code: `
class Foo {
readonly #privateField = 'foo';
#privateMember() {}
}
function foo(arg: Foo) {}
`,
options: [
{
treatMethodsAsReadonly: true,
},
],
},
Copy link
Contributor Author

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,
     }, 
   ],
},

@lonyele
Copy link
Contributor Author

lonyele commented Jan 1, 2022

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...

@lonyele lonyele force-pushed the fix/handle-private-identifier branch from b63de76 to 43e34e4 Compare January 1, 2022 07:35
@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed awaiting response Issues waiting for a reply from the OP or another party labels Jan 11, 2022
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a 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 :)

@lonyele lonyele force-pushed the fix/handle-private-identifier branch from 43e34e4 to cf9199f Compare February 11, 2022 20:50
Comment on lines 225 to 239
// PrivateIdentifier is handled without throwing error.
{
code: `
class Foo {
readonly #privateField = 'foo';
#privateMember() {}
}
function foo(arg: Foo) {}
`,
options: [
{
treatMethodsAsReadonly: true,
},
],
},
Copy link
Contributor Author

@lonyele lonyele Feb 12, 2022

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

Comment on lines 160 to 165
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;
    }
    ........
}

@lonyele
Copy link
Contributor Author

lonyele commented Feb 12, 2022

@JoshuaKGoldberg Thanks! just to double-check, what does it mean by some tests of this behavior? add more tests at prefer-readonly-parameter-types.test.ts or somewhere else?(codecov?)

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 12, 2022
@JoshuaKGoldberg
Copy link
Member

some tests of this behavior

What I mean is, Codecov is pointing out that there is at least one line in isTypeReadonly.ts that isn't covered by unit tests. So there should be some unit test(s) somewhere that run that line of code to make sure it works & stays working.

I think there are a couple of options you could go with:

Does that help @lonyele?

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Feb 12, 2022
@lonyele
Copy link
Contributor Author

lonyele commented Feb 13, 2022

@JoshuaKGoldberg Absolutely! thanks for the clear guide!

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 14, 2022
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a 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.

packages/type-utils/src/propertyTypes.ts Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Feb 23, 2022
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a 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.

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 1, 2022
@bradzacher
Copy link
Member

LGTM! Thanks!

@bradzacher bradzacher merged commit a65713a into typescript-eslint:main Mar 1, 2022
@lonyele lonyele deleted the fix/handle-private-identifier branch March 1, 2022 04:45
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 8, 2022
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 (@&#8203;typescript-eslint/eslint-plugin)</summary>

### [`v5.14.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#&#8203;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 ([#&#8203;4582](typescript-eslint/typescript-eslint#4582)) ([3ea0947](typescript-eslint/typescript-eslint@3ea0947))
-   **eslint-plugin:** \[no-misused-promises] factor thenable returning function overload signatures ([#&#8203;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 ([#&#8203;4343](typescript-eslint/typescript-eslint#4343)) ([a65713a](typescript-eslint/typescript-eslint@a65713a))
-   **eslint-plugin:** \[return-await] correct autofixer in binary expression ([#&#8203;4401](typescript-eslint/typescript-eslint#4401)) ([5fa2fad](typescript-eslint/typescript-eslint@5fa2fad))

##### Features

-   **eslint-plugin:** \[no-misused-promises] add granular options within `checksVoidReturns` ([#&#8203;4623](typescript-eslint/typescript-eslint#4623)) ([1085177](typescript-eslint/typescript-eslint@1085177))

</details>

<details>
<summary>typescript-eslint/typescript-eslint (@&#8203;typescript-eslint/parser)</summary>

### [`v5.14.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#&#8203;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 [@&#8203;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 <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1201
Reviewed-by: 6543 <6543@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
3 participants