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.

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

Jump to bottom

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

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 1 commit
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/eslint-plugin/tests/rules/prefer-readonly-parameter-types.test.ts
View file
Open in desktop
Expand Up @@ -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,
},
],
},
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 •
edited

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


// parameter properties should work fine
{
Expand Down
17 changes: 16 additions & 1 deletion packages/type-utils/src/propertyTypes.ts
View file
Open in desktop
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('__#');
}