fix(lint): merge suggestions from overlapping lints instead of dropping them#3468
fix(lint): merge suggestions from overlapping lints instead of dropping them#3468jlaportebot wants to merge 34 commits into
Conversation
…ng them When two lints cover the same span (e.g., a misspelling that triggers both 'although' and 'a though' suggestions for 'athough'), the remove_overlaps function would silently discard the lower-priority lint, losing its suggestions entirely. This change collects suggestions from overlapping same-span lints and merges them into the surviving lint before removal, deduplicating while preserving order. Both remove_overlaps and remove_overlaps_map are updated with this merge logic. The fix also addresses a borrow-checker issue in the original implementation by deferring mutation to a post-iteration pass. Closes Automattic#3362
hippietrail
left a comment
hippietrail
left a comment
There was a problem hiding this comment.
I haven't gone through the whole code but check out the previous issues for more realistic sentences for unit tests.
| assert_suggestion_result( | ||
| "I went athough the door.", | ||
| SpellCheck::new(FstDictionary::curated(), Dialect::American), | ||
| "I went although the door.", |
…stions - Update Linter.test.ts: 'that that' now returns 2 suggestions (remove first or second occurrence) since overlapping lints merge suggestions - Improve spell_check.rs test sentence: use more natural 'I walked athough the park' per review feedback (hippietrail)
|
Thanks for the review @hippietrail! I've updated the test sentence from the unnatural "I went athough the door" to the more natural "I walked athough the park". I also checked issues #3019 and #2460 — the core overlap-merging logic in this PR addresses exactly the problem described there (SpellCheck and SplitWords suggestions being dropped when they overlap). For additional test coverage of that scenario, the I also fixed the CI failure: the JS tests were asserting |
| assert_suggestion_result( | ||
| "I walked athough the park.", | ||
| SpellCheck::new(FstDictionary::curated(), Dialect::American), | ||
| "I walked although the park.", |
There was a problem hiding this comment.
This is still unnatural. The AI seems to be confusing "athough" and "although" and "through".
…message instead of trigging the first recipe, soft-clean (Automattic#3414)
* feat: for free of charge → for free / free of charge Fixes Automattic#3290 * fix: config JSON * test: snapshots fixed
…ectory() and fixing a bug where node_modules shell expansion would not work (Automattic#3415)
) Bumps [tree-sitter-swift](https://github.com/alex-pinkus/tree-sitter-swift) from 0.7.1 to 0.7.2. - [Release notes](https://github.com/alex-pinkus/tree-sitter-swift/releases) - [Commits](alex-pinkus/tree-sitter-swift@0.7.1...0.7.2) --- updated-dependencies: - dependency-name: tree-sitter-swift dependency-version: 0.7.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ic#3410) Bumps [tree-sitter-c-sharp](https://github.com/tree-sitter/tree-sitter-c-sharp) from 0.23.1 to 0.23.5. - [Release notes](https://github.com/tree-sitter/tree-sitter-c-sharp/releases) - [Commits](tree-sitter/tree-sitter-c-sharp@v0.23.1...v0.23.5) --- updated-dependencies: - dependency-name: tree-sitter-c-sharp dependency-version: 0.23.5 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…omattic#3407) Replaces std::sync::Arc with crate::sync::Lrc to fix clippy::arc_with_non_send_sync lint warnings. Lrc is an alias for Rc when not concurrent, and Arc when concurrent, making these types properly Send+Sync. Generated by Mistral Vibe. Co-authored-by: Mistral Vibe <vibe@mistral.ai>
* feat(desktop): add debounce setting * refactor(desktop): debounce state * feat(desktop): move to general page * feat: keep highlights on-screen while waiting
* feat(desktop): highlights should follow windows * fix(desktop): broken button and irrelevant settings * fix(desktop): use CoreGraphics instead * feat(desktop): change tact; hide highlights instead * refactor(desktop): remove dead code and simplify * fix(desktop): clone instead
* fix(desktop): wire up manage and launch buttons * fix(desktop): don't use placeholder state for text edit card
* chore: dictionary curation -fork - generate "forkable" -stochastic - generate "stochastically" * chore: dictionary curation +salaryman +salarymen +questionability * chore: dictionary curation Fixes Automattic#3416
… first 3 (Automattic#3424) The BFS in the Weir test runner was hardcoded to only try the first 3 suggestions (0..3), which meant that 'let becomes' arrays with more than 3 alternatives could never reach the 4th+ option during testing. This caused tests to fail when the expected output used a later alternative. Changed the BFS to iterate over all suggestions (0..lint.suggestions.len()), and renamed transform_top3_to_expected to transform_to_expected since it no longer limits to 3. Fixes Automattic#3393
* feat: `AnchorEnd` finally fixed * fix: appease clippy
* feat(core): new `AvoidContractions` rule * fix(core): address feedback
* feat(desktop): install auto-update plugin and create public key * fix(desktop): update action to pull in secrets * feat(desktop): fill out fields for the plugin * fix(desktop): disable unsupported platform and upload update artifacts * fix(ci): only expose secrets for release builds * feat(web): create update endpoint * feat(desktop): implement weekly auto update * feat(web): cache signature * refactor(web): simplify caching logic * fix(desktop): check for updates daily * fix(ci): use dedicated workflow for desktop * fix(ci): don't sign for PRs * fix(desktop): match version from rest of repo
harper-asciidoc@2.2.0 harper-brill@2.2.0 harper-comments@2.2.0 harper-core@2.2.0 harper-desktop@2.2.0 harper-dictionary-wordlist@2.2.0 harper-html@2.2.0 harper-ink@2.2.0 harper-jjdescription@2.2.0 harper-literate-haskell@2.2.0 harper-ls@2.2.0 harper-pos-utils@2.2.0 harper-python@2.2.0 harper-stats@2.2.0 harper-tex@2.2.0 harper-thesaurus@2.2.0 harper-tree-sitter@2.2.0 harper-typst@2.2.0 Generated by cargo-workspaces
The previous test sentence 'I walked athough the park' was unnatural since 'although' doesn't make sense in that context. Replaced with 'He came athough he was tired' where 'although' fits naturally.
|
Addressed @hippietrail's feedback about the unnatural test sentence. Replaced |
- Replace unnatural test sentence 'He came athough he was tired' with natural 'We kept going athough it was raining' (review by @hippietrail) - Add regression test for issue Automattic#2460 (SpellCheck + SplitWords overlap on compounds like 'titlecase') - Add deduplication test ensuring identical suggestions from overlapping lints are not duplicated
|
Addressed @hippietrail's feedback:
All core tests pass. The merge conflict flag should resolve once the PR syncs with the latest push. |
|
Superseded by #3480 — a clean squashed version that incorporates @hippietrail's review feedback and is rebased onto the latest master (resolving merge conflicts). |
5 participants
Closes #3362
When two lints cover the same span (e.g. both 'although' and 'a though' suggested for 'athough'), 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. Both remove_overlaps and remove_overlaps_map are updated. Borrow-checker issue fixed by deferring mutation to a post-iteration pass.