Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Index all assigned variables #620

Merged
merged 5 commits into from
Nov 5, 2024
Merged

Index all assigned variables #620

merged 5 commits into from
Nov 5, 2024

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Nov 5, 2024

Quick fix to improve S7 workflow.
Addresses posit-dev/positron#5274

Also a nice improvement all around as you can jump to the definition of e.g. a data frame in a script.

Positron Release Notes

New Features

  • All top-level variables in R files are now indexed as workspace symbols. You can now find assigned objects in the "Go to symbol in Workspace feature" and use "Go to definition" to jump from a variable name to its assignment (R: Navigating to assigned objects positron#5274). Repeated assignments to the same name are not indexed yet but we plan to do so in the future.

Bug Fixes

  • N/A

Stopgap until we are in a better position to track the scopes
of objects in a document. This causes a first definition to be
the symbol of reference. Jumping from a reference to a definition
then always moves up.
CHANGELOG.md Outdated Show resolved Hide resolved
crates/ark/src/lsp/indexer.rs Outdated Show resolved Hide resolved
Comment on lines +294 to +306
// Super hacky but let's wait until the typed API to do better
if !lhs_text.starts_with("method(") && !lhs.is_identifier_or_string() {
return Ok(None);
}

let start = convert_point_to_position(contents, lhs.start_position());
let end = convert_point_to_position(contents, lhs.end_position());

Ok(Some(IndexEntry {
key: lhs_text.clone(),
range: Range { start, end },
data: IndexEntryData::Variable { name: lhs_text },
}))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems pretty reasonable even with the hackiness for method(

Comment on lines +101 to +110
indexer::IndexEntryData::Variable { name } => {
let completion = match completion_item_from_variable(name) {
Ok(item) => item,
Err(err) => {
log::error!("{err:?}");
return;
},
};
completions.push(completion);
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this ensures that for the workspace we now add variables to the completion list even if they don't exist yet in the R session. For example in a fresh restart of clock we now get these global constants:

Screenshot 2024-11-05 at 8 49 23 AM

I think that's probably fine, even though sending that to the console will result in an error. I guess its still useful when writing in the editor without loading the package?

However, note that after you load the package, the completion shows up with a different symbol:

Screenshot 2024-11-05 at 8 50 08 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will all get cleaned up once we build a stronger model of object scopes.

Comment on lines +98 to +110
IndexEntryData::Variable { name } => {
info.push(SymbolInformation {
name: name.clone(),
kind: SymbolKind::VARIABLE,
location: Location {
uri: Url::from_file_path(path).unwrap(),
range: entry.range,
},
tags: None,
deprecated: None,
container_name: None,
});
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like workspace symbols are working here:

Screenshot 2024-11-05 at 8 40 50 AM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess workspace symbols are enough to power jump to definition, neat

deprecated: None,
container_name: None,
});
},
};
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's somewhat interesting to note that for document symbols, i.e. with @, we were already tracking all variables due to how document_symbols() and eventually index_assignment() works.

It even already worked with method( calls. We don't check that the LHS is an identifier, we just accept anything. That does lead to some weirdness but its probably ok.

Screenshot 2024-11-05 at 8 58 59 AM

So it was just the following two places where we weren't indexing variables but we do now:

  • Workspace level symbols (so #name works across files, and so you can jump to defn)
  • Workspace level static analysis completions (even though we previously did get a completion item already due to runtime completions once the user loaded the package)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I hadn't caught up on that, good catch

@lionel- lionel- merged commit f2800d1 into main Nov 5, 2024
6 checks passed
@lionel- lionel- deleted the feature/variable-indexing branch November 5, 2024 14:22
@github-actions github-actions bot locked and limited conversation to collaborators Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants