fix(eslint-plugin): [consistent-indexed-object-style] do not remove comments when fixing#12396
Conversation
…omments when fixing The autofix rebuilds the type from the text of a couple of sub-nodes (the key/value types or the mapped type constraint/value). Any comments that sat between those sub-nodes were silently dropped, so applying the fix could delete code. Withhold the fix (and the index-signature suggestion) whenever the node being replaced contains comments, mirroring how the rule already skips fixing when it cannot safely produce equivalent output. Fixes typescript-eslint#10577
|
Thanks for the PR, @sarathfrancis90! 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 project configuration. |
|
View your CI Pipeline Execution ↗ for commit ce6eaa9
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12396 +/- ##
==========================================
- Coverage 86.99% 86.98% -0.02%
==========================================
Files 513 513
Lines 16546 16553 +7
Branches 5165 5169 +4
==========================================
+ Hits 14394 14398 +4
- Misses 1461 1464 +3
Partials 691 691
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| // The fixers rebuild the type from the text of a few sub-nodes, which would | ||
| // silently drop any comments that sit between those sub-nodes. To avoid | ||
| // deleting code, we withhold the fix when the node to be replaced contains | ||
| // comments. |
There was a problem hiding this comment.
This comment does not describe the function, but rather the PR overall... let's just remove it?
| // The fixers rebuild the type from the text of a few sub-nodes, which would | |
| // silently drop any comments that sit between those sub-nodes. To avoid | |
| // deleting code, we withhold the fix when the node to be replaced contains | |
| // comments. |
| // silently drop any comments that sit between those sub-nodes. To avoid | ||
| // deleting code, we withhold the fix when the node to be replaced contains | ||
| // comments. | ||
| function hasComments(node: TSESTree.Node): boolean { |
There was a problem hiding this comment.
I don't think this is the right condition we want to use for when to disable the autofix, because many cases are perfectly fine. For example:
type Foo = {
[key: string]: {
/** important comment */
property: number
}
};is perfectly safe to autofix to
type Foo = Record<string, {
/** important comment */
property: number
}>;, which the rule happily did before this change, but no longer does. Can you find a way to refine the comment detection to allow these unproblematic comments?
| } | ||
| : null, | ||
| fix: | ||
| safeFix && !hasComments(node) |
There was a problem hiding this comment.
removing the fixer completely is overkill (here and below). let's instead turn the autofix into a suggestion please.
kirkwaiblinger
added
the
awaiting response
|
FYI - this PR appears borderline at best as far as complying with the https://typescript-eslint.io/contributing/ai-policy/. Please give it a read to ensure this PR can continue to be reviewed. Thanks! |
|
Thanks @kirkwaiblinger — and fair on the policy; I've read it and will keep what I work on to changes I understand and can stand behind, with descriptions in my own words. On the substance you're right, and I'd over-corrected. The fixer rebuilds the type from the key-type and value-type sub-nodes' text, so any comment that lives inside those sub-nodes (like the And agreed that dropping the fixer entirely is too much. I'll narrow the check to the genuinely-lossy case and, for that case, offer the reshape as a suggestion rather than an autofix (so the safe cases keep autofixing and the lossy one becomes opt-in instead of silently dropping comments). I'll also remove that stray comment. Reworking along those lines now. |
… instead of dropping the fix on comments Narrow the comment check so only comments the fixer would actually drop - those inside the reported node but outside every sub-node whose text the fix reuses - block the autofix. Comments that live inside a preserved sub-node ride along and keep autofixing as before. For the genuinely-lossy case, offer the reshape as a suggestion rather than withholding it entirely, so converting the type stays available as an explicit opt-in action instead of silently discarding the comment. The `-readonly` mapped type stays untouched since there is no builtin Mutable<T> to express it.
|
Reworked along those lines, thanks for the steer:
Tests updated: the previously-withheld cases now assert the suggestion output (including the #10577 repro). Rule tests, typecheck, lint and prettier all pass locally. |
2 participants
PR Checklist
Overview
The
consistent-indexed-object-stylefixers build their replacement text by reading the text of a few specific sub-nodes (the key/value type annotations, or a mapped type's constraint and value) and stitching them into aRecord<...>/ index-signature string. Comments that sit anywhere else within the node being replaced are never copied over, so running the fix silently deletes them. In the mapped-type case from #10577 it can even produce syntactically broken output, because a trailing line comment ends up inside the generatedRecord<...>arguments.This adds a small
hasCommentsguard (usinggetCommentsInside) and uses it at each of the three fix sites. When the reported node contains any comments, the rule still reports the violation but withholds the fix and the index-signature suggestion, rather than emitting output that loses code. This follows the rule's existing pattern of skipping the fix whenever it can't safely produce equivalent output (e.g. the existingMutable<T>and unsafe-fix cases).Tests cover record mode (interface, type literal, and mapped type), index-signature mode, and the exact reproduction from the issue; each asserts that no fix or suggestion is applied.