Fix hover info & go to definition for named parameters

starlark/src/docs/markdown.rs

@@ -343,10 +343,10 @@ impl<'a> RenderMarkdown for TypeRenderer<'a> {
                         DocParam::NoArgs => "*".to_owned(),
                         DocParam::OnlyPosBefore => "/".to_owned(),
                         DocParam::Args { typ, name, .. } => {
-                            format!("{}{}", name, raw_type_prefix(": ", typ))
+                            format!("*{}{}", name, raw_type_prefix(": ", typ))
                         }
                         DocParam::Kwargs { typ, name, .. } => {
-                            format!("{}{}", name, raw_type_prefix(": ", typ))
+                            format!("**{}{}", name, raw_type_prefix(": ", typ))
                         }
                     });
 

starlark/src/docs/mod.rs

@@ -509,14 +509,21 @@ impl DocFunction {
                     Some(args) => {
                         let entries = Self::parse_params(kind, args);
                         for x in &mut params {
-                            match x {
-                                DocParam::Arg { name, docs, .. }
-                                | DocParam::Args { name, docs, .. }
-                                | DocParam::Kwargs { name, docs, .. } => match entries.get(name) {
-                                    Some(raw) => *docs = DocString::from_docstring(kind, raw),
-                                    _ => (),
-                                },
-                                _ => (),
+                            if let Some((docs, raw)) = match x {
+                                DocParam::Arg { name, docs, .. } => {
+                                    entries.get(name).map(|raw| (docs, raw))
+                                }
+                                DocParam::Args { name, docs, .. } => entries
+                                    .get(name)
+                                    .or_else(|| entries.get(&format!("*{}", name)))
+                                    .map(|raw| (docs, raw)),
+                                DocParam::Kwargs { name, docs, .. } => entries
+                                    .get(name)
+                                    .or_else(|| entries.get(&format!("**{}", name)))
+                                    .map(|raw| (docs, raw)),
+                                _ => None,
+                            } {
+                                *docs = DocString::from_docstring(kind, raw);
                             }
                         }
                     }
@@ -647,7 +654,7 @@ impl DocParam {
         Some(indented)
     }
 
-    fn render_as_code(&self) -> String {
+    pub fn render_as_code(&self) -> String {
         match self {
             DocParam::Arg {
                 name,
@@ -662,9 +669,13 @@ impl DocParam {
             },
             DocParam::NoArgs => "*".to_owned(),
             DocParam::OnlyPosBefore => "/".to_owned(),
-            DocParam::Args { name, typ, .. } | DocParam::Kwargs { name, typ, .. } => match typ {
-                t if t.is_any() => name.clone(),
-                typ => format!("{}: {}", name, typ),
+            DocParam::Args { name, typ, .. } => match typ {
+                t if t.is_any() => format!("*{}", name),
+                typ => format!("*{}: {}", name, typ),
+            },
+            DocParam::Kwargs { name, typ, .. } => match typ {
+                t if t.is_any() => format!("**{}", name),
+                typ => format!("**{}: {}", name, typ),
             },
         }
     }

starlark_bin/bin/bazel.rs

@@ -141,6 +141,7 @@ pub(crate) fn main(
     prelude: &[PathBuf],
     dialect: Dialect,
     globals: Globals,
+    eager: bool,
 ) -> anyhow::Result<()> {
     if !lsp {
         return Err(anyhow::anyhow!("Bazel mode only supports `--lsp`"));
@@ -154,6 +155,7 @@ pub(crate) fn main(
         is_interactive,
         dialect,
         globals,
+        eager,
     )?;
 
     ctx.mode = ContextMode::Check;
@@ -173,6 +175,7 @@ pub(crate) struct BazelContext {
     pub(crate) globals: Globals,
     pub(crate) builtin_docs: HashMap<LspUrl, String>,
     pub(crate) builtin_symbols: HashMap<String, LspUrl>,
+    pub(crate) eager: bool,
 }
 
 impl BazelContext {
@@ -187,6 +190,7 @@ impl BazelContext {
         module: bool,
         dialect: Dialect,
         globals: Globals,
+        eager: bool,
     ) -> anyhow::Result<Self> {
         let prelude: Vec<_> = prelude
             .iter()
@@ -263,6 +267,7 @@ impl BazelContext {
             }),
             external_output_base: output_base
                 .map(|output_base| PathBuf::from(output_base).join("external")),
+            eager,
         })
     }
 
@@ -886,4 +891,8 @@ impl LspContext for BazelContext {
 
         Ok(names)
     }
+
+    fn is_eager(&self) -> bool {
+        self.eager
+    }
 }

starlark_bin/bin/eval.rs

@@ -70,6 +70,7 @@ pub(crate) struct Context {
     pub(crate) globals: Globals,
     pub(crate) builtin_docs: HashMap<LspUrl, String>,
     pub(crate) builtin_symbols: HashMap<String, LspUrl>,
+    pub(crate) eager: bool,
 }
 
 /// The outcome of evaluating (checking, parsing or running) given starlark code.
@@ -101,6 +102,7 @@ impl Context {
         module: bool,
         dialect: Dialect,
         globals: Globals,
+        eager: bool,
     ) -> anyhow::Result<Self> {
         let prelude: Vec<_> = prelude
             .iter()
@@ -143,6 +145,7 @@ impl Context {
             globals,
             builtin_docs,
             builtin_symbols,
+            eager,
         })
     }
 
@@ -379,4 +382,8 @@ impl LspContext for Context {
     fn get_environment(&self, _uri: &LspUrl) -> DocModule {
         DocModule::default()
     }
+
+    fn is_eager(&self) -> bool {
+        self.eager
+    }
 }

starlark_bin/bin/main.rs

@@ -142,6 +142,9 @@ struct Args {
     )]
     files: Vec<PathBuf>,
 
+    #[arg(long = "eager", help = "In LSP mode, eagerly load all files.")]
+    eager: bool,
+
     #[arg(
         long = "bazel",
         help = "Run in Bazel mode (temporary, will be removed)"
@@ -298,6 +301,7 @@ fn main() -> anyhow::Result<()> {
                 &prelude,
                 dialect,
                 globals,
+                args.eager,
             )?;
             return Ok(());
         }
@@ -313,6 +317,7 @@ fn main() -> anyhow::Result<()> {
             is_interactive,
             dialect,
             globals,
+            args.eager,
         )?;
 
         if args.lsp {

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<Bind
                 attributes.push(attribute);
                 f(lhs, attributes, res);
             }
-            Expr::Call(name, parameters) => {
-                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, _) = &parameter.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<Bind>) {
             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)),

