Implement goto-definition for local symbols#1230
Conversation
| /// same name. | ||
| pub fn reaching_definitions( | ||
| &self, | ||
| scope: ScopeId, |
There was a problem hiding this comment.
nit: since we have Scope and ScopeId, i have the mildest of consistency preference for scope_id: ScopeId and scope: Scope as a naming scheme (similarly for Use and UseId, etc)
There was a problem hiding this comment.
it looks like future you agrees with me and made this change in #1231
| index: &SemanticIndex, | ||
| scope: &ExternalScope, | ||
| root: &RSyntaxNode, | ||
| pos: &FileOffset, |
There was a problem hiding this comment.
| pos: &FileOffset, | |
| file_offset: &FileOffset, |
?
When in doubt of a good name, I like explicit ones
There was a problem hiding this comment.
Actually, this struct doesn't currently feel all that useful to me.
Looking at its uses, you kind of just combine into a FileOffset at the last moment only to destructure it. So I also would be fine with just file: Url and offset: TextSize as args (oh, I guess what I'm saying is that I was fine with what was there before)
There was a problem hiding this comment.
i do see how FileRange is a useful return value from find_references() in a future pr, so keeping it is fine
but maybe lets still change the pos name to file_offset?
There was a problem hiding this comment.
The offset is only meaningful in the context of the file, and the arguments are meant to be passed together, so gathering them in a struct reflects that ("keeps it honest" in claude-linguo).
That's what R-A does too. They call the type FilePosition even though the position field is offset: TextSize. I thought I'd go ahead and name it FileOffset
file_offset seems like an awfully long name for this parameter. How about just offset? In R-A the arguments take a FilePosition are called position. But unlike position, offset really does not evoke the file-anchored aspect. Now I'm tempted to just rename to FilePosition and use position.
I'm not sure discussing this sort of stuff asynchronously is useful, how about we take a few minutes to talk it through in our next sync?
| }; | ||
| vec![external.into()] | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
2 definitions because there's a conditional path? yep nice!
|
|
||
| if std::env::var("ARK_OAK_GOTO_DEF").is_ok() { | ||
| Ok(goto_definition(document, params, state).log_err().flatten()) | ||
| } else { | ||
| Ok(goto_definition_legacy::goto_definition(document, params) | ||
| .log_err() | ||
| .flatten()) | ||
| } | ||
| Ok(goto_definition(document, params).log_err().flatten()) | ||
| } |
There was a problem hiding this comment.
Probably very little risk of anything bad, right?
No salsa involved yet, so the main issue would be some slowness from computing a single file SemanticIndex on every goto definition call?
There was a problem hiding this comment.
yep but for a single file I'm not worried at all about performance
Neither ty nor r-a have this
lionel-
force-pushed
the
oak/goto-def-intra-file
branch
from
7e6ad7c to
6fa14ce
Compare
2 participants
Part of posit-dev/positron#13749
Part of #1149
Closes posit-dev/positron#8631 (should extract NSE bullet in its own issue, also affecting find-refs and rename).
Adds precise within-file goto-definition driven by Oak's semantic index.
The Salsa rewrite is taking more time and merging right before release seems a bit risky, given the added complexity. So it looks like full cross-file support for goto-def, find-refs, and rename is not going to make it to the June release. However we already have everything we need for within-file analysis.
This PR adds support for jumping to definition of locally-defined symbols (
def <-and function parameters):Removes the
ARK_OAK_GOTO_DEF-gated goto-definition handler, and all its non-Salsa baggage. That's a regression in our dev builds (we can no longer exercise the oak_sources stuff to visit installed package definitions), but that will be restored later this week with the proper Salsa implementation.Adds within-file support in Ark's goto-definition handler, based on Oak's semantic index. If the symbol is not bound in the file, we use the legacy indexer-based handler. This legacy handler includes all symbols of the same name in other files, including unrelated files. This will be fixed once we fully switch to Oak.
Note that for conditionally unbound (via e.g.
if (cond) x <- 1), we don't use the legacy handlers to populate external symbols, because the complexity wasn't worth the slight and temporary inconsistency.