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
| // 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()?
There was a problem hiding this comment.
yep good catch, now done. Part of the feature needs to be implemented in #1258 (comment)
| 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?
There was a problem hiding this comment.
Renamed to test_function_scope_target_stays_in_file and there's a similar test in IDE layer
| /// | ||
| /// 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?
There was a problem hiding this comment.
I thought I'd leave it off until later, but resurrected it as part of #1257 (comment)
| /// 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?
There was a problem hiding this comment.
Yep it does, I added tests for this
| 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?
There was a problem hiding this comment.
I think you're right, that's a better separation of concerns
lionel-
force-pushed
the
oak/salsa-19-find-refs-rename-migration
branch
from
9b0c99e to
8363250
Compare
lionel-
force-pushed
the
oak/salsa-18-goto-def-migration
branch
from
d73c0d5 to
77bd0a4
Compare
lionel-
force-pushed
the
oak/salsa-19-find-refs-rename-migration
branch
from
1ab24cd to
b09b25b
Compare
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.