starlark_lsp/src/completion.rs

@@ -31,7 +31,6 @@ use lsp_types::TextEdit;
 use starlark::codemap::ResolvedSpan;
 use starlark::docs::markdown::render_doc_item;
 use starlark::docs::markdown::render_doc_param;
-use starlark::docs::DocItem;
 use starlark::docs::DocMember;
 use starlark::docs::DocParam;
 use starlark_syntax::codemap::ResolvedPos;
@@ -42,7 +41,6 @@ use crate::definition::Definition;
 use crate::definition::DottedDefinition;
 use crate::definition::IdentifierDefinition;
 use crate::definition::LspModule;
-use crate::exported::SymbolKind as ExportedSymbolKind;
 use crate::server::Backend;
 use crate::server::LspContext;
 use crate::server::LspUrl;
@@ -96,17 +94,14 @@ impl<T: LspContext> Backend<T> {
             (
                 key,
                 CompletionItem {
-                    kind: Some(match value.kind {
-                        SymbolKind::Method => CompletionItemKind::METHOD,
-                        SymbolKind::Variable => CompletionItemKind::VARIABLE,
-                    }),
+                    kind: Some(value.kind.into()),
                     detail: value.detail,
                     documentation: value
                         .doc
                         .map(|doc| {
                             Documentation::MarkupContent(MarkupContent {
                                 kind: MarkupKind::Markdown,
-                                value: render_doc_item(&value.name, &doc),
+                                value: render_doc_item(&value.name, &doc.to_doc_item()),
                             })
                         })
                         .or_else(|| {
@@ -258,11 +253,11 @@ impl<T: LspContext> Backend<T> {
                 )
                 .remove(name)
                 .and_then(|symbol| match symbol.kind {
-                    SymbolKind::Method => symbol.doc,
-                    SymbolKind::Variable => None,
+                    SymbolKind::Method { .. } => symbol.doc,
+                    SymbolKind::Constant | SymbolKind::Variable => None,
                 })
                 .and_then(|docs| match docs {
-                    DocItem::Function(doc_function) => Some(
+                    DocMember::Function(doc_function) => Some(
                         doc_function
                             .params
                             .into_iter()
@@ -286,12 +281,12 @@ impl<T: LspContext> Backend<T> {
                 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
+                        SymbolKind::Constant | SymbolKind::Variable => None,
+                        SymbolKind::Method { arguments } => Some(
+                            arguments
                                 .into_iter()
-                                .map(|name| CompletionItem {
-                                    label: name,
+                                .map(|arg| CompletionItem {
+                                    label: arg.name,
                                     kind: Some(CompletionItemKind::PROPERTY),
                                     ..Default::default()
                                 })
@@ -330,6 +325,8 @@ impl<T: LspContext> Backend<T> {
             }
             // None of these can be functions, so can't have any parameters.
             IdentifierDefinition::LoadPath { .. }
+            | IdentifierDefinition::LocationWithParameterReference { .. }
+            | IdentifierDefinition::LoadedLocationWithParameterReference { .. }
             | IdentifierDefinition::StringLiteral { .. }
             | IdentifierDefinition::NotFound => None,
         })