[ls] Refactor textDocument/documentHighlight implementation#157
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Confidence 4/5 - "Large refactor with good test coverage; watch for rare resolution edge-cases"SummaryThis PR refactors textDocument/documentHighlight to fully support local bindings (named binders, let-bindings, and record puns) by porting the resolution-and-scan pattern used in references.rs into document_highlight.rs. It expands locate dispatching to cover imports, term/type operators, let-bindings, binder and expression record-puns, and definition occurrences for terms and types. Position helpers for UTF‑8 ranges were added to improve range accuracy for a variety of CST/AST name/operator tokens. Key implementations
Position helpers
Tests and fixtures
Code size & effort
Risks / open points
Files of interest (non-exhaustive)
WalkthroughDocument highlighting now routes locate::Located results through an expanded dispatcher, adds CST-backed UTF‑8 range helpers, and implements highlight paths for imports, binders, let-bindings, record-puns, and term/type operators, plus integration fixtures exercising these flows. ChangesLocal bindings document highlighting
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
- Coverage 83.25% 83.25% -0.01%
==========================================
Files 134 134
Lines 24772 25121 +349
Branches 24772 25121 +349
==========================================
+ Hits 20624 20914 +290
+ Misses 2263 2257 -6
- Partials 1885 1950 +65 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Main.purs (2)
33-33: ⚡ Quick winAdd type signature for consistency.
The
plusfunction is missing a type signature, which is inconsistent with other top-level definitions in this module. Type signatures improve clarity and enable better type checking.📝 Proposed fix to add type signature
+plus :: Local -> Local -> Local plus left right = left🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Main.purs` at line 33, The top-level function `plus` lacks an explicit type signature; add a type signature for `plus` above its definition (the `plus` function) consistent with other top-level declarations in the module so it explicitly declares the argument and return types used by `plus` and satisfies the module's style and compiler checks; ensure the signature matches how `plus` is used (same type variables/constraints as other functions) and place it immediately before the `plus left right = left` definition.
31-31: ⚡ Quick winParameter name shadows imported binding.
The parameter
valuein thelocalMemberimplementation shadows the importedvaluefromLib(line 3). Whilst syntactically valid, this reduces clarity and could cause confusion when reasoning about whichvalueis being used.♻️ Proposed fix to rename parameter
- localMember value = value + localMember x = x🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Main.purs` at line 31, The parameter name `value` in the function `localMember` is shadowing the imported `value` from `Lib`; rename the parameter (e.g., to `localValue` or `v`) in the `localMember` definition and update all references inside `localMember`'s body to use the new name so the imported `value` remains unshadowed and intent is clear; ensure any related pattern matches or type annotations in `localMember` are adjusted to the new parameter name.tests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Lib.purs (1)
15-15: ⚡ Quick winAdd type signature for consistency.
The
appendfunction is missing a type signature, which is inconsistent with other top-level definitions likevalue(lines 12-13). Even in test fixtures, type signatures improve clarity and type safety.📝 Proposed fix to add type signature
+append :: Box -> Box -> Box append left right = left🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Lib.purs` at line 15, Add an explicit polymorphic type signature for the top-level function append to match the style of nearby definitions (e.g., value) and the implementation append left right = left; for example declare append with two arguments returning the first (e.g., append :: forall a b. a -> b -> a) so the signature matches the function name append and its behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@compiler-lsp/analyzer/src/document_highlight.rs`:
- Around line 794-795: The code calls highlights.extend(highlight.into_iter())
which is redundant; change the call to highlights.extend(highlight) so
Vec::extend uses the IntoIterator impl directly. Locate the variable named
highlight created via range.and_then(|range| document_highlight(content,
encoding, range)) and update the extend call to pass highlight without
.into_iter() (in the scope where highlights is extended).
---
Nitpick comments:
In
`@tests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Lib.purs`:
- Line 15: Add an explicit polymorphic type signature for the top-level function
append to match the style of nearby definitions (e.g., value) and the
implementation append left right = left; for example declare append with two
arguments returning the first (e.g., append :: forall a b. a -> b -> a) so the
signature matches the function name append and its behavior.
In
`@tests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Main.purs`:
- Line 33: The top-level function `plus` lacks an explicit type signature; add a
type signature for `plus` above its definition (the `plus` function) consistent
with other top-level declarations in the module so it explicitly declares the
argument and return types used by `plus` and satisfies the module's style and
compiler checks; ensure the signature matches how `plus` is used (same type
variables/constraints as other functions) and place it immediately before the
`plus left right = left` definition.
- Line 31: The parameter name `value` in the function `localMember` is shadowing
the imported `value` from `Lib`; rename the parameter (e.g., to `localValue` or
`v`) in the `localMember` definition and update all references inside
`localMember`'s body to use the new name so the imported `value` remains
unshadowed and intent is clear; ensure any related pattern matches or type
annotations in `localMember` are adjusted to the new parameter name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb7b1731-5936-4b94-8e71-6ef5725bdf12
⛔ Files ignored due to path filters (5)
tests-integration/fixtures/lsp/1778600460_document_highlight_cross_file/Main.snapis excluded by!**/*.snaptests-integration/fixtures/lsp/1778600760_document_highlight_constructor_binder/Main.snapis excluded by!**/*.snaptests-integration/fixtures/lsp/1778600820_document_highlight_record_pun/Main.snapis excluded by!**/*.snaptests-integration/fixtures/lsp/1778600940_document_highlight_constructor_declaration/Main.snapis excluded by!**/*.snaptests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Main.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
compiler-lsp/analyzer/src/document_highlight.rscompiler-lsp/analyzer/src/position.rstests-integration/fixtures/lsp/1778600820_document_highlight_record_pun/Main.purstests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Lib.purstests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Main.purs
purefunctor
force-pushed
the
justin/pr-156
branch
from
2e68004 to
7a11f98
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@compiler-lsp/analyzer/src/document_highlight.rs`:
- Around line 87-94: The current code passes the full ImportItem range from
locate::id_range(import_id) to document_highlight, which highlights
keywords/parentheses for forms like `type Foo` or operator imports; instead, add
a small helper (e.g., import_item_name_range) that takes a cst::ImportItem node
(reuse the same variant handling as import_target) and returns the token-only
name range, then change the pipeline: call
locate::id_range(&content,&parsed,&stabilized, import_id), map that ImportItem
range through import_item_name_range to extract the symbol-only range, and pass
that resulting range into document_highlight (keeping context.encoding). Ensure
import_item_name_range covers all ImportItem variants
(type/class/operator/alias) so only the symbol token is highlighted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d745d61-abea-4fc1-b03e-fc885f1ba676
⛔ Files ignored due to path filters (6)
tests-integration/fixtures/lsp/1778600460_document_highlight_cross_file/Main.snapis excluded by!**/*.snaptests-integration/fixtures/lsp/1778600760_document_highlight_constructor_binder/Main.snapis excluded by!**/*.snaptests-integration/fixtures/lsp/1778600820_document_highlight_record_pun/Main.snapis excluded by!**/*.snaptests-integration/fixtures/lsp/1778600940_document_highlight_constructor_declaration/Main.snapis excluded by!**/*.snaptests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Main.snapis excluded by!**/*.snaptests-integration/fixtures/lsp/1780512780_document_highlight_uncovered_cases/Main.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
compiler-lsp/analyzer/src/document_highlight.rscompiler-lsp/analyzer/src/position.rstests-integration/fixtures/lsp/1778600820_document_highlight_record_pun/Main.purstests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Lib.purstests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Main.purstests-integration/fixtures/lsp/1780512780_document_highlight_uncovered_cases/Main.purs
🚧 Files skipped from review as they are similar to previous changes (3)
- tests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Main.purs
- tests-integration/fixtures/lsp/1780504560_document_highlight_types_and_imports/Lib.purs
- compiler-lsp/analyzer/src/position.rs
purefunctor
force-pushed
the
justin/pr-156
branch
from
7a11f98 to
840ba38
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #156