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

[prefer-readonly] False positive on fields within mixin classes #2590

Closed
WGroenestein opened this issue Sep 22, 2020 · 3 comments · Fixed by #4974
Closed

[prefer-readonly] False positive on fields within mixin classes #2590

WGroenestein opened this issue Sep 22, 2020 · 3 comments · Fixed by #4974
Assignees
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@WGroenestein
Copy link

Repro
Using the repo code from https://github.com/WGroenestein/typescript-eslint-prefer-readonly-class-mixin

  1. run "npm install"
  2. run "npm run build"
  3. see that the build compiles without errors
  4. run "npm run lint"
  5. see that it fails with error
file.ts
  7:3  error  Member '_name' is never reassigned; mark it as `readonly`  @typescript-eslint/prefer-readonly

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

  1. run "npm run fix-lint"
  2. see that it prefixed "_name" in "file.ts" with readonly
  3. run "npm run build"
  4. see that the build fails with error
file.ts(10,9): error TS2540: Cannot assign to '_name' because it is a read-only property.

Expected Result

Fields which are assigned outside the constructor (e.g. setters) in mixin classes are not marked as not assigned / readonly

Actual Result

They are reported as errors, and when running --fix it breaks the build

Additional Info

The debug log can be found in the repo (https://github.com/WGroenestein/typescript-eslint-prefer-readonly-class-mixin/blob/master/debug.log)

Versions

package version
@typescript-eslint/eslint-plugin 4.2.0
@typescript-eslint/parser 4.2.0
TypeScript 4.0.3
ESLint 7.9.0
node 12.18.3
@WGroenestein WGroenestein added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Sep 22, 2020
@bradzacher bradzacher added bug Something isn't working and removed triage Waiting for maintainers to take a look labels Sep 22, 2020
@bradzacher
Copy link
Member

Had a quick look and the bug occurs due to the type of the class that TS reports.
It reports that the type of the parent class is a dynamic type: {new (...args: any[]) => T} & TBase
This doesn't have a symbol attached, so the tooling just skips it, hence it messes up.

@sosoba
Copy link

sosoba commented Jul 14, 2021

Another example:

export class Foo {

  constructor(private bar: string) {
  }

  getBar(){
    return this.bar;
  }

  setBar(){
    this.bar = '';
  }

}

Rule says: 3:15 warning Member 'bar: string' is never reassigned; mark it as 'readonly' which is not true .

@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Oct 25, 2021
@armano2
Copy link
Member

armano2 commented May 14, 2022

this seem like issue with types produced by typescript, generally i do not like that this rule is using type checking, as there should no be a need for it, we could achieve all of this by doing static code analysis,

function ClassWithName<TBase extends new (...args: any[]) => {}>(Base: TBase) {
	return class extends Base {

		private _name: string;

		public test(value: string) {
			this._name = value;
		}
	}
}

in this case, typescript returns us IntersectionType without symbol

in typeFlags we are receiving Intersection = 2097152 instead of Object = 524288, we can simply fix it by adding handling to this case, but we should def report this to typescript team

microsoft/TypeScript#49116

@armano2 armano2 added has pr there is a PR raised to close this and removed accepting prs Go ahead, send a pull request that resolves this issue labels May 14, 2022
@armano2 armano2 self-assigned this May 14, 2022
@armano2 armano2 added the accepting prs Go ahead, send a pull request that resolves this issue label May 21, 2022
bradzacher added a commit that referenced this issue May 23, 2022
…ctions (#4974)

* fix(eslint-plugin): [prefer-readonly] correct issue with anonymus functions #2590

* test(eslint-plugin): add missing invalid case

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
5 participants