From 115e8a2be0ae5a10908046b533a92e21f1c8d444 Mon Sep 17 00:00:00 2001 From: Maarten Staa Date: Fri, 15 Mar 2024 14:28:05 +0100 Subject: [PATCH] feat: handle hover info & go to definition for named arguments in calls --- starlark_lsp/src/bind.rs | 74 +++++++++++++---- starlark_lsp/src/completion.rs | 10 ++- starlark_lsp/src/definition.rs | 148 ++++++++++++++++++++++++++------- starlark_lsp/src/exported.rs | 17 +++- starlark_lsp/src/server.rs | 145 ++++++++++++++++++++++++++++---- starlark_lsp/src/symbols.rs | 104 ++++++++++++++++------- 6 files changed, 404 insertions(+), 94 deletions(-) diff --git a/starlark_lsp/src/bind.rs b/starlark_lsp/src/bind.rs index f25690e25..56bd141b2 100644 --- a/starlark_lsp/src/bind.rs +++ b/starlark_lsp/src/bind.rs @@ -20,6 +20,8 @@ use std::collections::HashMap; use starlark::codemap::Pos; use starlark::codemap::Span; use starlark::syntax::AstModule; +use starlark_syntax::syntax::ast::Argument; +use starlark_syntax::syntax::ast::ArgumentP; use starlark_syntax::syntax::ast::AssignIdentP; use starlark_syntax::syntax::ast::AssignP; use starlark_syntax::syntax::ast::AstAssignIdent; @@ -33,6 +35,7 @@ use starlark_syntax::syntax::ast::AstTypeExpr; use starlark_syntax::syntax::ast::Clause; use starlark_syntax::syntax::ast::DefP; use starlark_syntax::syntax::ast::Expr; +use starlark_syntax::syntax::ast::ExprP; use starlark_syntax::syntax::ast::ForClause; use starlark_syntax::syntax::ast::ForP; use starlark_syntax::syntax::ast::IdentP; @@ -43,21 +46,35 @@ use starlark_syntax::syntax::module::AstModuleFields; #[derive(Debug, Clone, Eq, PartialEq)] pub(crate) enum Assigner { /// Obtained from `load`. `name` is the symbol in that file, not necessarily the local name - Load { - path: AstString, - name: AstString, - }, - Argument, // From a function argument - Assign, // From an assignment + Load { path: AstString, name: AstString }, + /// From a function call argument + Argument, + /// From an assignment + Assign, } #[derive(Debug)] pub(crate) enum Bind { - Set(Assigner, AstAssignIdent), // Variable assigned to directly - Get(AstIdent), // Variable that is referenced - GetDotted(GetDotted), // Variable is referenced, but is part of a dotted access - Flow, // Flow control occurs here (if, for etc) - can arrive or leave at this point - Scope(Scope), // Entering a new scope (lambda/def/comprehension) + /// Variable assigned to directly + Set(Assigner, AstAssignIdent), + /// Variable that is referenced + Get(AstIdent), + /// Variable is referenced, but is part of a dotted access + GetDotted(GetDotted), + /// An indirect reference, i.e. a named argument in a function call + IndirectReference(IndirectReference), + /// Flow control occurs here (if, for etc) - can arrive or leave at this point + Flow, + /// Entering a new scope (lambda/def/comprehension) + Scope(Scope), +} + +#[derive(Debug)] +pub(crate) struct IndirectReference { + pub(crate) argument_name: AstString, + // TODO: This could also be a dotted access, another function call, etc. These kinds of + // references are not captured at the moment. + pub(crate) function: AstIdent, } /// A 'get' bind that was part of a dotted member access pattern. @@ -123,7 +140,7 @@ impl Scope { Bind::Scope(scope) => scope.free.iter().for_each(|(k, v)| { free.entry(k.clone()).or_insert(*v); }), - Bind::Flow => {} + Bind::IndirectReference(_) | Bind::Flow => {} } } for x in bound.keys() { @@ -183,10 +200,19 @@ fn dot_access<'a>(lhs: &'a AstExpr, attribute: &'a AstString, res: &mut Vec { - f(name, attributes, res); - // make sure that if someone does a(b).c, 'b' is bound and considered used. + Expr::Call(func_name, parameters) => { + f(func_name, attributes, res); for parameter in parameters { + if let ExprP::Identifier(func_name) = &func_name.node { + if let ArgumentP::Named(arg_name, _) = ¶meter.node { + res.push(Bind::IndirectReference(IndirectReference { + argument_name: arg_name.clone(), + function: func_name.clone(), + })) + } + } + + // make sure that if someone does a(b).c, 'b' is bound and considered used. expr(parameter.expr(), res); } } @@ -221,6 +247,24 @@ fn expr(x: &AstExpr, res: &mut Vec) { expr(&x.0, res); expr(&x.1, res) }), + Expr::Call(func, args) => { + expr(func, res); + for x in args { + if let ExprP::Identifier(function_ident) = &func.node { + match &**x { + Argument::Named(name, value) => { + res.push(Bind::IndirectReference(IndirectReference { + argument_name: name.clone(), + function: function_ident.clone(), + })); + expr(value, res); + } + _ => expr(x.expr(), res), + } + } + expr(x.expr(), res) + } + } // Uninteresting - just recurse _ => x.visit_expr(|x| expr(x, res)), diff --git a/starlark_lsp/src/completion.rs b/starlark_lsp/src/completion.rs index 3cae213ee..5acc69fda 100644 --- a/starlark_lsp/src/completion.rs +++ b/starlark_lsp/src/completion.rs @@ -282,11 +282,11 @@ impl Backend { .and_then(|ast| ast.find_exported_symbol(name)) .and_then(|symbol| match symbol.kind { SymbolKind::Constant | SymbolKind::Variable => None, - SymbolKind::Method { argument_names } => Some( - argument_names + SymbolKind::Method { arguments } => Some( + arguments .into_iter() - .map(|name| CompletionItem { - label: name, + .map(|arg| CompletionItem { + label: arg.name, kind: Some(CompletionItemKind::PROPERTY), ..Default::default() }) @@ -325,6 +325,8 @@ impl Backend { } // None of these can be functions, so can't have any parameters. IdentifierDefinition::LoadPath { .. } + | IdentifierDefinition::LocationWithParameterReference { .. } + | IdentifierDefinition::LoadedLocationWithParameterReference { .. } | IdentifierDefinition::StringLiteral { .. } | IdentifierDefinition::NotFound => None, }) diff --git a/starlark_lsp/src/definition.rs b/starlark_lsp/src/definition.rs index 318f91ff3..875967200 100644 --- a/starlark_lsp/src/definition.rs +++ b/starlark_lsp/src/definition.rs @@ -42,13 +42,14 @@ use starlark_syntax::syntax::uniplate::Visit; use crate::bind::scope; use crate::bind::Assigner; use crate::bind::Bind; +use crate::bind::IndirectReference; use crate::bind::Scope; use crate::exported::AstModuleExportedSymbols; use crate::loaded::AstModuleLoadedSymbols; use crate::loaded::LoadedSymbol; use crate::symbols::Symbol; -/// The location of a definition for a given identifier. See [`AstModule::find_definition_at_location`]. +/// The location of a definition for a given identifier. See [`LspModule::find_definition_at_location`]. #[derive(Debug, Clone, Eq, PartialEq)] pub(crate) enum IdentifierDefinition { /// The definition was found at this location in the current file. @@ -57,6 +58,12 @@ pub(crate) enum IdentifierDefinition { destination: ResolvedSpan, name: String, }, + LocationWithParameterReference { + source: ResolvedSpan, + destination: ResolvedSpan, + name: String, + parameter: 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 /// name of the symbol within that file are provided so that additional lookups can @@ -67,6 +74,13 @@ pub(crate) enum IdentifierDefinition { path: String, name: String, }, + LoadedLocationWithParameterReference { + source: ResolvedSpan, + destination: ResolvedSpan, + path: String, + name: String, + parameter: String, + }, /// The symbol is the path component of a `load` statement. This is the raw string /// that is in the AST, and needs to be properly resolved to a path to be useful. LoadPath { source: ResolvedSpan, path: String }, @@ -88,7 +102,9 @@ impl IdentifierDefinition { fn source(&self) -> Option { match self { IdentifierDefinition::Location { source, .. } + | IdentifierDefinition::LocationWithParameterReference { source, .. } | IdentifierDefinition::LoadedLocation { source, .. } + | IdentifierDefinition::LoadedLocationWithParameterReference { source, .. } | IdentifierDefinition::LoadPath { source, .. } | IdentifierDefinition::StringLiteral { source, .. } | IdentifierDefinition::Unresolved { source, .. } => Some(*source), @@ -124,17 +140,27 @@ enum TempIdentifierDefinition<'a> { source: Span, destination: Span, name: &'a str, + /// In the case that the symbol is a parameter to a function, this is the name of the + /// parameter that the symbol is being used as. + parameter_name: Option<&'a str>, }, LoadedLocation { source: Span, destination: Span, path: &'a str, name: &'a str, + /// In the case that the symbol is a parameter to a function, this is the name of the + /// parameter that the symbol is being used as. + parameter_name: Option<&'a str>, }, /// The definition was not found in the current scope but the name of an identifier /// was found at the given position. This should be checked in outer scopes /// to attempt to find the definition. - Name { source: Span, name: &'a str }, + Name { + source: Span, + name: &'a str, + parameter_name: Option<&'a str>, + }, /// None of the accesses matched the position that was provided. NotFound, } @@ -282,6 +308,7 @@ impl LspModule { scope: &'a Scope, name: &'a str, source: Span, + parameter_name: Option<&'a str>, ) -> TempIdentifierDefinition<'a> { match scope.bound.get(name) { Some((Assigner::Load { path, name }, span)) => { @@ -290,31 +317,51 @@ impl LspModule { destination: *span, path, name: name.as_str(), + parameter_name, } } Some((_, span)) => TempIdentifierDefinition::Location { source, destination: *span, name, + parameter_name, }, // We know the symbol name, but it might only be available in // an outer scope. - None => TempIdentifierDefinition::Name { source, name }, + None => TempIdentifierDefinition::Name { + source, + name, + parameter_name, + }, } } for bind in &scope.inner { match bind { Bind::Get(g) if g.span.contains(pos) => { - return resolve_get_in_scope(scope, g.node.ident.as_str(), g.span).into(); + return resolve_get_in_scope(scope, g.node.ident.as_str(), g.span, None).into(); + } + Bind::IndirectReference(IndirectReference { + argument_name, + function, + }) if argument_name.span.contains(pos) => { + return resolve_get_in_scope( + scope, + function.node.ident.as_str(), + argument_name.span, + Some(argument_name.node.as_str()), + ) + .into(); } Bind::Scope(inner_scope) => { match Self::find_definition_in_scope(inner_scope, pos) { TempDefinition::Identifier(TempIdentifierDefinition::Name { source, name, + parameter_name, }) => { - return resolve_get_in_scope(scope, name, source).into(); + return resolve_get_in_scope(scope, name, source, parameter_name) + .into(); } TempDefinition::Identifier(TempIdentifierDefinition::NotFound) => {} TempDefinition::Dotted(TempDottedDefinition { @@ -323,6 +370,7 @@ impl LspModule { TempIdentifierDefinition::Name { source: root_source, name: root_name, + parameter_name: None, }, variable, attributes, @@ -333,6 +381,7 @@ impl LspModule { scope, root_name, root_source, + None, ), variable, attributes, @@ -356,6 +405,7 @@ impl LspModule { scope, root_identifier.node.ident.as_str(), root_identifier.span, + None, ); // If someone clicks on the "root" identifier, just treat it as a "get" match idx { @@ -372,15 +422,15 @@ impl LspModule { } } } - // For everything else, just ignore it. Note that the `Get` is ignored - // because we already checked the pos above. - Bind::Set(_, _) | Bind::Flow | Bind::Get(_) => {} + // For everything else, just ignore it. Note that the `Get` and `IndirectReference` + // are ignored because we already checked the pos above. + Bind::Set(_, _) | Bind::Flow | Bind::Get(_) | Bind::IndirectReference(_) => {} } } TempIdentifierDefinition::NotFound.into() } - /// Converts a `TempIdentifierDefinition` to an `IdentifierDefinition`, resolving spans + /// Converts a [`TempIdentifierDefinition`] to an [`IdentifierDefinition`], resolving spans /// against the current AST, owning strings, etc. fn get_definition_location( &self, @@ -393,28 +443,58 @@ impl LspModule { source, destination, name, - } => IdentifierDefinition::Location { - source: self.ast.codemap().resolve_span(source), - destination: self.ast.codemap().resolve_span(destination), - name: name.to_owned(), + parameter_name, + } => match parameter_name { + Some(argument_name) => IdentifierDefinition::LocationWithParameterReference { + source: self.ast.codemap().resolve_span(source), + destination: self.ast.codemap().resolve_span(destination), + name: name.to_owned(), + parameter: argument_name.to_owned(), + }, + None => 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) { + TempIdentifierDefinition::Name { + source, + name, + parameter_name, + } => match scope.bound.get(name) { None => IdentifierDefinition::Unresolved { source: self.ast.codemap().resolve_span(source), name: name.to_owned(), }, - Some((Assigner::Load { path, name }, span)) => { - IdentifierDefinition::LoadedLocation { + Some((Assigner::Load { path, name }, span)) => match parameter_name { + Some(parameter_name) => { + IdentifierDefinition::LoadedLocationWithParameterReference { + source: self.ast.codemap().resolve_span(source), + destination: self.ast.codemap().resolve_span(*span), + path: path.node.clone(), + name: name.node.clone(), + parameter: parameter_name.to_owned(), + } + } + None => IdentifierDefinition::LoadedLocation { source: self.ast.codemap().resolve_span(source), destination: self.ast.codemap().resolve_span(*span), path: path.node.clone(), name: name.node.clone(), - } - } - Some((_, span)) => IdentifierDefinition::Location { - source: self.ast.codemap().resolve_span(source), - destination: self.ast.codemap().resolve_span(*span), - name: name.to_owned(), + }, + }, + Some((_, span)) => match parameter_name { + Some(parameter_name) => IdentifierDefinition::LocationWithParameterReference { + source: self.ast.codemap().resolve_span(source), + destination: self.ast.codemap().resolve_span(*span), + name: name.to_owned(), + parameter: parameter_name.to_owned(), + }, + None => 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 @@ -425,11 +505,23 @@ impl LspModule { destination, path, name, - } => IdentifierDefinition::LoadedLocation { - source: self.ast.codemap().resolve_span(source), - destination: self.ast.codemap().resolve_span(destination), - path: path.to_owned(), - name: name.to_owned(), + parameter_name, + } => match parameter_name { + Some(parameter_name) => { + IdentifierDefinition::LoadedLocationWithParameterReference { + source: self.ast.codemap().resolve_span(source), + destination: self.ast.codemap().resolve_span(destination), + path: path.to_owned(), + name: name.to_owned(), + parameter: parameter_name.to_owned(), + } + } + None => IdentifierDefinition::LoadedLocation { + source: self.ast.codemap().resolve_span(source), + destination: self.ast.codemap().resolve_span(destination), + path: path.to_owned(), + name: name.to_owned(), + }, }, } } @@ -620,7 +712,7 @@ pub(crate) mod helpers { use super::*; - /// Result of parsing a starlark fixture that has range markers in it. See `FixtureWithRanges::from_fixture` + /// Result of parsing a starlark fixture that has range markers in it. See [`FixtureWithRanges::from_fixture`] #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct FixtureWithRanges { filename: String, diff --git a/starlark_lsp/src/exported.rs b/starlark_lsp/src/exported.rs index 41d01c2bf..815db39cf 100644 --- a/starlark_lsp/src/exported.rs +++ b/starlark_lsp/src/exported.rs @@ -30,6 +30,7 @@ use starlark_syntax::syntax::top_level_stmts::top_level_stmts; use crate::docs::get_doc_item_for_assign; use crate::docs::get_doc_item_for_def; +use crate::symbols::MethodSymbolArgument; use crate::symbols::Symbol; use crate::symbols::SymbolKind; @@ -103,17 +104,27 @@ impl AstModuleExportedSymbols for AstModule { }); } Stmt::Def(def) => { + let doc_item = get_doc_item_for_def(def); add( &mut result, &def.name, SymbolKind::Method { - argument_names: def + arguments: def .params .iter() - .filter_map(|param| param.split().0.map(|name| name.to_string())) + .filter_map(|param| { + MethodSymbolArgument::from_ast_parameter( + param, + doc_item.as_ref().and_then(|doc| { + param.node.ident().and_then(|ident| { + doc.find_param_with_name(&ident.ident) + }) + }), + ) + }) .collect(), }, - || get_doc_item_for_def(def).map(DocMember::Function), + || doc_item.map(DocMember::Function), ); } _ => {} diff --git a/starlark_lsp/src/server.rs b/starlark_lsp/src/server.rs index e0ef94c55..4e23887f2 100644 --- a/starlark_lsp/src/server.rs +++ b/starlark_lsp/src/server.rs @@ -130,6 +130,7 @@ use crate::inspect::AutocompleteType; use crate::signature; use crate::symbols; use crate::symbols::find_symbols_at_location; +use crate::symbols::MethodSymbolArgument; use crate::symbols::Symbol; use crate::symbols::SymbolKind; @@ -650,6 +651,7 @@ impl Backend { definition: IdentifierDefinition, source: ResolvedSpan, member: Option<&str>, + document: &LspModule, uri: &LspUrl, workspace_root: Option<&Path>, ) -> anyhow::Result> { @@ -658,6 +660,41 @@ impl Backend { destination: target, .. } => Self::location_link(source, uri, target)?, + IdentifierDefinition::LocationWithParameterReference { + destination: target, + name, + parameter, + .. + } => { + // TODO: This seems very inefficient. Once the document starts + // holding the `Scope` including AST nodes, this indirection + // should be removed. + let location = find_symbols_at_location( + document.ast.codemap(), + document.ast.statement(), + ResolvedPos { + line: target.begin.line, + column: target.begin.column, + }, + ) + .remove(&name) + .and_then(|symbol| match symbol.kind { + SymbolKind::Method { arguments } => arguments + .iter() + .find(|arg| arg.name == parameter) + .or_else(|| arguments.iter().find(|arg| arg.is_kwargs)) + .and_then(|arg| arg.span), + SymbolKind::Constant | SymbolKind::Variable => symbol.span, + }); + + Self::location_link( + source, + uri, + location + .map(|span| document.ast.codemap().resolve_span(span)) + .unwrap_or(target), + )? + } IdentifierDefinition::LoadedLocation { destination: location, path, @@ -678,6 +715,37 @@ impl Backend { } } } + IdentifierDefinition::LoadedLocationWithParameterReference { + destination: location, + path, + name, + parameter, + .. + } => { + assert!(member.is_none()); + + let load_uri = self.resolve_load_path(&path, uri, workspace_root)?; + let Some(ast) = self.get_ast_or_load_from_disk(&load_uri)? else { + return Self::location_link(source, uri, location); + }; + + let resolved_symbol = ast.find_exported_symbol(&name); + let resolved_location: Option = resolved_symbol + .and_then(|symbol| match symbol.kind { + SymbolKind::Method { arguments } => Some(arguments), + _ => None, + }) + .and_then(|arguments| arguments.into_iter().find(|arg| arg.name == parameter)) + .and_then(|arg| arg.span) + .map(|span| ast.ast.codemap().resolve_span(span)); + + match resolved_location { + None => Self::location_link(source, uri, location)?, + Some(resolved_location) => { + Self::location_link(source, &load_uri, resolved_location)? + } + } + } IdentifierDefinition::NotFound => None, IdentifierDefinition::LoadPath { path, .. } => { match self.resolve_load_path(&path, uri, workspace_root) { @@ -769,6 +837,7 @@ impl Backend { definition, source, None, + &ast, &uri, workspace_root.as_deref(), )?, @@ -798,6 +867,7 @@ impl Backend { .expect("to have at least one component") .as_str(), ), + &ast, &uri, workspace_root.as_deref(), )?, @@ -1177,6 +1247,28 @@ impl Backend { }) }) }), + IdentifierDefinition::LocationWithParameterReference { + source, parameter, .. + } + | IdentifierDefinition::LoadedLocationWithParameterReference { + source, + parameter, + .. + } => symbol.and_then(|symbol| match symbol.kind { + SymbolKind::Method { arguments } => arguments + .iter() + .find(|arg| arg.name == parameter) + .or_else(|| arguments.iter().find(|arg| arg.is_kwargs)) + .and_then(|arg| { + Some(Hover { + contents: HoverContents::Array(vec![MarkedString::String( + render_doc_param(arg.doc.as_ref()?), + )]), + range: Some(source.into()), + }) + }), + _ => None, + }), IdentifierDefinition::LoadedLocation { source, .. } => { // Symbol loaded from another file. Find the file and get the definition // from there, hopefully including the docs. @@ -1250,6 +1342,9 @@ impl Backend { match identifier_definition { IdentifierDefinition::Location { destination, name, .. + } + | IdentifierDefinition::LocationWithParameterReference { + destination, name, .. } => { // TODO: This seems very inefficient. Once the document starts // holding the `Scope` including AST nodes, this indirection @@ -1264,7 +1359,8 @@ impl Backend { ) .remove(name)) } - IdentifierDefinition::LoadedLocation { path, name, .. } => { + IdentifierDefinition::LoadedLocation { path, name, .. } + | IdentifierDefinition::LoadedLocationWithParameterReference { path, name, .. } => { // 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, document_uri, workspace_root)?; @@ -1286,14 +1382,33 @@ impl Backend { kind: match &doc { DocMember::Property(_) => SymbolKind::Constant, DocMember::Function(doc) => SymbolKind::Method { - argument_names: doc + arguments: doc .params .iter() .filter_map(|param| match param { - DocParam::Arg { name, .. } - | DocParam::Args { name, .. } - | DocParam::Kwargs { name, .. } => { - Some(name.clone()) + DocParam::Arg { name, .. } => { + Some(MethodSymbolArgument { + name: name.clone(), + doc: Some(param.clone()), + span: None, + is_kwargs: false, + }) + } + DocParam::Args { name, .. } => { + Some(MethodSymbolArgument { + name: name.clone(), + doc: Some(param.clone()), + span: None, + is_kwargs: false, + }) + } + DocParam::Kwargs { name, .. } => { + Some(MethodSymbolArgument { + name: name.clone(), + doc: Some(param.clone()), + span: None, + is_kwargs: true, + }) } DocParam::NoArgs | DocParam::OnlyPosBefore => None, }) @@ -1358,7 +1473,7 @@ impl Backend { workspace_root.as_deref(), )? { match symbol.kind { - SymbolKind::Method { argument_names } => { + SymbolKind::Method { arguments } => { let value = lsp_types::SignatureInformation { documentation: symbol.doc.as_ref().map(|doc| { Documentation::MarkupContent(MarkupContent { @@ -1377,15 +1492,15 @@ impl Backend { }).map(|doc| { doc.params.iter().map(|param| param.render_as_code()).collect::>().join(", ") }) - .unwrap_or_else(|| argument_names.join(", ")), + .unwrap_or_else(|| arguments.iter().map(|arg| &arg.name).join(", ")), ), active_parameter: match call.current_argument { signature::CallArgument::Positional(i) => Some(i as u32), - signature::CallArgument::Named(name) => argument_names + signature::CallArgument::Named(name) => arguments .iter() .enumerate() - .find_map(|(i, arg_name)| { - if arg_name == &name { + .find_map(|(i, arg)| { + if arg.name == name { Some(i as u32) } else { None @@ -1394,9 +1509,9 @@ impl Backend { signature::CallArgument::None => None, }, parameters: Some( - argument_names + arguments .into_iter() - .map(|arg_name| ParameterInformation { + .map(|arg| ParameterInformation { documentation: symbol.doc.as_ref().and_then(|doc| { match doc { DocMember::Property(_) => todo!(), @@ -1408,7 +1523,7 @@ impl Backend { DocParam::NoArgs | DocParam::OnlyPosBefore => return None, }; - if param_name == &arg_name { + if param_name == &arg.name { Some(Documentation::MarkupContent(MarkupContent { kind: MarkupKind::Markdown, value: render_doc_param(param), @@ -1419,7 +1534,7 @@ impl Backend { }), } }), - label: ParameterLabel::Simple(arg_name), + label: ParameterLabel::Simple(arg.name), }) .collect(), ), diff --git a/starlark_lsp/src/symbols.rs b/starlark_lsp/src/symbols.rs index c6bc42e84..f219176cc 100644 --- a/starlark_lsp/src/symbols.rs +++ b/starlark_lsp/src/symbols.rs @@ -25,6 +25,7 @@ use lsp_types::DocumentSymbol; use lsp_types::SymbolKind as LspSymbolKind; use starlark::codemap::CodeMap; use starlark::codemap::Span; +use starlark::docs::DocFunction; use starlark::docs::DocMember; use starlark::docs::DocParam; use starlark_syntax::codemap::ResolvedPos; @@ -47,10 +48,49 @@ use crate::docs::get_doc_item_for_def; #[derive(Debug, PartialEq)] pub(crate) enum SymbolKind { Constant, - Method { argument_names: Vec }, + Method { + arguments: Vec, + }, Variable, } +#[derive(Debug, PartialEq)] +pub(crate) struct MethodSymbolArgument { + pub(crate) name: String, + pub(crate) span: Option, + pub(crate) doc: Option, + pub(crate) is_kwargs: bool, +} + +impl MethodSymbolArgument { + pub(crate) fn from_ast_parameter( + param: &ParameterP

