SCIP: Exclude leading/trailing trivia in definition ranges#22595
Open
Wilfred wants to merge 1 commit into
Open
SCIP: Exclude leading/trailing trivia in definition ranges#22595Wilfred wants to merge 1 commit into
Wilfred wants to merge 1 commit into
Conversation
rustbot
added
the
S-waiting-on-review
Previously, the navigation position (which we compute SCIP positions from) would include the full CST node position, including trivia. This generally isn't what we want in SCIP: we just want the definition itself. This is particularly noticeable when looking at consts: ``` const FOO_ONE: i32 = 123; // one const FOO_TWO: i32 = 124; // two ``` The `// one` comment was included in the position of `FOO_TWO`, which is confusing. I noticed this when working on dead code deletion tools that use SCIP data, because they'd delete the `// one` comment if `FOO_TWO` was unused. Instead, trim trivia from the beginning and end of the range when considering navigation. AI disclosure: Written with help by Codex and GPT-5.5.
Collaborator
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Wilfred
commented
Jun 17, 2026
| //- /main.rs crate:main deps:core | ||
| // we don't use minicore here so that this test doesn't randomly fail | ||
| // when someone edits minicore | ||
| struct S; |
Contributor
Author
There was a problem hiding this comment.
The hover position for S now no longer includes the comment, which seems correct. The comment doesn't actually relate to S.
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
2 participants
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.
Previously, the navigation position (which we compute SCIP positions from) would include the full CST node position, including trivia. This generally isn't what we want in SCIP: we just want the definition itself.
This is particularly noticeable when looking at consts:
The
// onecomment was included in the position ofFOO_TWO, which is confusing.I noticed this when working on dead code deletion tools that use SCIP data, because they'd delete the
// onecomment ifFOO_TWOwas unused.Instead, trim trivia from the beginning and end of the range when considering navigation.
AI disclosure: Written with help by Codex and GPT-5.5.