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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@

## 2024-10

- Objects assigned at top level are now indexed, in addition to assigned functions. When a name is assigned multiple times, we now only index the first occurrence. This allows you to jump to the first "declaration" of the variable. In the future we'll improve this mechanism so that you can jump to the most recent assignment.

We also index `method(generic, class) <-` assignment to help with S7 development. This might be replaced by a "Find implementations" mechanism in the future.

- Results from completions have been improved with extra details.
Package functions now display the package name (posit-dev/positron#5225)
and namespace completions now display `::` to hint at what is being
Expand Down
8 changes: 8 additions & 0 deletions crates/ark/src/lsp/completions/completion_item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,14 @@ pub(super) unsafe fn completion_item_from_object(
Ok(item)
}

pub(super) fn completion_item_from_variable(name: &str) -> anyhow::Result<CompletionItem> {
let mut item = completion_item(String::from(name), CompletionData::Object {
name: String::from(name),
})?;
item.kind = Some(CompletionItemKind::VALUE);
Ok(item)
}

pub(super) unsafe fn completion_item_from_promise(
name: &str,
object: SEXP,
Expand Down
1 change: 1 addition & 0 deletions crates/ark/src/lsp/completions/sources/composite/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ fn completions_from_workspace_arguments(
// Not a function
return Ok(None);
},
indexer::IndexEntryData::Variable { .. } => return Ok(None),
}

// Only 1 call worth of arguments are added to the completion set.
Expand Down
11 changes: 11 additions & 0 deletions crates/ark/src/lsp/completions/sources/composite/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use tower_lsp::lsp_types::MarkupContent;
use tower_lsp::lsp_types::MarkupKind;

use crate::lsp::completions::completion_item::completion_item_from_function;
use crate::lsp::completions::completion_item::completion_item_from_variable;
use crate::lsp::completions::sources::utils::filter_out_dot_prefixes;
use crate::lsp::document_context::DocumentContext;
use crate::lsp::indexer;
Expand Down Expand Up @@ -97,6 +98,16 @@ pub(super) fn completions_from_workspace(
},

indexer::IndexEntryData::Section { level: _, title: _ } => {},
indexer::IndexEntryData::Variable { name } => {
let completion = match completion_item_from_variable(name) {
Ok(item) => item,
Err(err) => {
log::error!("{err:?}");
return;
},
};
completions.push(completion);
},
Comment on lines +101 to +110
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.

}
});

Expand Down
50 changes: 49 additions & 1 deletion crates/ark/src/lsp/indexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ use crate::treesitter::NodeTypeExt;

#[derive(Clone, Debug)]
pub enum IndexEntryData {
Variable {
name: String,
},
Function {
name: String,
arguments: Vec<String>,
Expand Down Expand Up @@ -117,7 +120,13 @@ fn insert(path: &Path, entry: IndexEntry) -> anyhow::Result<()> {
let path = str_from_path(path)?;

let index = index.entry(path.to_string()).or_default();
index.insert(entry.key.clone(), entry);

// Retain the first occurrence in the index. In the future we'll track every occurrences and
// their scopes but for now we only track the first definition of an object (in a way, its
// declaration).
if !index.contains_key(&entry.key) {
index.insert(entry.key.clone(), entry);
}

Ok(())
}
Expand Down Expand Up @@ -205,6 +214,11 @@ fn index_node(path: &Path, contents: &Rope, node: &Node) -> anyhow::Result<Optio
return Ok(Some(entry));
}

// Should be after function indexing as this is a more general case
if let Ok(Some(entry)) = index_variable(path, contents, node) {
return Ok(Some(entry));
}

if let Ok(Some(entry)) = index_comment(path, contents, node) {
return Ok(Some(entry));
}
Expand Down Expand Up @@ -262,6 +276,40 @@ fn index_function(
}))
}

fn index_variable(
_path: &Path,
contents: &Rope,
node: &Node,
) -> anyhow::Result<Option<IndexEntry>> {
if !matches!(
node.node_type(),
NodeType::BinaryOperator(BinaryOperatorType::LeftAssignment) |
NodeType::BinaryOperator(BinaryOperatorType::EqualsAssignment)
) {
return Ok(None);
}

let Some(lhs) = node.child_by_field_name("lhs") else {
return Ok(None);
};

let lhs_text = contents.node_slice(&lhs)?.to_string();

// 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 },
}))
Comment on lines +298 to +310
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(

}

fn index_comment(_path: &Path, contents: &Rope, node: &Node) -> anyhow::Result<Option<IndexEntry>> {
// check for comment
node.is_comment().into_result()?;
Expand Down
13 changes: 13 additions & 0 deletions crates/ark/src/lsp/symbols.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,19 @@ pub fn symbols(params: &WorkspaceSymbolParams) -> anyhow::Result<Vec<SymbolInfor
container_name: None,
});
},
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,
});
},
Comment on lines +98 to +110
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

};
});

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

Expand Down
Loading