Use Salsa infrastructure for find-references and rename#1257
Conversation
lionel-
force-pushed
the
oak/salsa-19-find-refs-rename-migration
branch
from
11d6ce1 to
31b65be
Compare
lionel-
force-pushed
the
oak/salsa-18-goto-def-migration
branch
from
399f3f3 to
ec4c775
Compare
lionel-
force-pushed
the
oak/salsa-19-find-refs-rename-migration
branch
from
31b65be to
2fcc1d2
Compare
lionel-
force-pushed
the
oak/salsa-18-goto-def-migration
branch
from
fa5b1c0 to
cacffa1
Compare
lionel-
force-pushed
the
oak/salsa-19-find-refs-rename-migration
branch
from
2fcc1d2 to
9b0c99e
Compare
| pub fn uses_of(&self, name: &str) -> Vec<TextRange> { | ||
| let mut ranges = Vec::new(); | ||
| for scope_id in self.scope_ids() { | ||
| let Some(symbol_id) = self.symbols(scope_id).id(name) else { | ||
| continue; | ||
| }; | ||
| for (_use_id, use_site) in self.uses(scope_id).iter() { | ||
| if use_site.symbol() == symbol_id { | ||
| ranges.push(use_site.range()); | ||
| } | ||
| } | ||
| } | ||
| ranges |
There was a problem hiding this comment.
Looking at the definition of this function and the return value of use_at() being Option<(ScopeId, UseId, &Use)>, to me it feels like the right return value for this would be Vec<(ScopeId, UseId, &Use)>.
You've got all the pieces to build that return value already. It feeeels like it would make sense to return the maximal amount of looked up info here
| // Cursor on `mutate` in `dplyr::mutate` is a namespace access, returns nothing. | ||
| let code = "dplyr::mutate\n"; |
There was a problem hiding this comment.
i guess it could return every time you call dplyr's mutate()?
| Location::new(file1_uri, range((1, 2), (1, 3))), | ||
| ]; | ||
| assert_eq!(locs, expected); | ||
| assert!(locs.is_empty()); |
There was a problem hiding this comment.
did we lose test_function_scope_target_skips_cross_file_walk as a test?
| /// | ||
| /// Returns `None` when the cursor isn't on a variable binding/use or | ||
| /// a member name (e.g. cursor is on an operator, a keyword, or a | ||
| /// `pkg::sym` namespace access). |
There was a problem hiding this comment.
I noticed that NamespaceAccess went away as an Identifier type?
| /// Cursor on the RHS name of a `$` or `@` extract expression. Member | ||
| /// names are not tracked by the semantic index. | ||
| Member { |
There was a problem hiding this comment.
does this all work with nested $ calls? like foo$bar$baz regardless of where the cursor is?
| fn find_variable_references( | ||
| db: &dyn Db, | ||
| file: File, | ||
| snapped: TextSize, |
There was a problem hiding this comment.
| snapped: TextSize, | |
| offset: TextSize, |
idk, i'd prefer this? just for overall consistency of using offset as much as possible
| Identifier::Variable { range, .. } => { | ||
| find_variable_references(db, file, range.start(), include_declaration) |
There was a problem hiding this comment.
What if you expanded the .. and passed name through so you don't need to do let name = target_defs[0].name(db); later on? That currently feels a bit awkward to me. Like, is target_defs[1].name() potentially different??
| db: &dyn Db, | ||
| primary: File, |
There was a problem hiding this comment.
| db: &dyn Db, | |
| primary: File, | |
| db: &dyn Db, | |
| file: File, |
For consistency with find_variable_references(). Only sort_file_ranges() should probably have the concept of primary?
| } | ||
| } | ||
|
|
||
| #[allow(dead_code)] |
| offset: TextSize, | ||
| new_name: &str, | ||
| ) -> anyhow::Result<RenameTargets> { | ||
| let new_text = to_identifier_text(new_name)?; |
There was a problem hiding this comment.
I could see this call moving out of rename() up to the lsp level
So then you would not pass new_name through or return it. And RenameTargets could just be Vec<FileRange>
Mostly I was a bit surprised to see it flow through to here since this function is mostly about "find the sites to rename at", only to find that its passed through here just for name normalization.
I guess its a question of who owns the normalization process, oak_ide or the lsp layer?
2 participants
Branched from #1254
Progress towards #1212
Closes #1149
This PR moves find-references and rename onto the Salsa infra and deletes the old implementation (the textual workspace walk).
Candidate search: Unlike Rust-Analyzer that does a textual search across the workspace and resolves all occurrences, we're more aligned with ty: we filter files to inspect with a textual search, but then use a post-parse structure to filter occurrences. ty walks the AST, we walk the index instead with a new
uses_of()method that is more to the point than the raw AST.Candidates are then narrowed with
File::resolve_at()to check that they match the definition of the symbol at point.New supporting infra:
Identifier::classify()all_files()(Salsa query) that returns all files known to the databaseroot_by_file()that disambiguates to which workspace root a file belong when roots are nested. That is meant to be the one source of truth for root ownership of files.Removed infra: The
Documentcopies of the Rowan parse tree and semantic index are now removed, in favour of the Salsa DB.Other fix: Goto-definition now benefits from the new Salsa-based
Identifier::classify()which snaps the cursor to the symbol at point. Fixes goto-def when cursor in on RHS boundary.I recommend testing with #1259 to benefit from bug fixes and improvements in ulterior branches.