From 490c8476738879e4d66b32681eb9d1236e13df96 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 7 Mar 2023 09:32:20 +0100 Subject: [PATCH 01/41] feat: first implementation of auto-complete --- starlark/src/analysis/definition.rs | 9 ++ starlark/src/lsp/server.rs | 225 +++++++++++++++++++++++++++- 2 files changed, 233 insertions(+), 1 deletion(-) diff --git a/starlark/src/analysis/definition.rs b/starlark/src/analysis/definition.rs index 4e0825248..ed6c1d198 100644 --- a/starlark/src/analysis/definition.rs +++ b/starlark/src/analysis/definition.rs @@ -416,6 +416,15 @@ impl LspModule { } } + /// Get the list of symbols exported by this module. + pub(crate) fn get_exported_symbols(&self) -> Vec<&str> { + self.ast + .exported_symbols() + .into_iter() + .map(|(_span, name)| name) + .collect() + } + /// Attempt to find the location in this module where an exported symbol is defined. pub(crate) fn find_exported_symbol(&self, name: &str) -> Option { self.ast diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 6c4251cc7..27b2a9db0 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -28,6 +28,8 @@ use derivative::Derivative; use derive_more::Display; use dupe::Dupe; use dupe::OptionDupedExt; +use gazebo::prelude::SliceExt; +use logos::Source; use lsp_server::Connection; use lsp_server::Message; use lsp_server::Notification; @@ -40,7 +42,13 @@ use lsp_types::notification::DidCloseTextDocument; use lsp_types::notification::DidOpenTextDocument; use lsp_types::notification::LogMessage; use lsp_types::notification::PublishDiagnostics; +use lsp_types::request::Completion; use lsp_types::request::GotoDefinition; +use lsp_types::CompletionItem; +use lsp_types::CompletionItemKind; +use lsp_types::CompletionOptions; +use lsp_types::CompletionParams; +use lsp_types::CompletionResponse; use lsp_types::DefinitionOptions; use lsp_types::Diagnostic; use lsp_types::DidChangeTextDocumentParams; @@ -53,13 +61,16 @@ use lsp_types::LocationLink; use lsp_types::LogMessageParams; use lsp_types::MessageType; use lsp_types::OneOf; +use lsp_types::Position; use lsp_types::PublishDiagnosticsParams; use lsp_types::Range; use lsp_types::ServerCapabilities; use lsp_types::TextDocumentSyncCapability; use lsp_types::TextDocumentSyncKind; +use lsp_types::TextEdit; use lsp_types::Url; use lsp_types::WorkDoneProgressOptions; +use lsp_types::WorkspaceFolder; use serde::de::DeserializeOwned; use serde::Deserialize; use serde::Deserializer; @@ -72,6 +83,11 @@ use crate::analysis::definition::IdentifierDefinition; use crate::analysis::definition::LspModule; use crate::codemap::ResolvedSpan; use crate::lsp::server::LoadContentsError::WrongScheme; +use crate::syntax::ast::AstNoPayload; +use crate::syntax::ast::AstStmt; +use crate::syntax::ast::ExprP; +use crate::syntax::ast::ParameterP; +use crate::syntax::ast::StmtP; use crate::syntax::AstModule; /// The request to get the file contents for a starlark: URI @@ -332,6 +348,17 @@ impl Backend { ServerCapabilities { text_document_sync: Some(TextDocumentSyncCapability::Kind(TextDocumentSyncKind::FULL)), definition_provider, + completion_provider: Some(CompletionOptions { + trigger_characters: Some(vec![ + "(".to_string(), + ",".to_string(), + "=".to_string(), + "[".to_string(), + ]), + + // all_commit_characters: Some(vec![" ".to_string()]), + ..Default::default() + }), ..ServerCapabilities::default() } } @@ -401,6 +428,19 @@ impl Backend { self.send_response(new_response(id, self.find_definition(params))); } + /// Offers completion of known symbols in the current file. + fn completion( + &self, + id: RequestId, + params: CompletionParams, + initialize_params: &InitializeParams, + ) { + self.send_response(new_response( + id, + self.completion_options(params, initialize_params), + )); + } + /// Get the file contents of a starlark: URI. fn get_starlark_file_contents(&self, id: RequestId, params: StarlarkFileContentsParams) { let response: anyhow::Result<_> = match params.uri { @@ -587,6 +627,187 @@ impl Backend { }; Ok(GotoDefinitionResponse::Link(response)) } + + fn completion_options( + &self, + params: CompletionParams, + initialize_params: &InitializeParams, + ) -> anyhow::Result { + let uri = params.text_document_position.text_document.uri.try_into()?; + + let symbols: Option> = match self.get_ast(&uri) { + Some(ast) => { + let mut symbols = Vec::new(); + + // Scan through current document + self.collect_symbols(&ast.ast.statement, &mut symbols); + + // Discover exported symbols from other documents + let docs = self.last_valid_parse.read().unwrap(); + if docs.len() > 1 { + // Find the position of the last load in the current file. + let mut last_load = None; + ast.ast.statement.visit_stmt(|node| { + if matches!(&node.node, StmtP::Load(_)) { + // if let StmtP::Load(load) = &node.node { + last_load = Some(node.span); + } + }); + let last_load = last_load.map(|span| ast.ast.codemap.resolve_span(span)); + + for (doc_uri, doc) in self + .last_valid_parse + .read() + .unwrap() + .iter() + .filter(|(&ref doc_uri, _)| doc_uri != &uri) + { + symbols.extend(doc.get_exported_symbols().map(|name| CompletionItem { + label: name.to_string(), + detail: Some(format!("Load from {doc_uri}")), + // TODO: Should be based on actual type of exported symbol. + kind: Some(CompletionItemKind::METHOD), + additional_text_edits: Some(vec![TextEdit::new( + match last_load { + Some(span) => Range::new( + Position::new(span.end_line as u32, span.end_column as u32), + Position::new(span.end_line as u32, span.end_column as u32), + ), + None => Range::new(Position::new(0, 0), Position::new(0, 0)), + }, + format!( + "{}load(\"{}\", \"{}\")", + if last_load.is_some() { "\n" } else { "" }, + self.format_load_path( + &initialize_params.workspace_folders, + doc_uri.path().to_str().unwrap() + ), + name + ), + )]), + ..Default::default() + })); + } + } + + Some(symbols) + } + None => None, + }; + + Ok(CompletionResponse::Array(symbols.unwrap_or_default())) + } + + /// Walk the AST recursively and discover symbols. + fn collect_symbols(&self, ast: &AstStmt, mut symbols: &mut Vec) { + match &ast.node { + StmtP::Assign(dest, rhs) => { + let source = &rhs.as_ref().1; + dest.visit_lvalue(|x| { + symbols.push(CompletionItem { + label: x.0.to_string(), + kind: Some(match source.node { + ExprP::Lambda(_) => CompletionItemKind::METHOD, + _ => CompletionItemKind::VARIABLE, + }), + ..Default::default() + }) + }) + } + StmtP::AssignModify(dest, _, source) => dest.visit_lvalue(|x| { + symbols.push(CompletionItem { + label: x.0.to_string(), + kind: Some(match source.node { + ExprP::Lambda(_) => CompletionItemKind::METHOD, + _ => CompletionItemKind::VARIABLE, + }), + ..Default::default() + }) + }), + StmtP::For(dest, over_body) => { + let (_, body) = &**over_body; + dest.visit_lvalue(|x| { + symbols.push(CompletionItem { + label: x.0.to_string(), + kind: Some(CompletionItemKind::VARIABLE), + ..Default::default() + }) + }); + self.collect_symbols(body, &mut symbols); + } + StmtP::Def(def) => { + symbols.push(CompletionItem { + label: def.name.to_string(), + kind: Some(CompletionItemKind::METHOD), + ..Default::default() + }); + symbols.extend(def.params.iter().filter_map(|param| match ¶m.node { + ParameterP::Normal(p, _) | ParameterP::WithDefaultValue(p, _, _) => { + Some(CompletionItem { + label: p.0.clone(), + kind: Some(CompletionItemKind::VARIABLE), + ..Default::default() + }) + } + _ => None, + })); + self.collect_symbols(&def.body, &mut symbols); + } + StmtP::Load(load) => symbols.extend(load.args.iter().map(|(name, _)| CompletionItem { + label: name.0.clone(), + detail: Some(format!("Loaded from {}", load.module.node)), + // TODO: This should be dynamic based on the actual loaded value. + kind: Some(CompletionItemKind::METHOD), + ..Default::default() + })), + + stmt => stmt.visit_stmt(|x| self.collect_symbols(x, &mut symbols)), + } + } + + /// Given the workspace root and a target file, format the path as a path that load() understands. + /// TODO: Handle cases other than Bazel, Windows paths, other repositories, other workspaces. + fn format_load_path( + &self, + workspace_roots: &Option>, + target_file: &str, + ) -> String { + let target = if let Some(roots) = workspace_roots { + let mut result = target_file; + for root in roots { + if let Some(stripped) = target_file.strip_prefix(root.uri.path()) { + result = stripped; + break; + } + } + + result + } else { + target_file + } + .strip_prefix("/") + .unwrap(); + + let default_suffixes = ["/BUILD", "/BUILD.bzl", "/BUILD.bazel"]; + for s in default_suffixes { + if let Some(p) = target.strip_suffix(s) { + return p.to_string(); + } + } + + let last_slash = target.rfind('/'); + if let Some(pos) = last_slash { + let mut result = "//".to_string(); + result.push_str(target.slice(0..pos).unwrap()); + result.push(':'); + result.push_str(target.slice((pos + 1)..target.len()).unwrap()); + + result + } else { + // Must be a file in the root of the workspace + format!("//:{target}") + } + } } /// The library style pieces @@ -615,7 +836,7 @@ impl Backend { )); } - fn main_loop(&self, _params: InitializeParams) -> anyhow::Result<()> { + fn main_loop(&self, initialize_params: InitializeParams) -> anyhow::Result<()> { self.log_message(MessageType::INFO, "Starlark server initialised"); for msg in &self.connection.receiver { match msg { @@ -626,6 +847,8 @@ impl Backend { self.goto_definition(req.id, params); } else if let Some(params) = as_request::(&req) { self.get_starlark_file_contents(req.id, params); + } else if let Some(params) = as_request::(&req) { + self.completion(req.id, params, &initialize_params); } else if self.connection.handle_shutdown(&req)? { return Ok(()); } From 32dfb2d1b7905bc394aed1759e52dec125a06729 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 7 Mar 2023 10:35:22 +0100 Subject: [PATCH 02/41] feat: update existing load when inserting load from same module --- starlark/src/lsp/server.rs | 107 ++++++++++++++++++++++++++++--------- 1 file changed, 82 insertions(+), 25 deletions(-) diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 27b2a9db0..e9d9b4f19 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -29,6 +29,7 @@ use derive_more::Display; use dupe::Dupe; use dupe::OptionDupedExt; use gazebo::prelude::SliceExt; +use itertools::Itertools; use logos::Source; use lsp_server::Connection; use lsp_server::Message; @@ -83,7 +84,6 @@ use crate::analysis::definition::IdentifierDefinition; use crate::analysis::definition::LspModule; use crate::codemap::ResolvedSpan; use crate::lsp::server::LoadContentsError::WrongScheme; -use crate::syntax::ast::AstNoPayload; use crate::syntax::ast::AstStmt; use crate::syntax::ast::ExprP; use crate::syntax::ast::ParameterP; @@ -647,10 +647,11 @@ impl Backend { if docs.len() > 1 { // Find the position of the last load in the current file. let mut last_load = None; + let mut loads = HashMap::new(); ast.ast.statement.visit_stmt(|node| { - if matches!(&node.node, StmtP::Load(_)) { - // if let StmtP::Load(load) = &node.node { + if let StmtP::Load(load) = &node.node { last_load = Some(node.span); + loads.insert(load.module.node.clone(), (load.args.clone(), node.span)); } }); let last_load = last_load.map(|span| ast.ast.codemap.resolve_span(span)); @@ -662,30 +663,86 @@ impl Backend { .iter() .filter(|(&ref doc_uri, _)| doc_uri != &uri) { - symbols.extend(doc.get_exported_symbols().map(|name| CompletionItem { - label: name.to_string(), - detail: Some(format!("Load from {doc_uri}")), - // TODO: Should be based on actual type of exported symbol. - kind: Some(CompletionItemKind::METHOD), - additional_text_edits: Some(vec![TextEdit::new( - match last_load { - Some(span) => Range::new( - Position::new(span.end_line as u32, span.end_column as u32), - Position::new(span.end_line as u32, span.end_column as u32), + let load_path = self.format_load_path( + &initialize_params.workspace_folders, + doc_uri.path().to_str().unwrap(), + ); + symbols.extend(doc.get_exported_symbols().map(|name| { + // Construct the text edit to insert a load for this exported symbol. + let text_edit = if let Some(existing_load) = loads.get(&load_path) { + // We're already loading a symbol from this module path, construct + // a text edit that amends the existing load. + let (previously_loaded_symbols, load_span) = existing_load; + let load_span = ast.ast.codemap.resolve_span(*load_span); + let mut load_args: Vec<(&str, &str)> = previously_loaded_symbols + .iter() + .map(|(assign, name)| (assign.0.as_str(), name.node.as_str())) + .collect(); + load_args.push((name, name)); + load_args.sort_by(|(_, a), (_, b)| a.cmp(b)); + + TextEdit::new( + Range::new( + Position::new( + load_span.begin_line as u32, + load_span.begin_column as u32, + ), + Position::new( + load_span.end_line as u32, + load_span.end_column as u32, + ), ), - None => Range::new(Position::new(0, 0), Position::new(0, 0)), - }, - format!( - "{}load(\"{}\", \"{}\")", - if last_load.is_some() { "\n" } else { "" }, - self.format_load_path( - &initialize_params.workspace_folders, - doc_uri.path().to_str().unwrap() + format!( + "load(\"{}\", {})", + &load_path, + load_args + .into_iter() + .map(|(assign, import)| { + if assign == import { + format!("\"{}\"", import) + } else { + format!("{} = \"{}\"", assign, import) + } + }) + .join(", ") ), - name - ), - )]), - ..Default::default() + ) + } else { + // We're not yet loading from this module, construct a text edit that + // inserts a new load statement after the last one we found. + TextEdit::new( + match last_load { + Some(span) => Range::new( + Position::new( + span.end_line as u32, + span.end_column as u32, + ), + Position::new( + span.end_line as u32, + span.end_column as u32, + ), + ), + None => { + Range::new(Position::new(0, 0), Position::new(0, 0)) + } + }, + format!( + "{}load(\"{}\", \"{}\")", + if last_load.is_some() { "\n" } else { "" }, + &load_path, + name + ), + ) + }; + + CompletionItem { + label: name.to_string(), + detail: Some(format!("Load from {doc_uri}")), + // TODO: Should be based on actual type of exported symbol. + kind: Some(CompletionItemKind::METHOD), + additional_text_edits: Some(vec![text_edit]), + ..Default::default() + } })); } } From 43ed7b1f91d155ba7841ba92f8d71fedc2e6ac93 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 7 Mar 2023 10:35:35 +0100 Subject: [PATCH 03/41] chore: tidy up --- starlark/src/lsp/server.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index e9d9b4f19..d73403fe5 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -640,7 +640,7 @@ impl Backend { let mut symbols = Vec::new(); // Scan through current document - self.collect_symbols(&ast.ast.statement, &mut symbols); + Self::collect_symbols(&ast.ast.statement, &mut symbols); // Discover exported symbols from other documents let docs = self.last_valid_parse.read().unwrap(); @@ -756,7 +756,7 @@ impl Backend { } /// Walk the AST recursively and discover symbols. - fn collect_symbols(&self, ast: &AstStmt, mut symbols: &mut Vec) { + fn collect_symbols(ast: &AstStmt, symbols: &mut Vec) { match &ast.node { StmtP::Assign(dest, rhs) => { let source = &rhs.as_ref().1; @@ -790,7 +790,7 @@ impl Backend { ..Default::default() }) }); - self.collect_symbols(body, &mut symbols); + Self::collect_symbols(body, symbols); } StmtP::Def(def) => { symbols.push(CompletionItem { @@ -808,7 +808,7 @@ impl Backend { } _ => None, })); - self.collect_symbols(&def.body, &mut symbols); + Self::collect_symbols(&def.body, symbols); } StmtP::Load(load) => symbols.extend(load.args.iter().map(|(name, _)| CompletionItem { label: name.0.clone(), @@ -818,7 +818,7 @@ impl Backend { ..Default::default() })), - stmt => stmt.visit_stmt(|x| self.collect_symbols(x, &mut symbols)), + stmt => stmt.visit_stmt(|x| Self::collect_symbols(x, symbols)), } } @@ -842,7 +842,7 @@ impl Backend { } else { target_file } - .strip_prefix("/") + .strip_prefix('/') .unwrap(); let default_suffixes = ["/BUILD", "/BUILD.bzl", "/BUILD.bazel"]; From f2484791c301f2d367adbf4046732f3a4dfbaac8 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 7 Mar 2023 10:51:56 +0100 Subject: [PATCH 04/41] feat: de-duplicate already loaded symbols in auto-complete --- starlark/src/lsp/server.rs | 236 ++++++++++++++++++++----------------- 1 file changed, 131 insertions(+), 105 deletions(-) diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index d73403fe5..762a65000 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -637,7 +637,7 @@ impl Backend { let symbols: Option> = match self.get_ast(&uri) { Some(ast) => { - let mut symbols = Vec::new(); + let mut symbols = HashMap::new(); // Scan through current document Self::collect_symbols(&ast.ast.statement, &mut symbols); @@ -667,87 +667,99 @@ impl Backend { &initialize_params.workspace_folders, doc_uri.path().to_str().unwrap(), ); - symbols.extend(doc.get_exported_symbols().map(|name| { - // Construct the text edit to insert a load for this exported symbol. - let text_edit = if let Some(existing_load) = loads.get(&load_path) { - // We're already loading a symbol from this module path, construct - // a text edit that amends the existing load. - let (previously_loaded_symbols, load_span) = existing_load; - let load_span = ast.ast.codemap.resolve_span(*load_span); - let mut load_args: Vec<(&str, &str)> = previously_loaded_symbols - .iter() - .map(|(assign, name)| (assign.0.as_str(), name.node.as_str())) - .collect(); - load_args.push((name, name)); - load_args.sort_by(|(_, a), (_, b)| a.cmp(b)); - - TextEdit::new( - Range::new( - Position::new( - load_span.begin_line as u32, - load_span.begin_column as u32, - ), - Position::new( - load_span.end_line as u32, - load_span.end_column as u32, - ), - ), - format!( - "load(\"{}\", {})", - &load_path, - load_args - .into_iter() - .map(|(assign, import)| { - if assign == import { - format!("\"{}\"", import) - } else { - format!("{} = \"{}\"", assign, import) - } + let load_symbols: Vec<_> = doc + .get_exported_symbols() + .into_iter() + .filter(|&name| !symbols.contains_key(name)) + .map(|name| { + // Construct the text edit to insert a load for this exported symbol. + let text_edit = if let Some(existing_load) = loads.get(&load_path) { + // We're already loading a symbol from this module path, construct + // a text edit that amends the existing load. + let (previously_loaded_symbols, load_span) = existing_load; + let load_span = ast.ast.codemap.resolve_span(*load_span); + let mut load_args: Vec<(&str, &str)> = + previously_loaded_symbols + .iter() + .map(|(assign, name)| { + (assign.0.as_str(), name.node.as_str()) }) - .join(", ") - ), - ) - } else { - // We're not yet loading from this module, construct a text edit that - // inserts a new load statement after the last one we found. - TextEdit::new( - match last_load { - Some(span) => Range::new( + .collect(); + load_args.push((name, name)); + load_args.sort_by(|(_, a), (_, b)| a.cmp(b)); + + TextEdit::new( + Range::new( Position::new( - span.end_line as u32, - span.end_column as u32, + load_span.begin_line as u32, + load_span.begin_column as u32, ), Position::new( - span.end_line as u32, - span.end_column as u32, + load_span.end_line as u32, + load_span.end_column as u32, ), ), - None => { - Range::new(Position::new(0, 0), Position::new(0, 0)) - } + format!( + "load(\"{}\", {})", + &load_path, + load_args + .into_iter() + .map(|(assign, import)| { + if assign == import { + format!("\"{}\"", import) + } else { + format!("{} = \"{}\"", assign, import) + } + }) + .join(", ") + ), + ) + } else { + // We're not yet loading from this module, construct a text edit that + // inserts a new load statement after the last one we found. + TextEdit::new( + match last_load { + Some(span) => Range::new( + Position::new( + span.end_line as u32, + span.end_column as u32, + ), + Position::new( + span.end_line as u32, + span.end_column as u32, + ), + ), + None => { + Range::new(Position::new(0, 0), Position::new(0, 0)) + } + }, + format!( + "{}load(\"{}\", \"{}\")", + if last_load.is_some() { "\n" } else { "" }, + &load_path, + name + ), + ) + }; + + ( + name.to_string(), + CompletionItem { + label: name.to_string(), + detail: Some(format!("Load from {load_path}")), + // TODO: Should be based on actual type of exported symbol. + kind: Some(CompletionItemKind::METHOD), + additional_text_edits: Some(vec![text_edit]), + ..Default::default() }, - format!( - "{}load(\"{}\", \"{}\")", - if last_load.is_some() { "\n" } else { "" }, - &load_path, - name - ), ) - }; - - CompletionItem { - label: name.to_string(), - detail: Some(format!("Load from {doc_uri}")), - // TODO: Should be based on actual type of exported symbol. - kind: Some(CompletionItemKind::METHOD), - additional_text_edits: Some(vec![text_edit]), - ..Default::default() - } - })); + }) + .collect(); + symbols.extend(load_symbols); } } - Some(symbols) + Some(symbols.into_values().collect()) } None => None, }; @@ -756,66 +768,80 @@ impl Backend { } /// Walk the AST recursively and discover symbols. - fn collect_symbols(ast: &AstStmt, symbols: &mut Vec) { + fn collect_symbols(ast: &AstStmt, symbols: &mut HashMap) { match &ast.node { StmtP::Assign(dest, rhs) => { let source = &rhs.as_ref().1; dest.visit_lvalue(|x| { - symbols.push(CompletionItem { + symbols + .entry(x.0.to_string()) + .or_insert_with(|| CompletionItem { + label: x.0.to_string(), + kind: Some(match source.node { + ExprP::Lambda(_) => CompletionItemKind::METHOD, + _ => CompletionItemKind::VARIABLE, + }), + ..Default::default() + }); + }) + } + StmtP::AssignModify(dest, _, source) => dest.visit_lvalue(|x| { + symbols + .entry(x.0.to_string()) + .or_insert_with(|| CompletionItem { label: x.0.to_string(), kind: Some(match source.node { ExprP::Lambda(_) => CompletionItemKind::METHOD, _ => CompletionItemKind::VARIABLE, }), ..Default::default() - }) - }) - } - StmtP::AssignModify(dest, _, source) => dest.visit_lvalue(|x| { - symbols.push(CompletionItem { - label: x.0.to_string(), - kind: Some(match source.node { - ExprP::Lambda(_) => CompletionItemKind::METHOD, - _ => CompletionItemKind::VARIABLE, - }), - ..Default::default() - }) + }); }), StmtP::For(dest, over_body) => { let (_, body) = &**over_body; dest.visit_lvalue(|x| { - symbols.push(CompletionItem { - label: x.0.to_string(), - kind: Some(CompletionItemKind::VARIABLE), - ..Default::default() - }) + symbols + .entry(x.0.to_string()) + .or_insert_with(|| CompletionItem { + label: x.0.to_string(), + kind: Some(CompletionItemKind::VARIABLE), + ..Default::default() + }); }); Self::collect_symbols(body, symbols); } StmtP::Def(def) => { - symbols.push(CompletionItem { - label: def.name.to_string(), - kind: Some(CompletionItemKind::METHOD), - ..Default::default() - }); + symbols + .entry(def.name.0.to_string()) + .or_insert_with(|| CompletionItem { + label: def.name.to_string(), + kind: Some(CompletionItemKind::METHOD), + ..Default::default() + }); symbols.extend(def.params.iter().filter_map(|param| match ¶m.node { - ParameterP::Normal(p, _) | ParameterP::WithDefaultValue(p, _, _) => { - Some(CompletionItem { + ParameterP::Normal(p, _) | ParameterP::WithDefaultValue(p, _, _) => Some(( + p.0.to_string(), + CompletionItem { label: p.0.clone(), kind: Some(CompletionItemKind::VARIABLE), ..Default::default() - }) - } + }, + )), _ => None, })); Self::collect_symbols(&def.body, symbols); } - StmtP::Load(load) => symbols.extend(load.args.iter().map(|(name, _)| CompletionItem { - label: name.0.clone(), - detail: Some(format!("Loaded from {}", load.module.node)), - // TODO: This should be dynamic based on the actual loaded value. - kind: Some(CompletionItemKind::METHOD), - ..Default::default() + StmtP::Load(load) => symbols.extend(load.args.iter().map(|(name, _)| { + ( + name.0.to_string(), + CompletionItem { + label: name.0.clone(), + detail: Some(format!("Loaded from {}", load.module.node)), + // TODO: This should be dynamic based on the actual loaded value. + kind: Some(CompletionItemKind::METHOD), + ..Default::default() + }, + ) })), stmt => stmt.visit_stmt(|x| Self::collect_symbols(x, symbols)), From 16872c95fb6c473ad30526132ec11e0a5a46335c Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 7 Mar 2023 11:49:05 +0100 Subject: [PATCH 05/41] feat: don't autocomplete variables from other methods --- starlark/src/codemap.rs | 9 +++++- starlark/src/lsp/server.rs | 58 +++++++++++++++++++++++++++----------- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/starlark/src/codemap.rs b/starlark/src/codemap.rs index 725e01a1d..7306221c1 100644 --- a/starlark/src/codemap.rs +++ b/starlark/src/codemap.rs @@ -392,7 +392,7 @@ impl CodeMap { /// A line and column. #[derive(Copy, Clone, Dupe, Hash, Eq, PartialEq, Debug)] -struct LineCol { +pub(crate) struct LineCol { /// The line number within the file (0-indexed). pub line: usize, @@ -542,6 +542,13 @@ impl From for lsp_types::Range { } impl ResolvedSpan { + pub(crate) fn contains(&self, pos: LineCol) -> bool { + (self.begin_line < pos.line + || (self.begin_line == pos.line && self.begin_column <= pos.column)) + && (self.end_line > pos.line + || (self.end_line == pos.line && self.end_column >= pos.column)) + } + fn from_span(begin: LineCol, end: LineCol) -> Self { Self { begin_line: begin.line, diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 762a65000..867385c33 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -82,6 +82,8 @@ use crate::analysis::definition::Definition; use crate::analysis::definition::DottedDefinition; use crate::analysis::definition::IdentifierDefinition; use crate::analysis::definition::LspModule; +use crate::codemap::CodeMap; +use crate::codemap::LineCol; use crate::codemap::ResolvedSpan; use crate::lsp::server::LoadContentsError::WrongScheme; use crate::syntax::ast::AstStmt; @@ -640,7 +642,16 @@ impl Backend { let mut symbols = HashMap::new(); // Scan through current document - Self::collect_symbols(&ast.ast.statement, &mut symbols); + let cursor_position = LineCol { + line: params.text_document_position.position.line as usize, + column: params.text_document_position.position.character as usize, + }; + Self::collect_symbols( + &ast.ast.codemap, + &ast.ast.statement, + cursor_position, + &mut symbols, + ); // Discover exported symbols from other documents let docs = self.last_valid_parse.read().unwrap(); @@ -768,7 +779,12 @@ impl Backend { } /// Walk the AST recursively and discover symbols. - fn collect_symbols(ast: &AstStmt, symbols: &mut HashMap) { + fn collect_symbols( + codemap: &CodeMap, + ast: &AstStmt, + cursor_position: LineCol, + symbols: &mut HashMap, + ) { match &ast.node { StmtP::Assign(dest, rhs) => { let source = &rhs.as_ref().1; @@ -808,7 +824,7 @@ impl Backend { ..Default::default() }); }); - Self::collect_symbols(body, symbols); + Self::collect_symbols(codemap, body, cursor_position, symbols); } StmtP::Def(def) => { symbols @@ -818,18 +834,25 @@ impl Backend { kind: Some(CompletionItemKind::METHOD), ..Default::default() }); - symbols.extend(def.params.iter().filter_map(|param| match ¶m.node { - ParameterP::Normal(p, _) | ParameterP::WithDefaultValue(p, _, _) => Some(( - p.0.to_string(), - CompletionItem { - label: p.0.clone(), - kind: Some(CompletionItemKind::VARIABLE), - ..Default::default() - }, - )), - _ => None, - })); - Self::collect_symbols(&def.body, symbols); + + // Only recurse into method if the cursor is in it. + if codemap + .resolve_span(def.body.span) + .contains(cursor_position) + { + symbols.extend(def.params.iter().filter_map(|param| match ¶m.node { + ParameterP::Normal(p, _) | ParameterP::WithDefaultValue(p, _, _) => Some(( + p.0.to_string(), + CompletionItem { + label: p.0.clone(), + kind: Some(CompletionItemKind::VARIABLE), + ..Default::default() + }, + )), + _ => None, + })); + Self::collect_symbols(codemap, &def.body, cursor_position, symbols); + } } StmtP::Load(load) => symbols.extend(load.args.iter().map(|(name, _)| { ( @@ -843,8 +866,9 @@ impl Backend { }, ) })), - - stmt => stmt.visit_stmt(|x| Self::collect_symbols(x, symbols)), + stmt => { + stmt.visit_stmt(|x| Self::collect_symbols(codemap, x, cursor_position, symbols)) + } } } From fdbdd11b853fbd00c9a034e165e001439e9a9691 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 7 Mar 2023 13:52:44 +0100 Subject: [PATCH 06/41] fix: handle windows file paths for inserted load statements --- starlark/src/lsp/server.rs | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 867385c33..c31947b0d 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -674,10 +674,8 @@ impl Backend { .iter() .filter(|(&ref doc_uri, _)| doc_uri != &uri) { - let load_path = self.format_load_path( - &initialize_params.workspace_folders, - doc_uri.path().to_str().unwrap(), - ); + let load_path = self + .format_load_path(&initialize_params.workspace_folders, doc_uri.path()); let load_symbols: Vec<_> = doc .get_exported_symbols() .into_iter() @@ -873,16 +871,16 @@ impl Backend { } /// Given the workspace root and a target file, format the path as a path that load() understands. - /// TODO: Handle cases other than Bazel, Windows paths, other repositories, other workspaces. + /// TODO: Handle cases other than Bazel, other repositories, other workspaces. fn format_load_path( &self, workspace_roots: &Option>, - target_file: &str, + target_file: &Path, ) -> String { let target = if let Some(roots) = workspace_roots { let mut result = target_file; for root in roots { - if let Some(stripped) = target_file.strip_prefix(root.uri.path()) { + if let Ok(stripped) = target_file.strip_prefix(root.uri.path()) { result = stripped; break; } @@ -891,28 +889,24 @@ impl Backend { result } else { target_file - } - .strip_prefix('/') - .unwrap(); - - let default_suffixes = ["/BUILD", "/BUILD.bzl", "/BUILD.bazel"]; - for s in default_suffixes { - if let Some(p) = target.strip_suffix(s) { - return p.to_string(); - } - } + }; - let last_slash = target.rfind('/'); - if let Some(pos) = last_slash { + if let Some(file) = target.file_name() { let mut result = "//".to_string(); - result.push_str(target.slice(0..pos).unwrap()); + result.push_str( + &*target + .parent() + .expect("parent() should succeed if there's a file_name()") + .to_string_lossy() + .replace('\\', "/"), + ); result.push(':'); - result.push_str(target.slice((pos + 1)..target.len()).unwrap()); + result.push_str(&*file.to_string_lossy()); result } else { // Must be a file in the root of the workspace - format!("//:{target}") + format!("//:{}", target.display()) } } } From b28e1e54c77d49136b29779b29d5fc558c4e6886 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 7 Mar 2023 13:54:09 +0100 Subject: [PATCH 07/41] feat: show correct symbol kind when suggesting exported symbol from other file this only works for symbols that aren't loaded yet, for the ones that are we'd need to do path resolution of the loaded file to check the type of the loaded symbol --- starlark/src/analysis/definition.rs | 26 ++++++++++---------------- starlark/src/analysis/exported.rs | 29 ++++++++++++++++++++++++----- starlark/src/analysis/mod.rs | 2 +- starlark/src/lsp/server.rs | 23 +++++++++++++++-------- 4 files changed, 50 insertions(+), 30 deletions(-) diff --git a/starlark/src/analysis/definition.rs b/starlark/src/analysis/definition.rs index ed6c1d198..87241ba21 100644 --- a/starlark/src/analysis/definition.rs +++ b/starlark/src/analysis/definition.rs @@ -21,6 +21,7 @@ use crate::analysis::bind::scope; use crate::analysis::bind::Assigner; use crate::analysis::bind::Bind; use crate::analysis::bind::Scope; +use crate::analysis::exported::ExportedSymbol; use crate::codemap::CodeMap; use crate::codemap::Pos; use crate::codemap::ResolvedSpan; @@ -417,26 +418,19 @@ impl LspModule { } /// Get the list of symbols exported by this module. - pub(crate) fn get_exported_symbols(&self) -> Vec<&str> { - self.ast - .exported_symbols() - .into_iter() - .map(|(_span, name)| name) - .collect() + pub(crate) fn get_exported_symbols(&self) -> Vec { + self.ast.exported_symbols() } /// Attempt to find the location in this module where an exported symbol is defined. pub(crate) fn find_exported_symbol(&self, name: &str) -> Option { - self.ast - .exported_symbols() - .iter() - .find_map(|(span, symbol)| { - if *symbol == name { - Some(span.resolve_span()) - } else { - None - } - }) + self.ast.exported_symbols().iter().find_map(|symbol| { + if symbol.name == name { + Some(symbol.span.resolve_span()) + } else { + None + } + }) } /// Attempt to find the location in this module where a member of a struct (named `name`) diff --git a/starlark/src/analysis/exported.rs b/starlark/src/analysis/exported.rs index 57c0e466a..cd7abe6b6 100644 --- a/starlark/src/analysis/exported.rs +++ b/starlark/src/analysis/exported.rs @@ -21,10 +21,21 @@ use crate::syntax::ast::DefP; use crate::syntax::ast::Stmt; use crate::syntax::AstModule; +pub enum ExportedSymbolKind { + Variable, + Method, +} + +pub struct ExportedSymbol<'a> { + pub name: &'a str, + pub span: FileSpan, + pub kind: ExportedSymbolKind, +} + impl AstModule { /// Which symbols are exported by this module. These are the top-level assignments, /// including function definitions. Any symbols that start with `_` are not exported. - pub fn exported_symbols(&self) -> Vec<(FileSpan, &str)> { + pub fn exported_symbols(&self) -> Vec { // Map since we only want to store the first of each export // IndexMap since we want the order to match the order they were defined in let mut result: SmallMap<&str, _> = SmallMap::new(); @@ -32,11 +43,19 @@ impl AstModule { match &**x { Stmt::Assign(dest, _) | Stmt::AssignModify(dest, _, _) => { dest.visit_lvalue(|name| { - result.entry(&name.0).or_insert(name.span); + result.entry(&name.0).or_insert(ExportedSymbol { + name: &name.0, + span: self.file_span(name.span), + kind: ExportedSymbolKind::Variable, + }); }); } Stmt::Def(DefP { name, .. }) => { - result.entry(&name.0).or_insert(name.span); + result.entry(&name.0).or_insert(ExportedSymbol { + name: &name.0, + span: self.file_span(name.span), + kind: ExportedSymbolKind::Method, + }); } _ => {} } @@ -44,7 +63,7 @@ impl AstModule { result .into_iter() .filter(|(name, _)| !name.starts_with('_')) - .map(|(name, span)| (self.file_span(span), name)) + .map(|(_, exported_symbol)| exported_symbol) .collect() } } @@ -73,7 +92,7 @@ d = 2 ); let res = modu.exported_symbols(); assert_eq!( - res.map(|(loc, name)| format!("{} {}", loc, name)), + res.map(|symbol| format!("{} {}", symbol.span, symbol.name)), &["X:3:5-6 b", "X:4:1-2 d"] ); } diff --git a/starlark/src/analysis/mod.rs b/starlark/src/analysis/mod.rs index c16eab8f6..00b86bcf8 100644 --- a/starlark/src/analysis/mod.rs +++ b/starlark/src/analysis/mod.rs @@ -27,7 +27,7 @@ use crate::syntax::AstModule; mod bind; pub(crate) mod definition; mod dubious; -mod exported; +pub(crate) mod exported; mod find_call_name; mod flow; mod incompatible; diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index c31947b0d..12a5277c0 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -82,6 +82,7 @@ use crate::analysis::definition::Definition; use crate::analysis::definition::DottedDefinition; use crate::analysis::definition::IdentifierDefinition; use crate::analysis::definition::LspModule; +use crate::analysis::exported::ExportedSymbolKind; use crate::codemap::CodeMap; use crate::codemap::LineCol; use crate::codemap::ResolvedSpan; @@ -679,8 +680,8 @@ impl Backend { let load_symbols: Vec<_> = doc .get_exported_symbols() .into_iter() - .filter(|&name| !symbols.contains_key(name)) - .map(|name| { + .filter(|symbol| !symbols.contains_key(symbol.name)) + .map(|symbol| { // Construct the text edit to insert a load for this exported symbol. let text_edit = if let Some(existing_load) = loads.get(&load_path) { // We're already loading a symbol from this module path, construct @@ -694,7 +695,7 @@ impl Backend { (assign.0.as_str(), name.node.as_str()) }) .collect(); - load_args.push((name, name)); + load_args.push((symbol.name, symbol.name)); load_args.sort_by(|(_, a), (_, b)| a.cmp(b)); TextEdit::new( @@ -746,18 +747,24 @@ impl Backend { "{}load(\"{}\", \"{}\")", if last_load.is_some() { "\n" } else { "" }, &load_path, - name + symbol.name ), ) }; ( - name.to_string(), + symbol.name.to_string(), CompletionItem { - label: name.to_string(), + label: symbol.name.to_string(), detail: Some(format!("Load from {load_path}")), - // TODO: Should be based on actual type of exported symbol. - kind: Some(CompletionItemKind::METHOD), + kind: Some(match symbol.kind { + ExportedSymbolKind::Variable => { + CompletionItemKind::CONSTANT + } + ExportedSymbolKind::Method => { + CompletionItemKind::METHOD + } + }), additional_text_edits: Some(vec![text_edit]), ..Default::default() }, From df738bd86cd6695908e0a8ccd192b4b4cad3d407 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Mon, 1 May 2023 14:08:23 +0200 Subject: [PATCH 08/41] fix: correctly set exported symbol kind in both assign and assign modify statements --- starlark/src/analysis/exported.rs | 40 ++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/starlark/src/analysis/exported.rs b/starlark/src/analysis/exported.rs index cd7abe6b6..8d7dda04a 100644 --- a/starlark/src/analysis/exported.rs +++ b/starlark/src/analysis/exported.rs @@ -17,15 +17,21 @@ use crate::codemap::FileSpan; use crate::collections::SmallMap; +use crate::syntax::ast::AstAssignIdentP; +use crate::syntax::ast::AstExprP; +use crate::syntax::ast::AstNoPayload; use crate::syntax::ast::DefP; +use crate::syntax::ast::ExprP; use crate::syntax::ast::Stmt; use crate::syntax::AstModule; +#[derive(Debug)] pub enum ExportedSymbolKind { Variable, Method, } +#[derive(Debug)] pub struct ExportedSymbol<'a> { pub name: &'a str, pub span: FileSpan, @@ -41,13 +47,19 @@ impl AstModule { let mut result: SmallMap<&str, _> = SmallMap::new(); for x in self.top_level_statements() { match &**x { - Stmt::Assign(dest, _) | Stmt::AssignModify(dest, _, _) => { + Stmt::Assign(dest, rhs) => { + let source = &rhs.as_ref().1; dest.visit_lvalue(|name| { - result.entry(&name.0).or_insert(ExportedSymbol { - name: &name.0, - span: self.file_span(name.span), - kind: ExportedSymbolKind::Variable, - }); + result + .entry(&name.0) + .or_insert(self.get_exported_symbol_from_assignment(name, source)); + }); + } + Stmt::AssignModify(dest, _, source) => { + dest.visit_lvalue(|name| { + result + .entry(&name.0) + .or_insert(self.get_exported_symbol_from_assignment(name, source)); }); } Stmt::Def(DefP { name, .. }) => { @@ -66,6 +78,22 @@ impl AstModule { .map(|(_, exported_symbol)| exported_symbol) .collect() } + + /// Construct an [`ExportedSymbol`] based on the given assignment components. + fn get_exported_symbol_from_assignment<'a>( + &self, + name: &'a AstAssignIdentP, + source: &AstExprP, + ) -> ExportedSymbol<'a> { + ExportedSymbol { + name: &name.0, + span: self.file_span(name.span), + kind: match source.node { + ExprP::Lambda(_) => ExportedSymbolKind::Method, + _ => ExportedSymbolKind::Variable, + }, + } + } } #[cfg(test)] From 372bdaaa640a47412fa37a00d4a8d229a72504fe Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Mon, 1 May 2023 14:09:33 +0200 Subject: [PATCH 09/41] feat: refactor find_symbols_at_location to its own module, including unit tests --- starlark/src/lsp/server.rs | 128 +++-------------- starlark/src/syntax/mod.rs | 1 + starlark/src/syntax/symbols.rs | 255 +++++++++++++++++++++++++++++++++ 3 files changed, 276 insertions(+), 108 deletions(-) create mode 100644 starlark/src/syntax/symbols.rs diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 12a5277c0..96e237a6f 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -28,9 +28,7 @@ use derivative::Derivative; use derive_more::Display; use dupe::Dupe; use dupe::OptionDupedExt; -use gazebo::prelude::SliceExt; use itertools::Itertools; -use logos::Source; use lsp_server::Connection; use lsp_server::Message; use lsp_server::Notification; @@ -83,14 +81,12 @@ use crate::analysis::definition::DottedDefinition; use crate::analysis::definition::IdentifierDefinition; use crate::analysis::definition::LspModule; use crate::analysis::exported::ExportedSymbolKind; -use crate::codemap::CodeMap; use crate::codemap::LineCol; use crate::codemap::ResolvedSpan; use crate::lsp::server::LoadContentsError::WrongScheme; -use crate::syntax::ast::AstStmt; -use crate::syntax::ast::ExprP; -use crate::syntax::ast::ParameterP; use crate::syntax::ast::StmtP; +use crate::syntax::symbols::find_symbols_at_location; +use crate::syntax::symbols::SymbolKind; use crate::syntax::AstModule; /// The request to get the file contents for a starlark: URI @@ -640,19 +636,29 @@ impl Backend { let symbols: Option> = match self.get_ast(&uri) { Some(ast) => { - let mut symbols = HashMap::new(); - // Scan through current document let cursor_position = LineCol { line: params.text_document_position.position.line as usize, column: params.text_document_position.position.character as usize, }; - Self::collect_symbols( - &ast.ast.codemap, - &ast.ast.statement, - cursor_position, - &mut symbols, - ); + let mut symbols: HashMap<_, _> = + find_symbols_at_location(&ast.ast.codemap, &ast.ast.statement, cursor_position) + .into_iter() + .map(|(key, value)| { + ( + key, + CompletionItem { + label: value.name, + kind: Some(match value.kind { + SymbolKind::Method => CompletionItemKind::METHOD, + SymbolKind::Variable => CompletionItemKind::VARIABLE, + }), + detail: value.detail, + ..Default::default() + }, + ) + }) + .collect(); // Discover exported symbols from other documents let docs = self.last_valid_parse.read().unwrap(); @@ -783,100 +789,6 @@ impl Backend { Ok(CompletionResponse::Array(symbols.unwrap_or_default())) } - /// Walk the AST recursively and discover symbols. - fn collect_symbols( - codemap: &CodeMap, - ast: &AstStmt, - cursor_position: LineCol, - symbols: &mut HashMap, - ) { - match &ast.node { - StmtP::Assign(dest, rhs) => { - let source = &rhs.as_ref().1; - dest.visit_lvalue(|x| { - symbols - .entry(x.0.to_string()) - .or_insert_with(|| CompletionItem { - label: x.0.to_string(), - kind: Some(match source.node { - ExprP::Lambda(_) => CompletionItemKind::METHOD, - _ => CompletionItemKind::VARIABLE, - }), - ..Default::default() - }); - }) - } - StmtP::AssignModify(dest, _, source) => dest.visit_lvalue(|x| { - symbols - .entry(x.0.to_string()) - .or_insert_with(|| CompletionItem { - label: x.0.to_string(), - kind: Some(match source.node { - ExprP::Lambda(_) => CompletionItemKind::METHOD, - _ => CompletionItemKind::VARIABLE, - }), - ..Default::default() - }); - }), - StmtP::For(dest, over_body) => { - let (_, body) = &**over_body; - dest.visit_lvalue(|x| { - symbols - .entry(x.0.to_string()) - .or_insert_with(|| CompletionItem { - label: x.0.to_string(), - kind: Some(CompletionItemKind::VARIABLE), - ..Default::default() - }); - }); - Self::collect_symbols(codemap, body, cursor_position, symbols); - } - StmtP::Def(def) => { - symbols - .entry(def.name.0.to_string()) - .or_insert_with(|| CompletionItem { - label: def.name.to_string(), - kind: Some(CompletionItemKind::METHOD), - ..Default::default() - }); - - // Only recurse into method if the cursor is in it. - if codemap - .resolve_span(def.body.span) - .contains(cursor_position) - { - symbols.extend(def.params.iter().filter_map(|param| match ¶m.node { - ParameterP::Normal(p, _) | ParameterP::WithDefaultValue(p, _, _) => Some(( - p.0.to_string(), - CompletionItem { - label: p.0.clone(), - kind: Some(CompletionItemKind::VARIABLE), - ..Default::default() - }, - )), - _ => None, - })); - Self::collect_symbols(codemap, &def.body, cursor_position, symbols); - } - } - StmtP::Load(load) => symbols.extend(load.args.iter().map(|(name, _)| { - ( - name.0.to_string(), - CompletionItem { - label: name.0.clone(), - detail: Some(format!("Loaded from {}", load.module.node)), - // TODO: This should be dynamic based on the actual loaded value. - kind: Some(CompletionItemKind::METHOD), - ..Default::default() - }, - ) - })), - stmt => { - stmt.visit_stmt(|x| Self::collect_symbols(codemap, x, cursor_position, symbols)) - } - } - } - /// Given the workspace root and a target file, format the path as a path that load() understands. /// TODO: Handle cases other than Bazel, other repositories, other workspaces. fn format_load_path( diff --git a/starlark/src/syntax/mod.rs b/starlark/src/syntax/mod.rs index 565460913..16fd4492c 100644 --- a/starlark/src/syntax/mod.rs +++ b/starlark/src/syntax/mod.rs @@ -50,4 +50,5 @@ mod grammar { } pub(crate) mod parser; +pub(crate) mod symbols; pub(crate) mod uniplate; diff --git a/starlark/src/syntax/symbols.rs b/starlark/src/syntax/symbols.rs new file mode 100644 index 000000000..60eea0b2e --- /dev/null +++ b/starlark/src/syntax/symbols.rs @@ -0,0 +1,255 @@ +use std::collections::HashMap; + +use crate::codemap::CodeMap; +use crate::codemap::LineCol; +use crate::syntax::ast::AstPayload; +use crate::syntax::ast::AstStmtP; +use crate::syntax::ast::ExprP; +use crate::syntax::ast::ParameterP; +use crate::syntax::ast::StmtP; + +#[derive(Debug, PartialEq)] +pub(crate) enum SymbolKind { + Method, + Variable, +} + +#[derive(Debug, PartialEq)] +pub(crate) struct Symbol { + pub(crate) name: String, + pub(crate) detail: Option, + pub(crate) kind: SymbolKind, +} + +/// Walk the AST recursively and discover symbols. +pub(crate) fn find_symbols_at_location( + codemap: &CodeMap, + ast: &AstStmtP

