Skip to content

fix(eslint-plugin): [class-literal-property-style] broken fixer when a readonly property is assigned in the constructor #2256

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

Closed
wants to merge 9 commits into from

Conversation

ChaiWithJai
Copy link

A breaking change was reported (#2206 ) such that when the following rules are applied, the following two cases are incorrectly showing errors:

Rules:

{
  "rules": {
    "@typescript-eslint/class-literal-property-style": ["error", "getters"],
    "@typescript-eslint/prefer-readonly": "warn"
  }
}
export class A {
  private readonly foo: string = 'bar';

  constructor(foo: string) {
    this.foo = foo;
  }
}

Expected Result
Code should be unchanged

Actual Result
Code after fix is invalid

export class A {
  private get foo() { return 'bar'; }

  constructor(foo: string) {
    this.foo = foo;
  }
}

Versions applied when bug was reported:

package version
@typescript-eslint/eslint-plugin 3.2.0
@typescript-eslint/parser 3.2.0
TypeScript 3.9.5
ESLint 7.2.0
node 14.2.0
npm 6.14.5
yarn 1.22.4

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @JBhagat841!

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.

@ChaiWithJai
Copy link
Author

@JoshuaKGoldberg throwing up a PR for this and wanted to keep you in the loop.

@bradzacher bradzacher added the bug Something isn't working label Jun 25, 2020
@codecov
Copy link

codecov bot commented Jun 25, 2020

Codecov Report

Merging #2256 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2256   +/-   ##
=======================================
  Coverage   93.12%   93.12%           
=======================================
  Files         283      283           
  Lines        9057     9060    +3     
  Branches     2482     2484    +2     
=======================================
+ Hits         8434     8437    +3     
  Misses        301      301           
  Partials      322      322           
Flag Coverage Δ
#unittest 93.12% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...t-plugin/src/rules/class-literal-property-style.ts 100.00% <100.00%> (ø)

@bradzacher bradzacher changed the title fix(eslint-plug): [class-literal property-style] fix breaking change fix(eslint-plugin): [class-literal-property-style] broken fixer when a readonly property is assigned in the constructor Jun 26, 2020
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.

This isn't quite right - we don't want to completely ignore private readonly properties with a type annotation.

Instead what we want to do is analyse the constructor for assignments, and disable the fixer if there is one.

We still want to report an error for these cases, we just don't want to automatically fix them.

I.e. in this case, we should not auto-fix, but should still report:

export class A {
  private readonly foo: string;

  constructor(foo: string) {
    this.foo = foo;
  }
}

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jul 6, 2020
{
code: `
class Mx {
private readonly foo: string = 'bar';
Copy link
Author

Choose a reason for hiding this comment

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

@bradzacher so change this test case to:

export class A {
  private readonly foo: string;

  constructor(foo: string) {
    this.foo = foo;
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

I would have both.

private readonly foo: string = 'bar'; should be an invalid case for the config options: ['getters'], and it should be fixed appropriately.

As mentioned; the case with the assignment in the constructor should still be invalid, but there should be no auto-fix for it (use output: null to test this).

@@ -100,7 +102,31 @@ export default util.createRule<Options, MessageIds>({
}

return {
'MemberExpression > Identifier:exit'(node: TSESTree.Identifier): void {
Copy link
Author

Choose a reason for hiding this comment

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

@JoshuaKGoldberg read the documentation you suggested. Do you think I'm getting warmer on having a solution that solves the actual bug?

I realize this code can be much cleaner, but wanted to just know if I'm going in the right direction.

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg Jul 26, 2020

Choose a reason for hiding this comment

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

Ok cool @JBhagat841 and I discussed this a bit; summarizing the key takeaways I had:

  • Rule walkers do a single linear traversal through the node tree, so these :exits don't quote match up with the expected behavior.
  • The strategy going forward will be to:
    1. Collect class properties that might need to be warned or errored on as they are found
    2. Inside the class constructor, mark any property assignments as warnings for those class properties
    3. Upon exiting the class constructor, mark any remaining property assignments as errors
  • As with prefer-readonly, you might have to use parser services and will almost certainly need a stack of classes to make sure you're applying node warnings to the right class

That last point is where the trickiness comes in. Please enjoy these very convoluted edge cases 😄

const keyName1 = 'wat1'
const keyName2 = 'wat2'

class Parent {
  private readonly wat1 = true;
  private readonly wat2 = true;

  constructor() {
    this['wat1'] = true;
    this[keyName1] = true;
    this.wat1 = true;

    class Child {
      private readonly wat1 = true;

      constructor() {
        this['wat1'] = true;
        this[keyName1] = true;

        console.log(class GrandChild extends Child {
          private readonly wat2 = true;

          constructor() {
            super();
            this.wat2 = true;
            this[keyName2] = true;
          }
        })
      }
    }
  }
}

(personally, I don't think we need to consider absurdities such as this[keyName1] as examples of a property being written to for this rule, as those are often impossible to accurately understand the resolved key name in static analysis...)

Copy link
Member

Choose a reason for hiding this comment

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

you might have to use parser services

Requiring type information is a breaking change, so I'd prefer to not use it unless we reallllyyyy need it.

Copy link
Author

Choose a reason for hiding this comment

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

Translating this into test cases:

  1. test case of computed value this['wat1'] = 123
  2. rescursive case of class in constructor

would the following 2 invalid cases capture what you listed?

class Mx {
  private readonly wat1 = true;

  constructor() {
    this['wat1'] = true;
  }
}
class Mx {
  constructor() {
    class Child {
      private readonly foo = true;

      constructor() {
        this.foo = true;
      }
  	}
  }
}

@bradzacher bradzacher closed this Oct 13, 2020
@bradzacher bradzacher added stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period and removed awaiting response Issues waiting for a reply from the OP or another party labels Oct 13, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants