From 489078c2553e235c99ea63c7071b06692b15b3e6 Mon Sep 17 00:00:00 2001 From: Wilfred Hughes Date: Mon, 15 Jun 2026 16:27:43 +0100 Subject: [PATCH] SCIP: Exclude leading/trailing trivia in definition ranges 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. --- crates/ide/src/hover/tests.rs | 2 +- crates/ide/src/navigation_target.rs | 49 ++++++++++++++++++++- crates/rust-analyzer/src/cli/scip.rs | 64 ++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 2 deletions(-) diff --git a/crates/ide/src/hover/tests.rs b/crates/ide/src/hover/tests.rs index 30644fe2db89..9cb907cfadd1 100644 --- a/crates/ide/src/hover/tests.rs +++ b/crates/ide/src/hover/tests.rs @@ -4145,7 +4145,7 @@ pub mod future { file_id: FileId( 0, ), - full_range: 0..110, + full_range: 101..110, focus_range: 108..109, name: "S", kind: Struct, diff --git a/crates/ide/src/navigation_target.rs b/crates/ide/src/navigation_target.rs index b8c14dc09f9a..798c7948b938 100644 --- a/crates/ide/src/navigation_target.rs +++ b/crates/ide/src/navigation_target.rs @@ -18,7 +18,7 @@ use ide_db::{ }; use stdx::never; use syntax::{ - AstNode, AstPtr, SyntaxNode, TextRange, + AstNode, AstPtr, AstToken, SyntaxKind, SyntaxNode, SyntaxToken, TextRange, ast::{self, HasName}, }; @@ -448,6 +448,8 @@ where NavigationTarget::from_named_with_range( db, src.map(|(full_range, node)| { + let full_range = + node.as_ref().map_or(full_range, |node| range_trim_trivia(node.syntax())); ( full_range, node.and_then(|node| { @@ -502,6 +504,8 @@ impl TryToNav for hir::Impl { ) -> Option> { let db = sema.db; let InFile { file_id, value: (full_range, source) } = self.source_with_range(db)?; + let full_range = + source.as_ref().map_or(full_range, |source| range_trim_trivia(source.syntax())); Some( orig_range_with_focus_r( @@ -930,6 +934,49 @@ fn orig_range_with_focus( ) } +/// Return the range of `value` without leading/trailing trivia. +fn range_trim_trivia(value: &SyntaxNode) -> TextRange { + let mut first = value.first_child_or_token(); + while let Some(element) = &first { + let Some(token) = element.as_token() else { break }; + if !is_trivia_not_doc_comment(token) { + break; + } + first = element.next_sibling_or_token(); + } + + let mut last = value.last_child_or_token(); + while let Some(element) = &last { + let Some(token) = element.as_token() else { break }; + if !is_trivia(token) { + break; + } + last = element.prev_sibling_or_token(); + } + + match (first, last) { + (Some(first), Some(last)) => { + TextRange::new(first.text_range().start(), last.text_range().end()) + } + _ => value.text_range(), + } +} + +fn is_trivia_not_doc_comment(token: &SyntaxToken) -> bool { + match token.kind() { + SyntaxKind::WHITESPACE => true, + SyntaxKind::COMMENT => match ast::Comment::cast(token.clone()) { + Some(comment) => !comment.is_outer(), + None => true, + }, + _ => false, + } +} + +fn is_trivia(token: &SyntaxToken) -> bool { + matches!(token.kind(), SyntaxKind::WHITESPACE | SyntaxKind::COMMENT) +} + pub(crate) fn orig_range_with_focus_r( db: &RootDatabase, hir_file: HirFileId, diff --git a/crates/rust-analyzer/src/cli/scip.rs b/crates/rust-analyzer/src/cli/scip.rs index bca38ed82fdd..c440b2947e40 100644 --- a/crates/rust-analyzer/src/cli/scip.rs +++ b/crates/rust-analyzer/src/cli/scip.rs @@ -948,4 +948,68 @@ pub mod example_mod { assert_eq!(token.definition_body, Some(expected_range)); } + + #[test] + fn function_enclosing_range_trivia() { + let s = "fn first() {}\n// belongs to first\n/// second docs\nfn second() {}"; + + let mut host = AnalysisHost::default(); + let change_fixture = ChangeFixture::parse(s); + host.raw_database_mut().apply_change(change_fixture.change); + + let analysis = host.analysis(); + let si = StaticIndex::compute( + &analysis, + VendoredLibrariesConfig::Included { + workspace_root: &VfsPath::new_virtual_path("/workspace".to_owned()), + }, + ); + + let file = si.files.first().unwrap(); + let token = file + .tokens + .iter() + .filter_map(|(_, token_id)| si.tokens.get(*token_id)) + .find(|token| token.display_name.as_deref() == Some("second")) + .unwrap(); + + let definition_body = token.definition_body.unwrap(); + assert_eq!( + definition_body.range.start(), + TextSize::new(s.find("/// second docs").unwrap() as u32) + ); + assert_eq!(definition_body.range.end(), TextSize::of(s)); + } + + #[test] + fn const_enclosing_range_trivia() { + let s = "const FOO_ONE: i32 = 123; // one\nconst FOO_TWO: i32 = 123; // two"; + + let mut host = AnalysisHost::default(); + let change_fixture = ChangeFixture::parse(s); + host.raw_database_mut().apply_change(change_fixture.change); + + let analysis = host.analysis(); + let si = StaticIndex::compute( + &analysis, + VendoredLibrariesConfig::Included { + workspace_root: &VfsPath::new_virtual_path("/workspace".to_owned()), + }, + ); + + let file = si.files.first().unwrap(); + let token = file + .tokens + .iter() + .filter_map(|(_, token_id)| si.tokens.get(*token_id)) + .find(|token| token.display_name.as_deref() == Some("FOO_TWO")) + .unwrap(); + + let definition_body = token.definition_body.unwrap(); + assert_eq!( + definition_body.range.start(), + TextSize::new(s.find("const FOO_TWO").unwrap() as u32) + ); + assert_eq!(definition_body.range.end(), TextSize::new(s.find(" // two").unwrap() as u32)); + } }