, + cursor_position: LineCol, +) -> HashMap { + let mut symbols = HashMap::new(); + fn walk( + codemap: &CodeMap, + ast: &AstStmtP

, + cursor_position: LineCol, + symbols: &mut HashMap, + ) { + match &ast.node { + StmtP::Assign(dest, rhs) => { + let source = &rhs.as_ref().1; + dest.visit_lvalue(|x| { + symbols.entry(x.0.to_string()).or_insert_with(|| Symbol { + name: x.0.to_string(), + kind: (match source.node { + ExprP::Lambda(_) => SymbolKind::Method, + _ => SymbolKind::Variable, + }), + detail: None, + }); + }) + } + StmtP::AssignModify(dest, _, source) => dest.visit_lvalue(|x| { + symbols.entry(x.0.to_string()).or_insert_with(|| Symbol { + name: x.0.to_string(), + kind: (match source.node { + ExprP::Lambda(_) => SymbolKind::Method, + _ => SymbolKind::Variable, + }), + detail: None, + }); + }), + StmtP::For(dest, over_body) => { + let (_, body) = &**over_body; + dest.visit_lvalue(|x| { + symbols.entry(x.0.to_string()).or_insert_with(|| Symbol { + name: x.0.to_string(), + kind: SymbolKind::Variable, + detail: None, + }); + }); + walk(codemap, body, cursor_position, symbols); + } + StmtP::Def(def) => { + symbols + .entry(def.name.0.to_string()) + .or_insert_with(|| Symbol { + name: def.name.0.to_string(), + kind: SymbolKind::Method, + detail: None, + }); + + // Only recurse into method if the cursor is in it. + if codemap + .resolve_span(def.body.span) + .contains(cursor_position) + { + symbols.extend(def.params.iter().filter_map(|param| match ¶m.node { + ParameterP::Normal(p, _) | ParameterP::WithDefaultValue(p, _, _) => Some(( + p.0.to_string(), + Symbol { + name: p.0.clone(), + kind: SymbolKind::Variable, + detail: None, + }, + )), + _ => None, + })); + walk(codemap, &def.body, cursor_position, symbols); + } + } + StmtP::Load(load) => symbols.extend(load.args.iter().map(|(name, _)| { + ( + name.0.to_string(), + Symbol { + name: name.0.clone(), + detail: Some(format!("Loaded from {}", load.module.node)), + // TODO: This should be dynamic based on the actual loaded value. + kind: SymbolKind::Method, + }, + ) + })), + stmt => stmt.visit_stmt(|x| walk(codemap, x, cursor_position, symbols)), + } + } + + walk(codemap, ast, cursor_position, &mut symbols); + symbols +} + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use super::find_symbols_at_location; + use super::Symbol; + use super::SymbolKind; + use crate::codemap::LineCol; + use crate::syntax::AstModule; + use crate::syntax::Dialect; + + #[test] + fn global_symbols() { + let ast_module = AstModule::parse( + "t.star", + r#"load("foo.star", "exported_a", renamed = "exported_b") + +def method(param): + pass + +my_var = True + "# + .to_string(), + &Dialect::Standard, + ) + .unwrap(); + + assert_eq!( + find_symbols_at_location( + &ast_module.codemap, + &ast_module.statement, + LineCol { line: 6, column: 0 }, + ), + HashMap::from([ + ( + "exported_a".to_string(), + Symbol { + name: "exported_a".to_string(), + detail: Some("Loaded from foo.star".to_string()), + kind: SymbolKind::Method, + }, + ), + ( + "renamed".to_string(), + Symbol { + name: "renamed".to_string(), + detail: Some("Loaded from foo.star".to_string()), + kind: SymbolKind::Method, + }, + ), + ( + "method".to_string(), + Symbol { + name: "method".to_string(), + detail: None, + kind: SymbolKind::Method, + }, + ), + ( + "my_var".to_string(), + Symbol { + name: "my_var".to_string(), + detail: None, + kind: SymbolKind::Variable, + }, + ), + ]) + ); + } + + #[test] + fn inside_method() { + let ast_module = AstModule::parse( + "t.star", + r#"load("foo.star", "exported_a", renamed = "exported_b") + +def method(param): + pass + +my_var = True + "# + .to_string(), + &Dialect::Standard, + ) + .unwrap(); + + assert_eq!( + find_symbols_at_location( + &ast_module.codemap, + &ast_module.statement, + LineCol { line: 3, column: 4 }, + ), + HashMap::from([ + ( + "exported_a".to_string(), + Symbol { + name: "exported_a".to_string(), + detail: Some("Loaded from foo.star".to_string()), + kind: SymbolKind::Method, + }, + ), + ( + "renamed".to_string(), + Symbol { + name: "renamed".to_string(), + detail: Some("Loaded from foo.star".to_string()), + kind: SymbolKind::Method, + }, + ), + ( + "method".to_string(), + Symbol { + name: "method".to_string(), + detail: None, + kind: SymbolKind::Method, + }, + ), + ( + "param".to_string(), + Symbol { + name: "param".to_string(), + detail: None, + kind: SymbolKind::Variable + } + ), + ( + "my_var".to_string(), + Symbol { + name: "my_var".to_string(), + detail: None, + kind: SymbolKind::Variable, + }, + ), + ]) + ); + } +} From 60d14914776adfccdcd20810a1b3b04a75ca6a63 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Mon, 1 May 2023 14:26:52 +0200 Subject: [PATCH 10/41] docs: add comment to show that `ResolvedSpan::contains` means 'entirely contains', not 'overlaps' --- starlark/src/codemap.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/starlark/src/codemap.rs b/starlark/src/codemap.rs index 7306221c1..87c968b65 100644 --- a/starlark/src/codemap.rs +++ b/starlark/src/codemap.rs @@ -542,6 +542,7 @@ impl From for lsp_types::Range { } impl ResolvedSpan { + /// Check that the given position is entirely contained within this span. pub(crate) fn contains(&self, pos: LineCol) -> bool { (self.begin_line < pos.line || (self.begin_line == pos.line && self.begin_column <= pos.column)) From 41b66ab5228714c8e08f88dfb6621e47d981ad59 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Mon, 1 May 2023 14:27:13 +0200 Subject: [PATCH 11/41] chore: remove commented out code --- starlark/src/lsp/server.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 96e237a6f..738630844 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -354,8 +354,6 @@ impl Backend { "=".to_string(), "[".to_string(), ]), - - // all_commit_characters: Some(vec![" ".to_string()]), ..Default::default() }), ..ServerCapabilities::default() From 086c0ba3dbd3ef3653438b2f0d6fcfee3a71d3c8 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Thu, 11 May 2023 11:23:49 +0200 Subject: [PATCH 12/41] feat: autocomplete keywords --- starlark/src/lsp/server.rs | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 738630844..62e8f6261 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -779,7 +779,12 @@ impl Backend { } } - Some(symbols.into_values().collect()) + Some( + symbols + .into_values() + .chain(Self::get_keyword_completion_items()) + .collect(), + ) } None => None, }; @@ -787,6 +792,24 @@ impl Backend { Ok(CompletionResponse::Array(symbols.unwrap_or_default())) } + /// Get completion items for each language keyword. + fn get_keyword_completion_items() -> impl Iterator { + [ + // Actual keywords + "and", "else", "load", "break", "for", "not", "continue", "if", "or", "def", "in", + "pass", "elif", "return", "lambda", // + // Reserved words + "as", "import", "is", "class", "nonlocal", "del", "raise", "except", "try", "finally", + "while", "from", "with", "global", "yield", + ] + .into_iter() + .map(|keyword| CompletionItem { + label: keyword.to_string(), + kind: Some(CompletionItemKind::KEYWORD), + ..Default::default() + }) + } + /// Given the workspace root and a target file, format the path as a path that load() understands. /// TODO: Handle cases other than Bazel, other repositories, other workspaces. fn format_load_path( From 369325f02fe37543b4b59c98e2f9decca14e7f1e Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 30 May 2023 12:14:16 +0200 Subject: [PATCH 13/41] feat: add function to get symbols loaded in a module --- starlark/src/analysis/loaded.rs | 76 +++++++++++++++++++++++++++++++++ starlark/src/analysis/mod.rs | 1 + 2 files changed, 77 insertions(+) create mode 100644 starlark/src/analysis/loaded.rs diff --git a/starlark/src/analysis/loaded.rs b/starlark/src/analysis/loaded.rs new file mode 100644 index 000000000..ab2f6ad65 --- /dev/null +++ b/starlark/src/analysis/loaded.rs @@ -0,0 +1,76 @@ +/* + * Copyright 2019 The Starlark in Rust Authors. + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +use dupe::Dupe; + +use crate::syntax::ast::StmtP; +use crate::syntax::AstModule; + +/// A loaded symbol. Returned from [`AstModule::loaded_symbols`]. +#[derive(Debug, PartialEq, Eq, Clone, Dupe, Hash)] +pub struct LoadedSymbol<'a> { + /// The name of the symbol. + pub name: &'a str, + /// The file it's loaded from. + pub module: &'a str, +} + +impl AstModule { + /// Which symbols are loaded by this module. These are the top-level load + /// statements. + pub fn loaded_symbols<'a>(&'a self) -> Vec> { + self.top_level_statements() + .into_iter() + .filter_map(|x| match &x.node { + StmtP::Load(l) => Some(l), + _ => None, + }) + .flat_map(|l| { + l.args.iter().map(|symbol| LoadedSymbol { + name: &symbol.1, + module: &l.module, + }) + }) + .collect() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::slice_vec_ext::SliceExt; + use crate::syntax::Dialect; + + fn module(x: &str) -> AstModule { + AstModule::parse("X", x.to_owned(), &Dialect::Extended).unwrap() + } + + #[test] + fn test_loaded() { + let modu = module( + r#" +load("test", "a", b = "c") +load("foo", "bar") +"#, + ); + let res = modu.loaded_symbols(); + assert_eq!( + res.map(|symbol| format!("{}:{}", symbol.module, symbol.name)), + &["test:a", "test:c", "foo:bar"] + ); + } +} diff --git a/starlark/src/analysis/mod.rs b/starlark/src/analysis/mod.rs index 00b86bcf8..7b85927a1 100644 --- a/starlark/src/analysis/mod.rs +++ b/starlark/src/analysis/mod.rs @@ -31,6 +31,7 @@ pub(crate) mod exported; mod find_call_name; mod flow; mod incompatible; +pub(crate) mod loaded; mod names; mod performance; mod types; From e3db1d6707148293299e84dfdd70437777ba3944 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 30 May 2023 16:19:13 +0200 Subject: [PATCH 14/41] feat: add autocomplete options from symbols loaded in open files also refactor `completion_options` a bit and extract text edit construction to its own function --- starlark/src/analysis/definition.rs | 6 + starlark/src/lsp/server.rs | 263 +++++++++++++++++----------- 2 files changed, 165 insertions(+), 104 deletions(-) diff --git a/starlark/src/analysis/definition.rs b/starlark/src/analysis/definition.rs index 02764000f..125d9c60f 100644 --- a/starlark/src/analysis/definition.rs +++ b/starlark/src/analysis/definition.rs @@ -17,6 +17,7 @@ use std::iter; +use super::loaded::LoadedSymbol; use crate::analysis::bind::scope; use crate::analysis::bind::Assigner; use crate::analysis::bind::Bind; @@ -422,6 +423,11 @@ impl LspModule { self.ast.exported_symbols() } + /// Get the list of symbols loaded by this module. + pub(crate) fn get_loaded_symbols(&self) -> Vec> { + self.ast.loaded_symbols() + } + /// Attempt to find the location in this module where an exported symbol is defined. pub(crate) fn find_exported_symbol(&self, name: &str) -> Option { self.ast.exported_symbols().iter().find_map(|symbol| { diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 3ac21b0d6..a2a6b63b9 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -18,6 +18,7 @@ //! Based on the reference lsp-server example at . use std::collections::HashMap; +use std::collections::HashSet; use std::fmt::Debug; use std::path::Path; use std::path::PathBuf; @@ -83,7 +84,11 @@ use crate::analysis::definition::LspModule; use crate::analysis::exported::SymbolKind as ExportedSymbolKind; use crate::codemap::LineCol; use crate::codemap::ResolvedSpan; +use crate::codemap::Span; +use crate::codemap::Spanned; use crate::lsp::server::LoadContentsError::WrongScheme; +use crate::syntax::ast::AssignIdentP; +use crate::syntax::ast::AstPayload; use crate::syntax::ast::StmtP; use crate::syntax::symbols::find_symbols_at_location; use crate::syntax::symbols::SymbolKind; @@ -672,109 +677,24 @@ impl Backend { }); let last_load = last_load.map(|span| ast.ast.codemap.resolve_span(span)); - for (doc_uri, doc) in self - .last_valid_parse - .read() - .unwrap() - .iter() - .filter(|(&ref doc_uri, _)| doc_uri != &uri) - { - let load_path = self - .format_load_path(&initialize_params.workspace_folders, doc_uri.path()); - let load_symbols: Vec<_> = doc - .get_exported_symbols() - .into_iter() - .filter(|symbol| !symbols.contains_key(symbol.name)) - .map(|symbol| { - // Construct the text edit to insert a load for this exported symbol. - let text_edit = if let Some(existing_load) = loads.get(&load_path) { - // We're already loading a symbol from this module path, construct - // a text edit that amends the existing load. - let (previously_loaded_symbols, load_span) = existing_load; - let load_span = ast.ast.codemap.resolve_span(*load_span); - let mut load_args: Vec<(&str, &str)> = - previously_loaded_symbols - .iter() - .map(|(assign, name)| { - (assign.0.as_str(), name.node.as_str()) - }) - .collect(); - load_args.push((symbol.name, symbol.name)); - load_args.sort_by(|(_, a), (_, b)| a.cmp(b)); - - TextEdit::new( - Range::new( - Position::new( - load_span.begin_line as u32, - load_span.begin_column as u32, - ), - Position::new( - load_span.end_line as u32, - load_span.end_column as u32, - ), - ), - format!( - "load(\"{}\", {})", - &load_path, - load_args - .into_iter() - .map(|(assign, import)| { - if assign == import { - format!("\"{}\"", import) - } else { - format!("{} = \"{}\"", assign, import) - } - }) - .join(", ") - ), - ) - } else { - // We're not yet loading from this module, construct a text edit that - // inserts a new load statement after the last one we found. - TextEdit::new( - match last_load { - Some(span) => Range::new( - Position::new( - span.end_line as u32, - span.end_column as u32, - ), - Position::new( - span.end_line as u32, - span.end_column as u32, - ), - ), - None => { - Range::new(Position::new(0, 0), Position::new(0, 0)) - } - }, - format!( - "{}load(\"{}\", \"{}\")", - if last_load.is_some() { "\n" } else { "" }, - &load_path, - symbol.name - ), - ) - }; - - ( - symbol.name.to_string(), - CompletionItem { - label: symbol.name.to_string(), - detail: Some(format!("Load from {load_path}")), - kind: Some(match symbol.kind { - ExportedSymbolKind::Any => CompletionItemKind::CONSTANT, - ExportedSymbolKind::Function => { - CompletionItemKind::METHOD - } - }), - additional_text_edits: Some(vec![text_edit]), - ..Default::default() - }, + symbols.extend( + self.get_all_exported_symbols( + Some(&uri), + &symbols, + initialize_params, + |module, symbol| { + Self::get_load_text_edit( + module, + symbol, + &ast, + last_load, + loads.get(module), ) - }) - .collect(); - symbols.extend(load_symbols); - } + }, + ) + .into_iter() + .map(|item| (item.label.clone(), item)), + ); } Some( @@ -790,6 +710,141 @@ impl Backend { Ok(CompletionResponse::Array(symbols.unwrap_or_default())) } + /// Using all currently loaded documents, gather a list of known exported + /// symbols. This list contains both the symbols exported from the loaded + /// files, as well as symbols loaded in the open files. Symbols that are + /// loaded from modules that are open are deduplicated. + fn get_all_exported_symbols( + &self, + except_from: Option<&LspUrl>, + symbols: &HashMap, + initialize_params: &InitializeParams, + format_text_edit: F, + ) -> Vec + where + F: Fn(&str, &str) -> TextEdit, + { + let mut seen = HashSet::new(); + let mut result = Vec::new(); + + let all_documents = self.last_valid_parse.read().unwrap(); + + for (doc_uri, doc) in all_documents + .iter() + .filter(|&(doc_uri, _)| match except_from { + Some(uri) => doc_uri != uri, + None => true, + }) + { + let load_path = + self.format_load_path(&initialize_params.workspace_folders, doc_uri.path()); + + for symbol in doc + .get_exported_symbols() + .into_iter() + .filter(|symbol| !symbols.contains_key(symbol.name)) + { + seen.insert(format!("{load_path}:{}", &symbol.name)); + + result.push(CompletionItem { + label: symbol.name.to_string(), + detail: Some(format!("Load from {load_path}")), + kind: Some(match symbol.kind { + ExportedSymbolKind::Any => CompletionItemKind::CONSTANT, + ExportedSymbolKind::Function => CompletionItemKind::METHOD, + }), + additional_text_edits: Some(vec![format_text_edit(&load_path, symbol.name)]), + ..Default::default() + }) + } + } + + for symbol in all_documents + .iter() + .filter(|&(doc_uri, _)| match except_from { + Some(uri) => doc_uri != uri, + None => true, + }) + .flat_map(|(_, doc)| doc.get_loaded_symbols()) + .filter(|symbol| !symbols.contains_key(symbol.name)) + { + if seen.insert(format!("{}:{}", symbol.module, symbol.name)) { + result.push(CompletionItem { + label: symbol.name.to_string(), + detail: Some(format!("Load from {}", symbol.module)), + kind: Some(CompletionItemKind::CONSTANT), + additional_text_edits: Some(vec![format_text_edit(symbol.module, symbol.name)]), + ..Default::default() + }) + } + } + + result + } + + fn get_load_text_edit

( + module: &str, + symbol: &str, + ast: &LspModule, + last_load: Option, + existing_load: Option<&(Vec<(Spanned>, Spanned)>, Span)>, + ) -> TextEdit + where + P: AstPayload, + { + match existing_load { + Some((previously_loaded_symbols, load_span)) => { + // We're already loading a symbol from this module path, construct + // a text edit that amends the existing load. + // let (previously_loaded_symbols, load_span) = existing_load; + let load_span = ast.ast.codemap.resolve_span(*load_span); + let mut load_args: Vec<(&str, &str)> = previously_loaded_symbols + .iter() + .map(|(assign, name)| (assign.0.as_str(), name.node.as_str())) + .collect(); + load_args.push((symbol, symbol)); + load_args.sort_by(|(_, a), (_, b)| a.cmp(b)); + + TextEdit::new( + Range::new( + Position::new(load_span.begin_line as u32, load_span.begin_column as u32), + Position::new(load_span.end_line as u32, load_span.end_column as u32), + ), + format!( + "load(\"{module}\", {})", + load_args + .into_iter() + .map(|(assign, import)| { + if assign == import { + format!("\"{}\"", import) + } else { + format!("{} = \"{}\"", assign, import) + } + }) + .join(", ") + ), + ) + } + None => { + // We're not yet loading from this module, construct a text edit that + // inserts a new load statement after the last one we found. + TextEdit::new( + match last_load { + Some(span) => Range::new( + Position::new(span.end_line as u32, span.end_column as u32), + Position::new(span.end_line as u32, span.end_column as u32), + ), + None => Range::new(Position::new(0, 0), Position::new(0, 0)), + }, + format!( + "{}load(\"{module}\", \"{symbol}\")", + if last_load.is_some() { "\n" } else { "" }, + ), + ) + } + } + } + /// Get completion items for each language keyword. fn get_keyword_completion_items() -> impl Iterator { [ @@ -832,14 +887,14 @@ impl Backend { if let Some(file) = target.file_name() { let mut result = "//".to_string(); result.push_str( - &*target + &target .parent() .expect("parent() should succeed if there's a file_name()") .to_string_lossy() .replace('\\', "/"), ); result.push(':'); - result.push_str(&*file.to_string_lossy()); + result.push_str(&file.to_string_lossy()); result } else { From a5694a5300f6d54a5b2c0f73ca7dc01991dc1dec Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Wed, 31 May 2023 14:55:30 +0200 Subject: [PATCH 15/41] feat: autocomplete global symbols --- starlark/bin/eval.rs | 7 +++++ starlark/src/docs/mod.rs | 8 +++--- starlark/src/environment/globals.rs | 35 +++++++++++++++++++++++++ starlark/src/lsp/server.rs | 40 +++++++++++++++++++++++++++++ starlark/src/lsp/test.rs | 13 ++++++++++ 5 files changed, 99 insertions(+), 4 deletions(-) diff --git a/starlark/bin/eval.rs b/starlark/bin/eval.rs index 22581dede..11a6c5f87 100644 --- a/starlark/bin/eval.rs +++ b/starlark/bin/eval.rs @@ -31,6 +31,7 @@ use starlark::docs::render_docs_as_code; use starlark::docs::Doc; use starlark::docs::DocItem; use starlark::environment::FrozenModule; +use starlark::environment::GlobalSymbol; use starlark::environment::Globals; use starlark::environment::Module; use starlark::errors::EvalMessage; @@ -66,6 +67,7 @@ pub(crate) struct Context { pub(crate) module: Option, pub(crate) builtin_docs: HashMap, pub(crate) builtin_symbols: HashMap, + pub(crate) globals: Globals, } /// The outcome of evaluating (checking, parsing or running) given starlark code. @@ -134,6 +136,7 @@ impl Context { module, builtin_docs, builtin_symbols, + globals, }) } @@ -345,6 +348,10 @@ impl LspContext for Context { ) -> anyhow::Result> { Ok(self.builtin_symbols.get(symbol).cloned()) } + + fn get_global_symbols(&self) -> Vec { + self.globals.symbols().collect() + } } pub(crate) fn globals() -> Globals { diff --git a/starlark/src/docs/mod.rs b/starlark/src/docs/mod.rs index 47a9149ed..d10a85cb6 100644 --- a/starlark/src/docs/mod.rs +++ b/starlark/src/docs/mod.rs @@ -365,7 +365,7 @@ pub struct DocModule { } impl DocModule { - fn render_as_code(&self) -> String { + pub(crate) fn render_as_code(&self) -> String { let mut res = self .docs .as_ref() @@ -449,7 +449,7 @@ impl DocFunction { } } - fn render_as_code(&self, name: &str) -> String { + pub(crate) fn render_as_code(&self, name: &str) -> String { let params: Vec<_> = self.params.iter().map(DocParam::render_as_code).collect(); let spacer_len = if params.is_empty() { 0 @@ -686,7 +686,7 @@ pub struct DocProperty { } impl DocProperty { - fn render_as_code(&self, name: &str) -> String { + pub(crate) fn render_as_code(&self, name: &str) -> String { match ( self.typ.as_ref(), self.docs.as_ref().map(DocString::render_as_quoted_code), @@ -750,7 +750,7 @@ pub struct DocObject { } impl DocObject { - fn render_as_code(&self, name: &str) -> String { + pub(crate) fn render_as_code(&self, name: &str) -> String { let summary = self .docs .as_ref() diff --git a/starlark/src/environment/globals.rs b/starlark/src/environment/globals.rs index 950787798..354dedf64 100644 --- a/starlark/src/environment/globals.rs +++ b/starlark/src/environment/globals.rs @@ -41,6 +41,7 @@ use crate::values::function::NativeAttribute; use crate::values::function::NativeCallableRawDocs; use crate::values::function::NativeFunc; use crate::values::function::NativeMeth; +use crate::values::function::FUNCTION_TYPE; use crate::values::layout::value_not_special::FrozenValueNotSpecial; use crate::values::structs::AllocStruct; use crate::values::types::function::NativeFunction; @@ -104,6 +105,24 @@ pub struct MethodsBuilder { docstring: Option, } +/// A globally available symbol, e.g. `True` or `dict`. +pub struct GlobalSymbol<'a> { + /// The name of the symbol. + pub(crate) name: &'a str, + /// The type of the symbol. + pub(crate) kind: GlobalSymbolKind, + /// The description of this symbol. + pub(crate) documentation: Option, +} + +/// A kind of globally available symbol. +pub enum GlobalSymbolKind { + /// A global function, e.g. `dict`. + Function, + /// A constant, e.g. `True`. + Constant, +} + impl Globals { /// Create an empty [`Globals`], with no functions in scope. pub fn new() -> Self { @@ -151,6 +170,22 @@ impl Globals { self.0.variable_names.iter().copied() } + /// Get all symbols defined in this environment. + pub fn symbols(&self) -> impl Iterator> { + self.0 + .variables + .iter() + .map(|(name, variable)| GlobalSymbol { + name: name.as_str(), + kind: if variable.to_value().get_type() == FUNCTION_TYPE { + GlobalSymbolKind::Function + } else { + GlobalSymbolKind::Constant + }, + documentation: variable.to_value().documentation(), + }) + } + pub(crate) fn heap(&self) -> &FrozenHeapRef { &self.0.heap } diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index a2a6b63b9..292c6457f 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -54,11 +54,14 @@ use lsp_types::Diagnostic; use lsp_types::DidChangeTextDocumentParams; use lsp_types::DidCloseTextDocumentParams; use lsp_types::DidOpenTextDocumentParams; +use lsp_types::Documentation; use lsp_types::GotoDefinitionParams; use lsp_types::GotoDefinitionResponse; use lsp_types::InitializeParams; use lsp_types::LocationLink; use lsp_types::LogMessageParams; +use lsp_types::MarkupContent; +use lsp_types::MarkupKind; use lsp_types::MessageType; use lsp_types::OneOf; use lsp_types::Position; @@ -86,6 +89,9 @@ use crate::codemap::LineCol; use crate::codemap::ResolvedSpan; use crate::codemap::Span; use crate::codemap::Spanned; +use crate::docs::DocItem; +use crate::environment::GlobalSymbol; +use crate::environment::GlobalSymbolKind; use crate::lsp::server::LoadContentsError::WrongScheme; use crate::syntax::ast::AssignIdentP; use crate::syntax::ast::AstPayload; @@ -304,6 +310,9 @@ pub trait LspContext { Ok(result) } + /// Get a list of all known global symbols. + fn get_global_symbols(&self) -> Vec; + /// Get the LSPUrl for a global symbol if possible. /// /// The current file is provided in case different files have different global symbols @@ -700,6 +709,7 @@ impl Backend { Some( symbols .into_values() + .chain(self.get_global_symbol_completion_items()) .chain(Self::get_keyword_completion_items()) .collect(), ) @@ -782,6 +792,36 @@ impl Backend { result } + fn get_global_symbol_completion_items(&self) -> impl Iterator + '_ { + self.context + .get_global_symbols() + .into_iter() + .map(|symbol| CompletionItem { + label: symbol.name.to_owned(), + kind: Some(match symbol.kind { + GlobalSymbolKind::Function => CompletionItemKind::FUNCTION, + GlobalSymbolKind::Constant => CompletionItemKind::CONSTANT, + }), + documentation: symbol.documentation.map(|docs| { + Documentation::MarkupContent(MarkupContent { + // The doc item is rendered as code, so embed it in markdown, indicating + // the syntax, in order to render correctly. + kind: MarkupKind::Markdown, + value: format!( + "```starlark\n{}\n```", + match docs { + DocItem::Module(m) => m.render_as_code(), + DocItem::Object(o) => o.render_as_code(symbol.name), + DocItem::Function(f) => f.render_as_code(symbol.name), + DocItem::Property(p) => p.render_as_code(symbol.name), + } + ), + }) + }), + ..Default::default() + }) + } + fn get_load_text_edit

( module: &str, symbol: &str, diff --git a/starlark/src/lsp/test.rs b/starlark/src/lsp/test.rs index 6c4c1a1ac..d05eef5b3 100644 --- a/starlark/src/lsp/test.rs +++ b/starlark/src/lsp/test.rs @@ -63,6 +63,8 @@ use crate::docs::DocFunction; use crate::docs::DocItem; use crate::docs::Identifier; use crate::docs::Location; +use crate::environment::GlobalSymbol; +use crate::environment::GlobalSymbolKind; use crate::errors::EvalMessage; use crate::lsp::server::new_notification; use crate::lsp::server::server_with_connection; @@ -227,6 +229,17 @@ impl LspContext for TestServerContext { ) -> anyhow::Result> { Ok(self.builtin_symbols.get(symbol).cloned()) } + + fn get_global_symbols(&self) -> Vec { + self.builtin_symbols + .keys() + .map(|name| GlobalSymbol { + name, + kind: GlobalSymbolKind::Function, + documentation: None, + }) + .collect() + } } /// A server for use in testing that provides helpers for sending requests, correlating From 6bb196cfe9b4851f92012ea8d81ecc0ebcbf775d Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Thu, 1 Jun 2023 10:36:16 +0200 Subject: [PATCH 16/41] feat: add `LspContext::render_as_load` --- starlark/bin/eval.rs | 56 ++++++++++++++++++++++++++++++++++++++ starlark/src/lsp/server.rs | 12 ++++++++ starlark/src/lsp/test.rs | 53 ++++++++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+) diff --git a/starlark/bin/eval.rs b/starlark/bin/eval.rs index 11a6c5f87..657afd6e2 100644 --- a/starlark/bin/eval.rs +++ b/starlark/bin/eval.rs @@ -91,6 +91,17 @@ enum ResolveLoadError { WrongScheme(String, LspUrl), } +/// Errors when [`LspContext::render_as_load()`] cannot render a given path. +#[derive(thiserror::Error, Debug)] +enum RenderLoadError { + /// Attempted to get the filename of a path that does not seem to contain a filename. + #[error("Path `{}` provided, which does not seem to contain a filename", .0.display())] + MissingTargetFilename(PathBuf), + /// The scheme provided was not correct or supported. + #[error("Urls `{}` and `{}` was expected to be of type `{}`", .1, .2, .0)] + WrongScheme(String, LspUrl, LspUrl), +} + impl Context { pub(crate) fn new( mode: ContextMode, @@ -313,6 +324,51 @@ impl LspContext for Context { } } + fn render_as_load( + &self, + target: &LspUrl, + current_file: &LspUrl, + workspace_root: Option<&Path>, + ) -> anyhow::Result { + match (target, current_file) { + (LspUrl::File(target_path), LspUrl::File(current_file_path)) => { + let target_package = target_path.parent(); + let current_file_package = current_file_path.parent(); + let target_filename = target_path.file_name(); + + // If both are in the same package, return a relative path. + if matches!((target_package, current_file_package), (Some(a), Some(b)) if a == b) { + return match target_filename { + Some(filename) => Ok(format!(":{}", filename.to_string_lossy())), + None => { + Err(RenderLoadError::MissingTargetFilename(target_path.clone()).into()) + } + }; + } + + let target_path = workspace_root + .and_then(|root| target_path.strip_prefix(root).ok()) + .unwrap_or(target_path); + + Ok(format!( + "//{}:{}", + target_package + .map(|path| path.to_string_lossy()) + .unwrap_or_default(), + target_filename + .unwrap_or(target_path.as_os_str()) + .to_string_lossy() + )) + } + _ => Err(RenderLoadError::WrongScheme( + "file://".to_owned(), + target.clone(), + current_file.clone(), + ) + .into()), + } + } + fn resolve_string_literal( &self, literal: &str, diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 292c6457f..e3feb7791 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -287,6 +287,18 @@ pub trait LspContext { /// if `path` is "relative" in a semantic sense. fn resolve_load(&self, path: &str, current_file: &LspUrl) -> anyhow::Result; + /// Render the target URL to use as a path in a `load()` statement. If `target` is + /// in the same package as `current_file`, the result is a relative path. + /// + /// `target` is the file that should be loaded by `load()`. + /// `current_file` is the file that the `load()` statement will be inserted into. + fn render_as_load( + &self, + target: &LspUrl, + current_file: &LspUrl, + workspace_root: Option<&Path>, + ) -> anyhow::Result; + /// Resolve a string literal into a Url and a function that specifies a locaction within that /// target file. /// diff --git a/starlark/src/lsp/test.rs b/starlark/src/lsp/test.rs index d05eef5b3..c3123db57 100644 --- a/starlark/src/lsp/test.rs +++ b/starlark/src/lsp/test.rs @@ -97,6 +97,14 @@ enum ResolveLoadError { WrongScheme(String, LspUrl), } +#[derive(thiserror::Error, Debug)] +enum RenderLoadError { + #[error("Path `{}` provided, which does not seem to contain a filename", .0.display())] + MissingTargetFilename(PathBuf), + #[error("Urls `{}` and `{}` was expected to be of type `{}`", .1, .2, .0)] + WrongScheme(String, LspUrl, LspUrl), +} + #[derive(thiserror::Error, Debug)] pub(crate) enum TestServerError { #[error("Attempted to set the contents of a file with a non-absolute path `{}`", .0.display())] @@ -167,6 +175,51 @@ impl LspContext for TestServerContext { } } + fn render_as_load( + &self, + target: &LspUrl, + current_file: &LspUrl, + workspace_root: Option<&Path>, + ) -> anyhow::Result { + match (target, current_file) { + (LspUrl::File(target_path), LspUrl::File(current_file_path)) => { + let target_package = target_path.parent(); + let current_file_package = current_file_path.parent(); + let target_filename = target_path.file_name(); + + // If both are in the same package, return a relative path. + if matches!((target_package, current_file_package), (Some(a), Some(b)) if a == b) { + return match target_filename { + Some(filename) => Ok(format!(":{}", filename.to_string_lossy())), + None => { + Err(RenderLoadError::MissingTargetFilename(target_path.clone()).into()) + } + }; + } + + let target_path = workspace_root + .and_then(|root| target_path.strip_prefix(root).ok()) + .unwrap_or(target_path); + + Ok(format!( + "//{}:{}", + target_package + .map(|path| path.to_string_lossy()) + .unwrap_or_default(), + target_filename + .unwrap_or(target_path.as_os_str()) + .to_string_lossy() + )) + } + _ => Err(RenderLoadError::WrongScheme( + "file://".to_owned(), + target.clone(), + current_file.clone(), + ) + .into()), + } + } + fn resolve_string_literal( &self, literal: &str, From cb0fc26863ca3bfbf551160776aa5d2554e352f4 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Thu, 1 Jun 2023 10:37:37 +0200 Subject: [PATCH 17/41] chore: remove old `format_as_load` in LSP and use the new function on `LspContext` --- starlark/src/lsp/server.rs | 58 +++++++++++++------------------------- 1 file changed, 20 insertions(+), 38 deletions(-) diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index e3feb7791..5dd012d4f 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -703,6 +703,7 @@ impl Backend { Some(&uri), &symbols, initialize_params, + &uri, |module, symbol| { Self::get_load_text_edit( module, @@ -741,6 +742,7 @@ impl Backend { except_from: Option<&LspUrl>, symbols: &HashMap, initialize_params: &InitializeParams, + current_document: &LspUrl, format_text_edit: F, ) -> Vec where @@ -758,8 +760,13 @@ impl Backend { None => true, }) { - let load_path = - self.format_load_path(&initialize_params.workspace_folders, doc_uri.path()); + let Ok(load_path) = self.context.render_as_load( + doc_uri, + current_document, + Self::get_workspace_root(initialize_params.workspace_folders.as_ref(), doc_uri), + ) else { + continue; + }; for symbol in doc .get_exported_symbols() @@ -915,44 +922,19 @@ impl Backend { }) } - /// Given the workspace root and a target file, format the path as a path that load() understands. - /// TODO: Handle cases other than Bazel, other repositories, other workspaces. - fn format_load_path( - &self, - workspace_roots: &Option>, - target_file: &Path, - ) -> String { - let target = if let Some(roots) = workspace_roots { - let mut result = target_file; - for root in roots { - if let Ok(stripped) = target_file.strip_prefix(root.uri.path()) { - result = stripped; - break; - } - } - - result - } else { - target_file + fn get_workspace_root<'a>( + workspace_roots: Option<&'_ Vec>, + target: &'a LspUrl, + ) -> Option<&'a Path> { + let LspUrl::File(target) = target else { + return None; }; - if let Some(file) = target.file_name() { - let mut result = "//".to_string(); - result.push_str( - &target - .parent() - .expect("parent() should succeed if there's a file_name()") - .to_string_lossy() - .replace('\\', "/"), - ); - result.push(':'); - result.push_str(&file.to_string_lossy()); - - result - } else { - // Must be a file in the root of the workspace - format!("//:{}", target.display()) - } + workspace_roots.and_then(|roots| { + roots + .iter() + .find_map(|root| target.strip_prefix(root.uri.path()).ok()) + }) } } From c6b6cc972d50d6d755b4620ea3bec926627e68dd Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Thu, 1 Jun 2023 15:23:53 +0200 Subject: [PATCH 18/41] fix: correctly handle `render_as_load` and `resolve_load`, and add tests --- starlark/bin/eval.rs | 176 ++++++++++++++++++++++++++++++++----- starlark/src/lsp/server.rs | 64 ++++++++++---- starlark/src/lsp/test.rs | 10 ++- 3 files changed, 211 insertions(+), 39 deletions(-) diff --git a/starlark/bin/eval.rs b/starlark/bin/eval.rs index 657afd6e2..284508a89 100644 --- a/starlark/bin/eval.rs +++ b/starlark/bin/eval.rs @@ -89,6 +89,13 @@ enum ResolveLoadError { /// The scheme provided was not correct or supported. #[error("Url `{}` was expected to be of type `{}`", .1, .0)] WrongScheme(String, LspUrl), + /// Received a load for an absolute path from the root of the workspace, but the + /// path to the workspace root was not provided. + #[error("Path `//{}` is absolute from the root of the workspace, but no workspace root was provided", .0)] + MissingWorkspaceRoot(String), + /// Unable to parse the given path. + #[error("Unable to parse the load path `{}`", .0)] + CannotParsePath(String), } /// Errors when [`LspContext::render_as_load()`] cannot render a given path. @@ -306,21 +313,45 @@ impl LspContext for Context { } } - fn resolve_load(&self, path: &str, current_file: &LspUrl) -> anyhow::Result { - let path = PathBuf::from(path); - match current_file { - LspUrl::File(current_file_path) => { - let current_file_dir = current_file_path.parent(); - let absolute_path = match (current_file_dir, path.is_absolute()) { - (_, true) => Ok(path), - (Some(current_file_dir), false) => Ok(current_file_dir.join(&path)), - (None, false) => Err(ResolveLoadError::MissingCurrentFilePath(path)), - }?; - Ok(Url::from_file_path(absolute_path).unwrap().try_into()?) + fn resolve_load( + &self, + path: &str, + current_file: &LspUrl, + workspace_root: Option<&Path>, + ) -> anyhow::Result { + if let Some(path) = path.strip_prefix(':') { + // Resolve relative paths from the current file. + let path = PathBuf::from(path); + match current_file { + LspUrl::File(current_file_path) => { + let current_file_dir = current_file_path.parent(); + let absolute_path = match current_file_dir { + Some(current_file_dir) => Ok(current_file_dir.join(&path)), + None => Err(ResolveLoadError::MissingCurrentFilePath(path)), + }?; + Ok(Url::from_file_path(absolute_path).unwrap().try_into()?) + } + _ => Err( + ResolveLoadError::WrongScheme("file://".to_owned(), current_file.clone()) + .into(), + ), } - _ => Err( - ResolveLoadError::WrongScheme("file://".to_owned(), current_file.clone()).into(), - ), + } else if let Some(path) = path.strip_prefix("//") { + // Resolve from the root of the workspace. + match (path.split_once(':'), workspace_root) { + (Some((subfolder, filename)), Some(workspace_root)) => { + let mut result = workspace_root.to_owned(); + result.push(subfolder); + result.push(filename); + Ok(Url::from_file_path(result).unwrap().try_into()?) + } + (Some(_), None) => { + Err(ResolveLoadError::MissingWorkspaceRoot(path.to_owned()).into()) + } + (None, _) => Err(ResolveLoadError::CannotParsePath(format!("//{path}")).into()), + } + } else { + Err(ResolveLoadError::CannotParsePath(path.to_owned()).into()) } } @@ -352,7 +383,8 @@ impl LspContext for Context { Ok(format!( "//{}:{}", - target_package + target_path + .parent() .map(|path| path.to_string_lossy()) .unwrap_or_default(), target_filename @@ -373,13 +405,15 @@ impl LspContext for Context { &self, literal: &str, current_file: &LspUrl, + workspace_root: Option<&Path>, ) -> anyhow::Result> { - self.resolve_load(literal, current_file).map(|url| { - Some(StringLiteralResult { - url, - location_finder: None, + self.resolve_load(literal, current_file, workspace_root) + .map(|url| { + Some(StringLiteralResult { + url, + location_finder: None, + }) }) - }) } fn get_load_contents(&self, uri: &LspUrl) -> anyhow::Result> { @@ -417,3 +451,105 @@ pub(crate) fn globals() -> Globals { pub(crate) fn dialect() -> Dialect { Dialect::Extended } + +#[cfg(test)] +mod tests { + use super::*; + + fn make_context() -> Context { + Context::new(ContextMode::Run, false, &[], false).unwrap() + } + + #[test] + fn resolve_load() { + let context = make_context(); + + // Successful cases + let current_file = LspUrl::File(PathBuf::from("/absolute/path/from.star")); + assert_eq!( + context + .resolve_load(":relative.star", ¤t_file, None,) + .unwrap(), + LspUrl::File(PathBuf::from("/absolute/path/relative.star")) + ); + assert_eq!( + context + .resolve_load("//:root.star", ¤t_file, Some(Path::new("/foo/bar")),) + .unwrap(), + LspUrl::File(PathBuf::from("/foo/bar/root.star")) + ); + assert_eq!( + context + .resolve_load( + "//baz:root.star", + ¤t_file, + Some(Path::new("/foo/bar")), + ) + .unwrap(), + LspUrl::File(PathBuf::from("/foo/bar/baz/root.star")) + ); + + // Error cases + let starlark_url = LspUrl::Starlark(PathBuf::new()); + assert!(matches!( + context + .resolve_load(":relative.star", &starlark_url, None) + .expect_err("should return an error") + .downcast::() + .expect("should return correct error type"), + ResolveLoadError::WrongScheme(scheme, url) if scheme == "file://" && url == starlark_url + )); + assert!(matches!( + context + .resolve_load("//something-absolute", &starlark_url, Some(Path::new("/foo/bar"))) + .expect_err("should return an error") + .downcast::() + .expect("should return correct error type"), + ResolveLoadError::CannotParsePath(url) if url == "//something-absolute" + )); + assert!(matches!( + context + .resolve_load("//something:absolute.star", &starlark_url, None) + .expect_err("should return an error") + .downcast::() + .expect("should return correct error type"), + ResolveLoadError::MissingWorkspaceRoot(_) + )); + } + + #[test] + fn render_as_load() { + let context = make_context(); + + assert_eq!( + context + .render_as_load( + &LspUrl::File(PathBuf::from("/foo/bar/baz/target.star")), + &LspUrl::File(PathBuf::from("/foo/bar/baz/current.star")), + None + ) + .expect("should succeed"), + ":target.star" + ); + assert_eq!( + context + .render_as_load( + &LspUrl::File(PathBuf::from("/foo/bar/baz/target.star")), + &LspUrl::File(PathBuf::from("/foo/bar/current.star")), + Some(Path::new("/foo/bar")) + ) + .expect("should succeed"), + "//baz:target.star" + ); + assert_eq!( + context + .render_as_load( + &LspUrl::File(PathBuf::from("/foo/bar/target.star")), + &LspUrl::File(PathBuf::from("/foo/bar/baz/current.star")), + Some(Path::new("/foo/bar")) + ) + .expect("should succeed"), + "//:target.star" + ); + } +} diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 5dd012d4f..e2fb2cf32 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -285,7 +285,12 @@ pub trait LspContext { /// implementation defined. /// `current_file` is the the file that is including the `load()` statement, and should be used /// if `path` is "relative" in a semantic sense. - fn resolve_load(&self, path: &str, current_file: &LspUrl) -> anyhow::Result; + fn resolve_load( + &self, + path: &str, + current_file: &LspUrl, + workspace_root: Option<&Path>, + ) -> anyhow::Result; /// Render the target URL to use as a path in a `load()` statement. If `target` is /// in the same package as `current_file`, the result is a relative path. @@ -309,6 +314,7 @@ pub trait LspContext { &self, literal: &str, current_file: &LspUrl, + workspace_root: Option<&Path>, ) -> anyhow::Result>; /// Get the contents of a starlark program at a given path, if it exists. @@ -447,8 +453,16 @@ impl Backend { /// NOTE: This uses the last valid parse of a file as a basis for symbol locations. /// If a file has changed and does result in a valid parse, then symbol locations may /// be slightly incorrect. - fn goto_definition(&self, id: RequestId, params: GotoDefinitionParams) { - self.send_response(new_response(id, self.find_definition(params))); + fn goto_definition( + &self, + id: RequestId, + params: GotoDefinitionParams, + initialize_params: &InitializeParams, + ) { + self.send_response(new_response( + id, + self.find_definition(params, initialize_params), + )); } /// Offers completion of known symbols in the current file. @@ -476,9 +490,14 @@ impl Backend { self.send_response(new_response(id, response)); } - fn resolve_load_path(&self, path: &str, current_uri: &LspUrl) -> anyhow::Result { + fn resolve_load_path( + &self, + path: &str, + current_uri: &LspUrl, + workspace_root: Option<&Path>, + ) -> anyhow::Result { match current_uri { - LspUrl::File(_) => self.context.resolve_load(path, current_uri), + LspUrl::File(_) => self.context.resolve_load(path, current_uri, workspace_root), LspUrl::Starlark(_) | LspUrl::Other(_) => { Err(ResolveLoadError::WrongScheme("file://".to_owned(), current_uri.clone()).into()) } @@ -510,20 +529,21 @@ impl Backend { definition: IdentifierDefinition, source: ResolvedSpan, member: Option<&str>, - uri: LspUrl, + uri: &LspUrl, + workspace_root: Option<&Path>, ) -> anyhow::Result> { let ret = match definition { IdentifierDefinition::Location { destination: target, .. - } => Self::location_link(source, &uri, target)?, + } => Self::location_link(source, uri, target)?, IdentifierDefinition::LoadedLocation { destination: location, path, name, .. } => { - let load_uri = self.resolve_load_path(&path, &uri)?; + let load_uri = self.resolve_load_path(&path, uri, workspace_root)?; let loaded_location = self.get_ast_or_load_from_disk(&load_uri)? .and_then(|ast| match member { @@ -531,7 +551,7 @@ impl Backend { None => ast.find_exported_symbol(&name), }); match loaded_location { - None => Self::location_link(source, &uri, location)?, + None => Self::location_link(source, uri, location)?, Some(loaded_location) => { Self::location_link(source, &load_uri, loaded_location)? } @@ -539,13 +559,15 @@ impl Backend { } IdentifierDefinition::NotFound => None, IdentifierDefinition::LoadPath { path, .. } => { - match self.resolve_load_path(&path, &uri) { + match self.resolve_load_path(&path, uri, workspace_root) { Ok(load_uri) => Self::location_link(source, &load_uri, Range::default())?, Err(_) => None, } } IdentifierDefinition::StringLiteral { literal, .. } => { - let literal = self.context.resolve_string_literal(&literal, &uri)?; + let literal = self + .context + .resolve_string_literal(&literal, uri, workspace_root)?; match literal { Some(StringLiteralResult { url, @@ -573,7 +595,7 @@ impl Backend { } } IdentifierDefinition::Unresolved { name, .. } => { - match self.context.get_url_for_global_symbol(&uri, &name)? { + match self.context.get_url_for_global_symbol(uri, &name)? { Some(uri) => { let loaded_location = self.get_ast_or_load_from_disk(&uri)? @@ -596,6 +618,7 @@ impl Backend { fn find_definition( &self, params: GotoDefinitionParams, + initialize_params: &InitializeParams, ) -> anyhow::Result { let uri = params .text_document_position_params @@ -604,15 +627,21 @@ impl Backend { .try_into()?; let line = params.text_document_position_params.position.line; let character = params.text_document_position_params.position.character; + let workspace_root = + Self::get_workspace_root(initialize_params.workspace_folders.as_ref(), &uri); let location = match self.get_ast(&uri) { Some(ast) => { let location = ast.find_definition(line, character); let source = location.source().unwrap_or_default(); match location { - Definition::Identifier(definition) => { - self.resolve_definition_location(definition, source, None, uri)? - } + Definition::Identifier(definition) => self.resolve_definition_location( + definition, + source, + None, + &uri, + workspace_root, + )?, // In this case we don't pass the name along in the root_definition_location, // so it's simpler to do the lookup here, rather than threading a ton of // information through. @@ -637,7 +666,8 @@ impl Backend { .expect("to have at least one component") .as_str(), ), - uri, + &uri, + workspace_root, )?, } } @@ -972,7 +1002,7 @@ impl Backend { // TODO(nmj): Also implement DocumentSymbols so that some logic can // be handled client side. if let Some(params) = as_request::(&req) { - self.goto_definition(req.id, params); + self.goto_definition(req.id, params, &initialize_params); } else if let Some(params) = as_request::(&req) { self.get_starlark_file_contents(req.id, params); } else if let Some(params) = as_request::(&req) { diff --git a/starlark/src/lsp/test.rs b/starlark/src/lsp/test.rs index c3123db57..e0248f31c 100644 --- a/starlark/src/lsp/test.rs +++ b/starlark/src/lsp/test.rs @@ -157,7 +157,12 @@ impl LspContext for TestServerContext { } } - fn resolve_load(&self, path: &str, current_file: &LspUrl) -> anyhow::Result { + fn resolve_load( + &self, + path: &str, + current_file: &LspUrl, + workspace_root: Option<&Path>, + ) -> anyhow::Result { let path = PathBuf::from(path); match current_file { LspUrl::File(current_file_path) => { @@ -224,6 +229,7 @@ impl LspContext for TestServerContext { &self, literal: &str, current_file: &LspUrl, + workspace_root: Option<&Path>, ) -> anyhow::Result> { let re = regex::Regex::new(r#"--(\d+):(\d+):(\d+):(\d+)$"#)?; let (literal, range) = match re.captures(literal) { @@ -243,7 +249,7 @@ impl LspContext for TestServerContext { } None => (literal.to_owned(), None), }; - self.resolve_load(&literal, current_file) + self.resolve_load(&literal, current_file, workspace_root) .map(|url| match &url { LspUrl::File(u) => match u.extension() { Some(e) if e == "star" => Some(StringLiteralResult { From 4246c329432d00258c44bee54716fd7e86b4220b Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Thu, 1 Jun 2023 15:24:35 +0200 Subject: [PATCH 19/41] fix: resolve load paths when discovering symbols loaded in other open files --- starlark/src/analysis/loaded.rs | 9 +++++---- starlark/src/lsp/server.rs | 32 +++++++++++++++++++++++++------- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/starlark/src/analysis/loaded.rs b/starlark/src/analysis/loaded.rs index ab2f6ad65..da350a3eb 100644 --- a/starlark/src/analysis/loaded.rs +++ b/starlark/src/analysis/loaded.rs @@ -25,8 +25,9 @@ use crate::syntax::AstModule; pub struct LoadedSymbol<'a> { /// The name of the symbol. pub name: &'a str, - /// The file it's loaded from. - pub module: &'a str, + /// The file it's loaded from. Note that this is an unresolved path, so it + /// might be a relative load. + pub loaded_from: &'a str, } impl AstModule { @@ -42,7 +43,7 @@ impl AstModule { .flat_map(|l| { l.args.iter().map(|symbol| LoadedSymbol { name: &symbol.1, - module: &l.module, + loaded_from: &l.module, }) }) .collect() @@ -69,7 +70,7 @@ load("foo", "bar") ); let res = modu.loaded_symbols(); assert_eq!( - res.map(|symbol| format!("{}:{}", symbol.module, symbol.name)), + res.map(|symbol| format!("{}:{}", symbol.loaded_from, symbol.name)), &["test:a", "test:c", "foo:bar"] ); } diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index e2fb2cf32..8145a95c0 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -818,21 +818,40 @@ impl Backend { } } - for symbol in all_documents + for (doc_uri, symbol) in all_documents .iter() .filter(|&(doc_uri, _)| match except_from { Some(uri) => doc_uri != uri, None => true, }) - .flat_map(|(_, doc)| doc.get_loaded_symbols()) - .filter(|symbol| !symbols.contains_key(symbol.name)) + .flat_map(|(doc_uri, doc)| { + doc.get_loaded_symbols() + .into_iter() + .map(move |symbol| (doc_uri, symbol)) + }) + .filter(|(_, symbol)| !symbols.contains_key(symbol.name)) { - if seen.insert(format!("{}:{}", symbol.module, symbol.name)) { + let workspace_root = + Self::get_workspace_root(initialize_params.workspace_folders.as_ref(), doc_uri); + let Ok(url) = self.context + .resolve_load(symbol.loaded_from, doc_uri, workspace_root) + else { + continue; + }; + let Ok(load_path) = self.context.render_as_load( + &url, + current_document, + workspace_root + ) else { + continue; + }; + + if seen.insert(format!("{}:{}", &load_path, symbol.name)) { result.push(CompletionItem { label: symbol.name.to_string(), - detail: Some(format!("Load from {}", symbol.module)), + detail: Some(format!("Load from {}", &load_path)), kind: Some(CompletionItemKind::CONSTANT), - additional_text_edits: Some(vec![format_text_edit(symbol.module, symbol.name)]), + additional_text_edits: Some(vec![format_text_edit(&load_path, symbol.name)]), ..Default::default() }) } @@ -885,7 +904,6 @@ impl Backend { Some((previously_loaded_symbols, load_span)) => { // We're already loading a symbol from this module path, construct // a text edit that amends the existing load. - // let (previously_loaded_symbols, load_span) = existing_load; let load_span = ast.ast.codemap.resolve_span(*load_span); let mut load_args: Vec<(&str, &str)> = previously_loaded_symbols .iter() From e518e90139bca0cb0a6b18dccde71415b286da77 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Thu, 1 Jun 2023 16:07:53 +0200 Subject: [PATCH 20/41] fix: correctly determine workspace root from `LspUrl` --- starlark/src/lsp/server.rs | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 8145a95c0..8f0396838 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -640,7 +640,7 @@ impl Backend { source, None, &uri, - workspace_root, + workspace_root.as_deref(), )?, // In this case we don't pass the name along in the root_definition_location, // so it's simpler to do the lookup here, rather than threading a ton of @@ -667,7 +667,7 @@ impl Backend { .as_str(), ), &uri, - workspace_root, + workspace_root.as_deref(), )?, } } @@ -793,7 +793,7 @@ impl Backend { let Ok(load_path) = self.context.render_as_load( doc_uri, current_document, - Self::get_workspace_root(initialize_params.workspace_folders.as_ref(), doc_uri), + Self::get_workspace_root(initialize_params.workspace_folders.as_ref(), doc_uri).as_deref(), ) else { continue; }; @@ -834,14 +834,14 @@ impl Backend { let workspace_root = Self::get_workspace_root(initialize_params.workspace_folders.as_ref(), doc_uri); let Ok(url) = self.context - .resolve_load(symbol.loaded_from, doc_uri, workspace_root) + .resolve_load(symbol.loaded_from, doc_uri, workspace_root.as_deref()) else { continue; }; let Ok(load_path) = self.context.render_as_load( &url, current_document, - workspace_root + workspace_root.as_deref() ) else { continue; }; @@ -970,19 +970,19 @@ impl Backend { }) } - fn get_workspace_root<'a>( - workspace_roots: Option<&'_ Vec>, - target: &'a LspUrl, - ) -> Option<&'a Path> { - let LspUrl::File(target) = target else { - return None; - }; - - workspace_roots.and_then(|roots| { - roots - .iter() - .find_map(|root| target.strip_prefix(root.uri.path()).ok()) - }) + fn get_workspace_root( + workspace_roots: Option<&Vec>, + target: &LspUrl, + ) -> Option { + match target { + LspUrl::File(target) => workspace_roots.and_then(|roots| { + roots + .iter() + .filter_map(|root| root.uri.to_file_path().ok()) + .find(|root| target.starts_with(root)) + }), + _ => None, + } } } From 78c2e9ccadb1eb7c168ab854b66a1d09ef1eb5b8 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Mon, 5 Jun 2023 10:08:41 +0200 Subject: [PATCH 21/41] fix: support `resolve_load` for relative paths with subfolder --- starlark/bin/eval.rs | 50 ++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/starlark/bin/eval.rs b/starlark/bin/eval.rs index 284508a89..8d8fdff59 100644 --- a/starlark/bin/eval.rs +++ b/starlark/bin/eval.rs @@ -84,8 +84,8 @@ pub(crate) struct EvalResult> { enum ResolveLoadError { /// Attempted to resolve a relative path, but no current_file_path was provided, /// so it is not known what to resolve the path against. - #[error("Relative path `{}` provided, but current_file_path could not be determined", .0.display())] - MissingCurrentFilePath(PathBuf), + #[error("Relative path `{}` provided, but current_file_path could not be determined", .0)] + MissingCurrentFilePath(String), /// The scheme provided was not correct or supported. #[error("Url `{}` was expected to be of type `{}`", .1, .0)] WrongScheme(String, LspUrl), @@ -319,24 +319,7 @@ impl LspContext for Context { current_file: &LspUrl, workspace_root: Option<&Path>, ) -> anyhow::Result { - if let Some(path) = path.strip_prefix(':') { - // Resolve relative paths from the current file. - let path = PathBuf::from(path); - match current_file { - LspUrl::File(current_file_path) => { - let current_file_dir = current_file_path.parent(); - let absolute_path = match current_file_dir { - Some(current_file_dir) => Ok(current_file_dir.join(&path)), - None => Err(ResolveLoadError::MissingCurrentFilePath(path)), - }?; - Ok(Url::from_file_path(absolute_path).unwrap().try_into()?) - } - _ => Err( - ResolveLoadError::WrongScheme("file://".to_owned(), current_file.clone()) - .into(), - ), - } - } else if let Some(path) = path.strip_prefix("//") { + if let Some(path) = path.strip_prefix("//") { // Resolve from the root of the workspace. match (path.split_once(':'), workspace_root) { (Some((subfolder, filename)), Some(workspace_root)) => { @@ -350,6 +333,27 @@ impl LspContext for Context { } (None, _) => Err(ResolveLoadError::CannotParsePath(format!("//{path}")).into()), } + } else if let Some((folder, filename)) = path.split_once(':') { + // Resolve relative paths from the current file. + match current_file { + LspUrl::File(current_file_path) => { + let current_file_dir = current_file_path.parent(); + let absolute_path = match current_file_dir { + Some(current_file_dir) => { + let mut result = current_file_dir.to_owned(); + result.push(folder); + result.push(filename); + Ok(result) + } + None => Err(ResolveLoadError::MissingCurrentFilePath(path.to_owned())), + }?; + Ok(Url::from_file_path(absolute_path).unwrap().try_into()?) + } + _ => Err( + ResolveLoadError::WrongScheme("file://".to_owned(), current_file.clone()) + .into(), + ), + } } else { Err(ResolveLoadError::CannotParsePath(path.to_owned()).into()) } @@ -472,6 +476,12 @@ mod tests { .unwrap(), LspUrl::File(PathBuf::from("/absolute/path/relative.star")) ); + assert_eq!( + context + .resolve_load("subpath:relative.star", ¤t_file, None) + .unwrap(), + LspUrl::File(PathBuf::from("/absolute/path/subpath/relative.star")) + ); assert_eq!( context .resolve_load("//:root.star", ¤t_file, Some(Path::new("/foo/bar")),) From f3af9e3b9d96359a986cd34b63a5912c14671968 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Mon, 26 Jun 2023 12:08:50 +0200 Subject: [PATCH 22/41] feat: retrieve info about the build system --- starlark/bin/eval.rs | 9 ++++ starlark/bin/main.rs | 5 +++ starlark/src/build_system/bazel.rs | 70 ++++++++++++++++++++++++++++++ starlark/src/build_system/buck.rs | 0 starlark/src/build_system/buck2.rs | 59 +++++++++++++++++++++++++ starlark/src/build_system/mod.rs | 61 ++++++++++++++++++++++++++ starlark/src/lib.rs | 1 + 7 files changed, 205 insertions(+) create mode 100644 starlark/src/build_system/bazel.rs create mode 100644 starlark/src/build_system/buck.rs create mode 100644 starlark/src/build_system/buck2.rs create mode 100644 starlark/src/build_system/mod.rs diff --git a/starlark/bin/eval.rs b/starlark/bin/eval.rs index 8d8fdff59..50686bfc6 100644 --- a/starlark/bin/eval.rs +++ b/starlark/bin/eval.rs @@ -26,6 +26,9 @@ use std::path::PathBuf; use itertools::Either; use lsp_types::Diagnostic; use lsp_types::Url; +use starlark::build_system::try_resolve_build_system; +use starlark::build_system::BuildSystem; +use starlark::build_system::BuildSystemHint; use starlark::docs::get_registered_starlark_docs; use starlark::docs::render_docs_as_code; use starlark::docs::Doc; @@ -68,6 +71,7 @@ pub(crate) struct Context { pub(crate) builtin_docs: HashMap, pub(crate) builtin_symbols: HashMap, pub(crate) globals: Globals, + pub(crate) build_system: Option>, } /// The outcome of evaluating (checking, parsing or running) given starlark code. @@ -115,6 +119,7 @@ impl Context { print_non_none: bool, prelude: &[PathBuf], module: bool, + build_system_hint: Option, ) -> anyhow::Result { let globals = globals(); let prelude: Vec<_> = prelude @@ -147,6 +152,9 @@ impl Context { .map(|(u, ds)| (u, render_docs_as_code(&ds))) .collect(); + let build_system = + try_resolve_build_system(std::env::current_dir().ok().as_ref(), build_system_hint); + Ok(Self { mode, print_non_none, @@ -155,6 +163,7 @@ impl Context { builtin_docs, builtin_symbols, globals, + build_system, }) } diff --git a/starlark/bin/main.rs b/starlark/bin/main.rs index 358394bff..3020a33a2 100644 --- a/starlark/bin/main.rs +++ b/starlark/bin/main.rs @@ -35,6 +35,7 @@ use dupe::Dupe; use eval::Context; use itertools::Either; use itertools::Itertools; +use starlark::build_system::BuildSystemHint; use starlark::docs::get_registered_starlark_docs; use starlark::docs::render_docs_as_code; use starlark::docs::Doc; @@ -103,6 +104,9 @@ struct Args { )] json: bool, + #[arg(long = "build-system", help = "Build system to use.")] + build_system: Option, + #[arg( long = "docs", help = "Generate documentation output.", @@ -264,6 +268,7 @@ fn main() -> anyhow::Result<()> { !args.evaluate.is_empty() || is_interactive, &expand_dirs(ext, args.prelude).collect::>(), is_interactive, + args.build_system, )?; if args.lsp { diff --git a/starlark/src/build_system/bazel.rs b/starlark/src/build_system/bazel.rs new file mode 100644 index 000000000..f5c5e5e3e --- /dev/null +++ b/starlark/src/build_system/bazel.rs @@ -0,0 +1,70 @@ +use std::borrow::Cow; +use std::path::Path; +use std::path::PathBuf; +use std::process::Command; + +use crate::build_system::BuildSystem; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct BazelBuildSystem { + workspace_name: Option, + output_base: PathBuf, +} + +impl BazelBuildSystem { + const DEFAULT_WORKSPACE_NAME: &'static str = "__main__"; + + pub(crate) fn new(workspace_dir: Option<&PathBuf>) -> Option { + let mut raw_command = Command::new("bazel"); + let mut command = raw_command.arg("info"); + if let Some(workspace_dir) = workspace_dir { + command = command.current_dir(workspace_dir); + } + + let output = command.output().ok()?; + if !output.status.success() { + return None; + } + + let output = String::from_utf8(output.stdout).ok()?; + let mut execroot = None; + let mut output_base = None; + for line in output.lines() { + if let Some((key, value)) = line.split_once(": ") { + match key { + "execution_root" => execroot = Some(value), + "output_base" => output_base = Some(value), + _ => {} + } + } + } + + if let (Some(execroot), Some(output_base)) = (execroot, output_base) { + Some(Self { + workspace_name: match PathBuf::from(execroot) + .file_name()? + .to_string_lossy() + .to_string() + { + name if name == Self::DEFAULT_WORKSPACE_NAME => None, + name => Some(name), + }, + output_base: PathBuf::from(output_base), + }) + } else { + None + } + } +} + +impl BuildSystem for BazelBuildSystem { + fn root_repository_name(&self) -> Option<&str> { + self.workspace_name.as_deref() + } + + fn repository_path(&self, repository_name: &str) -> Option> { + let mut path = self.output_base.join("external"); + path.push(repository_name); + Some(Cow::Owned(path)) + } +} diff --git a/starlark/src/build_system/buck.rs b/starlark/src/build_system/buck.rs new file mode 100644 index 000000000..e69de29bb diff --git a/starlark/src/build_system/buck2.rs b/starlark/src/build_system/buck2.rs new file mode 100644 index 000000000..287ed0f54 --- /dev/null +++ b/starlark/src/build_system/buck2.rs @@ -0,0 +1,59 @@ +use std::borrow::Cow; +use std::collections::HashMap; +use std::path::Path; +use std::path::PathBuf; +use std::process::Command; + +use crate::build_system::BuildSystem; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct Buck2BuildSystem { + workspace_name: String, + repositories: HashMap, +} + +impl Buck2BuildSystem { + pub(crate) fn new(workspace_dir: Option<&PathBuf>) -> Option { + // We always need the workspace dir to resolve the workspace name. + let workspace_dir = workspace_dir?; + + let mut raw_command = Command::new("buck2"); + let command = raw_command + .arg("audit") + .arg("cell") + .arg("--json") + .current_dir(workspace_dir); + + let output = command.output().ok()?; + if !output.status.success() { + return None; + } + + let repositories = + serde_json::from_slice::>(&output.stdout).ok()?; + let workspace_name = repositories.iter().find_map(|(name, path)| { + if path == workspace_dir { + Some(name.clone()) + } else { + None + } + })?; + + Some(Self { + workspace_name, + repositories, + }) + } +} + +impl BuildSystem for Buck2BuildSystem { + fn root_repository_name(&self) -> Option<&str> { + Some(&self.workspace_name) + } + + fn repository_path(&self, repository_name: &str) -> Option> { + self.repositories + .get(repository_name) + .map(|path| Cow::Borrowed(path.as_path())) + } +} diff --git a/starlark/src/build_system/mod.rs b/starlark/src/build_system/mod.rs new file mode 100644 index 000000000..cf835e921 --- /dev/null +++ b/starlark/src/build_system/mod.rs @@ -0,0 +1,61 @@ +//! Build system support. Allows to resolve the repository structure, including external +//! repositories. + +use std::borrow::Cow; +use std::path::Path; +use std::path::PathBuf; + +use clap::ValueEnum; + +mod bazel; +mod buck2; + +/// A hint for which build system to resolve. +#[derive(ValueEnum, Debug, Clone, PartialEq, Eq)] +pub enum BuildSystemHint { + /// Try to resolve Bazel. + Bazel, + /// Try to resolve Buck2. + Buck2, +} + +/// A build system provides information about the repository structure. +pub trait BuildSystem: std::fmt::Debug + Send + Sync { + /// Returns the name of the root repository. + fn root_repository_name(&self) -> Option<&str>; + + /// Returns the path of the repository with the given name. + fn repository_path(&self, repository_name: &str) -> Option>; +} + +/// Tries to resolve the build system from the current working directory. +/// You can optionally provide a hint to only try a specific build system. +/// If no hint is provided, the build systems are tried in the following order: +/// - Buck2 +/// - Bazel +pub fn try_resolve_build_system( + workspace_dir: Option<&PathBuf>, + hint: Option, +) -> Option> { + match hint { + Some(BuildSystemHint::Bazel) => { + Some(Box::new(bazel::BazelBuildSystem::new(workspace_dir)?)) + } + Some(BuildSystemHint::Buck2) => { + Some(Box::new(buck2::Buck2BuildSystem::new(workspace_dir)?)) + } + None => { + if let Some(build_system) = + try_resolve_build_system(workspace_dir, Some(BuildSystemHint::Buck2)) + { + Some(build_system) + } else if let Some(build_system) = + try_resolve_build_system(workspace_dir, Some(BuildSystemHint::Bazel)) + { + Some(build_system) + } else { + None + } + } + } +} diff --git a/starlark/src/lib.rs b/starlark/src/lib.rs index 3c261c619..23c0f36b9 100644 --- a/starlark/src/lib.rs +++ b/starlark/src/lib.rs @@ -389,6 +389,7 @@ pub use stdlib::PrintHandler; pub(crate) mod analysis; pub mod any; pub mod assert; +pub mod build_system; pub mod codemap; pub mod collections; pub mod debug; From 3d45a62c16ebbaec779bb0143c7c457ca27de6c7 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Mon, 26 Jun 2023 13:08:52 +0200 Subject: [PATCH 23/41] feat: use build system info to resolve load paths containing repository names --- starlark/bin/eval.rs | 42 +++++++++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/starlark/bin/eval.rs b/starlark/bin/eval.rs index 50686bfc6..133deb951 100644 --- a/starlark/bin/eval.rs +++ b/starlark/bin/eval.rs @@ -15,6 +15,7 @@ * limitations under the License. */ +use std::borrow::Cow; use std::collections::HashMap; use std::collections::HashSet; use std::fs; @@ -100,6 +101,9 @@ enum ResolveLoadError { /// Unable to parse the given path. #[error("Unable to parse the load path `{}`", .0)] CannotParsePath(String), + /// Cannot resolve path containing workspace without information from the build system. + #[error("Cannot resolve path `{}` without build system info", .0)] + MissingBuildSystem(String), } /// Errors when [`LspContext::render_as_load()`] cannot render a given path. @@ -328,19 +332,43 @@ impl LspContext for Context { current_file: &LspUrl, workspace_root: Option<&Path>, ) -> anyhow::Result { - if let Some(path) = path.strip_prefix("//") { - // Resolve from the root of the workspace. - match (path.split_once(':'), workspace_root) { - (Some((subfolder, filename)), Some(workspace_root)) => { - let mut result = workspace_root.to_owned(); + let original_path = path; + if let Some((repository, path)) = path.split_once("//") { + // The repository may be prefixed with an '@', but it's optional in Buck2. + let repository = if let Some(without_at) = repository.strip_prefix('@') { + without_at + } else { + repository + }; + + // Check if the repository refers to the workspace root, or if not, if it is a known + // repository, and what its path is. + let repository_root = if repository.is_empty() { + workspace_root.map(Cow::Borrowed) + } else if let Some(build_system) = &self.build_system { + if matches!(build_system.root_repository_name(), Some(name) if name == repository) { + workspace_root.map(Cow::Borrowed) + } else { + build_system.repository_path(repository) + } + } else { + return Err(ResolveLoadError::MissingBuildSystem(path.to_owned()).into()); + }; + + // Resolve from the root of the repository. + match (path.split_once(':'), repository_root) { + (Some((subfolder, filename)), Some(repository_root)) => { + let mut result = repository_root.into_owned(); result.push(subfolder); result.push(filename); Ok(Url::from_file_path(result).unwrap().try_into()?) } (Some(_), None) => { - Err(ResolveLoadError::MissingWorkspaceRoot(path.to_owned()).into()) + Err(ResolveLoadError::MissingWorkspaceRoot(original_path.to_owned()).into()) + } + (None, _) => { + Err(ResolveLoadError::CannotParsePath(original_path.to_string()).into()) } - (None, _) => Err(ResolveLoadError::CannotParsePath(format!("//{path}")).into()), } } else if let Some((folder, filename)) = path.split_once(':') { // Resolve relative paths from the current file. From 6933b9d4ffc67967e30e2497b07f2dc5d921c3e0 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Mon, 26 Jun 2023 13:15:46 +0200 Subject: [PATCH 24/41] fix: when inserting first load statement, add line breaks after it --- starlark/src/lsp/server.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 8f0396838..bfaa5a5d7 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -944,8 +944,9 @@ impl Backend { None => Range::new(Position::new(0, 0), Position::new(0, 0)), }, format!( - "{}load(\"{module}\", \"{symbol}\")", + "{}load(\"{module}\", \"{symbol}\"){}", if last_load.is_some() { "\n" } else { "" }, + if last_load.is_some() { "" } else { "\n\n" }, ), ) } From d05fe299fd8581e069434f5907c77f927c035567 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Mon, 26 Jun 2023 15:26:24 +0200 Subject: [PATCH 25/41] feat: improve docs rendering and include older `detail` Note: still only for global symbols at the moment --- starlark/src/docs/markdown.rs | 2 +- starlark/src/docs/mod.rs | 18 ++++++++++++++++++ starlark/src/lsp/server.rs | 16 ++++++---------- 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/starlark/src/docs/markdown.rs b/starlark/src/docs/markdown.rs index 5ad6b71eb..9bc053b7c 100644 --- a/starlark/src/docs/markdown.rs +++ b/starlark/src/docs/markdown.rs @@ -234,7 +234,7 @@ fn render_object(name: &str, object: &DocObject) -> String { render_members(name, true, &object.docs, &object.members) } -fn render_doc_item(name: &str, item: &DocItem) -> String { +pub(crate) fn render_doc_item(name: &str, item: &DocItem) -> String { match &item { DocItem::Module(m) => render_module(name, m), DocItem::Object(o) => render_object(name, o), diff --git a/starlark/src/docs/mod.rs b/starlark/src/docs/mod.rs index d10a85cb6..d73a797a6 100644 --- a/starlark/src/docs/mod.rs +++ b/starlark/src/docs/mod.rs @@ -27,6 +27,7 @@ use std::collections::HashMap; use allocative::Allocative; use dupe::Dupe; use itertools::Itertools; +pub(crate) use markdown::render_doc_item; pub use markdown::MarkdownFlavor; pub use markdown::RenderMarkdown; use once_cell::sync::Lazy; @@ -799,6 +800,23 @@ pub enum DocItem { Property(DocProperty), } +impl DocItem { + /// Get the underlying [`DocString`] for this item, if it exists. + pub fn get_doc_string(&self) -> Option<&DocString> { + match self { + DocItem::Module(m) => m.docs.as_ref(), + DocItem::Object(o) => o.docs.as_ref(), + DocItem::Function(f) => f.docs.as_ref(), + DocItem::Property(p) => p.docs.as_ref(), + } + } + + /// Get the summary of the underlying [`DocString`] for this item, if it exists. + pub fn get_doc_summary(&self) -> Option<&str> { + self.get_doc_string().map(|ds| ds.summary.as_str()) + } +} + /// The main structure that represents the documentation for a given symbol / module. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct Doc { diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index bfaa5a5d7..bd85094f1 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -89,7 +89,7 @@ use crate::codemap::LineCol; use crate::codemap::ResolvedSpan; use crate::codemap::Span; use crate::codemap::Spanned; -use crate::docs::DocItem; +use crate::docs::render_doc_item; use crate::environment::GlobalSymbol; use crate::environment::GlobalSymbolKind; use crate::lsp::server::LoadContentsError::WrongScheme; @@ -870,20 +870,16 @@ impl Backend { GlobalSymbolKind::Function => CompletionItemKind::FUNCTION, GlobalSymbolKind::Constant => CompletionItemKind::CONSTANT, }), + detail: symbol + .documentation + .as_ref() + .and_then(|docs| docs.get_doc_summary().map(|str| str.to_string())), documentation: symbol.documentation.map(|docs| { Documentation::MarkupContent(MarkupContent { // The doc item is rendered as code, so embed it in markdown, indicating // the syntax, in order to render correctly. kind: MarkupKind::Markdown, - value: format!( - "```starlark\n{}\n```", - match docs { - DocItem::Module(m) => m.render_as_code(), - DocItem::Object(o) => o.render_as_code(symbol.name), - DocItem::Function(f) => f.render_as_code(symbol.name), - DocItem::Property(p) => p.render_as_code(symbol.name), - } - ), + value: render_doc_item(symbol.name, &docs), }) }), ..Default::default() From d5cd6c803a537760b1296f2158cc89f3718a84be Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Mon, 26 Jun 2023 17:23:02 +0200 Subject: [PATCH 26/41] fix: make tests work again and clean up old comment --- starlark/bin/eval.rs | 2 +- starlark/src/lsp/server.rs | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/starlark/bin/eval.rs b/starlark/bin/eval.rs index 133deb951..1dcd4c368 100644 --- a/starlark/bin/eval.rs +++ b/starlark/bin/eval.rs @@ -498,7 +498,7 @@ mod tests { use super::*; fn make_context() -> Context { - Context::new(ContextMode::Run, false, &[], false).unwrap() + Context::new(ContextMode::Run, false, &[], false, None).unwrap() } #[test] diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index bd85094f1..e159ca9c6 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -876,8 +876,6 @@ impl Backend { .and_then(|docs| docs.get_doc_summary().map(|str| str.to_string())), documentation: symbol.documentation.map(|docs| { Documentation::MarkupContent(MarkupContent { - // The doc item is rendered as code, so embed it in markdown, indicating - // the syntax, in order to render correctly. kind: MarkupKind::Markdown, value: render_doc_item(symbol.name, &docs), }) From 47134b42074f0fad1007049ba751ecba93672b0e Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Mon, 26 Jun 2023 17:23:37 +0200 Subject: [PATCH 27/41] feat: allow creating `DocItem::Param` variant --- starlark/bin/eval.rs | 2 +- starlark/src/docs/markdown.rs | 3 +++ starlark/src/docs/mod.rs | 21 +++++++++++++++++++++ starlark/src/typing/oracle/docs.rs | 1 + 4 files changed, 26 insertions(+), 1 deletion(-) diff --git a/starlark/bin/eval.rs b/starlark/bin/eval.rs index 1dcd4c368..4ace5bd77 100644 --- a/starlark/bin/eval.rs +++ b/starlark/bin/eval.rs @@ -177,7 +177,7 @@ impl Context { DocItem::Object(_) => { Url::parse(&format!("starlark:/native/builtins/{}.bzl", doc.id.name)).unwrap() } - DocItem::Function(_) | DocItem::Property(_) => { + DocItem::Function(_) | DocItem::Property(_) | DocItem::Param(_) => { Url::parse("starlark:/native/builtins.bzl").unwrap() } }; diff --git a/starlark/src/docs/markdown.rs b/starlark/src/docs/markdown.rs index 9bc053b7c..155381d89 100644 --- a/starlark/src/docs/markdown.rs +++ b/starlark/src/docs/markdown.rs @@ -240,6 +240,9 @@ pub(crate) fn render_doc_item(name: &str, item: &DocItem) -> String { DocItem::Object(o) => render_object(name, o), DocItem::Function(f) => render_function(name, f), DocItem::Property(p) => render_property(name, p), + DocItem::Param(p) => { + render_function_parameters(std::slice::from_ref(p)).unwrap_or_default() + } } } diff --git a/starlark/src/docs/mod.rs b/starlark/src/docs/mod.rs index d73a797a6..43d8dc1c7 100644 --- a/starlark/src/docs/mod.rs +++ b/starlark/src/docs/mod.rs @@ -480,6 +480,19 @@ impl DocFunction { format!("def {}{}{}:\n{} pass", name, params, ret, docstring) } + pub(crate) fn find_param_with_name(&self, param_name: &str) -> Option<&DocParam> { + self.params.iter().find(|p| match p { + DocParam::Arg { name, .. } + | DocParam::Args { name, .. } + | DocParam::Kwargs { name, .. } + if name == param_name => + { + true + } + _ => false, + }) + } + /// Parses function documentation out of a docstring /// /// # Arguments @@ -798,6 +811,7 @@ pub enum DocItem { Object(DocObject), Function(DocFunction), Property(DocProperty), + Param(DocParam), } impl DocItem { @@ -808,6 +822,12 @@ impl DocItem { DocItem::Object(o) => o.docs.as_ref(), DocItem::Function(f) => f.docs.as_ref(), DocItem::Property(p) => p.docs.as_ref(), + DocItem::Param(p) => match p { + DocParam::Arg { docs, .. } + | DocParam::Args { docs, .. } + | DocParam::Kwargs { docs, .. } => docs.as_ref(), + _ => None, + }, } } @@ -838,6 +858,7 @@ impl Doc { DocItem::Object(o) => o.render_as_code(&self.id.name), DocItem::Function(f) => f.render_as_code(&self.id.name), DocItem::Property(p) => p.render_as_code(&self.id.name), + DocItem::Param(p) => p.render_as_code(), } } } diff --git a/starlark/src/typing/oracle/docs.rs b/starlark/src/typing/oracle/docs.rs index 0a1cb0f9b..73bbe2293 100644 --- a/starlark/src/typing/oracle/docs.rs +++ b/starlark/src/typing/oracle/docs.rs @@ -67,6 +67,7 @@ impl OracleDocs { self.functions .insert(doc.id.name.clone(), Ty::from_docs_function(x)); } + DocItem::Param(_) => {} } } From 0778b88f827635f53740df1e34f63634c58ed707 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Mon, 26 Jun 2023 17:24:07 +0200 Subject: [PATCH 28/41] feat: render docs for functions and arguments in the current file --- starlark/src/lsp/server.rs | 8 +++- starlark/src/syntax/symbols.rs | 83 +++++++++++++++++++++++++++++++++- 2 files changed, 89 insertions(+), 2 deletions(-) diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index e159ca9c6..bd824a6c1 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -702,12 +702,18 @@ impl Backend { ( key, CompletionItem { - label: value.name, kind: Some(match value.kind { SymbolKind::Method => CompletionItemKind::METHOD, SymbolKind::Variable => CompletionItemKind::VARIABLE, }), detail: value.detail, + documentation: value.doc.map(|doc| { + Documentation::MarkupContent(MarkupContent { + kind: MarkupKind::Markdown, + value: render_doc_item(&value.name, &doc), + }) + }), + label: value.name, ..Default::default() }, ) diff --git a/starlark/src/syntax/symbols.rs b/starlark/src/syntax/symbols.rs index 60eea0b2e..f5972019e 100644 --- a/starlark/src/syntax/symbols.rs +++ b/starlark/src/syntax/symbols.rs @@ -1,7 +1,15 @@ use std::collections::HashMap; +use super::ast::AstExprP; +use super::ast::DefP; use crate::codemap::CodeMap; use crate::codemap::LineCol; +use crate::docs::DocFunction; +use crate::docs::DocItem; +use crate::docs::DocParam; +use crate::docs::DocStringKind; +use crate::docs::DocType; +use crate::syntax::ast::AstLiteral; use crate::syntax::ast::AstPayload; use crate::syntax::ast::AstStmtP; use crate::syntax::ast::ExprP; @@ -19,6 +27,7 @@ pub(crate) struct Symbol { pub(crate) name: String, pub(crate) detail: Option, pub(crate) kind: SymbolKind, + pub(crate) doc: Option, } /// Walk the AST recursively and discover symbols. @@ -45,6 +54,7 @@ pub(crate) fn find_symbols_at_location( _ => SymbolKind::Variable, }), detail: None, + doc: None, }); }) } @@ -56,6 +66,7 @@ pub(crate) fn find_symbols_at_location( _ => SymbolKind::Variable, }), detail: None, + doc: None, }); }), StmtP::For(dest, over_body) => { @@ -65,17 +76,21 @@ pub(crate) fn find_symbols_at_location( name: x.0.to_string(), kind: SymbolKind::Variable, detail: None, + doc: None, }); }); walk(codemap, body, cursor_position, symbols); } StmtP::Def(def) => { + // Peek into the function definition to find the docstring. + let doc = get_doc_item_for_def(def); symbols .entry(def.name.0.to_string()) .or_insert_with(|| Symbol { name: def.name.0.to_string(), kind: SymbolKind::Method, detail: None, + doc: doc.clone().map(DocItem::Function), }); // Only recurse into method if the cursor is in it. @@ -90,6 +105,10 @@ pub(crate) fn find_symbols_at_location( name: p.0.clone(), kind: SymbolKind::Variable, detail: None, + doc: doc.as_ref().and_then(|doc| { + doc.find_param_with_name(&p.0) + .map(|param| DocItem::Param(param.clone())) + }), }, )), _ => None, @@ -105,6 +124,8 @@ pub(crate) fn find_symbols_at_location( detail: Some(format!("Loaded from {}", load.module.node)), // TODO: This should be dynamic based on the actual loaded value. kind: SymbolKind::Method, + // TODO: Pull from the original file. + doc: None, }, ) })), @@ -116,6 +137,57 @@ pub(crate) fn find_symbols_at_location( symbols } +fn get_doc_item_for_def(def: &DefP

) -> Option { + if let Some(doc_string) = peek_docstring(&def.body) { + let args: Vec<_> = def + .params + .iter() + .filter_map(|param| match ¶m.node { + ParameterP::Normal(p, _) + | ParameterP::WithDefaultValue(p, _, _) + | ParameterP::Args(p, _) + | ParameterP::KwArgs(p, _) => Some(DocParam::Arg { + name: p.0.to_owned(), + docs: None, + typ: None, + default_value: None, + }), + _ => None, + }) + .collect(); + let return_type = + def.return_type + .as_ref() + .and_then(|return_type| match &return_type.node { + ExprP::Identifier(i, _) => Some(DocType { + raw_type: i.node.to_owned(), + }), + _ => None, + }); + + let doc_function = DocFunction::from_docstring( + DocStringKind::Starlark, + args, + return_type, + Some(doc_string), + ); + Some(doc_function) + } else { + None + } +} + +fn peek_docstring(stmt: &AstStmtP

) -> Option<&str> { + match &stmt.node { + StmtP::Statements(stmts) => stmts.first().and_then(peek_docstring), + StmtP::Expression(expr) => match &expr.node { + ExprP::Literal(AstLiteral::String(s)) => Some(s.node.as_str()), + _ => None, + }, + _ => None, + } +} + #[cfg(test)] mod tests { use std::collections::HashMap; @@ -156,6 +228,7 @@ my_var = True name: "exported_a".to_string(), detail: Some("Loaded from foo.star".to_string()), kind: SymbolKind::Method, + doc: None, }, ), ( @@ -164,6 +237,7 @@ my_var = True name: "renamed".to_string(), detail: Some("Loaded from foo.star".to_string()), kind: SymbolKind::Method, + doc: None, }, ), ( @@ -172,6 +246,7 @@ my_var = True name: "method".to_string(), detail: None, kind: SymbolKind::Method, + doc: None, }, ), ( @@ -180,6 +255,7 @@ my_var = True name: "my_var".to_string(), detail: None, kind: SymbolKind::Variable, + doc: None, }, ), ]) @@ -215,6 +291,7 @@ my_var = True name: "exported_a".to_string(), detail: Some("Loaded from foo.star".to_string()), kind: SymbolKind::Method, + doc: None, }, ), ( @@ -223,6 +300,7 @@ my_var = True name: "renamed".to_string(), detail: Some("Loaded from foo.star".to_string()), kind: SymbolKind::Method, + doc: None, }, ), ( @@ -231,6 +309,7 @@ my_var = True name: "method".to_string(), detail: None, kind: SymbolKind::Method, + doc: None, }, ), ( @@ -238,7 +317,8 @@ my_var = True Symbol { name: "param".to_string(), detail: None, - kind: SymbolKind::Variable + kind: SymbolKind::Variable, + doc: None, } ), ( @@ -247,6 +327,7 @@ my_var = True name: "my_var".to_string(), detail: None, kind: SymbolKind::Variable, + doc: None, }, ), ]) From 3fc4ae0afaf18f749d8366b0473b74ce1e229146 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 27 Jun 2023 11:15:14 +0200 Subject: [PATCH 29/41] fix: improve handling of remote repositories during resolving and rendering loads --- starlark/bin/eval.rs | 151 +++++++++++++++++++++-------- starlark/src/build_system/bazel.rs | 22 ++++- starlark/src/build_system/buck2.rs | 14 +++ starlark/src/build_system/mod.rs | 11 +++ 4 files changed, 154 insertions(+), 44 deletions(-) diff --git a/starlark/bin/eval.rs b/starlark/bin/eval.rs index 4ace5bd77..65c6fb41a 100644 --- a/starlark/bin/eval.rs +++ b/starlark/bin/eval.rs @@ -104,6 +104,9 @@ enum ResolveLoadError { /// Cannot resolve path containing workspace without information from the build system. #[error("Cannot resolve path `{}` without build system info", .0)] MissingBuildSystem(String), + /// The path contained a repository name that is not known to the build system. + #[error("Cannot resolve path `{}` because the repository `{}` is unknown", .0, .1)] + UnknownRepository(String, String), } /// Errors when [`LspContext::render_as_load()`] cannot render a given path. @@ -341,24 +344,63 @@ impl LspContext for Context { repository }; - // Check if the repository refers to the workspace root, or if not, if it is a known - // repository, and what its path is. - let repository_root = if repository.is_empty() { - workspace_root.map(Cow::Borrowed) - } else if let Some(build_system) = &self.build_system { - if matches!(build_system.root_repository_name(), Some(name) if name == repository) { - workspace_root.map(Cow::Borrowed) - } else { - build_system.repository_path(repository) + // Find the root we're resolving from. There's quite a few cases to consider here: + // - `repository` is empty, and we're resolving from the workspace root. + // - `repository` is empty, and we're resolving from a known remote repository. + // - `repository` is not empty, and refers to the current repository (the workspace). + // - `repository` is not empty, and refers to a known remote repository. + // + // Also with all of these cases, we need to consider if we have build system + // information or not. If not, we can't resolve any remote repositories, and we can't + // know whether a repository name refers to the workspace or not. + let resolve_root = match (repository, current_file, self.build_system.as_ref()) { + // Repository is empty, and we know what file we're resolving from. Use the build + // system information to check if we're in a known remote repository, and what the + // root is. Fall back to the `workspace_root` otherwise. + ("", LspUrl::File(current_file), Some(build_system)) => { + if let Some((_, remote_repository_root)) = + build_system.repository_for_path(current_file) + { + Some(Cow::Borrowed(remote_repository_root)) + } else { + workspace_root.map(Cow::Borrowed) + } + } + // No repository in the load path, and we don't have build system information, or + // an `LspUrl` we can't use to check the root. Use the workspace root. + ("", _, _) => workspace_root.map(Cow::Borrowed), + // We have a repository name and build system information. Check if the repository + // name refers to the workspace, and if so, use the workspace root. If not, check + // if it refers to a known remote repository, and if so, use that root. + // Otherwise, fail with an error. + (repository, _, Some(build_system)) => { + if matches!(build_system.root_repository_name(), Some(name) if name == repository) + { + workspace_root.map(Cow::Borrowed) + } else if let Some(remote_repository_root) = + build_system.repository_path(repository) + { + Some(remote_repository_root) + } else { + return Err(ResolveLoadError::UnknownRepository( + original_path.to_owned(), + repository.to_owned(), + ) + .into()); + } + } + // Finally, fall back to the workspace root. + _ => { + return Err( + ResolveLoadError::MissingBuildSystem(original_path.to_owned()).into(), + ); } - } else { - return Err(ResolveLoadError::MissingBuildSystem(path.to_owned()).into()); }; // Resolve from the root of the repository. - match (path.split_once(':'), repository_root) { - (Some((subfolder, filename)), Some(repository_root)) => { - let mut result = repository_root.into_owned(); + match (path.split_once(':'), resolve_root) { + (Some((subfolder, filename)), Some(resolve_root)) => { + let mut result = resolve_root.into_owned(); result.push(subfolder); result.push(filename); Ok(Url::from_file_path(result).unwrap().try_into()?) @@ -403,35 +445,64 @@ impl LspContext for Context { workspace_root: Option<&Path>, ) -> anyhow::Result { match (target, current_file) { - (LspUrl::File(target_path), LspUrl::File(current_file_path)) => { - let target_package = target_path.parent(); - let current_file_package = current_file_path.parent(); + // Check whether the target and the current file are in the same package. + (LspUrl::File(target_path), LspUrl::File(current_file_path)) if matches!((target_path.parent(), current_file_path.parent()), (Some(a), Some(b)) if a == b) => + { + // Then just return a relative path. let target_filename = target_path.file_name(); - - // If both are in the same package, return a relative path. - if matches!((target_package, current_file_package), (Some(a), Some(b)) if a == b) { - return match target_filename { - Some(filename) => Ok(format!(":{}", filename.to_string_lossy())), - None => { - Err(RenderLoadError::MissingTargetFilename(target_path.clone()).into()) - } - }; + match target_filename { + Some(filename) => Ok(format!(":{}", filename.to_string_lossy())), + None => Err(RenderLoadError::MissingTargetFilename(target_path.clone()).into()), } + } + (LspUrl::File(target_path), _) => { + // Try to find a repository that contains the target, as well as the path to the + // target relative to the repository root. If we can't find a repository, we'll + // try to resolve the target relative to the workspace root. If we don't have a + // workspace root, we'll just use the target path as-is. + let (repository, target_path) = &self + .build_system + .as_ref() + .and_then(|build_system| { + build_system + .repository_for_path(target_path) + .map(|(repository, target_path)| (Some(repository), target_path)) + }) + .or_else(|| { + workspace_root + .and_then(|root| target_path.strip_prefix(root).ok()) + .map(|path| (None, path)) + }) + .unwrap_or((None, target_path)); - let target_path = workspace_root - .and_then(|root| target_path.strip_prefix(root).ok()) - .unwrap_or(target_path); - - Ok(format!( - "//{}:{}", - target_path - .parent() - .map(|path| path.to_string_lossy()) - .unwrap_or_default(), - target_filename - .unwrap_or(target_path.as_os_str()) - .to_string_lossy() - )) + let target_filename = target_path.file_name(); + match target_filename { + Some(filename) => Ok(format!( + "{}{}//{}:{}", + if repository.is_some() + && self + .build_system + .as_ref() + .map(|build_system| { + build_system.should_use_at_sign_before_repository_name() + }) + .unwrap_or(true) + { + "@" + } else { + "" + }, + repository.as_ref().unwrap_or(&Cow::Borrowed("")), + target_path + .parent() + .map(|path| path.to_string_lossy()) + .unwrap_or_default(), + filename.to_string_lossy() + )), + None => Err( + RenderLoadError::MissingTargetFilename(target_path.to_path_buf()).into(), + ), + } } _ => Err(RenderLoadError::WrongScheme( "file://".to_owned(), diff --git a/starlark/src/build_system/bazel.rs b/starlark/src/build_system/bazel.rs index f5c5e5e3e..2ea1e0a6c 100644 --- a/starlark/src/build_system/bazel.rs +++ b/starlark/src/build_system/bazel.rs @@ -8,7 +8,7 @@ use crate::build_system::BuildSystem; #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct BazelBuildSystem { workspace_name: Option, - output_base: PathBuf, + external_output_base: PathBuf, } impl BazelBuildSystem { @@ -49,7 +49,7 @@ impl BazelBuildSystem { name if name == Self::DEFAULT_WORKSPACE_NAME => None, name => Some(name), }, - output_base: PathBuf::from(output_base), + external_output_base: PathBuf::from(output_base).join("external"), }) } else { None @@ -63,8 +63,22 @@ impl BuildSystem for BazelBuildSystem { } fn repository_path(&self, repository_name: &str) -> Option> { - let mut path = self.output_base.join("external"); - path.push(repository_name); + let path = self.external_output_base.join(repository_name); Some(Cow::Owned(path)) } + + fn repository_for_path<'a>(&'a self, path: &'a Path) -> Option<(Cow<'a, str>, &'a Path)> { + if let Ok(path) = path.strip_prefix(&self.external_output_base) { + let mut path_components = path.components(); + + let repository_name = path_components.next()?.as_os_str().to_string_lossy(); + dbg!(&repository_name); + let repository_path = path_components.as_path(); + dbg!(&repository_path); + + Some((repository_name, repository_path)) + } else { + None + } + } } diff --git a/starlark/src/build_system/buck2.rs b/starlark/src/build_system/buck2.rs index 287ed0f54..ddaed0804 100644 --- a/starlark/src/build_system/buck2.rs +++ b/starlark/src/build_system/buck2.rs @@ -56,4 +56,18 @@ impl BuildSystem for Buck2BuildSystem { .get(repository_name) .map(|path| Cow::Borrowed(path.as_path())) } + + fn repository_for_path<'a>(&'a self, path: &'a Path) -> Option<(Cow<'a, str>, &'a Path)> { + self.repositories + .iter() + .find_map(|(name, repository_path)| { + path.strip_prefix(repository_path) + .ok() + .map(|stripped_path| (Cow::Borrowed(name.as_str()), stripped_path)) + }) + } + + fn should_use_at_sign_before_repository_name(&self) -> bool { + false + } } diff --git a/starlark/src/build_system/mod.rs b/starlark/src/build_system/mod.rs index cf835e921..cdb32d844 100644 --- a/starlark/src/build_system/mod.rs +++ b/starlark/src/build_system/mod.rs @@ -26,6 +26,17 @@ pub trait BuildSystem: std::fmt::Debug + Send + Sync { /// Returns the path of the repository with the given name. fn repository_path(&self, repository_name: &str) -> Option>; + + /// Given a path, tries to resolve the repository name and the path + /// relative to the root of repository. Returns `None` if the path is not + /// part of a known repository. + fn repository_for_path<'a>(&'a self, path: &'a Path) -> Option<(Cow<'a, str>, &'a Path)>; + + /// Whether to prefix absolute paths with `@` when that path contains a + /// repository name. + fn should_use_at_sign_before_repository_name(&self) -> bool { + true + } } /// Tries to resolve the build system from the current working directory. From 14cb7315d28505c0a0a4b658951294ce26ad065a Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 27 Jun 2023 12:35:15 +0200 Subject: [PATCH 30/41] feat: jump to definition for labels in build files --- starlark/bin/eval.rs | 77 +++++++++++++++++++++++------- starlark/src/build_system/bazel.rs | 4 ++ starlark/src/build_system/buck2.rs | 4 ++ starlark/src/build_system/mod.rs | 3 ++ starlark/src/lsp/server.rs | 19 +++++--- 5 files changed, 84 insertions(+), 23 deletions(-) diff --git a/starlark/bin/eval.rs b/starlark/bin/eval.rs index 65c6fb41a..8f61d01bf 100644 --- a/starlark/bin/eval.rs +++ b/starlark/bin/eval.rs @@ -107,6 +107,9 @@ enum ResolveLoadError { /// The path contained a repository name that is not known to the build system. #[error("Cannot resolve path `{}` because the repository `{}` is unknown", .0, .1)] UnknownRepository(String, String), + /// The path contained a target name that does not resolve to an existing file. + #[error("Cannot resolve path `{}` because the file does not exist", .0)] + TargetNotFound(String), } /// Errors when [`LspContext::render_as_load()`] cannot render a given path. @@ -335,6 +338,32 @@ impl LspContext for Context { current_file: &LspUrl, workspace_root: Option<&Path>, ) -> anyhow::Result { + let try_file = |folder: PathBuf, + presumed_filename: &str, + build_system: Option<&dyn BuildSystem>| + -> anyhow::Result { + // Try the presumed filename first, and check if it exists. + { + let path = folder.join(presumed_filename); + if path.exists() { + return Ok(Url::from_file_path(path).unwrap().try_into()?); + } + } + + // If the presumed filename doesn't exist, try to find a build file from the build system + // and use that instead. + if let Some(build_system) = build_system { + for build_file_name in build_system.get_build_file_names() { + let path = folder.join(build_file_name); + if path.exists() { + return Ok(Url::from_file_path(path).unwrap().try_into()?); + } + } + } + + Err(ResolveLoadError::TargetNotFound(path.to_owned()).into()) + }; + let original_path = path; if let Some((repository, path)) = path.split_once("//") { // The repository may be prefixed with an '@', but it's optional in Buck2. @@ -399,12 +428,11 @@ impl LspContext for Context { // Resolve from the root of the repository. match (path.split_once(':'), resolve_root) { - (Some((subfolder, filename)), Some(resolve_root)) => { - let mut result = resolve_root.into_owned(); - result.push(subfolder); - result.push(filename); - Ok(Url::from_file_path(result).unwrap().try_into()?) - } + (Some((subfolder, filename)), Some(resolve_root)) => try_file( + resolve_root.join(subfolder), + filename, + self.build_system.as_deref(), + ), (Some(_), None) => { Err(ResolveLoadError::MissingWorkspaceRoot(original_path.to_owned()).into()) } @@ -417,16 +445,16 @@ impl LspContext for Context { match current_file { LspUrl::File(current_file_path) => { let current_file_dir = current_file_path.parent(); - let absolute_path = match current_file_dir { - Some(current_file_dir) => { - let mut result = current_file_dir.to_owned(); - result.push(folder); - result.push(filename); - Ok(result) + match current_file_dir { + Some(current_file_dir) => Ok(try_file( + current_file_dir.join(folder), + filename, + self.build_system.as_deref(), + )?), + None => { + Err(ResolveLoadError::MissingCurrentFilePath(path.to_owned()).into()) } - None => Err(ResolveLoadError::MissingCurrentFilePath(path.to_owned())), - }?; - Ok(Url::from_file_path(absolute_path).unwrap().try_into()?) + } } _ => Err( ResolveLoadError::WrongScheme("file://".to_owned(), current_file.clone()) @@ -521,9 +549,24 @@ impl LspContext for Context { ) -> anyhow::Result> { self.resolve_load(literal, current_file, workspace_root) .map(|url| { + let original_target_name = Path::new(literal).file_name(); + let path_file_name = url.path().file_name(); + let same_filename = original_target_name == path_file_name; + Some(StringLiteralResult { - url, - location_finder: None, + url: url.clone(), + // If the target name is the same as the original target name, we don't need to + // do anything. Otherwise, we need to find the function call in the target file + // that has a `name` parameter with the same value as the original target name. + location_finder: if same_filename { + None + } else { + Some(Box::new(|ast, name, _| { + Ok(ast + .find_function_call_with_name(name) + .map(|resolved_span| resolved_span.into())) + })) + }, }) }) } diff --git a/starlark/src/build_system/bazel.rs b/starlark/src/build_system/bazel.rs index 2ea1e0a6c..06a83d5ed 100644 --- a/starlark/src/build_system/bazel.rs +++ b/starlark/src/build_system/bazel.rs @@ -81,4 +81,8 @@ impl BuildSystem for BazelBuildSystem { None } } + + fn get_build_file_names(&self) -> Vec<&str> { + vec!["BUILD", "BUILD.bazel"] + } } diff --git a/starlark/src/build_system/buck2.rs b/starlark/src/build_system/buck2.rs index ddaed0804..fca189ed5 100644 --- a/starlark/src/build_system/buck2.rs +++ b/starlark/src/build_system/buck2.rs @@ -70,4 +70,8 @@ impl BuildSystem for Buck2BuildSystem { fn should_use_at_sign_before_repository_name(&self) -> bool { false } + + fn get_build_file_names(&self) -> Vec<&str> { + vec!["BUCK"] + } } diff --git a/starlark/src/build_system/mod.rs b/starlark/src/build_system/mod.rs index cdb32d844..f78d0bb3d 100644 --- a/starlark/src/build_system/mod.rs +++ b/starlark/src/build_system/mod.rs @@ -32,6 +32,9 @@ pub trait BuildSystem: std::fmt::Debug + Send + Sync { /// part of a known repository. fn repository_for_path<'a>(&'a self, path: &'a Path) -> Option<(Cow<'a, str>, &'a Path)>; + /// Get valid build file names for this build system. + fn get_build_file_names(&self) -> Vec<&str>; + /// Whether to prefix absolute paths with `@` when that path contains a /// repository name. fn should_use_at_sign_before_repository_name(&self) -> bool { diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 1f81d693e..f91796fd6 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -246,7 +246,7 @@ pub struct StringLiteralResult { /// If `None`, then just jump to the URL. Do not attempt to load the file. #[derivative(Debug = "ignore")] pub location_finder: - Option anyhow::Result> + Send>>, + Option anyhow::Result> + Send>>, } fn _assert_string_literal_result_is_send() { @@ -570,10 +570,10 @@ impl Backend { } } IdentifierDefinition::StringLiteral { literal, .. } => { - let literal = self - .context - .resolve_string_literal(&literal, uri, workspace_root)?; - match literal { + let resolved_literal = + self.context + .resolve_string_literal(&literal, uri, workspace_root)?; + match resolved_literal { Some(StringLiteralResult { url, location_finder: Some(location_finder), @@ -583,7 +583,14 @@ impl Backend { let result = self.get_ast_or_load_from_disk(&url) .and_then(|ast| match ast { - Some(module) => location_finder(&module.ast, &url), + Some(module) => location_finder( + &module.ast, + literal + .split_once(':') + .map(|(_, rest)| rest) + .unwrap_or_default(), + &url, + ), None => Ok(None), }); if let Err(e) = &result { From bacf4d0fe345d156f09e2101528ade871ec668a4 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 27 Jun 2023 14:38:10 +0200 Subject: [PATCH 31/41] chore: move doc item extraction from AST into its own file --- starlark/src/syntax/docs.rs | 56 ++++++++++++++++++++++++++++++++++ starlark/src/syntax/mod.rs | 3 +- starlark/src/syntax/symbols.rs | 50 +----------------------------- 3 files changed, 59 insertions(+), 50 deletions(-) create mode 100644 starlark/src/syntax/docs.rs diff --git a/starlark/src/syntax/docs.rs b/starlark/src/syntax/docs.rs new file mode 100644 index 000000000..6c14c2380 --- /dev/null +++ b/starlark/src/syntax/docs.rs @@ -0,0 +1,56 @@ +use crate::docs::DocFunction; +use crate::docs::DocParam; +use crate::docs::DocStringKind; +use crate::syntax::ast::AstLiteral; +use crate::syntax::ast::AstPayload; +use crate::syntax::ast::AstStmtP; +use crate::syntax::ast::DefP; +use crate::syntax::ast::ExprP; +use crate::syntax::ast::ParameterP; +use crate::syntax::ast::StmtP; + +/// Given the AST node for a `def` statement, return a `DocFunction` if the +/// `def` statement has a docstring as its first statement. +pub(crate) fn get_doc_item_for_def(def: &DefP

) -> Option { + if let Some(doc_string) = peek_docstring(&def.body) { + let args: Vec<_> = def + .params + .iter() + .filter_map(|param| match ¶m.node { + ParameterP::Normal(p, _) + | ParameterP::WithDefaultValue(p, _, _) + | ParameterP::Args(p, _) + | ParameterP::KwArgs(p, _) => Some(DocParam::Arg { + name: p.0.to_owned(), + docs: None, + typ: None, + default_value: None, + }), + _ => None, + }) + .collect(); + + let doc_function = DocFunction::from_docstring( + DocStringKind::Starlark, + args, + // TODO: Figure out how to get a `Ty` from the `def.return_type`. + None, + Some(doc_string), + None, + ); + Some(doc_function) + } else { + None + } +} + +fn peek_docstring(stmt: &AstStmtP

) -> Option<&str> { + match &stmt.node { + StmtP::Statements(stmts) => stmts.first().and_then(peek_docstring), + StmtP::Expression(expr) => match &expr.node { + ExprP::Literal(AstLiteral::String(s)) => Some(s.node.as_str()), + _ => None, + }, + _ => None, + } +} diff --git a/starlark/src/syntax/mod.rs b/starlark/src/syntax/mod.rs index 2167999c1..1bbbabe0c 100644 --- a/starlark/src/syntax/mod.rs +++ b/starlark/src/syntax/mod.rs @@ -25,6 +25,7 @@ pub use parser::AstLoad; pub(crate) mod ast; pub(crate) mod cursors; mod dialect; +pub(crate) mod docs; #[cfg(test)] mod grammar_tests; pub(crate) mod lexer; @@ -33,9 +34,9 @@ mod lexer_tests; pub(crate) mod module; pub(crate) mod parser; pub(crate) mod payload_map; +pub(crate) mod symbols; #[cfg(test)] mod testcases; -pub(crate) mod symbols; pub(crate) mod type_expr; pub(crate) mod uniplate; pub(crate) mod validate; diff --git a/starlark/src/syntax/symbols.rs b/starlark/src/syntax/symbols.rs index 850f519eb..e3a73177d 100644 --- a/starlark/src/syntax/symbols.rs +++ b/starlark/src/syntax/symbols.rs @@ -1,18 +1,14 @@ use std::collections::HashMap; -use super::ast::DefP; use crate::codemap::CodeMap; use crate::codemap::LineCol; -use crate::docs::DocFunction; use crate::docs::DocItem; -use crate::docs::DocParam; -use crate::docs::DocStringKind; -use crate::syntax::ast::AstLiteral; use crate::syntax::ast::AstPayload; use crate::syntax::ast::AstStmtP; use crate::syntax::ast::ExprP; use crate::syntax::ast::ParameterP; use crate::syntax::ast::StmtP; +use crate::syntax::docs::get_doc_item_for_def; #[derive(Debug, PartialEq)] pub(crate) enum SymbolKind { @@ -135,50 +131,6 @@ pub(crate) fn find_symbols_at_location( symbols } -fn get_doc_item_for_def(def: &DefP

) -> Option { - if let Some(doc_string) = peek_docstring(&def.body) { - let args: Vec<_> = def - .params - .iter() - .filter_map(|param| match ¶m.node { - ParameterP::Normal(p, _) - | ParameterP::WithDefaultValue(p, _, _) - | ParameterP::Args(p, _) - | ParameterP::KwArgs(p, _) => Some(DocParam::Arg { - name: p.0.to_owned(), - docs: None, - typ: None, - default_value: None, - }), - _ => None, - }) - .collect(); - - let doc_function = DocFunction::from_docstring( - DocStringKind::Starlark, - args, - // TODO: Figure out how to get a `Ty` from the `def.return_type`. - None, - Some(doc_string), - None, - ); - Some(doc_function) - } else { - None - } -} - -fn peek_docstring(stmt: &AstStmtP

) -> Option<&str> { - match &stmt.node { - StmtP::Statements(stmts) => stmts.first().and_then(peek_docstring), - StmtP::Expression(expr) => match &expr.node { - ExprP::Literal(AstLiteral::String(s)) => Some(s.node.as_str()), - _ => None, - }, - _ => None, - } -} - #[cfg(test)] mod tests { use std::collections::HashMap; From 2a6128468c3d0218af849b9157353878db9115ce Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 27 Jun 2023 15:15:05 +0200 Subject: [PATCH 32/41] feat: show documentation for completion options exported from open files --- starlark/src/analysis/exported.rs | 29 ++++++++++++++++++++++++----- starlark/src/lsp/server.rs | 6 ++++++ starlark/src/syntax/docs.rs | 16 ++++++++++++++++ 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/starlark/src/analysis/exported.rs b/starlark/src/analysis/exported.rs index 516d9df47..b3264638d 100644 --- a/starlark/src/analysis/exported.rs +++ b/starlark/src/analysis/exported.rs @@ -19,10 +19,13 @@ use dupe::Dupe; use crate::codemap::FileSpan; use crate::collections::SmallMap; +use crate::docs::DocItem; use crate::syntax::ast::AstAssignIdent; use crate::syntax::ast::DefP; use crate::syntax::ast::Expr; use crate::syntax::ast::Stmt; +use crate::syntax::docs::get_doc_item_for_assign; +use crate::syntax::docs::get_doc_item_for_def; use crate::syntax::AstModule; /// The type of an exported symbol. @@ -46,7 +49,7 @@ impl SymbolKind { } /// A symbol. Returned from [`AstModule::exported_symbols`]. -#[derive(Debug, PartialEq, Eq, Clone, Dupe, Hash)] +#[derive(Debug, PartialEq, Clone)] pub struct Symbol<'a> { /// The name of the symbol. pub name: &'a str, @@ -54,6 +57,8 @@ pub struct Symbol<'a> { pub span: FileSpan, /// The type of symbol it represents. pub kind: SymbolKind, + /// The documentation for this symbol. + pub docs: Option, } impl AstModule { @@ -69,34 +74,48 @@ impl AstModule { result: &mut SmallMap<&'a str, Symbol<'a>>, name: &'a AstAssignIdent, kind: SymbolKind, + resolve_docs: impl FnOnce() -> Option, ) { if !name.0.starts_with('_') { result.entry(&name.0).or_insert(Symbol { name: &name.0, span: me.file_span(name.span), kind, + docs: resolve_docs(), }); } } + let mut last_node = None; for x in self.top_level_statements() { match &**x { Stmt::Assign(dest, rhs) => { dest.visit_lvalue(|name| { let kind = SymbolKind::from_expr(&rhs.1); - add(self, &mut result, name, kind); + add(self, &mut result, name, kind, || { + last_node + .and_then(|last| get_doc_item_for_assign(last, dest)) + .map(DocItem::Property) + }); }); } Stmt::AssignModify(dest, _, _) => { dest.visit_lvalue(|name| { - add(self, &mut result, name, SymbolKind::Any); + add(self, &mut result, name, SymbolKind::Any, || { + last_node + .and_then(|last| get_doc_item_for_assign(last, dest)) + .map(DocItem::Property) + }); }); } - Stmt::Def(DefP { name, .. }) => { - add(self, &mut result, name, SymbolKind::Function); + Stmt::Def(def) => { + add(self, &mut result, &def.name, SymbolKind::Function, || { + get_doc_item_for_def(def).map(DocItem::Function) + }); } _ => {} } + last_node = Some(x); } result.into_values().collect() } diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index f91796fd6..665e7de05 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -830,6 +830,12 @@ impl Backend { ExportedSymbolKind::Any => CompletionItemKind::CONSTANT, ExportedSymbolKind::Function => CompletionItemKind::METHOD, }), + documentation: symbol.docs.map(|docs| { + Documentation::MarkupContent(MarkupContent { + kind: MarkupKind::Markdown, + value: render_doc_item(symbol.name, &docs), + }) + }), additional_text_edits: Some(vec![format_text_edit(&load_path, symbol.name)]), ..Default::default() }) diff --git a/starlark/src/syntax/docs.rs b/starlark/src/syntax/docs.rs index 6c14c2380..5b9a76ad9 100644 --- a/starlark/src/syntax/docs.rs +++ b/starlark/src/syntax/docs.rs @@ -1,6 +1,9 @@ use crate::docs::DocFunction; use crate::docs::DocParam; +use crate::docs::DocProperty; +use crate::docs::DocString; use crate::docs::DocStringKind; +use crate::syntax::ast::AstAssignP; use crate::syntax::ast::AstLiteral; use crate::syntax::ast::AstPayload; use crate::syntax::ast::AstStmtP; @@ -44,6 +47,19 @@ pub(crate) fn get_doc_item_for_def(def: &DefP

) -> Option( + previous_node: &AstStmtP

, + _assign: &AstAssignP

, +) -> Option { + peek_docstring(previous_node).map(|doc_string| { + DocProperty { + docs: DocString::from_docstring(DocStringKind::Starlark, doc_string), + // TODO: Can constants have a type? + typ: None, + } + }) +} + fn peek_docstring(stmt: &AstStmtP

) -> Option<&str> { match &stmt.node { StmtP::Statements(stmts) => stmts.first().and_then(peek_docstring), From 88824f27e85c8ba8c6dd1c0880bba535d623c762 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 27 Jun 2023 17:15:39 +0200 Subject: [PATCH 33/41] feat: implement rudimentary hover support --- starlark/src/analysis/definition.rs | 21 +++-- starlark/src/analysis/exported.rs | 1 - starlark/src/lsp/server.rs | 122 ++++++++++++++++++++++++++-- 3 files changed, 127 insertions(+), 17 deletions(-) diff --git a/starlark/src/analysis/definition.rs b/starlark/src/analysis/definition.rs index 6c8405580..13c2acc0c 100644 --- a/starlark/src/analysis/definition.rs +++ b/starlark/src/analysis/definition.rs @@ -266,7 +266,7 @@ impl LspModule { /// accessed at Pos is defined. fn find_definition_in_scope<'a>(scope: &'a Scope, pos: Pos) -> TempDefinition<'a> { /// Look for a name in the given scope, with a given source, and return the right - /// type of `TempIdentifierDefinition` based on whether / how the variable is bound. + /// type of [`TempIdentifierDefinition`] based on whether / how the variable is bound. fn resolve_get_in_scope<'a>( scope: &'a Scope, name: &'a str, @@ -429,15 +429,18 @@ impl LspModule { self.ast.loaded_symbols() } + /// Attempt to find an exported symbol with the given name. + pub(crate) fn find_exported_symbol(&self, name: &str) -> Option { + self.ast + .exported_symbols() + .into_iter() + .find(|symbol| symbol.name == name) + } + /// Attempt to find the location in this module where an exported symbol is defined. - pub(crate) fn find_exported_symbol(&self, name: &str) -> Option { - self.ast.exported_symbols().iter().find_map(|symbol| { - if symbol.name == name { - Some(symbol.span.resolve_span()) - } else { - None - } - }) + pub(crate) fn find_exported_symbol_span(&self, name: &str) -> Option { + self.find_exported_symbol(name) + .map(|symbol| symbol.span.resolve_span()) } /// Attempt to find the location in this module where a member of a struct (named `name`) diff --git a/starlark/src/analysis/exported.rs b/starlark/src/analysis/exported.rs index b3264638d..45047ab8e 100644 --- a/starlark/src/analysis/exported.rs +++ b/starlark/src/analysis/exported.rs @@ -21,7 +21,6 @@ use crate::codemap::FileSpan; use crate::collections::SmallMap; use crate::docs::DocItem; use crate::syntax::ast::AstAssignIdent; -use crate::syntax::ast::DefP; use crate::syntax::ast::Expr; use crate::syntax::ast::Stmt; use crate::syntax::docs::get_doc_item_for_assign; diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 665e7de05..81a494728 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -44,6 +44,7 @@ use lsp_types::notification::LogMessage; use lsp_types::notification::PublishDiagnostics; use lsp_types::request::Completion; use lsp_types::request::GotoDefinition; +use lsp_types::request::HoverRequest; use lsp_types::CompletionItem; use lsp_types::CompletionItemKind; use lsp_types::CompletionOptions; @@ -57,9 +58,14 @@ use lsp_types::DidOpenTextDocumentParams; use lsp_types::Documentation; use lsp_types::GotoDefinitionParams; use lsp_types::GotoDefinitionResponse; +use lsp_types::Hover; +use lsp_types::HoverContents; +use lsp_types::HoverParams; +use lsp_types::HoverProviderCapability; use lsp_types::InitializeParams; use lsp_types::LocationLink; use lsp_types::LogMessageParams; +use lsp_types::MarkedString; use lsp_types::MarkupContent; use lsp_types::MarkupKind; use lsp_types::MessageType; @@ -309,7 +315,7 @@ pub trait LspContext { workspace_root: Option<&Path>, ) -> anyhow::Result; - /// Resolve a string literal into a Url and a function that specifies a locaction within that + /// Resolve a string literal into a Url and a function that specifies a location within that /// target file. /// /// This can be used for things like file paths in string literals, build targets, etc. @@ -393,6 +399,7 @@ impl Backend { ]), ..Default::default() }), + hover_provider: Some(HoverProviderCapability::Simple(true)), ..ServerCapabilities::default() } } @@ -483,6 +490,11 @@ impl Backend { )); } + /// Offers hover information for the symbol at the current cursor. + fn hover(&self, id: RequestId, params: HoverParams, initialize_params: &InitializeParams) { + self.send_response(new_response(id, self.hover_info(params, initialize_params))); + } + /// Get the file contents of a starlark: URI. fn get_starlark_file_contents(&self, id: RequestId, params: StarlarkFileContentsParams) { let response: anyhow::Result<_> = match params.uri { @@ -553,7 +565,7 @@ impl Backend { self.get_ast_or_load_from_disk(&load_uri)? .and_then(|ast| match member { Some(member) => ast.find_exported_symbol_and_member(&name, member), - None => ast.find_exported_symbol(&name), + None => ast.find_exported_symbol_span(&name), }); match loaded_location { None => Self::location_link(source, uri, location)?, @@ -615,7 +627,7 @@ impl Backend { Some(member) => { ast.find_exported_symbol_and_member(&name, member) } - None => ast.find_exported_symbol(&name), + None => ast.find_exported_symbol_span(&name), }); Self::location_link(source, &uri, loaded_location.unwrap_or_default())? @@ -955,10 +967,7 @@ impl Backend { // inserts a new load statement after the last one we found. TextEdit::new( match last_load { - Some(span) => Range::new( - Position::new(span.end_line as u32, span.end_column as u32), - Position::new(span.end_line as u32, span.end_column as u32), - ), + Some(span) => span.into(), None => Range::new(Position::new(0, 0), Position::new(0, 0)), }, format!( @@ -989,6 +998,103 @@ impl Backend { }) } + fn hover_info( + &self, + params: HoverParams, + initialize_params: &InitializeParams, + ) -> anyhow::Result { + let uri = params + .text_document_position_params + .text_document + .uri + .try_into()?; + let line = params.text_document_position_params.position.line; + let character = params.text_document_position_params.position.character; + let workspace_root = + Self::get_workspace_root(initialize_params.workspace_folders.as_ref(), &uri); + + // Return an empty result as a "not found" + let not_found = Hover { + contents: HoverContents::Array(vec![]), + range: None, + }; + + Ok(match self.get_ast(&uri) { + Some(ast) => { + let location = ast.find_definition(line, character); + match location { + Definition::Identifier(identifier_definition) => self + .get_hover_for_identifier_definition( + identifier_definition, + &uri, + workspace_root.as_deref(), + )?, + Definition::Dotted(DottedDefinition { + root_definition_location, + .. + }) => { + // Not something we really support yet, so just provide hover information for + // the root definition. + self.get_hover_for_identifier_definition( + root_definition_location, + &uri, + workspace_root.as_deref(), + )? + } + } + .unwrap_or(not_found) + } + None => not_found, + }) + } + + fn get_hover_for_identifier_definition( + &self, + identifier_definition: IdentifierDefinition, + current_uri: &LspUrl, + workspace_root: Option<&Path>, + ) -> anyhow::Result> { + Ok(match identifier_definition { + IdentifierDefinition::LoadedLocation { + path, name, source, .. + } => { + // Symbol loaded from another file. Find the file and get the definition + // from there, hopefully including the docs. + let load_uri = self.resolve_load_path(&path, current_uri, workspace_root)?; + self.get_ast_or_load_from_disk(&load_uri)?.and_then(|ast| { + ast.find_exported_symbol(&name).and_then(|symbol| { + symbol.docs.map(|docs| Hover { + contents: HoverContents::Array(vec![MarkedString::String( + render_doc_item(symbol.name, &docs), + )]), + range: Some(source.into()), + }) + }) + }) + } + IdentifierDefinition::Unresolved { source, name } => { + // Try to resolve as a global symbol. + self.context + .get_global_symbols() + .into_iter() + .find(|symbol| symbol.name == name) + .and_then(|symbol| { + symbol.documentation.map(|docs| Hover { + contents: HoverContents::Array(vec![MarkedString::String( + render_doc_item(symbol.name.clone(), &docs), + )]), + range: Some(source.into()), + }) + }) + } + IdentifierDefinition::NotFound => None, + _ => { + // Don't support other cases just yet. + None + } + }) + } + fn get_workspace_root( workspace_roots: Option<&Vec>, target: &LspUrl, @@ -1044,6 +1150,8 @@ impl Backend { self.get_starlark_file_contents(req.id, params); } else if let Some(params) = as_request::(&req) { self.completion(req.id, params, &initialize_params); + } else if let Some(params) = as_request::(&req) { + self.hover(req.id, params, &initialize_params); } else if self.connection.handle_shutdown(&req)? { return Ok(()); } From 78cd4a64af8c342430bb574a5eb21817469b0c8a Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Tue, 27 Jun 2023 17:57:37 +0200 Subject: [PATCH 34/41] test: make tests pass again --- starlark/bin/eval.rs | 19 ++++++++----------- starlark/src/lsp/test.rs | 2 +- starlark/testcases/resolve/baz/root.star | 0 starlark/testcases/resolve/from.star | 0 starlark/testcases/resolve/relative.star | 0 starlark/testcases/resolve/root.star | 0 .../testcases/resolve/subpath/relative.star | 0 7 files changed, 9 insertions(+), 12 deletions(-) create mode 100644 starlark/testcases/resolve/baz/root.star create mode 100644 starlark/testcases/resolve/from.star create mode 100644 starlark/testcases/resolve/relative.star create mode 100644 starlark/testcases/resolve/root.star create mode 100644 starlark/testcases/resolve/subpath/relative.star diff --git a/starlark/bin/eval.rs b/starlark/bin/eval.rs index 8f61d01bf..5f9a37bf0 100644 --- a/starlark/bin/eval.rs +++ b/starlark/bin/eval.rs @@ -618,36 +618,33 @@ mod tests { #[test] fn resolve_load() { let context = make_context(); + let root = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("testcases/resolve"); // Successful cases - let current_file = LspUrl::File(PathBuf::from("/absolute/path/from.star")); + let current_file = LspUrl::File(root.join("from.star")); assert_eq!( context .resolve_load(":relative.star", ¤t_file, None,) .unwrap(), - LspUrl::File(PathBuf::from("/absolute/path/relative.star")) + LspUrl::File(root.join("relative.star")) ); assert_eq!( context .resolve_load("subpath:relative.star", ¤t_file, None) .unwrap(), - LspUrl::File(PathBuf::from("/absolute/path/subpath/relative.star")) + LspUrl::File(root.join("subpath/relative.star")) ); assert_eq!( context - .resolve_load("//:root.star", ¤t_file, Some(Path::new("/foo/bar")),) + .resolve_load("//:root.star", ¤t_file, Some(root.as_path()),) .unwrap(), - LspUrl::File(PathBuf::from("/foo/bar/root.star")) + LspUrl::File(root.join("root.star")) ); assert_eq!( context - .resolve_load( - "//baz:root.star", - ¤t_file, - Some(Path::new("/foo/bar")), - ) + .resolve_load("//baz:root.star", ¤t_file, Some(root.as_path()),) .unwrap(), - LspUrl::File(PathBuf::from("/foo/bar/baz/root.star")) + LspUrl::File(root.join("baz/root.star")) ); // Error cases diff --git a/starlark/src/lsp/test.rs b/starlark/src/lsp/test.rs index c92189076..db8957a1c 100644 --- a/starlark/src/lsp/test.rs +++ b/starlark/src/lsp/test.rs @@ -254,7 +254,7 @@ impl LspContext for TestServerContext { LspUrl::File(u) => match u.extension() { Some(e) if e == "star" => Some(StringLiteralResult { url, - location_finder: Some(Box::new(move |_ast, _url| Ok(range))), + location_finder: Some(Box::new(move |_ast, _name, _url| Ok(range))), }), _ => Some(StringLiteralResult { url, diff --git a/starlark/testcases/resolve/baz/root.star b/starlark/testcases/resolve/baz/root.star new file mode 100644 index 000000000..e69de29bb diff --git a/starlark/testcases/resolve/from.star b/starlark/testcases/resolve/from.star new file mode 100644 index 000000000..e69de29bb diff --git a/starlark/testcases/resolve/relative.star b/starlark/testcases/resolve/relative.star new file mode 100644 index 000000000..e69de29bb diff --git a/starlark/testcases/resolve/root.star b/starlark/testcases/resolve/root.star new file mode 100644 index 000000000..e69de29bb diff --git a/starlark/testcases/resolve/subpath/relative.star b/starlark/testcases/resolve/subpath/relative.star new file mode 100644 index 000000000..e69de29bb From 1415d9e7a0dbd1261f26ea5620e6e5fc1fed2f87 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Fri, 30 Jun 2023 09:55:05 +0200 Subject: [PATCH 35/41] feat: provide argument names for exported function symbols --- starlark/src/analysis/exported.rs | 73 +++++++++++++++++++++++++------ starlark/src/lsp/symbols.rs | 30 +++++++++++-- 2 files changed, 87 insertions(+), 16 deletions(-) diff --git a/starlark/src/analysis/exported.rs b/starlark/src/analysis/exported.rs index 45047ab8e..9b7bc3633 100644 --- a/starlark/src/analysis/exported.rs +++ b/starlark/src/analysis/exported.rs @@ -15,10 +15,15 @@ * limitations under the License. */ -use dupe::Dupe; +use lsp_types::CompletionItem; +use lsp_types::CompletionItemKind; +use lsp_types::Documentation; +use lsp_types::MarkupContent; +use lsp_types::MarkupKind; use crate::codemap::FileSpan; use crate::collections::SmallMap; +use crate::docs::render_doc_item; use crate::docs::DocItem; use crate::syntax::ast::AstAssignIdent; use crate::syntax::ast::Expr; @@ -29,29 +34,44 @@ use crate::syntax::AstModule; /// The type of an exported symbol. /// If unknown, will use `Any`. -#[derive(Debug, PartialEq, Eq, Copy, Clone, Dupe, Hash)] +#[derive(Debug, PartialEq, Eq, Clone, Hash)] pub enum SymbolKind { /// Any kind of symbol. Any, /// The symbol represents something that can be called, for example /// a `def` or a variable assigned to a `lambda`. - Function, + Function { argument_names: Vec }, } impl SymbolKind { pub(crate) fn from_expr(x: &Expr) -> Self { match x { - Expr::Lambda(..) => Self::Function, + Expr::Lambda(lambda) => Self::Function { + argument_names: lambda + .params + .iter() + .filter_map(|param| param.split().0.map(|name| name.to_string())) + .collect(), + }, _ => Self::Any, } } } +impl From for CompletionItemKind { + fn from(value: SymbolKind) -> Self { + match value { + SymbolKind::Any => CompletionItemKind::CONSTANT, + SymbolKind::Function { .. } => CompletionItemKind::FUNCTION, + } + } +} + /// A symbol. Returned from [`AstModule::exported_symbols`]. #[derive(Debug, PartialEq, Clone)] -pub struct Symbol<'a> { +pub struct Symbol { /// The name of the symbol. - pub name: &'a str, + pub name: String, /// The location of its definition. pub span: FileSpan, /// The type of symbol it represents. @@ -60,24 +80,41 @@ pub struct Symbol<'a> { pub docs: Option, } +impl From for CompletionItem { + fn from(value: Symbol) -> Self { + let documentation = value.docs.map(|docs| { + Documentation::MarkupContent(MarkupContent { + kind: MarkupKind::Markdown, + value: render_doc_item(&value.name, &docs), + }) + }); + Self { + label: value.name, + kind: Some(value.kind.into()), + documentation, + ..Default::default() + } + } +} + impl AstModule { /// Which symbols are exported by this module. These are the top-level assignments, /// including function definitions. Any symbols that start with `_` are not exported. - pub fn exported_symbols<'a>(&'a self) -> Vec> { + pub fn exported_symbols(&self) -> Vec { // Map since we only want to store the first of each export // IndexMap since we want the order to match the order they were defined in - let mut result: SmallMap<&'a str, _> = SmallMap::new(); + let mut result: SmallMap<&str, _> = SmallMap::new(); fn add<'a>( me: &AstModule, - result: &mut SmallMap<&'a str, Symbol<'a>>, + result: &mut SmallMap<&'a str, Symbol>, name: &'a AstAssignIdent, kind: SymbolKind, resolve_docs: impl FnOnce() -> Option, ) { if !name.0.starts_with('_') { result.entry(&name.0).or_insert(Symbol { - name: &name.0, + name: name.0.to_string(), span: me.file_span(name.span), kind, docs: resolve_docs(), @@ -108,9 +145,19 @@ impl AstModule { }); } Stmt::Def(def) => { - add(self, &mut result, &def.name, SymbolKind::Function, || { - get_doc_item_for_def(def).map(DocItem::Function) - }); + add( + self, + &mut result, + &def.name, + SymbolKind::Function { + argument_names: def + .params + .iter() + .filter_map(|param| param.split().0.map(|name| name.to_string())) + .collect(), + }, + || get_doc_item_for_def(def).map(DocItem::Function), + ); } _ => {} } diff --git a/starlark/src/lsp/symbols.rs b/starlark/src/lsp/symbols.rs index 8df3aba49..98e7f7d59 100644 --- a/starlark/src/lsp/symbols.rs +++ b/starlark/src/lsp/symbols.rs @@ -85,7 +85,19 @@ pub(crate) fn find_symbols_at_position<'a>( walk(codemap, position, body, top_level, symbols); } StmtP::Def(def) => { - add(symbols, top_level, &def.name, SymbolKind::Function, None); + add( + symbols, + top_level, + &def.name, + SymbolKind::Function { + argument_names: def + .params + .iter() + .filter_map(|param| param.split().0.map(|name| name.to_string())) + .collect(), + }, + None, + ); // Only recurse into method if the cursor is in it. if codemap.resolve_span(def.body.span).contains(position) { @@ -161,7 +173,13 @@ my_var = True vec![ sym("exported_a", SymbolKind::Any, Some("foo.star")), sym("renamed", SymbolKind::Any, Some("foo.star")), - sym("_method", SymbolKind::Function, None), + sym( + "_method", + SymbolKind::Function { + argument_names: vec!["param".to_owned()], + }, + None + ), sym("my_var", SymbolKind::Any, None), ] ); @@ -170,7 +188,13 @@ my_var = True inside_method, vec![ sym("param", SymbolKind::Any, None), - sym("x", SymbolKind::Function, None), + sym( + "x", + SymbolKind::Function { + argument_names: vec!["_".to_owned()] + }, + None + ), ] ); } From 8497455931b0347ad2bccc049cdc0b7f45b78b75 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Fri, 30 Jun 2023 09:57:11 +0200 Subject: [PATCH 36/41] feat: add module that inspects AST and decides type of autocomplete --- starlark/src/analysis/inspect.rs | 305 +++++++++++++++++++++++++++++++ starlark/src/analysis/mod.rs | 1 + 2 files changed, 306 insertions(+) create mode 100644 starlark/src/analysis/inspect.rs diff --git a/starlark/src/analysis/inspect.rs b/starlark/src/analysis/inspect.rs new file mode 100644 index 000000000..f4e538fe8 --- /dev/null +++ b/starlark/src/analysis/inspect.rs @@ -0,0 +1,305 @@ +use crate::codemap::CodeMap; +use crate::codemap::Pos; +use crate::codemap::ResolvedSpan; +use crate::codemap::Span; +use crate::syntax::ast::ArgumentP; +use crate::syntax::ast::AstExprP; +use crate::syntax::ast::AstLiteral; +use crate::syntax::ast::AstNoPayload; +use crate::syntax::ast::AstStmtP; +use crate::syntax::ast::ExprP; +use crate::syntax::ast::ParameterP; +use crate::syntax::ast::StmtP; +use crate::syntax::uniplate::Visit; +use crate::syntax::AstModule; + +#[derive(Debug, PartialEq, Eq, Clone, Hash)] +pub enum AutocompleteType { + /// Offer completions of all available symbol. Cursor is e.g. at the start of a line, + /// or in the right hand side of an assignment. + Default, + /// Offer completions of loadable modules. Cursor is in the module path of a load statement. + LoadPath { + current_value: String, + current_span: ResolvedSpan, + }, + /// Offer completions of symbols in a loaded module. Cursor is in a load statement, but + /// after the module path. + LoadSymbol { + path: String, + current_span: ResolvedSpan, + previously_loaded: Vec, + }, + /// Offer completions of target names. Cursor is in a literal string, but not in a load statement + /// or a visibility-like declaration. + String { + current_value: String, + current_span: ResolvedSpan, + }, + /// Offer completions of function parameters names. Cursor is in a function call. ALSO offer + /// regular symbol completions, since this might be e.g. a positional argument, in which cases + /// parameter names don't matter/help the user. + Parameter { + function_name: String, + function_name_span: ResolvedSpan, + }, + /// Offer completions of type names. + Type, + /// Don't offer any completions. Cursor is e.g. in a comment. + None, +} + +impl AstModule { + /// Walks through the AST to find the type of the expression at the given position. + /// Based on that, returns an enum that can be used to determine what kind of + /// autocomplete should be performed. For example, path in a `load` statement versus + /// a variable name. + pub fn get_auto_complete_type(&self, line: u32, col: u32) -> Option { + let line_span = match self.codemap.line_span_opt(line as usize) { + None => { + // The document got edited to add new lines, just bail out + return None; + } + Some(line_span) => line_span, + }; + let current_pos = std::cmp::min(line_span.begin() + col, line_span.end()); + + // Walk through the AST to find a node matching the current position. + fn walk_and_find_completion_type( + codemap: &CodeMap, + position: Pos, + stmt: Visit, + ) -> Option { + // Utility function to get the span of a string literal without the quotes. + fn string_span_without_quotes(codemap: &CodeMap, span: Span) -> ResolvedSpan { + let mut span = codemap.resolve_span(span); + span.begin_column += 1; + span.end_column -= 1; + span + } + + let span = match &stmt { + Visit::Stmt(stmt) => stmt.span, + Visit::Expr(expr) => expr.span, + }; + let contains_pos = span.contains(position); + if !contains_pos { + return None; + } + + match &stmt { + Visit::Stmt(AstStmtP { + node: StmtP::Assign(dest, rhs), + .. + }) => { + if dest.span.contains(position) { + return Some(AutocompleteType::None); + } + let (type_, expr) = &**rhs; + if let Some(type_) = type_ { + if type_.span.contains(position) { + return Some(AutocompleteType::Type); + } + } + if expr.span.contains(position) { + return walk_and_find_completion_type(codemap, position, Visit::Expr(expr)); + } + } + Visit::Stmt(AstStmtP { + node: StmtP::AssignModify(dest, _, expr), + .. + }) => { + if dest.span.contains(position) { + return Some(AutocompleteType::None); + } else if expr.span.contains(position) { + return walk_and_find_completion_type(codemap, position, Visit::Expr(expr)); + } + } + Visit::Stmt(AstStmtP { + node: StmtP::Load(load), + .. + }) => { + if load.module.span.contains(position) { + return Some(AutocompleteType::LoadPath { + current_value: load.module.to_string(), + current_span: string_span_without_quotes(codemap, load.module.span), + }); + } + + for (name, _) in &load.args { + if name.span.contains(position) { + return Some(AutocompleteType::LoadSymbol { + path: load.module.to_string(), + current_span: string_span_without_quotes(codemap, name.span), + previously_loaded: load + .args + .iter() + .filter(|(n, _)| n != name) + .map(|(n, _)| n.to_string()) + .collect(), + }); + } + } + + return Some(AutocompleteType::None); + } + Visit::Stmt(AstStmtP { + node: StmtP::Def(def), + .. + }) => { + // If the cursor is in the name of the function, don't offer any completions. + if def.name.span.contains(position) { + return Some(AutocompleteType::None); + } + // If the cursor is in one of the arguments, only offer completions for + // default values for the arguments. + for arg in def.params.iter() { + if !arg.span.contains(position) { + continue; + } + match &arg.node { + ParameterP::Normal(_, type_) => { + if let Some(type_) = type_ { + if type_.span.contains(position) { + return Some(AutocompleteType::Type); + } + } + } + ParameterP::WithDefaultValue(_, type_, expr) => { + if let Some(type_) = type_ { + if type_.span.contains(position) { + return Some(AutocompleteType::Type); + } + } + if expr.span.contains(position) { + return walk_and_find_completion_type( + codemap, + position, + Visit::Expr(expr), + ); + } + } + _ => {} + } + + return Some(AutocompleteType::None); + } + if let Some(return_type) = &def.return_type { + if return_type.span.contains(position) { + return Some(AutocompleteType::Type); + } + } + + return walk_and_find_completion_type( + codemap, + position, + Visit::Stmt(&def.body), + ); + } + Visit::Expr(AstExprP { + node: ExprP::Call(name, args), + span, + }) => { + if name.span.contains(position) { + return Some(AutocompleteType::Default); + } + for arg in args { + if !arg.span.contains(position) { + continue; + } + match &arg.node { + ArgumentP::Named(arg_name, value) => { + if arg_name.span.contains(position) { + return Some(AutocompleteType::Parameter { + function_name: name.to_string(), + function_name_span: codemap.resolve_span(name.span), + }); + } else if value.span.contains(position) { + return walk_and_find_completion_type( + codemap, + position, + Visit::Expr(value), + ); + } + } + ArgumentP::Positional(expr) => { + return match expr { + AstExprP { + node: ExprP::Identifier(_), + .. + } => { + // Typing a literal, might be meant as a parameter name. + Some(AutocompleteType::Parameter { + function_name: name.to_string(), + function_name_span: codemap.resolve_span(name.span), + }) + } + _ => walk_and_find_completion_type( + codemap, + position, + Visit::Expr(expr), + ), + }; + } + ArgumentP::Args(expr) | ArgumentP::KwArgs(expr) => { + return walk_and_find_completion_type( + codemap, + position, + Visit::Expr(expr), + ); + } + } + } + // No matches? We might be in between empty braces (new function call), + // e.g. `foo(|)`. However, we don't want to offer completions for + // when the cursor is at the very end of the function call, e.g. `foo()|`. + return Some(if args.is_empty() && span.end() != position { + AutocompleteType::Parameter { + function_name: name.to_string(), + function_name_span: codemap.resolve_span(name.span), + } + } else if !args.is_empty() { + AutocompleteType::Default + } else { + // Don't offer completions right after the function call. + AutocompleteType::None + }); + } + Visit::Expr(AstExprP { + node: ExprP::Literal(AstLiteral::String(str)), + .. + }) => { + return Some(AutocompleteType::String { + current_value: str.to_string(), + current_span: string_span_without_quotes(codemap, span), + }); + } + Visit::Stmt(stmt) => { + let mut result = None; + stmt.visit_children(|stmt| { + if let Some(r) = walk_and_find_completion_type(codemap, position, stmt) { + result = Some(r); + } + }); + return result; + } + Visit::Expr(expr) => { + let mut result = None; + expr.visit_expr(|expr| { + if let Some(r) = + walk_and_find_completion_type(codemap, position, Visit::Expr(expr)) + { + result = Some(r); + } + }); + return result; + } + } + + None + } + + walk_and_find_completion_type(&self.codemap, current_pos, Visit::Stmt(&self.statement)) + .or(Some(AutocompleteType::Default)) + } +} diff --git a/starlark/src/analysis/mod.rs b/starlark/src/analysis/mod.rs index 7b85927a1..b608ac0fa 100644 --- a/starlark/src/analysis/mod.rs +++ b/starlark/src/analysis/mod.rs @@ -31,6 +31,7 @@ pub(crate) mod exported; mod find_call_name; mod flow; mod incompatible; +pub(crate) mod inspect; pub(crate) mod loaded; mod names; mod performance; From e598992fa699086ddb9b2f8a3805d072fb7d4804 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Fri, 30 Jun 2023 10:13:05 +0200 Subject: [PATCH 37/41] feat: provide more information from build systems --- starlark/src/build_system/bazel.rs | 60 ++++++++++++++++++++-- starlark/src/build_system/buck2.rs | 80 +++++++++++++++++++++++------- starlark/src/build_system/mod.rs | 16 +++++- 3 files changed, 134 insertions(+), 22 deletions(-) diff --git a/starlark/src/build_system/bazel.rs b/starlark/src/build_system/bazel.rs index 06a83d5ed..d79211610 100644 --- a/starlark/src/build_system/bazel.rs +++ b/starlark/src/build_system/bazel.rs @@ -13,6 +13,8 @@ pub(crate) struct BazelBuildSystem { impl BazelBuildSystem { const DEFAULT_WORKSPACE_NAME: &'static str = "__main__"; + const BUILD_FILE_NAMES: [&'static str; 2] = ["BUILD", "BUILD.bazel"]; + const LOADABLE_EXTENSIONS: [&'static str; 1] = ["bzl"]; pub(crate) fn new(workspace_dir: Option<&PathBuf>) -> Option { let mut raw_command = Command::new("bazel"); @@ -72,9 +74,7 @@ impl BuildSystem for BazelBuildSystem { let mut path_components = path.components(); let repository_name = path_components.next()?.as_os_str().to_string_lossy(); - dbg!(&repository_name); let repository_path = path_components.as_path(); - dbg!(&repository_path); Some((repository_name, repository_path)) } else { @@ -82,7 +82,59 @@ impl BuildSystem for BazelBuildSystem { } } - fn get_build_file_names(&self) -> Vec<&str> { - vec!["BUILD", "BUILD.bazel"] + fn repository_names(&self) -> Vec> { + let mut names = Vec::new(); + if let Some(workspace_name) = &self.workspace_name { + names.push(Cow::Borrowed(workspace_name.as_str())); + } + + // Look for existing folders in `external_output_base`. + if let Ok(entries) = std::fs::read_dir(&self.external_output_base) { + for entry in entries { + if let Ok(entry) = entry { + if let Ok(file_type) = entry.file_type() { + if file_type.is_dir() { + if let Some(name) = entry.file_name().to_str() { + names.push(Cow::Owned(name.to_string())); + } + } + } + } + } + } + names + } + + fn get_build_file_names(&self) -> &[&str] { + &Self::BUILD_FILE_NAMES + } + + fn get_loadable_extensions(&self) -> &[&str] { + &Self::LOADABLE_EXTENSIONS + } + + fn query_buildable_targets( + &self, + module: &str, + workspace_dir: Option<&Path>, + ) -> Option> { + let mut raw_command = Command::new("bazel"); + let mut command = raw_command.arg("query").arg(format!("{module}*")); + if let Some(workspace_dir) = workspace_dir { + command = command.current_dir(workspace_dir); + } + + let output = command.output().ok()?; + if !output.status.success() { + return None; + } + + let output = String::from_utf8(output.stdout).ok()?; + Some( + output + .lines() + .filter_map(|line| line.strip_prefix(module).map(|str| str.to_owned())) + .collect(), + ) } } diff --git a/starlark/src/build_system/buck2.rs b/starlark/src/build_system/buck2.rs index fca189ed5..8078426d5 100644 --- a/starlark/src/build_system/buck2.rs +++ b/starlark/src/build_system/buck2.rs @@ -13,6 +13,9 @@ pub(crate) struct Buck2BuildSystem { } impl Buck2BuildSystem { + const BUILD_FILE_NAMES: [&'static str; 1] = ["BUCK"]; + const LOADABLE_EXTENSIONS: [&'static str; 1] = ["bzl"]; + pub(crate) fn new(workspace_dir: Option<&PathBuf>) -> Option { // We always need the workspace dir to resolve the workspace name. let workspace_dir = workspace_dir?; @@ -21,6 +24,7 @@ impl Buck2BuildSystem { let command = raw_command .arg("audit") .arg("cell") + .arg("--aliases") .arg("--json") .current_dir(workspace_dir); @@ -29,18 +33,22 @@ impl Buck2BuildSystem { return None; } - let repositories = - serde_json::from_slice::>(&output.stdout).ok()?; - let workspace_name = repositories.iter().find_map(|(name, path)| { - if path == workspace_dir { - Some(name.clone()) - } else { - None - } - })?; + let mut workspace_name = None; + let repositories = serde_json::from_slice::>(&output.stdout) + .ok()? + .into_iter() + .filter_map(|(name, path)| { + if &path == workspace_dir { + workspace_name = Some(name); + None + } else { + Some((name, path)) + } + }) + .collect(); Some(Self { - workspace_name, + workspace_name: workspace_name?, repositories, }) } @@ -61,17 +69,55 @@ impl BuildSystem for Buck2BuildSystem { self.repositories .iter() .find_map(|(name, repository_path)| { - path.strip_prefix(repository_path) - .ok() - .map(|stripped_path| (Cow::Borrowed(name.as_str()), stripped_path)) + if path.starts_with(repository_path) { + Some((Cow::Borrowed(name.as_str()), repository_path.as_path())) + } else { + None + } }) } - fn should_use_at_sign_before_repository_name(&self) -> bool { - false + fn repository_names(&self) -> Vec> { + self.repositories + .keys() + .map(|name| name.as_str()) + .map(Cow::Borrowed) + .collect() } - fn get_build_file_names(&self) -> Vec<&str> { - vec!["BUCK"] + fn get_build_file_names(&self) -> &[&str] { + &Self::BUILD_FILE_NAMES + } + + fn get_loadable_extensions(&self) -> &[&str] { + &Self::LOADABLE_EXTENSIONS + } + + fn query_buildable_targets( + &self, + module: &str, + workspace_dir: Option<&Path>, + ) -> Option> { + let mut raw_command = Command::new("buck2"); + let mut command = raw_command + .arg("uquery") + // buck2 query doesn't like `@` prefixes. + .arg(module.trim_start_matches('@')) + .arg("--json"); + if let Some(workspace_dir) = workspace_dir { + command = command.current_dir(workspace_dir); + } + + let output = command.output().ok()?; + if !output.status.success() { + return None; + } + + let output = String::from_utf8(output.stdout).ok()?; + serde_json::from_str(&output).ok() + } + + fn should_use_at_sign_before_repository_name(&self) -> bool { + false } } diff --git a/starlark/src/build_system/mod.rs b/starlark/src/build_system/mod.rs index f78d0bb3d..475117077 100644 --- a/starlark/src/build_system/mod.rs +++ b/starlark/src/build_system/mod.rs @@ -32,8 +32,22 @@ pub trait BuildSystem: std::fmt::Debug + Send + Sync { /// part of a known repository. fn repository_for_path<'a>(&'a self, path: &'a Path) -> Option<(Cow<'a, str>, &'a Path)>; + /// Returns the names of all known repositories. + fn repository_names(&self) -> Vec>; + /// Get valid build file names for this build system. - fn get_build_file_names(&self) -> Vec<&str>; + fn get_build_file_names(&self) -> &[&str]; + + /// Get valid file extensions for this build system. + fn get_loadable_extensions(&self) -> &[&str]; + + /// Ask the build system for the build targets that are buildable from the + /// given module. The `module` parameter should always end with a `:`. + fn query_buildable_targets( + &self, + module: &str, + workspace_dir: Option<&Path>, + ) -> Option>; /// Whether to prefix absolute paths with `@` when that path contains a /// repository name. From fdaa22bf10aca407db093dc4eeaeecb3a52afab2 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Fri, 30 Jun 2023 10:28:46 +0200 Subject: [PATCH 38/41] feat: implement hover for string literal & location, and go to string literal for target names NOTE: Unfortunately this has the side effect of having to return a `Span` (unresolved) from the `LspContext`, the hover needs this to extract the string contents from the codemap, to show in the hover popup. --- starlark/bin/eval.rs | 121 +++++++------- starlark/src/analysis/definition.rs | 213 ++++++++++++++++-------- starlark/src/analysis/find_call_name.rs | 11 +- starlark/src/codemap.rs | 8 +- starlark/src/environment/globals.rs | 1 + starlark/src/lsp/server.rs | 188 +++++++++++++++------ starlark/src/lsp/test.rs | 23 +-- 7 files changed, 366 insertions(+), 199 deletions(-) diff --git a/starlark/bin/eval.rs b/starlark/bin/eval.rs index 5f9a37bf0..73a3fa5ba 100644 --- a/starlark/bin/eval.rs +++ b/starlark/bin/eval.rs @@ -315,55 +315,14 @@ impl Context { .into_iter() .map(EvalMessage::from) } -} -impl LspContext for Context { - fn parse_file_with_contents(&self, uri: &LspUrl, content: String) -> LspEvalResult { - match uri { - LspUrl::File(uri) => { - let EvalResult { messages, ast } = - self.file_with_contents(&uri.to_string_lossy(), content); - LspEvalResult { - diagnostics: messages.map(Diagnostic::from).collect(), - ast, - } - } - _ => LspEvalResult::default(), - } - } - - fn resolve_load( + fn resolve_folder<'a>( &self, - path: &str, + path: &'a str, current_file: &LspUrl, workspace_root: Option<&Path>, - ) -> anyhow::Result { - let try_file = |folder: PathBuf, - presumed_filename: &str, - build_system: Option<&dyn BuildSystem>| - -> anyhow::Result { - // Try the presumed filename first, and check if it exists. - { - let path = folder.join(presumed_filename); - if path.exists() { - return Ok(Url::from_file_path(path).unwrap().try_into()?); - } - } - - // If the presumed filename doesn't exist, try to find a build file from the build system - // and use that instead. - if let Some(build_system) = build_system { - for build_file_name in build_system.get_build_file_names() { - let path = folder.join(build_file_name); - if path.exists() { - return Ok(Url::from_file_path(path).unwrap().try_into()?); - } - } - } - - Err(ResolveLoadError::TargetNotFound(path.to_owned()).into()) - }; - + resolved_filename: &mut Option<&'a str>, + ) -> anyhow::Result { let original_path = path; if let Some((repository, path)) = path.split_once("//") { // The repository may be prefixed with an '@', but it's optional in Buck2. @@ -428,11 +387,11 @@ impl LspContext for Context { // Resolve from the root of the repository. match (path.split_once(':'), resolve_root) { - (Some((subfolder, filename)), Some(resolve_root)) => try_file( - resolve_root.join(subfolder), - filename, - self.build_system.as_deref(), - ), + (Some((subfolder, filename)), Some(resolve_root)) => { + resolved_filename.replace(filename); + Ok(resolve_root.join(subfolder)) + } + (None, Some(resolve_root)) => Ok(resolve_root.join(path)), (Some(_), None) => { Err(ResolveLoadError::MissingWorkspaceRoot(original_path.to_owned()).into()) } @@ -441,16 +400,14 @@ impl LspContext for Context { } } } else if let Some((folder, filename)) = path.split_once(':') { + resolved_filename.replace(filename); + // Resolve relative paths from the current file. match current_file { LspUrl::File(current_file_path) => { let current_file_dir = current_file_path.parent(); match current_file_dir { - Some(current_file_dir) => Ok(try_file( - current_file_dir.join(folder), - filename, - self.build_system.as_deref(), - )?), + Some(current_file_dir) => Ok(current_file_dir.join(folder)), None => { Err(ResolveLoadError::MissingCurrentFilePath(path.to_owned()).into()) } @@ -465,6 +422,56 @@ impl LspContext for Context { Err(ResolveLoadError::CannotParsePath(path.to_owned()).into()) } } +} + +impl LspContext for Context { + fn parse_file_with_contents(&self, uri: &LspUrl, content: String) -> LspEvalResult { + match uri { + LspUrl::File(uri) => { + let EvalResult { messages, ast } = + self.file_with_contents(&uri.to_string_lossy(), content); + LspEvalResult { + diagnostics: messages.map(Diagnostic::from).collect(), + ast, + } + } + _ => LspEvalResult::default(), + } + } + + fn resolve_load( + &self, + path: &str, + current_file: &LspUrl, + workspace_root: Option<&Path>, + ) -> anyhow::Result { + let mut presumed_filename = None; + let folder = + self.resolve_folder(path, current_file, workspace_root, &mut presumed_filename)?; + + // Try the presumed filename first, and check if it exists. + if let Some(presumed_filename) = presumed_filename { + let path = folder.join(presumed_filename); + if path.exists() { + return Ok(Url::from_file_path(path).unwrap().try_into()?); + } + } else { + return Err(ResolveLoadError::CannotParsePath(path.to_owned()).into()); + } + + // If the presumed filename doesn't exist, try to find a build file from the build system + // and use that instead. + if let Some(build_system) = self.build_system.as_ref() { + for build_file_name in build_system.get_build_file_names() { + let path = folder.join(build_file_name); + if path.exists() { + return Ok(Url::from_file_path(path).unwrap().try_into()?); + } + } + } + + Err(ResolveLoadError::TargetNotFound(path.to_owned()).into()) + } fn render_as_load( &self, @@ -562,9 +569,7 @@ impl LspContext for Context { None } else { Some(Box::new(|ast, name, _| { - Ok(ast - .find_function_call_with_name(name) - .map(|resolved_span| resolved_span.into())) + Ok(ast.find_function_call_with_name(name)) })) }, }) diff --git a/starlark/src/analysis/definition.rs b/starlark/src/analysis/definition.rs index 13c2acc0c..11e4f38a0 100644 --- a/starlark/src/analysis/definition.rs +++ b/starlark/src/analysis/definition.rs @@ -49,6 +49,7 @@ pub(crate) enum IdentifierDefinition { Location { source: ResolvedSpan, destination: ResolvedSpan, + name: String, }, /// The symbol was loaded from another file. "destination" is the position within the /// "load()" statement, but additionally, the path in that load statement, and the @@ -113,7 +114,11 @@ pub(crate) struct DottedDefinition { #[derive(Debug, Clone, Eq, PartialEq)] enum TempIdentifierDefinition<'a> { /// The location of the definition of the symbol at the current line/column - Location { source: Span, destination: Span }, + Location { + source: Span, + destination: Span, + name: &'a str, + }, LoadedLocation { source: Span, destination: Span, @@ -226,7 +231,7 @@ impl LspModule { /// /// This method also handles scoping properly (i.e. an access of "foo" in a function /// will return location of the parameter "foo", even if there is a global called "foo"). - pub(crate) fn find_definition(&self, line: u32, col: u32) -> Definition { + pub(crate) fn find_definition_at_location(&self, line: u32, col: u32) -> Definition { // TODO(nmj): This should probably just store references to all of the AST nodes // when the LSPModule object is created, and then we can do a much faster // lookup, especially in cases where a file has not been changed, so the @@ -284,6 +289,7 @@ impl LspModule { Some((_, span)) => TempIdentifierDefinition::Location { source, destination: *span, + name, }, // We know the symbol name, but it might only be available in // an outer scope. @@ -380,9 +386,11 @@ impl LspModule { TempIdentifierDefinition::Location { source, destination, + name, } => IdentifierDefinition::Location { source: self.ast.codemap.resolve_span(source), destination: self.ast.codemap.resolve_span(destination), + name: name.to_owned(), }, TempIdentifierDefinition::Name { source, name } => match scope.bound.get(name) { None => IdentifierDefinition::Unresolved { @@ -400,6 +408,7 @@ impl LspModule { Some((_, span)) => IdentifierDefinition::Location { source: self.ast.codemap.resolve_span(source), destination: self.ast.codemap.resolve_span(*span), + name: name.to_owned(), }, }, // If we could not find the symbol, see if the current position is within @@ -420,7 +429,7 @@ impl LspModule { } /// Get the list of symbols exported by this module. - pub(crate) fn get_exported_symbols(&self) -> Vec> { + pub(crate) fn get_exported_symbols(&self) -> Vec { self.ast.exported_symbols() } @@ -516,8 +525,11 @@ impl LspModule { symbol_to_lookup .and_then(|span| { let resolved = self.ast.codemap.resolve_span(span); - self.find_definition(resolved.begin_line as u32, resolved.begin_column as u32) - .local_destination() + self.find_definition_at_location( + resolved.begin_line as u32, + resolved.begin_column as u32, + ) + .local_destination() }) .or_else(|| match (arg_span, identifier_span) { (Some(span), _) => Some(self.ast.codemap.resolve_span(span)), @@ -590,6 +602,7 @@ pub(crate) mod helpers { use std::collections::hash_map::Entry; use std::collections::HashMap; + use derivative::Derivative; use textwrap::dedent; use super::*; @@ -601,13 +614,19 @@ pub(crate) mod helpers { use crate::syntax::Dialect; /// Result of parsing a starlark fixture that has range markers in it. See `FixtureWithRanges::from_fixture` - #[derive(Debug, Clone, PartialEq, Eq)] + #[derive(Clone, Eq)] + #[derive(Derivative)] + #[derivative(Debug, PartialEq)] pub(crate) struct FixtureWithRanges { filename: String, /// The starlark program with markers removed. program: String, /// The location of all of the symbols that were indicated by the test fixture. - ranges: HashMap, + ranges: HashMap, + /// The codemap to resolve spans. + #[derivative(Debug = "ignore")] + #[derivative(PartialEq = "ignore")] + codemap: Option, } impl FixtureWithRanges { @@ -672,20 +691,23 @@ pub(crate) mod helpers { program.push_str(&fixture[fixture_idx..fixture.len()]); let code_map = CodeMap::new(filename.to_owned(), program.clone()); - let spans: HashMap = ranges + let spans: HashMap = ranges .into_iter() .map(|(id, (start, end))| { - let span = Span::new( - Pos::new(start.unwrap() as u32), - Pos::new(end.unwrap() as u32), - ); - (id, code_map.resolve_span(span)) + ( + id, + Span::new( + Pos::new(start.unwrap() as u32), + Pos::new(end.unwrap() as u32), + ), + ) }) .collect(); Ok(Self { filename: filename.to_owned(), program, + codemap: Some(code_map), ranges: spans, }) } @@ -693,6 +715,12 @@ pub(crate) mod helpers { pub(crate) fn begin_line(&self, identifier: &str) -> u32 { self.ranges .get(identifier) + .map(|span| { + self.codemap + .as_ref() + .expect("codemap to be set") + .resolve_span(*span) + }) .expect("identifier to be present") .begin_line as u32 } @@ -700,17 +728,35 @@ pub(crate) mod helpers { pub(crate) fn begin_column(&self, identifier: &str) -> u32 { self.ranges .get(identifier) + .map(|span| { + self.codemap + .as_ref() + .expect("codemap to be set") + .resolve_span(*span) + }) .expect("identifier to be present") .begin_column as u32 } - pub(crate) fn span(&self, identifier: &str) -> ResolvedSpan { + pub(crate) fn span(&self, identifier: &str) -> Span { *self .ranges .get(identifier) .expect("identifier to be present") } + pub(crate) fn resolve_span(&self, identifier: &str) -> ResolvedSpan { + self.ranges + .get(identifier) + .map(|span| { + self.codemap + .as_ref() + .expect("codemap to be set") + .resolve_span(*span) + }) + .expect("identifier to be present") + } + pub(crate) fn module(&self) -> anyhow::Result { Ok(LspModule::new(AstModule::parse( &self.filename, @@ -750,25 +796,21 @@ pub(crate) mod helpers { .to_owned(); let expected_locations = [ - ("a", 0, 17, 0, 22), - ("bar_highlight", 3, 0, 3, 3), - ("bar_click", 3, 0, 3, 1), - ("x", 3, 4, 3, 5), + ("a", 17, 22), + ("bar_highlight", 32, 35), + ("bar_click", 32, 33), + ("x", 36, 37), ] .into_iter() - .map(|(id, begin_line, begin_column, end_line, end_column)| { - let span = ResolvedSpan { - begin_line, - begin_column, - end_line, - end_column, - }; + .map(|(id, begin_pos, end_pos)| { + let span = Span::new(Pos::new(begin_pos), Pos::new(end_pos)); (id.to_owned(), span) }) .collect(); let expected = FixtureWithRanges { filename: "test.star".to_owned(), + codemap: None, program: expected_program, ranges: expected_locations, }; @@ -814,23 +856,23 @@ mod test { let module = parsed.module()?; let expected: Definition = IdentifierDefinition::LoadedLocation { - source: parsed.span("print_click"), - destination: parsed.span("print"), + source: parsed.resolve_span("print_click"), + destination: parsed.resolve_span("print"), path: "bar.star".to_owned(), name: "other_print".to_owned(), } .into(); assert_eq!( expected, - module.find_definition(parsed.begin_line("p1"), parsed.begin_column("p1")) + module.find_definition_at_location(parsed.begin_line("p1"), parsed.begin_column("p1")) ); assert_eq!( expected, - module.find_definition(parsed.begin_line("p2"), parsed.begin_column("p2")) + module.find_definition_at_location(parsed.begin_line("p2"), parsed.begin_column("p2")) ); assert_eq!( expected, - module.find_definition(parsed.begin_line("p3"), parsed.begin_column("p3")) + module.find_definition_at_location(parsed.begin_line("p3"), parsed.begin_column("p3")) ); Ok(()) } @@ -861,38 +903,40 @@ mod test { let module = parsed.module()?; let expected_add = Definition::from(IdentifierDefinition::Location { - source: parsed.span("add_click"), - destination: parsed.span("add"), + source: parsed.resolve_span("add_click"), + destination: parsed.resolve_span("add"), + name: "add".to_owned(), }); let expected_invalid = Definition::from(IdentifierDefinition::Location { - source: parsed.span("invalid_symbol_click"), - destination: parsed.span("invalid_symbol"), + source: parsed.resolve_span("invalid_symbol_click"), + destination: parsed.resolve_span("invalid_symbol"), + name: "invalid_symbol".to_owned(), }); assert_eq!( expected_add, - module.find_definition(parsed.begin_line("a1"), parsed.begin_column("a1")) + module.find_definition_at_location(parsed.begin_line("a1"), parsed.begin_column("a1")) ); assert_eq!( expected_add, - module.find_definition(parsed.begin_line("a2"), parsed.begin_column("a2")) + module.find_definition_at_location(parsed.begin_line("a2"), parsed.begin_column("a2")) ); assert_eq!( expected_add, - module.find_definition(parsed.begin_line("a3"), parsed.begin_column("a3")) + module.find_definition_at_location(parsed.begin_line("a3"), parsed.begin_column("a3")) ); assert_eq!( expected_invalid, - module.find_definition(parsed.begin_line("i1"), parsed.begin_column("i1")) + module.find_definition_at_location(parsed.begin_line("i1"), parsed.begin_column("i1")) ); assert_eq!( expected_invalid, - module.find_definition(parsed.begin_line("i2"), parsed.begin_column("i2")) + module.find_definition_at_location(parsed.begin_line("i2"), parsed.begin_column("i2")) ); assert_eq!( expected_invalid, - module.find_definition(parsed.begin_line("i3"), parsed.begin_column("i3")) + module.find_definition_at_location(parsed.begin_line("i3"), parsed.begin_column("i3")) ); Ok(()) } @@ -924,10 +968,14 @@ mod test { assert_eq!( Definition::from(IdentifierDefinition::Location { - source: parsed.span("x_param"), - destination: parsed.span("x") + source: parsed.resolve_span("x_param"), + destination: parsed.resolve_span("x"), + name: "x".to_owned(), }), - module.find_definition(parsed.begin_line("x_param"), parsed.begin_column("x_param")) + module.find_definition_at_location( + parsed.begin_line("x_param"), + parsed.begin_column("x_param") + ) ); Ok(()) } @@ -959,32 +1007,47 @@ mod test { assert_eq!( Definition::from(IdentifierDefinition::Location { - source: parsed.span("x_var"), - destination: parsed.span("x") + source: parsed.resolve_span("x_var"), + destination: parsed.resolve_span("x"), + name: "x".to_owned(), }), - module.find_definition(parsed.begin_line("x_var"), parsed.begin_column("x_var")) + module.find_definition_at_location( + parsed.begin_line("x_var"), + parsed.begin_column("x_var") + ) ); assert_eq!( Definition::from(IdentifierDefinition::Location { - source: parsed.span("y_var1"), - destination: parsed.span("y2") + source: parsed.resolve_span("y_var1"), + destination: parsed.resolve_span("y2"), + name: "y".to_owned(), }), - module.find_definition(parsed.begin_line("y_var1"), parsed.begin_column("y_var1")) + module.find_definition_at_location( + parsed.begin_line("y_var1"), + parsed.begin_column("y_var1") + ) ); assert_eq!( Definition::from(IdentifierDefinition::Location { - source: parsed.span("y_var2"), - destination: parsed.span("y1") + source: parsed.resolve_span("y_var2"), + destination: parsed.resolve_span("y1"), + name: "y".to_owned(), }), - module.find_definition(parsed.begin_line("y_var2"), parsed.begin_column("y_var2")) + module.find_definition_at_location( + parsed.begin_line("y_var2"), + parsed.begin_column("y_var2") + ) ); assert_eq!( Definition::from(IdentifierDefinition::Unresolved { - source: parsed.span("z_var"), + source: parsed.resolve_span("z_var"), name: "z".to_owned() }), - module.find_definition(parsed.begin_line("z_var"), parsed.begin_column("z_var")) + module.find_definition_at_location( + parsed.begin_line("z_var"), + parsed.begin_column("z_var") + ) ); Ok(()) } @@ -1016,7 +1079,10 @@ mod test { assert_eq!( Definition::from(IdentifierDefinition::NotFound), - module.find_definition(parsed.begin_line("no_def"), parsed.begin_column("no_def")) + module.find_definition_at_location( + parsed.begin_line("no_def"), + parsed.begin_column("no_def") + ) ); Ok(()) @@ -1072,10 +1138,11 @@ mod test { fn test(parsed: &FixtureWithRanges, module: &LspModule, name: &str) { let expected = Definition::from(IdentifierDefinition::StringLiteral { - source: parsed.span(&format!("{}_click", name)), + source: parsed.resolve_span(&format!("{}_click", name)), literal: name.to_owned(), }); - let actual = module.find_definition(parsed.begin_line(name), parsed.begin_column(name)); + let actual = module + .find_definition_at_location(parsed.begin_line(name), parsed.begin_column(name)); assert_eq!( expected, actual, @@ -1122,12 +1189,12 @@ mod test { let expected = |span_id: &str, segments: &[&str]| -> Definition { let root_definition_location = IdentifierDefinition::Unresolved { - source: parsed.span(&format!("{}_root", span_id)), + source: parsed.resolve_span(&format!("{}_root", span_id)), name: "foo".to_owned(), }; if segments.len() > 1 { DottedDefinition { - source: parsed.span(span_id), + source: parsed.resolve_span(span_id), root_definition_location, segments: segments.iter().map(|s| (*s).to_owned()).collect(), } @@ -1138,7 +1205,10 @@ mod test { }; let find_definition = |span_id: &str| { - module.find_definition(parsed.begin_line(span_id), parsed.begin_column(span_id)) + module.find_definition_at_location( + parsed.begin_line(span_id), + parsed.begin_column(span_id), + ) }; let expected_foo = expected("foo", &["foo"]); @@ -1174,14 +1244,14 @@ mod test { let expected = |span_id: &str, segments: &[&str]| -> Definition { let root_definition_location = IdentifierDefinition::LoadedLocation { - source: parsed.span(&format!("{}_root", span_id)), - destination: parsed.span("root"), + source: parsed.resolve_span(&format!("{}_root", span_id)), + destination: parsed.resolve_span("root"), path: "defs.bzl".to_owned(), name: "foo".to_owned(), }; if segments.len() > 1 { DottedDefinition { - source: parsed.span(span_id), + source: parsed.resolve_span(span_id), root_definition_location, segments: segments.iter().map(|s| (*s).to_owned()).collect(), } @@ -1192,7 +1262,10 @@ mod test { }; let find_definition = |span_id: &str| { - module.find_definition(parsed.begin_line(span_id), parsed.begin_column(span_id)) + module.find_definition_at_location( + parsed.begin_line(span_id), + parsed.begin_column(span_id), + ) }; let expected_foo = expected("foo", &["foo"]); @@ -1227,12 +1300,13 @@ mod test { let expected = |span_id: &str, segments: &[&str]| -> Definition { let root_definition_location = IdentifierDefinition::Location { - source: parsed.span(&format!("{}_root", span_id)), - destination: parsed.span("root"), + source: parsed.resolve_span(&format!("{}_root", span_id)), + destination: parsed.resolve_span("root"), + name: "foo".to_owned(), }; if segments.len() > 1 { DottedDefinition { - source: parsed.span(span_id), + source: parsed.resolve_span(span_id), root_definition_location, segments: segments.iter().map(|s| (*s).to_owned()).collect(), } @@ -1243,7 +1317,10 @@ mod test { }; let find_definition = |span_id: &str| { - module.find_definition(parsed.begin_line(span_id), parsed.begin_column(span_id)) + module.find_definition_at_location( + parsed.begin_line(span_id), + parsed.begin_column(span_id), + ) }; let expected_foo = expected("foo", &["foo"]); diff --git a/starlark/src/analysis/find_call_name.rs b/starlark/src/analysis/find_call_name.rs index 098e214ad..ad02d8941 100644 --- a/starlark/src/analysis/find_call_name.rs +++ b/starlark/src/analysis/find_call_name.rs @@ -15,7 +15,6 @@ * limitations under the License. */ -use crate::codemap::ResolvedSpan; use crate::codemap::Span; use crate::codemap::Spanned; use crate::syntax::ast::Argument; @@ -30,7 +29,7 @@ impl AstModule { /// /// NOTE: If the AST is exposed in the future, this function may be removed and implemented /// by specific programs instead. - pub fn find_function_call_with_name(&self, name: &str) -> Option { + pub fn find_function_call_with_name(&self, name: &str) -> Option { let mut ret = None; fn visit_expr(ret: &mut Option, name: &str, node: &AstExpr) { @@ -64,7 +63,7 @@ impl AstModule { } self.statement.visit_expr(|x| visit_expr(&mut ret, name, x)); - ret.map(|span| self.codemap.resolve_span(span)) + ret } } @@ -93,9 +92,11 @@ def x(name = "foo_name"): begin_line: 1, begin_column: 0, end_line: 1, - end_column: 3 + end_column: 22 }), - module.find_function_call_with_name("foo_name") + module + .find_function_call_with_name("foo_name") + .map(|span| module.codemap.resolve_span(span)) ); assert_eq!(None, module.find_function_call_with_name("bar_name")); Ok(()) diff --git a/starlark/src/codemap.rs b/starlark/src/codemap.rs index 2937565fb..f7763c28c 100644 --- a/starlark/src/codemap.rs +++ b/starlark/src/codemap.rs @@ -56,9 +56,15 @@ impl Add for Pos { } } +impl Display for Pos { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.0) + } +} + /// A range of text within a CodeMap. #[derive(Copy, Dupe, Clone, Hash, Eq, PartialEq, Debug, Default, Allocative)] -pub(crate) struct Span { +pub struct Span { /// The position in the codemap representing the first byte of the span. begin: Pos, diff --git a/starlark/src/environment/globals.rs b/starlark/src/environment/globals.rs index 61d326313..65e8c9c48 100644 --- a/starlark/src/environment/globals.rs +++ b/starlark/src/environment/globals.rs @@ -110,6 +110,7 @@ pub struct GlobalSymbol<'a> { } /// A kind of globally available symbol. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum GlobalSymbolKind { /// A global function, e.g. `dict`. Function, diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 81a494728..4249faa10 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -63,6 +63,7 @@ use lsp_types::HoverContents; use lsp_types::HoverParams; use lsp_types::HoverProviderCapability; use lsp_types::InitializeParams; +use lsp_types::LanguageString; use lsp_types::LocationLink; use lsp_types::LogMessageParams; use lsp_types::MarkedString; @@ -252,7 +253,7 @@ pub struct StringLiteralResult { /// If `None`, then just jump to the URL. Do not attempt to load the file. #[derivative(Debug = "ignore")] pub location_finder: - Option anyhow::Result> + Send>>, + Option anyhow::Result> + Send>>, } fn _assert_string_literal_result_is_send() { @@ -582,9 +583,11 @@ impl Backend { } } IdentifierDefinition::StringLiteral { literal, .. } => { - let resolved_literal = - self.context - .resolve_string_literal(&literal, uri, workspace_root)?; + let Ok(resolved_literal) = self.context + .resolve_string_literal(&literal, uri, workspace_root) + else { + return Ok(None); + }; match resolved_literal { Some(StringLiteralResult { url, @@ -600,15 +603,25 @@ impl Backend { literal .split_once(':') .map(|(_, rest)| rest) + .or_else(|| { + literal.rsplit_once('/').map(|(_, rest)| rest) + }) .unwrap_or_default(), &url, - ), + ) + .map(|span| { + span.map(|span| module.ast.codemap.resolve_span(span)) + }), None => Ok(None), }); - if let Err(e) = &result { - eprintln!("Error jumping to definition: {:#}", e); - } - let target_range = result.unwrap_or_default().unwrap_or_default(); + let result = match result { + Ok(result) => result, + Err(e) => { + eprintln!("Error jumping to definition: {:#}", e); + None + } + }; + let target_range = result.unwrap_or_default(); Self::location_link(source, &url, target_range)? } Some(StringLiteralResult { @@ -656,7 +669,7 @@ impl Backend { let location = match self.get_ast(&uri) { Some(ast) => { - let location = ast.find_definition(line, character); + let location = ast.find_definition_at_location(line, character); let source = location.source().unwrap_or_default(); match location { Definition::Identifier(definition) => self.resolve_definition_location( @@ -998,6 +1011,7 @@ impl Backend { }) } + /// Get hover information for a given position in a document. fn hover_info( &self, params: HoverParams, @@ -1020,12 +1034,13 @@ impl Backend { }; Ok(match self.get_ast(&uri) { - Some(ast) => { - let location = ast.find_definition(line, character); + Some(document) => { + let location = document.find_definition_at_location(line, character); match location { Definition::Identifier(identifier_definition) => self .get_hover_for_identifier_definition( identifier_definition, + &document, &uri, workspace_root.as_deref(), )?, @@ -1037,6 +1052,7 @@ impl Backend { // the root definition. self.get_hover_for_identifier_definition( root_definition_location, + &document, &uri, workspace_root.as_deref(), )? @@ -1051,27 +1067,99 @@ impl Backend { fn get_hover_for_identifier_definition( &self, identifier_definition: IdentifierDefinition, - current_uri: &LspUrl, + document: &LspModule, + document_uri: &LspUrl, workspace_root: Option<&Path>, ) -> anyhow::Result> { Ok(match identifier_definition { + IdentifierDefinition::Location { + destination, + name, + source, + } => { + // TODO: This seems very inefficient. Once the document starts + // holding the `Scope` including AST nodes, this indirection + // should be removed. + find_symbols_at_location( + &document.ast.codemap, + &document.ast.statement, + LineCol { + line: destination.begin_line, + column: destination.begin_column, + }, + ) + .remove(&name) + .and_then(|symbol| { + symbol.doc.map(|docs| Hover { + contents: HoverContents::Array(vec![MarkedString::String( + render_doc_item(&symbol.name, &docs), + )]), + range: Some(source.into()), + }) + }) + } IdentifierDefinition::LoadedLocation { path, name, source, .. } => { // Symbol loaded from another file. Find the file and get the definition // from there, hopefully including the docs. - let load_uri = self.resolve_load_path(&path, current_uri, workspace_root)?; + let load_uri = self.resolve_load_path(&path, document_uri, workspace_root)?; self.get_ast_or_load_from_disk(&load_uri)?.and_then(|ast| { ast.find_exported_symbol(&name).and_then(|symbol| { symbol.docs.map(|docs| Hover { contents: HoverContents::Array(vec![MarkedString::String( - render_doc_item(symbol.name, &docs), + render_doc_item(&symbol.name, &docs), )]), range: Some(source.into()), }) }) }) } + IdentifierDefinition::StringLiteral { source, literal } => { + let Ok(resolved_literal) = self.context.resolve_string_literal( + &literal, + document_uri, + workspace_root, + ) else { + // We might just be hovering a string that's not a file/target/etc, + // so just return nothing. + return Ok(None); + }; + match resolved_literal { + Some(StringLiteralResult { + url, + location_finder: Some(location_finder), + }) => { + // If there's an error loading the file to parse it, at least + // try to get to the file. + let module = if let Ok(Some(ast)) = self.get_ast_or_load_from_disk(&url) { + ast + } else { + return Ok(None); + }; + let result = location_finder( + &module.ast, + literal + .split_once(':') + .map(|(_, rest)| rest) + .or_else(|| literal.rsplit_once('/').map(|(_, rest)| rest)) + .unwrap_or_default(), + &url, + )?; + + result.map(|location| Hover { + contents: HoverContents::Array(vec![MarkedString::LanguageString( + LanguageString { + language: "python".to_string(), + value: module.ast.codemap.source_span(location).to_string(), + }, + )]), + range: Some(source.into()), + }) + } + _ => None, + } + } IdentifierDefinition::Unresolved { source, name } => { // Try to resolve as a global symbol. self.context @@ -1087,11 +1175,7 @@ impl Backend { }) }) } - IdentifierDefinition::NotFound => None, - _ => { - // Don't support other cases just yet. - None - } + IdentifierDefinition::LoadPath { .. } | IdentifierDefinition::NotFound => None, }) } @@ -1512,8 +1596,8 @@ mod test { let expected_location = expected_location_link_from_spans( bar_uri.clone(), - foo.span("baz_click"), - bar.span("baz"), + foo.resolve_span("baz_click"), + bar.resolve_span("baz"), ); let mut server = TestServer::new()?; @@ -1561,8 +1645,8 @@ mod test { let expected_location = expected_location_link_from_spans( bar_uri.clone(), - foo.span("baz_click"), - bar.span("baz"), + foo.resolve_span("baz_click"), + bar.resolve_span("baz"), ); let mut server = TestServer::new()?; @@ -1606,8 +1690,8 @@ mod test { let expected_location = expected_location_link_from_spans( bar_uri.clone(), - foo.span("baz_click"), - bar.span("baz"), + foo.resolve_span("baz_click"), + bar.resolve_span("baz"), ); let mut server = TestServer::new()?; @@ -1648,8 +1732,8 @@ mod test { let foo = FixtureWithRanges::from_fixture(foo_uri.path(), &foo_contents)?; let expected_location = expected_location_link_from_spans( foo_uri.clone(), - foo.span("baz_click"), - foo.span("baz_loc"), + foo.resolve_span("baz_click"), + foo.resolve_span("baz_loc"), ); let mut server = TestServer::new()?; @@ -1692,8 +1776,8 @@ mod test { let expected_location = expected_location_link_from_spans( foo_uri.clone(), - foo.span("not_baz"), - foo.span("not_baz_loc"), + foo.resolve_span("not_baz"), + foo.resolve_span("not_baz_loc"), ); let mut server = TestServer::new()?; @@ -1738,8 +1822,8 @@ mod test { let expected_location = expected_location_link_from_spans( bar_uri.clone(), - foo.span("baz_click"), - bar.span("baz"), + foo.resolve_span("baz_click"), + bar.resolve_span("baz"), ); let mut server = TestServer::new()?; @@ -1783,8 +1867,8 @@ mod test { let expected_location = expected_location_link_from_spans( foo_uri.clone(), - foo.span("not_baz_loc"), - foo.span("not_baz_loc"), + foo.resolve_span("not_baz_loc"), + foo.resolve_span("not_baz_loc"), ); let mut server = TestServer::new()?; @@ -1832,7 +1916,7 @@ mod test { let foo = FixtureWithRanges::from_fixture(foo_uri.path(), &foo_contents)?; let expected_location = LocationLink { - origin_selection_range: Some(foo.span("bar_click").into()), + origin_selection_range: Some(foo.resolve_span("bar_click").into()), target_uri: bar_uri, target_range: Default::default(), target_selection_range: Default::default(), @@ -1879,11 +1963,9 @@ mod test { let bar_contents = r#""Just a string""#; let bar = FixtureWithRanges::from_fixture(bar_uri.path(), bar_contents)?; - let bar_range = bar.span("bar"); - let bar_range_str = format!( - "{}:{}:{}:{}", - bar_range.begin_line, bar_range.begin_column, bar_range.end_line, bar_range.end_column - ); + let bar_span = bar.span("bar"); + let bar_resolved_span = bar.resolve_span("bar"); + let bar_span_str = format!("{}:{}", bar_span.begin(), bar_span.end()); let foo_contents = dedent( r#" @@ -1963,7 +2045,7 @@ mod test { "#, ) .trim() - .replace("{bar_range}", &bar_range_str); + .replace("{bar_range}", &bar_span_str); let foo = FixtureWithRanges::from_fixture(foo_uri.path(), &foo_contents)?; let mut server = TestServer::new()?; @@ -1972,13 +2054,13 @@ mod test { let mut test = |name: &str, expect_range: bool| -> anyhow::Result<()> { let range = if expect_range { - bar_range + bar_resolved_span } else { Default::default() }; let expected_location = expected_location_link_from_spans( bar_uri.clone(), - foo.span(&format!("{}_click", name)), + foo.resolve_span(&format!("{}_click", name)), range, ); @@ -2085,7 +2167,7 @@ mod test { let response = goto_definition_response_location(&mut server, req_id)?; let expected = LocationLink { - origin_selection_range: Some(foo.span("bar").into()), + origin_selection_range: Some(foo.resolve_span("bar").into()), target_uri: bar_uri, target_range: Default::default(), target_selection_range: Default::default(), @@ -2103,7 +2185,7 @@ mod test { let response = goto_definition_response_location(&mut server, req_id)?; let expected = LocationLink { - origin_selection_range: Some(foo.span("baz").into()), + origin_selection_range: Some(foo.resolve_span("baz").into()), target_uri: baz_uri, target_range: Default::default(), target_selection_range: Default::default(), @@ -2121,7 +2203,7 @@ mod test { let response = goto_definition_response_location(&mut server, req_id)?; let expected = LocationLink { - origin_selection_range: Some(foo.span("dir1").into()), + origin_selection_range: Some(foo.resolve_span("dir1").into()), target_uri: dir1_uri, target_range: Default::default(), target_selection_range: Default::default(), @@ -2140,7 +2222,7 @@ mod test { let response = goto_definition_response_location(&mut server, req_id)?; let expected = LocationLink { - origin_selection_range: Some(foo.span("dir2").into()), + origin_selection_range: Some(foo.resolve_span("dir2").into()), target_uri: dir2_uri, target_range: Default::default(), target_selection_range: Default::default(), @@ -2252,8 +2334,8 @@ mod test { let expected_n1_location = expected_location_link_from_spans( native_uri, - foo.span("click_n1"), - native.span("n1_loc"), + foo.resolve_span("click_n1"), + native.resolve_span("n1_loc"), ); let goto_definition = goto_definition_request( @@ -2269,8 +2351,8 @@ mod test { let expected_n2_location = expected_location_link_from_spans( foo_uri.clone(), - foo.span("click_n2"), - foo.span("n2_loc"), + foo.resolve_span("click_n2"), + foo.resolve_span("n2_loc"), ); let goto_definition = goto_definition_request( @@ -2377,8 +2459,8 @@ mod test { .map(|(fixture, uri, id)| { expected_location_link_from_spans( (*uri).clone(), - foo.span(id), - fixture.span(&format!("dest_{}", id)), + foo.resolve_span(id), + fixture.resolve_span(&format!("dest_{}", id)), ) }) .collect::>(); diff --git a/starlark/src/lsp/test.rs b/starlark/src/lsp/test.rs index db8957a1c..e6a8316b0 100644 --- a/starlark/src/lsp/test.rs +++ b/starlark/src/lsp/test.rs @@ -47,8 +47,6 @@ use lsp_types::GotoCapability; use lsp_types::InitializeParams; use lsp_types::InitializeResult; use lsp_types::InitializedParams; -use lsp_types::Position; -use lsp_types::Range; use lsp_types::TextDocumentClientCapabilities; use lsp_types::TextDocumentContentChangeEvent; use lsp_types::TextDocumentItem; @@ -57,6 +55,8 @@ use lsp_types::VersionedTextDocumentIdentifier; use maplit::hashmap; use serde::de::DeserializeOwned; +use crate::codemap::Pos; +use crate::codemap::Span; use crate::docs::render_docs_as_code; use crate::docs::Doc; use crate::docs::DocFunction; @@ -231,20 +231,15 @@ impl LspContext for TestServerContext { current_file: &LspUrl, workspace_root: Option<&Path>, ) -> anyhow::Result> { - let re = regex::Regex::new(r#"--(\d+):(\d+):(\d+):(\d+)$"#)?; - let (literal, range) = match re.captures(literal) { + let re = regex::Regex::new(r#"--(\d+):(\d+)$"#)?; + let (literal, span) = match re.captures(literal) { Some(cap) => { - let start_line = cap.get(1).unwrap().as_str().parse().unwrap(); - let start_col = cap.get(2).unwrap().as_str().parse().unwrap(); - let end_line = cap.get(3).unwrap().as_str().parse().unwrap(); - let end_col = cap.get(4).unwrap().as_str().parse().unwrap(); - let range = Range::new( - Position::new(start_line, start_col), - Position::new(end_line, end_col), - ); + let start_pos = cap.get(1).unwrap().as_str().parse().unwrap(); + let end_pos = cap.get(2).unwrap().as_str().parse().unwrap(); + let span = Span::new(Pos::new(start_pos), Pos::new(end_pos)); ( literal[0..cap.get(0).unwrap().start()].to_owned(), - Some(range), + Some(span), ) } None => (literal.to_owned(), None), @@ -254,7 +249,7 @@ impl LspContext for TestServerContext { LspUrl::File(u) => match u.extension() { Some(e) if e == "star" => Some(StringLiteralResult { url, - location_finder: Some(Box::new(move |_ast, _name, _url| Ok(range))), + location_finder: Some(Box::new(move |_ast, _name, _url| Ok(span))), }), _ => Some(StringLiteralResult { url, From 7200b1499a0ba9ddfb70fbdaed66cdd164a840cd Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Fri, 30 Jun 2023 10:36:02 +0200 Subject: [PATCH 39/41] feat: ability to autocomplete all sorts of different things Use the new auto complete type detection to offer different completion options in different scenarios: - Default: Just offer all known local symbols, global symbols, and those that can be loaded from other files. - Parameter: Same, but also offer known parameter names for the current function call. - LoadPath: Offer file system based completions, including files and folders, and if a build system was detected, known repository names. - LoadSymbol: Offer names of symbols exported from the file loaded in the current load statement. - String: Like LoadPath, but also offer files that can't be loaded according to the detected build system, as well as target names, again as detected by the detected build system. - Type: Offer type name completion (very basic at the moment). - None: Don't offer any completions. --- starlark/bin/eval.rs | 125 +++++++++ starlark/src/lsp/completion.rs | 498 +++++++++++++++++++++++++++++++++ starlark/src/lsp/mod.rs | 1 + starlark/src/lsp/server.rs | 231 ++++++++------- starlark/src/lsp/test.rs | 24 +- 5 files changed, 774 insertions(+), 105 deletions(-) create mode 100644 starlark/src/lsp/completion.rs diff --git a/starlark/bin/eval.rs b/starlark/bin/eval.rs index 73a3fa5ba..c38f416df 100644 --- a/starlark/bin/eval.rs +++ b/starlark/bin/eval.rs @@ -25,6 +25,7 @@ use std::path::Path; use std::path::PathBuf; use itertools::Either; +use lsp_types::CompletionItemKind; use lsp_types::Diagnostic; use lsp_types::Url; use starlark::build_system::try_resolve_build_system; @@ -40,6 +41,9 @@ use starlark::environment::Globals; use starlark::environment::Module; use starlark::errors::EvalMessage; use starlark::eval::Evaluator; +use starlark::lsp::completion::FilesystemCompletion; +use starlark::lsp::completion::FilesystemCompletionOptions; +use starlark::lsp::completion::FilesystemCompletionRoot; use starlark::lsp::server::LspContext; use starlark::lsp::server::LspEvalResult; use starlark::lsp::server::LspUrl; @@ -576,6 +580,113 @@ impl LspContext for Context { }) } + fn get_filesystem_entries( + &self, + from: FilesystemCompletionRoot, + current_file: &LspUrl, + workspace_root: Option<&Path>, + options: &FilesystemCompletionOptions, + ) -> anyhow::Result> { + // Find the actual folder on disk we're looking at. + let (from_path, render_base) = match from { + FilesystemCompletionRoot::Path(path) => (path.to_owned(), path.to_string_lossy()), + FilesystemCompletionRoot::String(str) => ( + self.resolve_folder(str, current_file, workspace_root, &mut None)?, + Cow::Borrowed(str), + ), + }; + + let build_file_names = self + .build_system + .as_ref() + .map(|build_system| build_system.get_build_file_names()) + .unwrap_or_default(); + let loadable_extensions = self + .build_system + .as_ref() + .map(|build_system| build_system.get_loadable_extensions()); + let mut result = Vec::new(); + for entry in fs::read_dir(from_path)? { + let entry = entry?; + let path = entry.path(); + // NOTE: Safe to `unwrap()` here, because we know that `path` is a file system path. And + // since it's an entry in a directory, it must have a file name. + let file_name = path.file_name().unwrap().to_string_lossy(); + if path.is_dir() && options.directories { + result.push(FilesystemCompletion::Entry { + label: file_name.to_string(), + insert_text: format!( + "{}{}", + if render_base.ends_with('/') || render_base.is_empty() { + "" + } else { + "/" + }, + file_name + ), + insert_text_offset: render_base.len(), + kind: CompletionItemKind::FOLDER, + }); + } else if path.is_file() { + if build_file_names.contains(&file_name.as_ref()) { + if options.targets { + if let Some(targets) = + self.build_system.as_ref().unwrap().query_buildable_targets( + &format!( + "{render_base}{}", + if render_base.ends_with(':') { "" } else { ":" } + ), + workspace_root, + ) + { + result.push(FilesystemCompletion::BuildFile { + targets, + prefix_with_colon: !render_base.ends_with(':'), + insert_text_offset: render_base.len(), + }); + } + } + continue; + } else if options.files { + // Check if it's in the list of allowed extensions. If we have a list, and it + // doesn't contain the extension, or the file has no extension, skip this file. + if !options.all_files { + let extension = path.extension().map(|ext| ext.to_string_lossy()); + if let Some(loadable_extensions) = loadable_extensions { + match extension { + Some(extension) => { + if !loadable_extensions.contains(&extension.as_ref()) { + continue; + } + } + None => { + continue; + } + } + } + } + + result.push(FilesystemCompletion::Entry { + label: file_name.to_string(), + insert_text: format!( + "{}{}", + if render_base.ends_with(':') || render_base.is_empty() { + "" + } else { + ":" + }, + file_name + ), + insert_text_offset: render_base.len(), + kind: CompletionItemKind::FILE, + }); + } + } + } + + Ok(result) + } + fn get_load_contents(&self, uri: &LspUrl) -> anyhow::Result> { match uri { LspUrl::File(path) => match path.is_absolute() { @@ -602,6 +713,20 @@ impl LspContext for Context { fn get_global_symbols(&self) -> Vec { self.globals.symbols().collect() } + + fn get_repository_names(&self) -> Vec> { + self.build_system + .as_ref() + .map(|build_system| build_system.repository_names()) + .unwrap_or_default() + } + + fn use_at_repository_prefix(&self) -> bool { + self.build_system + .as_ref() + .map(|build_system| build_system.should_use_at_sign_before_repository_name()) + .unwrap_or(true) + } } pub(crate) fn globals() -> Globals { diff --git a/starlark/src/lsp/completion.rs b/starlark/src/lsp/completion.rs new file mode 100644 index 000000000..bc1b6752c --- /dev/null +++ b/starlark/src/lsp/completion.rs @@ -0,0 +1,498 @@ +//! Collection of implementations for completions, and related types. + +use std::collections::HashMap; +use std::path::Path; + +use lsp_types::CompletionItem; +use lsp_types::CompletionItemKind; +use lsp_types::CompletionTextEdit; +use lsp_types::Documentation; +use lsp_types::MarkupContent; +use lsp_types::MarkupKind; +use lsp_types::Range; +use lsp_types::TextEdit; + +use crate::analysis::definition::Definition; +use crate::analysis::definition::DottedDefinition; +use crate::analysis::definition::IdentifierDefinition; +use crate::analysis::definition::LspModule; +use crate::analysis::exported::SymbolKind as ExportedSymbolKind; +use crate::codemap::LineCol; +use crate::codemap::ResolvedSpan; +use crate::docs::render_doc_item; +use crate::docs::DocItem; +use crate::docs::DocParam; +use crate::environment::GlobalSymbolKind; +use crate::lsp::server::Backend; +use crate::lsp::server::LspContext; +use crate::lsp::server::LspUrl; +use crate::syntax::ast::StmtP; +use crate::syntax::symbols::find_symbols_at_location; +use crate::syntax::symbols::SymbolKind; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum StringCompletionMode { + IncludeNamedTargets, + FilesOnly, +} + +/// Starting point for resolving filesystem completions. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum FilesystemCompletionRoot<'a> { + /// A resolved path, e.g. from an opened document. + Path(&'a Path), + /// An unresolved path, e.g. from a string literal in a `load` statement. + String(&'a str), +} + +/// Options for resolving filesystem completions. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct FilesystemCompletionOptions { + /// Whether to include directories in the results. + pub directories: bool, + /// Whether to include files in the results. + pub files: bool, + /// Whether to include files of any type in the results, as opposed to only files that are + /// loadable. + pub all_files: bool, + /// Whether to include target names from BUILD files. + pub targets: bool, +} + +/// A filesystem completion, e.g. for a `load` statement. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum FilesystemCompletion { + /// A regular file system entry, i.e. a directory or file. + Entry { + /// The label to show to the user. + label: String, + /// The text to insert when accepting the completion. + insert_text: String, + /// From where to start the insertion, compared to the start of the string. + insert_text_offset: usize, + /// The kind of completion. + kind: CompletionItemKind, + }, + /// A BUILD file containing targets buildable using the detected build system. + BuildFile { + /// The URI of the build file. + targets: Vec, + /// Whether to prefix the generated label with a colon. + prefix_with_colon: bool, + /// From where to start the insertion, compared to the start of the string. + insert_text_offset: usize, + }, +} + +impl Backend { + pub(crate) fn default_completion_options( + &self, + document_uri: &LspUrl, + document: &LspModule, + line: u32, + character: u32, + workspace_root: Option<&Path>, + ) -> impl Iterator + '_ { + let cursor_position = LineCol { + line: line as usize, + column: character as usize, + }; + + // Scan through current document + let mut symbols: HashMap<_, _> = find_symbols_at_location( + &document.ast.codemap, + &document.ast.statement, + cursor_position, + ) + .into_iter() + .map(|(key, value)| { + ( + key, + CompletionItem { + kind: Some(match value.kind { + SymbolKind::Method => CompletionItemKind::METHOD, + SymbolKind::Variable => CompletionItemKind::VARIABLE, + }), + detail: value.detail, + documentation: value.doc.map(|doc| { + Documentation::MarkupContent(MarkupContent { + kind: MarkupKind::Markdown, + value: render_doc_item(&value.name, &doc), + }) + }), + label: value.name, + ..Default::default() + }, + ) + }) + .collect(); + + // Discover exported symbols from other documents + let docs = self.last_valid_parse.read().unwrap(); + if docs.len() > 1 { + // Find the position of the last load in the current file. + let mut last_load = None; + let mut loads = HashMap::new(); + document.ast.statement.visit_stmt(|node| { + if let StmtP::Load(load) = &node.node { + last_load = Some(node.span); + loads.insert(load.module.node.clone(), (load.args.clone(), node.span)); + } + }); + let last_load = last_load.map(|span| document.ast.codemap.resolve_span(span)); + + symbols.extend( + self.get_all_exported_symbols( + Some(document_uri), + &symbols, + workspace_root, + document_uri, + |module, symbol| { + Self::get_load_text_edit( + module, + symbol, + document, + last_load, + loads.get(module), + ) + }, + ) + .into_iter() + .map(|item| (item.label.clone(), item)), + ); + } + + symbols + .into_values() + .chain(self.get_global_symbol_completion_items()) + .chain(Self::get_keyword_completion_items()) + } + + pub(crate) fn string_completion_options<'a>( + &'a self, + mode: StringCompletionMode, + document_uri: &LspUrl, + current_value: &str, + current_span: ResolvedSpan, + workspace_root: Option<&Path>, + ) -> impl Iterator + 'a { + // Figure out if we're just completing repository names for now, e.g.: + // "" (empty string) + // "@" (we want a repository) + // "@part" (partially typed repository) + // "foo" (might be relative, might be a repository) + // But not: + // "//" (no repository) + // ":" (relative path) + // "@repository//" (repository part is complete) + let render_at_prefix = + self.context.use_at_repository_prefix() || current_value.starts_with('@'); + let want_repository = current_value.is_empty() + || current_value == "@" + || (current_value.starts_with('@') && !current_value.contains('/')) + || (!render_at_prefix && !current_value.contains('/') && !current_value.contains(':')); + + let mut names = if want_repository { + self.context + .get_repository_names() + .into_iter() + .map(|name| { + let name_with_at = if render_at_prefix { + format!("@{}", name) + } else { + name.to_string() + }; + let insert_text = format!("{}//", &name_with_at); + + FilesystemCompletion::Entry { + label: name_with_at, + insert_text, + insert_text_offset: 0, + kind: CompletionItemKind::MODULE, + } + }) + .collect() + } else { + vec![] + }; + + // Complete filenames if we're not in the middle of typing a repository name: + // "@foo" -> don't complete filenames (still typing repository) + // "@foo/" -> don't complete filenames (need two separating slashes) + // "@foo//", "@foo//bar -> complete directories (from `//foo`) + // "@foo//bar/baz" -> complete directories (from `//foo/bar`) + // "@foo//bar:baz" -> complete filenames (from `//foo/bar`), and target names if `mode` is `IncludeNamedTargets` + // "foo" -> complete directories and filenames (ambiguous, might be a relative path or a repository) + let complete_directories = (!current_value.starts_with('@') + || current_value.contains("//")) + && !current_value.contains(':'); + let complete_filenames = + // Still typing repository + (!current_value.starts_with('@') || current_value.contains("//")) && + // Explicitly typing directory + (!current_value.contains('/') || current_value.contains(':')); + let complete_targets = + mode == StringCompletionMode::IncludeNamedTargets && complete_filenames; + if complete_directories || complete_filenames || complete_targets { + let complete_from = if complete_directories && complete_filenames { + // This must mean we don't have a `/` or `:` separator, so we're completing a relative path. + // Use the document URI's directory as the base. + document_uri + .path() + .parent() + .map(FilesystemCompletionRoot::Path) + } else { + // Complete from the last `:` or `/` in the current value. + current_value + .rfind(if complete_directories { '/' } else { ':' }) + .map(|pos| ¤t_value[..pos + 1]) + .map(FilesystemCompletionRoot::String) + }; + + if let Some(completion_root) = complete_from { + let other_names = self.context.get_filesystem_entries( + completion_root, + document_uri, + workspace_root, + &FilesystemCompletionOptions { + directories: complete_directories, + files: complete_filenames, + // I guess any place we can complete targets we can complete regular files? + all_files: complete_targets, + targets: complete_targets, + }, + ); + match other_names { + Ok(other_names) => { + for option in other_names { + match option { + FilesystemCompletion::Entry { .. } => names.push(option), + FilesystemCompletion::BuildFile { + targets, + insert_text_offset, + prefix_with_colon, + } => names.extend(targets.into_iter().map(|name| { + let insert_text = format!( + "{}{}", + if prefix_with_colon { ":" } else { "" }, + &name + ); + FilesystemCompletion::Entry { + label: name, + insert_text, + insert_text_offset, + kind: CompletionItemKind::PROPERTY, + } + })), + } + } + } + Err(e) => { + eprintln!("Error getting filesystem entries: {:?}", e); + } + } + } + } + + names.into_iter().filter_map(move |completion| { + let FilesystemCompletion::Entry { + label, + insert_text, + insert_text_offset, + kind, + } = completion else { + eprintln!("Unexpected filesystem completion: {:?}", completion); + return None; + }; + let mut range: Range = current_span.into(); + range.start.character += insert_text_offset as u32; + + Some(CompletionItem { + label, + insert_text: Some(insert_text.clone()), + text_edit: Some(CompletionTextEdit::Edit(TextEdit { + range, + new_text: insert_text, + })), + kind: Some(kind), + ..Default::default() + }) + }) + } + + pub(crate) fn exported_symbol_options( + &self, + load_path: &str, + current_span: ResolvedSpan, + previously_loaded: &[String], + document_uri: &LspUrl, + workspace_root: Option<&Path>, + ) -> Vec { + self.context + .resolve_load(load_path, document_uri, workspace_root) + .and_then(|url| self.get_ast_or_load_from_disk(&url)) + .into_iter() + .flatten() + .flat_map(|ast| { + ast.get_exported_symbols() + .into_iter() + .filter(|symbol| !previously_loaded.iter().any(|s| s == &symbol.name)) + .map(|symbol| { + let mut item: CompletionItem = symbol.into(); + item.insert_text = Some(item.label.clone()); + item.text_edit = Some(CompletionTextEdit::Edit(TextEdit { + range: current_span.into(), + new_text: item.label.clone(), + })); + item + }) + }) + .collect() + } + + pub(crate) fn parameter_name_options( + &self, + function_name_span: &ResolvedSpan, + document: &LspModule, + document_uri: &LspUrl, + workspace_root: Option<&Path>, + ) -> impl Iterator { + match document.find_definition_at_location( + function_name_span.begin_line as u32, + function_name_span.begin_column as u32, + ) { + Definition::Identifier(identifier) => self + .parameter_name_options_for_identifier_definition( + &identifier, + document, + document_uri, + workspace_root, + ) + .unwrap_or_default(), + Definition::Dotted(DottedDefinition { + root_definition_location, + .. + }) => self + .parameter_name_options_for_identifier_definition( + &root_definition_location, + document, + document_uri, + workspace_root, + ) + .unwrap_or_default(), + } + .into_iter() + .flatten() + } + + fn parameter_name_options_for_identifier_definition( + &self, + identifier_definition: &IdentifierDefinition, + document: &LspModule, + document_uri: &LspUrl, + workspace_root: Option<&Path>, + ) -> anyhow::Result>> { + Ok(match identifier_definition { + IdentifierDefinition::Location { + destination, name, .. + } => { + // Can we resolve it again at that location? + // TODO: This seems very inefficient. Once the document starts + // holding the `Scope` including AST nodes, this indirection + // should be removed. + find_symbols_at_location( + &document.ast.codemap, + &document.ast.statement, + LineCol { + line: destination.begin_line, + column: destination.begin_column, + }, + ) + .remove(name) + .and_then(|symbol| match symbol.kind { + SymbolKind::Method => symbol.doc, + SymbolKind::Variable => None, + }) + .and_then(|docs| match docs { + DocItem::Function(doc_function) => Some( + doc_function + .params + .into_iter() + .filter_map(|param| match param { + DocParam::Arg { name, .. } => Some(CompletionItem { + label: name, + kind: Some(CompletionItemKind::PROPERTY), + ..Default::default() + }), + _ => None, + }) + .collect(), + ), + _ => None, + }) + } + IdentifierDefinition::LoadedLocation { path, name, .. } => { + let load_uri = self.resolve_load_path(path, document_uri, workspace_root)?; + self.get_ast_or_load_from_disk(&load_uri)? + .and_then(|ast| ast.find_exported_symbol(name)) + .and_then(|symbol| match symbol.kind { + ExportedSymbolKind::Any => None, + ExportedSymbolKind::Function { argument_names } => Some( + argument_names + .into_iter() + .map(|name| CompletionItem { + label: name, + kind: Some(CompletionItemKind::PROPERTY), + ..Default::default() + }) + .collect(), + ), + }) + } + IdentifierDefinition::Unresolved { name, .. } => { + // Maybe it's a global symbol. + match self + .context + .get_global_symbols() + .into_iter() + .find(|symbol| symbol.name == name) + { + Some(symbol) if symbol.kind == GlobalSymbolKind::Function => { + match symbol.documentation { + Some(DocItem::Function(doc_function)) => Some( + doc_function + .params + .into_iter() + .filter_map(|param| match param { + DocParam::Arg { name, .. } => Some(CompletionItem { + label: name, + kind: Some(CompletionItemKind::PROPERTY), + ..Default::default() + }), + _ => None, + }) + .collect(), + ), + _ => None, + } + } + _ => None, + } + } + // None of these can be functions, so can't have any parameters. + IdentifierDefinition::LoadPath { .. } + | IdentifierDefinition::StringLiteral { .. } + | IdentifierDefinition::NotFound => None, + }) + } + + pub(crate) fn type_completion_options() -> impl Iterator { + ["str.type", "int.type", "bool.type", "None", "\"float\""] + .into_iter() + .map(|type_| CompletionItem { + label: type_.to_string(), + kind: Some(CompletionItemKind::TYPE_PARAMETER), + ..Default::default() + }) + } +} diff --git a/starlark/src/lsp/mod.rs b/starlark/src/lsp/mod.rs index 091d9e056..bc5f7ff24 100644 --- a/starlark/src/lsp/mod.rs +++ b/starlark/src/lsp/mod.rs @@ -18,6 +18,7 @@ //! The server that allows IDEs to evaluate and interpret starlark code according //! to the [Language Server Protocol](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/). +pub mod completion; pub mod server; mod symbols; #[cfg(all(test, not(windows)))] diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 4249faa10..1021e385a 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -17,6 +17,7 @@ //! Based on the reference lsp-server example at . +use std::borrow::Cow; use std::collections::HashMap; use std::collections::HashSet; use std::fmt::Debug; @@ -91,7 +92,7 @@ use crate::analysis::definition::Definition; use crate::analysis::definition::DottedDefinition; use crate::analysis::definition::IdentifierDefinition; use crate::analysis::definition::LspModule; -use crate::analysis::exported::SymbolKind as ExportedSymbolKind; +use crate::analysis::inspect::AutocompleteType; use crate::codemap::LineCol; use crate::codemap::ResolvedSpan; use crate::codemap::Span; @@ -99,12 +100,14 @@ use crate::codemap::Spanned; use crate::docs::render_doc_item; use crate::environment::GlobalSymbol; use crate::environment::GlobalSymbolKind; +use crate::lsp::completion::FilesystemCompletion; +use crate::lsp::completion::FilesystemCompletionOptions; +use crate::lsp::completion::FilesystemCompletionRoot; +use crate::lsp::completion::StringCompletionMode; use crate::lsp::server::LoadContentsError::WrongScheme; use crate::syntax::ast::AssignIdentP; use crate::syntax::ast::AstPayload; -use crate::syntax::ast::StmtP; use crate::syntax::symbols::find_symbols_at_location; -use crate::syntax::symbols::SymbolKind; use crate::syntax::AstModule; /// The request to get the file contents for a starlark: URI @@ -329,6 +332,16 @@ pub trait LspContext { workspace_root: Option<&Path>, ) -> anyhow::Result>; + /// Given a filesystem completion root, i.e. either a path based on an open file, or an + /// unparsed string, return a list of filesystem entries that match the given criteria. + fn get_filesystem_entries( + &self, + from: FilesystemCompletionRoot, + current_file: &LspUrl, + workspace_root: Option<&Path>, + options: &FilesystemCompletionOptions, + ) -> anyhow::Result>; + /// Get the contents of a starlark program at a given path, if it exists. fn get_load_contents(&self, uri: &LspUrl) -> anyhow::Result>; @@ -352,6 +365,12 @@ pub trait LspContext { current_file: &LspUrl, symbol: &str, ) -> anyhow::Result>; + + /// Get the list of known repository names. + fn get_repository_names(&self) -> Vec>; + + /// Whether to use the `@` prefix when rendering repository names. + fn use_at_repository_prefix(&self) -> bool; } /// Errors when [`LspContext::resolve_load()`] cannot resolve a given path. @@ -370,12 +389,12 @@ pub(crate) enum LoadContentsError { WrongScheme(String, LspUrl), } -struct Backend { +pub(crate) struct Backend { connection: Connection, - context: T, + pub(crate) context: T, /// The `AstModule` from the last time that a file was opened / changed and parsed successfully. /// Entries are evicted when the file is closed. - last_valid_parse: RwLock>>, + pub(crate) last_valid_parse: RwLock>>, } /// The logic implementations of stuff @@ -393,10 +412,20 @@ impl Backend { definition_provider, completion_provider: Some(CompletionOptions { trigger_characters: Some(vec![ + // e.g. function call "(".to_string(), + // e.g. list creation, function call ",".to_string(), + // e.g. when typing a load path + "/".to_string(), + // e.g. dict creation + ":".to_string(), + // e.g. variable assignment "=".to_string(), + // e.g. list creation "[".to_string(), + // e.g. string literal (load path, target name) + "\"".to_string(), ]), ..Default::default() }), @@ -410,7 +439,10 @@ impl Backend { last_valid_parse.get(uri).duped() } - fn get_ast_or_load_from_disk(&self, uri: &LspUrl) -> anyhow::Result>> { + pub(crate) fn get_ast_or_load_from_disk( + &self, + uri: &LspUrl, + ) -> anyhow::Result>> { let module = match self.get_ast(uri) { Some(result) => Some(result), None => self @@ -508,7 +540,7 @@ impl Backend { self.send_response(new_response(id, response)); } - fn resolve_load_path( + pub(crate) fn resolve_load_path( &self, path: &str, current_uri: &LspUrl, @@ -724,81 +756,82 @@ impl Backend { initialize_params: &InitializeParams, ) -> anyhow::Result { let uri = params.text_document_position.text_document.uri.try_into()?; + let line = params.text_document_position.position.line; + let character = params.text_document_position.position.character; let symbols: Option> = match self.get_ast(&uri) { - Some(ast) => { - // Scan through current document - let cursor_position = LineCol { - line: params.text_document_position.position.line as usize, - column: params.text_document_position.position.character as usize, - }; - let mut symbols: HashMap<_, _> = - find_symbols_at_location(&ast.ast.codemap, &ast.ast.statement, cursor_position) - .into_iter() - .map(|(key, value)| { - ( - key, - CompletionItem { - kind: Some(match value.kind { - SymbolKind::Method => CompletionItemKind::METHOD, - SymbolKind::Variable => CompletionItemKind::VARIABLE, - }), - detail: value.detail, - documentation: value.doc.map(|doc| { - Documentation::MarkupContent(MarkupContent { - kind: MarkupKind::Markdown, - value: render_doc_item(&value.name, &doc), - }) - }), - label: value.name, - ..Default::default() - }, - ) - }) - .collect(); - - // Discover exported symbols from other documents - let docs = self.last_valid_parse.read().unwrap(); - if docs.len() > 1 { - // Find the position of the last load in the current file. - let mut last_load = None; - let mut loads = HashMap::new(); - ast.ast.statement.visit_stmt(|node| { - if let StmtP::Load(load) = &node.node { - last_load = Some(node.span); - loads.insert(load.module.node.clone(), (load.args.clone(), node.span)); - } - }); - let last_load = last_load.map(|span| ast.ast.codemap.resolve_span(span)); - - symbols.extend( - self.get_all_exported_symbols( - Some(&uri), - &symbols, - initialize_params, + Some(document) => { + // Figure out what kind of position we are in, to determine the best type of + // autocomplete. + let autocomplete_type = document.ast.get_auto_complete_type(line, character); + let workspace_root = + Self::get_workspace_root(initialize_params.workspace_folders.as_ref(), &uri); + + match &autocomplete_type { + None | Some(AutocompleteType::None) => None, + Some(AutocompleteType::Default) => Some( + self.default_completion_options( &uri, - |module, symbol| { - Self::get_load_text_edit( - module, - symbol, - &ast, - last_load, - loads.get(module), - ) + &document, + line, + character, + workspace_root.as_deref(), + ) + .collect(), + ), + Some(AutocompleteType::LoadPath { + current_value, + current_span, + }) + | Some(AutocompleteType::String { + current_value, + current_span, + }) => Some( + self.string_completion_options( + if matches!(autocomplete_type, Some(AutocompleteType::LoadPath { .. })) + { + StringCompletionMode::FilesOnly + } else { + StringCompletionMode::IncludeNamedTargets }, + &uri, + current_value, + *current_span, + workspace_root.as_deref(), ) - .into_iter() - .map(|item| (item.label.clone(), item)), - ); - } - - Some( - symbols - .into_values() - .chain(self.get_global_symbol_completion_items()) - .chain(Self::get_keyword_completion_items()) .collect(), - ) + ), + Some(AutocompleteType::LoadSymbol { + path, + current_span, + previously_loaded, + }) => Some(self.exported_symbol_options( + path, + *current_span, + previously_loaded, + &uri, + workspace_root.as_deref(), + )), + Some(AutocompleteType::Parameter { + function_name_span, .. + }) => Some( + self.parameter_name_options( + function_name_span, + &document, + &uri, + workspace_root.as_deref(), + ) + .chain(self.default_completion_options( + &uri, + &document, + line, + character, + workspace_root.as_deref(), + )) + .collect(), + ), + Some(AutocompleteType::Type) => Some(Self::type_completion_options().collect()), + } } None => None, }; @@ -810,11 +843,11 @@ impl Backend { /// symbols. This list contains both the symbols exported from the loaded /// files, as well as symbols loaded in the open files. Symbols that are /// loaded from modules that are open are deduplicated. - fn get_all_exported_symbols( + pub(crate) fn get_all_exported_symbols( &self, except_from: Option<&LspUrl>, symbols: &HashMap, - initialize_params: &InitializeParams, + workspace_root: Option<&Path>, current_document: &LspUrl, format_text_edit: F, ) -> Vec @@ -836,7 +869,7 @@ impl Backend { let Ok(load_path) = self.context.render_as_load( doc_uri, current_document, - Self::get_workspace_root(initialize_params.workspace_folders.as_ref(), doc_uri).as_deref(), + workspace_root, ) else { continue; }; @@ -844,26 +877,16 @@ impl Backend { for symbol in doc .get_exported_symbols() .into_iter() - .filter(|symbol| !symbols.contains_key(symbol.name)) + .filter(|symbol| !symbols.contains_key(&symbol.name)) { seen.insert(format!("{load_path}:{}", &symbol.name)); - result.push(CompletionItem { - label: symbol.name.to_string(), - detail: Some(format!("Load from {load_path}")), - kind: Some(match symbol.kind { - ExportedSymbolKind::Any => CompletionItemKind::CONSTANT, - ExportedSymbolKind::Function => CompletionItemKind::METHOD, - }), - documentation: symbol.docs.map(|docs| { - Documentation::MarkupContent(MarkupContent { - kind: MarkupKind::Markdown, - value: render_doc_item(symbol.name, &docs), - }) - }), - additional_text_edits: Some(vec![format_text_edit(&load_path, symbol.name)]), - ..Default::default() - }) + let text_edits = Some(vec![format_text_edit(&load_path, &symbol.name)]); + let mut completion_item: CompletionItem = symbol.into(); + completion_item.detail = Some(format!("Load from {load_path}")); + completion_item.additional_text_edits = text_edits; + + result.push(completion_item) } } @@ -880,17 +903,15 @@ impl Backend { }) .filter(|(_, symbol)| !symbols.contains_key(symbol.name)) { - let workspace_root = - Self::get_workspace_root(initialize_params.workspace_folders.as_ref(), doc_uri); let Ok(url) = self.context - .resolve_load(symbol.loaded_from, doc_uri, workspace_root.as_deref()) + .resolve_load(symbol.loaded_from, doc_uri, workspace_root) else { continue; }; let Ok(load_path) = self.context.render_as_load( &url, current_document, - workspace_root.as_deref() + workspace_root ) else { continue; }; @@ -909,7 +930,9 @@ impl Backend { result } - fn get_global_symbol_completion_items(&self) -> impl Iterator + '_ { + pub(crate) fn get_global_symbol_completion_items( + &self, + ) -> impl Iterator + '_ { self.context .get_global_symbols() .into_iter() @@ -933,7 +956,7 @@ impl Backend { }) } - fn get_load_text_edit

( + pub(crate) fn get_load_text_edit

( module: &str, symbol: &str, ast: &LspModule, @@ -994,7 +1017,7 @@ impl Backend { } /// Get completion items for each language keyword. - fn get_keyword_completion_items() -> impl Iterator { + pub(crate) fn get_keyword_completion_items() -> impl Iterator { [ // Actual keywords "and", "else", "load", "break", "for", "not", "continue", "if", "or", "def", "in", diff --git a/starlark/src/lsp/test.rs b/starlark/src/lsp/test.rs index e6a8316b0..493c8fdb4 100644 --- a/starlark/src/lsp/test.rs +++ b/starlark/src/lsp/test.rs @@ -15,6 +15,7 @@ * limitations under the License. */ +use std::borrow::Cow; use std::collections::hash_map::Entry; use std::collections::HashMap; use std::collections::HashSet; @@ -66,6 +67,9 @@ use crate::docs::Location; use crate::environment::GlobalSymbol; use crate::environment::GlobalSymbolKind; use crate::errors::EvalMessage; +use crate::lsp::completion::FilesystemCompletion; +use crate::lsp::completion::FilesystemCompletionOptions; +use crate::lsp::completion::FilesystemCompletionRoot; use crate::lsp::server::new_notification; use crate::lsp::server::server_with_connection; use crate::lsp::server::LspContext; @@ -161,7 +165,7 @@ impl LspContext for TestServerContext { &self, path: &str, current_file: &LspUrl, - workspace_root: Option<&Path>, + _workspace_root: Option<&Path>, ) -> anyhow::Result { let path = PathBuf::from(path); match current_file { @@ -294,6 +298,24 @@ impl LspContext for TestServerContext { }) .collect() } + + fn get_filesystem_entries( + &self, + _from: FilesystemCompletionRoot, + _current_file: &LspUrl, + _workspace_root: Option<&Path>, + _options: &FilesystemCompletionOptions, + ) -> anyhow::Result> { + todo!() + } + + fn get_repository_names(&self) -> Vec> { + todo!() + } + + fn use_at_repository_prefix(&self) -> bool { + true + } } /// A server for use in testing that provides helpers for sending requests, correlating From 1d745b9b995b5438d5bad80fe33f2de3a518198e Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Fri, 30 Jun 2023 10:37:24 +0200 Subject: [PATCH 40/41] chore: allow parsing incomplete load statements By making the args optional, the parser is able to parse an incomplete load statement like this: `load("")` Without this, the LSP is unable to update the AST while this is typed, and as such we can't detect that the cursor is in a load statement, and offer auto complete for the load path. --- starlark/src/syntax/grammar.lalrpop | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/starlark/src/syntax/grammar.lalrpop b/starlark/src/syntax/grammar.lalrpop index 6f82a8f0c..0635f788e 100644 --- a/starlark/src/syntax/grammar.lalrpop +++ b/starlark/src/syntax/grammar.lalrpop @@ -186,7 +186,7 @@ ExprStmt_: Stmt = => Stmt::Expression(<>); LoadStmt: AstStmt = ASTS; LoadStmt_: Stmt = LoadStmtInner => Stmt::Load(<>); -LoadStmtInner: Load = "load" "(" )+> ","? ")" => { +LoadStmtInner: Load = "load" "(" )*> ","? ")" => { Load { module, args, From 384b9c16441fc50a67cf4cb3209f92f0a7f49547 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Wed, 5 Jul 2023 16:25:34 +0200 Subject: [PATCH 41/41] fix: target for load insertion was incorrect --- starlark/src/lsp/server.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/starlark/src/lsp/server.rs b/starlark/src/lsp/server.rs index 1021e385a..76faf6719 100644 --- a/starlark/src/lsp/server.rs +++ b/starlark/src/lsp/server.rs @@ -1003,7 +1003,10 @@ impl Backend { // inserts a new load statement after the last one we found. TextEdit::new( match last_load { - Some(span) => span.into(), + Some(span) => Range::new( + Position::new(span.end_line as u32, span.end_column as u32), + Position::new(span.end_line as u32, span.end_column as u32), + ), None => Range::new(Position::new(0, 0), Position::new(0, 0)), }, format!(