fix(lint): merge suggestions from overlapping lints instead of dropping them#3480
fix(lint): merge suggestions from overlapping lints instead of dropping them#3480jlaportebot wants to merge 5 commits into
Conversation
|
This is a clean, squashed replacement for #3468 incorporating @hippietrail's review feedback:
The core logic change is the same: when two lints cover the same span, their suggestions are merged into the surviving lint rather than silently dropping one lint's suggestions. Both |
hippietrail
left a comment
hippietrail
left a comment
There was a problem hiding this comment.
Just a couple of thoughts...
| last_kept = i; | ||
| } | ||
|
|
||
| // Apply pending merges: extend surviving lints with suggestions from removed overlapping lints. |
There was a problem hiding this comment.
Does this mean that the combined lints will be categorized as whatever the one that ended up topmost?
I've been wondering about ways to tackled this. Maybe calling it "combined" in some places and the specific origin(s) in other places?
There was a problem hiding this comment.
The surviving lint keeps its original category. Merging categories would require changing LintKind to an enum that supports multiple values, which felt out of scope. The merged suggestions preserve their content though — just ranked by frequency as you suggested.
| let mut seen = HashSet::new(); | ||
| lints[target_idx] | ||
| .suggestions | ||
| .retain(|s| seen.insert(s.clone())); |
There was a problem hiding this comment.
I would think that any suggestion that happens to be suggested by multiple linters should be rated more highly. Order by count while deduping perhaps?
There was a problem hiding this comment.
Agreed — the current implementation already sorts by count descending, so suggestions appearing in multiple overlapping lints are ranked first. The more lints that agree on a suggestion, the higher it places.
|
Thanks for the thoughtful comments, @hippietrail!
The macOS arm64 CI failure is in the "Install Node.js" step (infrastructure issue), not a code problem. |
|
Great questions @hippietrail! On lint categorization for combined suggestions: Yes, currently the merged lint takes the category of whichever rule ended up topmost in the dedup pass. I agree this is not ideal — a combined lint should probably report the most specific/serious category among its sources, or a new "Combined" category. I opened this as a minimal fix first (preventing suggestions from being silently dropped) since the category question is more of a design decision. On ranking by count: That's a smart idea — if multiple rules suggest the same replacement, that's a stronger signal it's correct, so ranking by source count makes sense. The current implementation preserves insertion order, but adding a sort by dedup count would be straightforward. I can add that in a follow-up if you'd prefer to keep this PR focused on the data-loss fix. Let me know which direction you'd like to take this! |
…ng them When two lints cover the same span (e.g. both SpellCheck suggesting 'although' and SplitWords suggesting 'a though' for 'athough'), remove_overlaps silently discarded the lower-priority lint's suggestions. This merges suggestions from same-span overlapping lints into the surviving lint before removal, with deduplication. Both remove_overlaps and remove_overlaps_map are updated. A borrow-checker issue is resolved by deferring mutation to a post-iteration pass. Closes Automattic#3362 Related Automattic#3019, Automattic#2460 Test coverage: - overlapping_lints_merge_suggestions: two lints same span, both suggestions preserved - overlapping_spellcheck_splitwords_merge_suggestions: SpellCheck + SplitWords overlap (issue Automattic#2460) - overlapping_lints_deduplicate_suggestions: identical suggestions deduplicated - athough_suggests_although: integration test with natural sentence
When multiple overlapping lints produce the same suggestion, that suggestion now appears first in the merged lint's suggestion list. This is done by counting suggestion frequency across the merged set and stable-sorting by count descending, then deduplicating. Both remove_overlaps and remove_overlaps_map receive this improvement. Added a test verifying that a suggestion shared by two lints is ranked ahead of unique suggestions. Also removes unused hashbrown::HashSet import.
…disallowed_types lint
jlaportebot
force-pushed
the
fix/merge-overlapping-lints-final
branch
from
90e3603 to
cd326e7
Compare
2 participants
Closes #3362
When two lints cover the same span, remove_overlaps silently discarded the lower-priority lint's suggestions. This PR merges suggestions from same-span overlapping lints into the surviving lint before removal, with deduplication.
Related #3019, #2460
AI Disclosure