fix(eslint-plugin): [prefer-readonly] autofixer doesn't add type to property that is mutated in the constructor #10552
Conversation
|
Thanks for the PR, @ronami! 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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
View your CI Pipeline Execution ↗ for commit 02c3124.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10552 +/- ##
==========================================
+ Coverage 87.15% 87.18% +0.02%
==========================================
Files 448 448
Lines 15576 15611 +35
Branches 4551 4562 +11
==========================================
+ Hits 13575 13610 +35
Misses 1645 1645
Partials 356 356
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| const annotation = checker.typeToString(type); | ||
|
|
||
| // verify the about-to-be-added type annotation is in-scope | ||
| if (tsutils.isTypeFlagSet(initializerType, ts.TypeFlags.EnumLiteral)) { |
There was a problem hiding this comment.
I wrote about this on #1056 (comment), but on some occasions (as far as I know only enums), the rule won't be able to add a type annotation:
// foo.ts
enum Foo {
Bar,
Bazz,
}
export const bar = Foo.Bar;
// index.ts
import { bar } from './foo';
export class Test {
private foo = bar;
}I chose not to add a type annotation on these cases, but an alternative I think is also valid is to have this be a suggestion instead of an auto-fix.
Would be happy to to hear opinions on this.
There was a problem hiding this comment.
I have a vague memory of this coming up once in a while... I think it's fine for now. We don't have any kind of shared/standard plumbing for importing constructs in fixes or suggestions.
|
Thinking some more about this: The rule will add a type annotation whenever literal widening happens, regardless of whether the property is mutated in the constructor or not. I'm slightly leaning towards adding a type annotation only when literal widening happens and the property is mutated in the constructor (in addition to an initializer). |
I'm not sold on this. My concerns are
My order of most to least preferred approaches for this case would be
But I'll defer to you guys if those points don't sway opinions. It's not a big thing. |
Interesting take! I didn't put much thought into it, but I agree that it can be confusing. It may seem unclear when the rule would add a type annotation and when it won't. Adding a type annotation whenever literals are involved simplifies how the rule behaves. I considered the type changing a desired consequence, and I think it will be a breaking change pretty rarely, but it is possible. For me, the increase in the level of surprise is a big reason to keep it simple and always auto-fix with the widened type 👍 |
|
Oh, I've misunderstood, sorry! - this rule only applies to |
@kirkwaiblinger I agree with your point that this change increases the level of surprise for the user by adding a special case within a special case. I think both options are viable (always adding a type annotation for literals or only when literals are modified in the constructor), and the difference between the two is subtle. I was going back and forth between what I consider the optimal solution here, but I think I'm +1 for adding a type annotation only when necessary, even though it adds some surprise factor. In my opinion, adding a type annotation may be already somewhat surprising, and adding it only when it's clear why it's necessary makes sense to me. I've updated the PR to match this 👍 |
| if (nameNode.type !== AST_NODE_TYPES.Identifier) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
To my understanding, this can realistically only be an identifier. This means some cases that should have a type annotation added won't receive it, for example:
class TestOverlappingClassVariable {
private 'overlappingClassVariable' = 7;
constructor() {
this.overlappingClassVariable = 10;
}
}This seems like an existing bug that's not necessarily related to this addition. I've opened #10655 as a separate issue to address this.
There was a problem hiding this comment.
LGTM - and such comprehensive unit tests! 😄
Will let you or @kirkwaiblinger merge if you're happy. Edit: it's been a few days and we're about to run a release - merging myself. 
| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.21.0 | 8.22.0 | | npm | @typescript-eslint/parser | 8.21.0 | 8.22.0 | ## [v8.22.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8220-2025-01-27) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-template-expression] handle template literal type ([#10612](typescript-eslint/typescript-eslint#10612)) - **eslint-plugin:** \[prefer-readonly] autofixer doesn't add type to property that is mutated in the constructor ([#10552](typescript-eslint/typescript-eslint#10552)) - **eslint-plugin:** \[no-extraneous-class] handle accessor keyword ([#10678](typescript-eslint/typescript-eslint#10678)) - **eslint-plugin:** \[no-shadow] don't report unnecessarily on valid ways of using module augmentation ([#10616](typescript-eslint/typescript-eslint#10616)) - **eslint-plugin:** \[no-duplicate-type-constituents] handle nested types ([#10638](typescript-eslint/typescript-eslint#10638)) - **eslint-plugin:** \[prefer-nullish-coalescing] doesn't report on ternary but on equivalent || ([#10517](typescript-eslint/typescript-eslint#10517)) ##### ❤️ Thank You - mdm317 - Olivier Zalmanski [@OlivierZal](https://github.com/OlivierZal) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.21.0 | 8.22.0 | | npm | @typescript-eslint/parser | 8.21.0 | 8.22.0 | ## [v8.22.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8220-2025-01-27) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-template-expression] handle template literal type ([#10612](typescript-eslint/typescript-eslint#10612)) - **eslint-plugin:** \[prefer-readonly] autofixer doesn't add type to property that is mutated in the constructor ([#10552](typescript-eslint/typescript-eslint#10552)) - **eslint-plugin:** \[no-extraneous-class] handle accessor keyword ([#10678](typescript-eslint/typescript-eslint#10678)) - **eslint-plugin:** \[no-shadow] don't report unnecessarily on valid ways of using module augmentation ([#10616](typescript-eslint/typescript-eslint#10616)) - **eslint-plugin:** \[no-duplicate-type-constituents] handle nested types ([#10638](typescript-eslint/typescript-eslint#10638)) - **eslint-plugin:** \[prefer-nullish-coalescing] doesn't report on ternary but on equivalent || ([#10517](typescript-eslint/typescript-eslint#10517)) ##### ❤️ Thank You - mdm317 - Olivier Zalmanski [@OlivierZal](https://github.com/OlivierZal) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
PR Checklist
Overview
This is a draft PR that attempts to tackle #1056. I've mentioned my thoughts about this at #1056 (comment).