Conversation
lionel-
force-pushed
the
oak/jump-to-def
branch
from
ce64b46 to
d6c8484
Compare
DavisVaughan
left a comment
DavisVaughan
left a comment
There was a problem hiding this comment.
Two main things
- #1159 to try and simplify some lookups
file_scope()feels like it could use another pass, found quite a few things there that smell off
| /// Find the definition site at `offset`, if any. | ||
| pub fn definition_at_offset(&self, offset: TextSize) -> Option<(ScopeId, DefinitionId)> { | ||
| let scope = self.scope_at(offset); | ||
| let def_id = self | ||
| .definitions(scope) | ||
| .iter() | ||
| .find_map(|(id, d)| d.range().contains(offset).then_some(id)); | ||
| Some((scope, def_id?)) | ||
| } | ||
|
|
||
| /// Find the use site at `offset`, if any. | ||
| pub fn use_at_offset(&self, offset: TextSize) -> Option<(ScopeId, UseId)> { | ||
| let scope = self.scope_at(offset); | ||
| let use_id = self | ||
| .uses(scope) | ||
| .iter() | ||
| .find_map(|(id, u)| u.range().contains(offset).then_some(id)); | ||
| Some((scope, use_id?)) | ||
| } |
There was a problem hiding this comment.
These felt a bit wrong to me because at the call site in goto_definition:
- You look up the scope twice (once for defn, once for use)
- You have the Definition and Use right here, but don't return them, then need to look them up again in goto_definition by their Id
Check out #1159 for an alternate approach that tries to simplify this
|
|
||
| /// The external scope chain for a file, determined by its project context. | ||
| #[derive(Debug)] | ||
| pub enum FileScope { |
There was a problem hiding this comment.
This feels worth of a file_scope.rs file
| /// Script or file outside a package. The scope chain is the R | ||
| /// search path: `library()` attachments from the file itself, | ||
| /// default packages (stats, graphics, etc.), and base. | ||
| SearchPath(Vec<BindingSource>), |
There was a problem hiding this comment.
...and any source() calls?
There was a problem hiding this comment.
Update: See i was confused by the name! I didn't realize this was only for external things
| let scope = index.scope_at(offset); | ||
| match index.scope(scope).kind() { |
There was a problem hiding this comment.
Oh, like in #1159 what if you make scope_at() return (ScopeId, Scope)? You've got it right there in scope_at(), so looking it up twice feels like a crime!
| } | ||
|
|
||
| /// The full scope for lazy contexts. Useful for features that don't | ||
| /// have a cursor position (e.g. completions, workspace symbols). |
There was a problem hiding this comment.
Completions definitely have a cursor position right?
| let filenames: Vec<String> = filenames.into_iter().collect(); | ||
| let ordered = collation_order(&pkg.description, &filenames); | ||
|
|
||
| let filename = file.path().rsplit('/').next().unwrap_or_default(); |
There was a problem hiding this comment.
again, probably want file_name() via a Path
| let path = r_dir.join(name); | ||
| let Some(uri) = Url::from_file_path(&path).log_err() else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
I think something is kinda wasteful here.
When we iterate over r_dir with list_r_files() we get a vector of PathBufs already, then we are converting them to strings in filenames/ordered, and now back to PathBufs.
It seems likely we can just keep them as PathBufs the whole time, and teach collation_order() how to sort a vector of PathBuf instead.
| let doc = if let Some(open) = self.documents.get(&uri) { | ||
| open | ||
| } else { | ||
| let Ok(contents) = std::fs::read_to_string(&path) else { |
| lazy.extend(layers.clone()); | ||
| if past_current { | ||
| top_level.extend(layers); |
There was a problem hiding this comment.
Tricky little use of past_current here!
I guess this also relies on the fact that collation_order() sorts in alphabetical order even without a Collate field, which is also the order that R sources them?
| top_level.push(BindingSource::PackageExports("base".to_string())); | ||
| lazy.extend(root_layers); | ||
| lazy.push(BindingSource::PackageExports("base".to_string())); |
There was a problem hiding this comment.
Not utils/tools and friends because in a package they require an Import, right?
2 participants
Branched from #1152
Part of #1151 and #1148
Progress towards posit-dev/positron#11112
Progress towards posit-dev/positron#8631
oak_idecrate. Like in rust-analyser and ty, the _ide crate implements IDE functionality decoupled from LSP considerations (LSP positions, WorldState, etc).oak_index(I'd like to rename it tooak_semantic) →oak_ide→ark(we'll move the worldstate and LSP handlers to a new crate later on).For packages, I realised that the collation approach laid out in #1148 needed a significant refinement: only top-level symbols are sensitive to the collation order. Symbols in lazy scopes (e.g. inside function definitions) see the whole finalised namespace. Because of this,
FileScope::at()takes a position in the file so it can select the right scope. You can also request the full lazy scope without a position.Currently, scripts are fully isolated from other files. I'm working on adding support for
source()directives in the semantic index. Then goto-definition will work more like in RStudio and only see symbols you've explicitly sourced (posit-dev/positron#11112).With this PR, we're ready to integrate with #1146 to jump at the source of package exports.