, + doc: Option<&DocParam>, + ) -> Option { + match param { + ParameterP::Normal(p, _) | ParameterP::WithDefaultValue(p, _, _) => Some(Self { + name: p.ident.clone(), + span: Some(p.span), + doc: doc.cloned(), + is_kwargs: false, + }), + ParameterP::Args(p, _) => Some(Self { + name: p.ident.clone(), + span: Some(p.span), + doc: doc.cloned(), + is_kwargs: false, + }), + ParameterP::KwArgs(p, _) => Some(Self { + name: p.ident.clone(), + span: Some(p.span), + doc: doc.cloned(), + is_kwargs: true, + }), + ParameterP::NoArgs => None, + } + } +} + impl From for CompletionItemKind { fn from(value: SymbolKind) -> Self { match value { @@ -65,10 +105,10 @@ impl SymbolKind { pub(crate) fn from_expr(x: &AstExprP

) -> Self { match &x.node { ExprP::Lambda(lambda) => Self::Method { - argument_names: lambda + arguments: lambda .params .iter() - .filter_map(|param| param.split().0.map(|name| name.node.ident.to_string())) + .filter_map(|param| MethodSymbolArgument::from_ast_parameter(¶m.node, None)) .collect(), }, _ => Self::Variable, @@ -106,7 +146,7 @@ pub(crate) fn find_symbols_at_location( span: Some(x.span), kind: (match &rhs.node { ExprP::Lambda(lambda) => SymbolKind::Method { - argument_names: argument_names(&lambda.params), + arguments: argument_names(&lambda.params, None), }, _ => SymbolKind::Variable, }), @@ -121,7 +161,7 @@ pub(crate) fn find_symbols_at_location( span: Some(x.span), kind: (match &source.node { ExprP::Lambda(lambda) => SymbolKind::Method { - argument_names: argument_names(&lambda.params), + arguments: argument_names(&lambda.params, None), }, _ => SymbolKind::Variable, }), @@ -152,7 +192,7 @@ pub(crate) fn find_symbols_at_location( name: def.name.ident.clone(), span: Some(def.name.span), kind: SymbolKind::Method { - argument_names: argument_names(&def.params), + arguments: argument_names(&def.params, doc.as_ref()), }, detail: None, doc: doc.clone().map(DocMember::Function), @@ -194,9 +234,7 @@ pub(crate) fn find_symbols_at_location( span: Some(local.span), detail: Some(format!("Loaded from {}", load.module.node)), // TODO: This should be dynamic based on the actual loaded value. - kind: SymbolKind::Method { - argument_names: vec![], - }, + kind: SymbolKind::Method { arguments: vec![] }, // TODO: Pull from the original file. doc: None, param: None, @@ -214,14 +252,19 @@ pub(crate) fn find_symbols_at_location( fn argument_names( params: &[starlark::codemap::Spanned>], -) -> Vec { + doc: Option<&DocFunction>, +) -> Vec { params .iter() - .filter_map(|param| match ¶m.node { - ParameterP::Normal(p, _) | ParameterP::WithDefaultValue(p, _, _) => { - Some(p.ident.clone()) - } - _ => None, + .filter_map(|param| { + MethodSymbolArgument::from_ast_parameter( + ¶m.node, + doc.and_then(|doc| { + param + .ident() + .and_then(|name| doc.find_param_with_name(&name.node.ident)) + }), + ) }) .collect() } @@ -481,6 +524,7 @@ mod tests { use super::find_symbols_at_location; use super::get_document_symbols; use super::DocumentSymbolMode; + use super::MethodSymbolArgument; use super::Symbol; use super::SymbolKind; @@ -513,9 +557,7 @@ my_var = True name: "exported_a".to_owned(), span: Some(Span::new(Pos::new(17), Pos::new(29))), detail: Some("Loaded from foo.star".to_owned()), - kind: SymbolKind::Method { - argument_names: vec![] - }, + kind: SymbolKind::Method { arguments: vec![] }, doc: None, param: None, }, @@ -526,9 +568,7 @@ my_var = True name: "renamed".to_owned(), span: Some(Span::new(Pos::new(31), Pos::new(38))), detail: Some("Loaded from foo.star".to_owned()), - kind: SymbolKind::Method { - argument_names: vec![] - }, + kind: SymbolKind::Method { arguments: vec![] }, doc: None, param: None, }, @@ -540,7 +580,12 @@ my_var = True span: Some(Span::new(Pos::new(60), Pos::new(66))), detail: None, kind: SymbolKind::Method { - argument_names: vec!["param".to_owned()] + arguments: vec![MethodSymbolArgument { + name: "param".to_owned(), + span: Some(Span::new(Pos::new(67), Pos::new(72))), + doc: None, + is_kwargs: false, + }] }, doc: None, param: None, @@ -590,9 +635,7 @@ my_var = True name: "exported_a".to_owned(), span: Some(Span::new(Pos::new(17), Pos::new(29))), detail: Some("Loaded from foo.star".to_owned()), - kind: SymbolKind::Method { - argument_names: vec![], - }, + kind: SymbolKind::Method { arguments: vec![] }, doc: None, param: None, }, @@ -603,9 +646,7 @@ my_var = True name: "renamed".to_owned(), span: Some(Span::new(Pos::new(31), Pos::new(38))), detail: Some("Loaded from foo.star".to_owned()), - kind: SymbolKind::Method { - argument_names: vec![], - }, + kind: SymbolKind::Method { arguments: vec![] }, doc: None, param: None, }, @@ -617,7 +658,12 @@ my_var = True span: Some(Span::new(Pos::new(60), Pos::new(66))), detail: None, kind: SymbolKind::Method { - argument_names: vec!["param".to_owned()], + arguments: vec![MethodSymbolArgument { + name: "param".to_owned(), + span: Some(Span::new(Pos::new(67), Pos::new(72))), + doc: None, + is_kwargs: false + }], }, doc: None, param: None,