Skip to content

Bug: [prefer-readonly] False positive if property was assigned after type guard for class properties #7221

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
4 tasks done
erengeez opened this issue Jul 14, 2023 · 3 comments · Fixed by #8169
Closed
4 tasks done
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@erengeez
Copy link

erengeez commented Jul 14, 2023

Before You File a Bug Report Please Confirm You Have Done The Following...

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=5.1.6&fileType=.tsx&code=MYGwhgzhAEAqCmEAu0DeAoa0AOBXARiAJbDQB288AJhLAPYBC8AIvAGZEVUD8AXObgC2%2BeACcA3Okw5RRAG5gk8cnSQAleGCp0yIAJ4AxOnT4DhYydOyyFSles3bdehmFGmyQkRKlY8hEmglZABZeCQACzoqIzoACgBKNGksIjZoOIBCSKIIADpQSAgABVE6bDEkPQBxXDcqRKSMLBboUXDcUTJJVoBfFKCI3LyyVQ0tHX1Y6ABeaABGHuh%2BqwJiUmCkMMjo11FE5Na0jOyh-NHRQTAQWD0K2vq4nPPKGnomVg4uBKaBrHakJ1ugN%2Bq1niMxo5Ji43LNoAAmJYrPw2RTKQpQUrlSo1OqiBoJfjPaC5QakgBkaHIr1ojBY7E41H4nnMomWhxaAKBZJe1FpHwZXGgmRmc1wZCoguoSN8MnkaJUl2ut3ueIaChAuHgzK8YmgAB9oOLJV9qIToBqtSSYCzvBz-h0uhbrlaRWKJVKqDLekA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Y6RAM0Wls4CGAEwD2TeIXRRe0EdEjgwAXxBKgA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylAGYBDVCXTgwAXxCygA&tokens=false

Repro Code

class Test {
  public needsToBeDefined?: number;

  private notReadonlyFoo?: number;

  private notReadonlyBar?: number;

  public testMethodFoo() {
    if (!this.classPropertyGuard()) {
      return;
    }
    this.notReadonlyFoo = 1;
  }

  public testMethodBar() {
    if (!this.normalTypeGuard(this.needsToBeDefined)) {
      return;
    }
    this.notReadonlyBar = 2;
  }

  private classPropertyGuard(): this is this & { needsToBeDefined: number } {
    return this.needsToBeDefined !== undefined;
  }

  private normalTypeGuard(value: number | undefined): value is number {
    return value !== undefined;
  }
}

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/prefer-readonly": "error"
  },
};

tsconfig

No response

Expected Result

I expected that private notReadonlyFoo?: number; would not report any errors.

Actual Result

@typescript-eslint/prefer-readonly error on the 4th line:

Member 'notReadonlyFoo' is never reassigned; mark it as `readonly`. 4:3 - 4:35

Additional Info

No response

@erengeez erengeez added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Jul 14, 2023
@JoshuaKGoldberg
Copy link
Member

🤔 I don't understand what's being asked for here. Also, the playground link and code snippet aren't the same code. Could you please edit the post so they're the same, and provide an explanation for why the private notReadonly?: number; shouldn't report any errors?

@JoshuaKGoldberg JoshuaKGoldberg added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels Jul 14, 2023
@erengeez
Copy link
Author

erengeez commented Jul 15, 2023

notReadonlyFoo is clearly reassigned in the testMethodFoo() method, but eslint instructs to mark it as readonly. It is possible to reach the code with the assignment after type guard. If you use classic type guard like normalTypeGuard(value) there is no error, but it is present if you use a hack to typeguard a class member (like here)

@JoshuaKGoldberg
Copy link
Member

Ah, got it - thanks. That makes more sense. This does seem like a bug.

I found the original code snippets a little confusing so I made a smaller reproduction case: https://typescript-eslint.io/play/#ts=5.1.6&fileType=.tsx&code=MYGwhgzhAEAqCmEAu0DeAoa0AOBXARiAJbDQC2YAnvvAPwBc0AdrmTQE4Dc6mO7RANzBJ4zAPZIASvDAATMUxCUAcmPYUQDZqw7de2fkJHipM%2BYsqwAFkQhaWbeF14B6F9ACy8R%2B2gByJglpOQUla1s-aFtmeAEnaHYZKCIAcyZ4WU5yMHYAayiUSGgAA0SQi2KAOmgeLDxCEmgRZFhKbHgAcVwc2QgACgBKNF4sIgAzaD6kGwhK4Ct4YFzVdTAQQaGMLG2mmcrA03KlFY1oAF5oAAZuHYBfWu3xyenbOYWl1vaunvD%2Bgc2RtsXrMDsFzGEZucrjdtvcsPd9IZhKJ5otlmoNINhjtEkhcOwmLtXhRqKIAIRnC64JiyeBjIjpTK8BF1JHGVEfNqdbrsWS-QaMYFRGBCgBkaGypMYDg40Fu2O2uPxhOBlRJNGgFKpNLpDIyMPutyAA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Y6RAM0Wls4CGAEwD2TeIXRRe0EdEjgwAXxBKgA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylAGYBDVCXTgwAXxCygA&tokens=false

class Test {
  public maybe?: number;

  private notReadonlyNormal?: number;

  private notReadonlyThis?: number;
  // Member 'notReadonlyThis' is never reassigned; mark it as `readonly`. 

  public testTypeGuards() {
    if (this.checkNormal()) {
      this.notReadonlyNormal = 0;
    }

    if (this.checkTypeGuardThis()) {
      this.notReadonlyThis = 0;
    }
  }

  private checkNormal() {
    return this.maybe !== undefined;
  }

  private checkTypeGuardThis(): this is this & { maybe: number } {
    return this.maybe !== undefined;
  }
}

@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed awaiting response Issues waiting for a reply from the OP or another party labels Jul 16, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 16, 2024
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 package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
2 participants