Suggest a list of possible arg labels when an argument is unlabelled (#6755)

Signed-off-by: Nick Cameron <nrc@ncameron.org>

rust/kcl-lib/src/execution/exec_ast.rs

@@ -1295,13 +1295,20 @@ impl Node<CallExpressionKw> {
 
         // Build a hashmap from argument labels to the final evaluated values.
         let mut fn_args = IndexMap::with_capacity(self.arguments.len());
+        let mut errors = Vec::new();
         for arg_expr in &self.arguments {
             let source_range = SourceRange::from(arg_expr.arg.clone());
             let metadata = Metadata { source_range };
             let value = ctx
                 .execute_expr(&arg_expr.arg, exec_state, &metadata, &[], StatementKind::Expression)
                 .await?;
-            fn_args.insert(arg_expr.label.name.clone(), Arg::new(value, source_range));
+            let arg = Arg::new(value, source_range);
+            match &arg_expr.label {
+                Some(l) => {
+                    fn_args.insert(l.name.clone(), arg);
+                }
+                None => errors.push(arg),
+            }
         }
         let fn_args = fn_args; // remove mutability
 
@@ -1321,6 +1328,7 @@ impl Node<CallExpressionKw> {
             KwArgs {
                 unlabeled,
                 labeled: fn_args,
+                errors,
             },
             self.into(),
             ctx.clone(),
@@ -1894,6 +1902,44 @@ fn type_check_params_kw(
         }
     }
 
+    if !args.errors.is_empty() {
+        let actuals = args.labeled.keys();
+        let formals: Vec<_> = function_expression
+            .params
+            .iter()
+            .filter_map(|p| {
+                if !p.labeled {
+                    return None;
+                }
+
+                let name = &p.identifier.name;
+                if actuals.clone().any(|a| a == name) {
+                    return None;
+                }
+
+                Some(format!("`{name}`"))
+            })
+            .collect();
+
+        let suggestion = if formals.is_empty() {
+            String::new()
+        } else {
+            format!("; suggested labels: {}", formals.join(", "))
+        };
+
+        let mut errors = args.errors.iter().map(|e| {
+            CompilationError::err(
+                e.source_range,
+                format!("This argument needs a label, but it doesn't have one{suggestion}"),
+            )
+        });
+
+        let first = errors.next().unwrap();
+        errors.for_each(|e| exec_state.err(e));
+
+        return Err(KclError::Semantic(first.into()));
+    }
+
     if let Some(arg) = &mut args.unlabeled {
         if let Some(p) = function_expression.params.iter().find(|p| !p.labeled) {
             if let Some(ty) = &p.type_ {
@@ -2313,6 +2359,7 @@ mod test {
             let args = KwArgs {
                 unlabeled: None,
                 labeled,
+                errors: Vec::new(),
             };
             let exec_ctxt = ExecutorContext {
                 engine: Arc::new(Box::new(
@@ -2552,4 +2599,30 @@ a = foo()
 
         parse_execute(program).await.unwrap_err();
     }
+
+    #[tokio::test(flavor = "multi_thread")]
+    async fn test_sensible_error_when_missing_equals_in_kwarg() {
+        for (i, call) in ["f(x=1,y)", "f(x=1,3,z)", "f(x=1,y,z=1)", "f(x=1, 3 + 4, z=1)"]
+            .into_iter()
+            .enumerate()
+        {
+            let program = format!(
+                "fn foo() {{ return 0 }}
+y = 42
+z = 0
+fn f(x, y, z) {{ return 0 }}
+{call}"
+            );
+            let err = parse_execute(&program).await.unwrap_err();
+            let msg = err.message();
+            assert!(
+                msg.contains("This argument needs a label, but it doesn't have one"),
+                "failed test {i}: {msg}"
+            );
+            assert!(msg.contains("`y`"), "failed test {i}, missing `y`: {msg}");
+            if i == 0 {
+                assert!(msg.contains("`z`"), "failed test {i}, missing `z`: {msg}");
+            }
+        }
+    }
 }

rust/kcl-lib/src/parsing/ast/digest.rs

@@ -488,7 +488,9 @@ impl CallExpressionKw {
         }
         hasher.update(slf.arguments.len().to_ne_bytes());
         for argument in slf.arguments.iter_mut() {
-            hasher.update(argument.label.compute_digest());
+            if let Some(l) = &mut argument.label {
+                hasher.update(l.compute_digest());
+            }
             hasher.update(argument.arg.compute_digest());
         }
     });

rust/kcl-lib/src/parsing/ast/types/mod.rs

@@ -460,7 +460,8 @@ impl Node<Program> {
                 crate::walk::Node::CallExpressionKw(call) => {
                     if call.inner.callee.inner.name.inner.name == "appearance" {
                         for arg in &call.arguments {
-                            if arg.label.inner.name == "color" {
+                            if let Some(l) = &arg.label {
+                                if l.inner.name == "color" {
                                     // Get the value of the argument.
                                     if let Expr::Literal(literal) = &arg.arg {
                                         add_color(literal);
@@ -469,6 +470,7 @@ impl Node<Program> {
                             }
                         }
                     }
+                }
                 crate::walk::Node::Literal(literal) => {
                     // Check if the literal is a color.
                     add_color(literal);
@@ -1872,7 +1874,7 @@ pub struct CallExpressionKw {
 #[ts(export)]
 #[serde(tag = "type")]
 pub struct LabeledArg {
-    pub label: Node<Identifier>,
+    pub label: Option<Node<Identifier>>,
     pub arg: Expr,
 }
 
@@ -1917,7 +1919,7 @@ impl CallExpressionKw {
         self.unlabeled
             .iter()
             .map(|e| (None, e))
-            .chain(self.arguments.iter().map(|arg| (Some(&arg.label), &arg.arg)))
+            .chain(self.arguments.iter().map(|arg| (arg.label.as_ref(), &arg.arg)))
     }
 
     pub fn replace_value(&mut self, source_range: SourceRange, new_value: Expr) {

rust/kcl-lib/src/parsing/parser.rs

@@ -2714,12 +2714,17 @@ fn pipe_sep(i: &mut TokenSlice) -> PResult<()> {
 }
 
 fn labeled_argument(i: &mut TokenSlice) -> PResult<LabeledArg> {
-    separated_pair(
+    (
+        opt((
             terminated(nameable_identifier, opt(whitespace)),
             terminated(one_of((TokenType::Operator, "=")), opt(whitespace)),
+        )),
         expression,
     )
-    .map(|(label, arg)| LabeledArg { label, arg })
+        .map(|(label, arg)| LabeledArg {
+            label: label.map(|(l, _)| l),
+            arg,
+        })
         .parse_next(i)
 }
 
@@ -3040,6 +3045,7 @@ fn fn_call_kw(i: &mut TokenSlice) -> PResult<Node<CallExpressionKw>> {
         return Ok(result);
     }
 
+    #[derive(Debug)]
     #[allow(clippy::large_enum_variant)]
     enum ArgPlace {
         NonCode(Node<NonCodeNode>),
@@ -3068,24 +3074,17 @@ fn fn_call_kw(i: &mut TokenSlice) -> PResult<Node<CallExpressionKw>> {
                 }
                 ArgPlace::UnlabeledArg(arg) => {
                     let followed_by_equals = peek((opt(whitespace), equals)).parse_next(i).is_ok();
-                    let err = if followed_by_equals {
+                    if followed_by_equals {
-                        ErrMode::Cut(
+                        return Err(ErrMode::Cut(
                             CompilationError::fatal(
                                 SourceRange::from(arg),
                                 "This argument has a label, but no value. Put some value after the equals sign",
                             )
                             .into(),
-                        )
+                        ));
                     } else {
-                        ErrMode::Cut(
+                        args.push(LabeledArg { label: None, arg });
-                            CompilationError::fatal(
+                    }
-                                SourceRange::from(arg),
-                                "This argument needs a label, but it doesn't have one",
-                            )
-                            .into(),
-                        )
-                    };
-                    return Err(err);
                 }
             }
             Ok((args, non_code_nodes))
@@ -3098,7 +3097,9 @@ fn fn_call_kw(i: &mut TokenSlice) -> PResult<Node<CallExpressionKw>> {
     // Validate there aren't any duplicate labels.
     let mut counted_labels = IndexMap::with_capacity(args.len());
     for arg in &args {
-        *counted_labels.entry(&arg.label.inner.name).or_insert(0) += 1;
+        if let Some(l) = &arg.label {
+            *counted_labels.entry(&l.inner.name).or_insert(0) += 1;
+        }
     }
     if let Some((duplicated, n)) = counted_labels.iter().find(|(_label, n)| n > &&1) {
         let msg = format!(
@@ -4923,27 +4924,6 @@ bar = 1
         crate::parsing::top_level_parse(some_program_string).unwrap(); // Updated import path
     }
 
-    #[test]
-    fn test_sensible_error_when_missing_equals_in_kwarg() {
-        for (i, program) in ["f(x=1,y)", "f(x=1,y,z)", "f(x=1,y,z=1)", "f(x=1, y, z=1)"]
-            .into_iter()
-            .enumerate()
-        {
-            let tokens = crate::parsing::token::lex(program, ModuleId::default()).unwrap();
-            let err = fn_call_kw.parse(tokens.as_slice()).unwrap_err();
-            let cause = err.inner().cause.as_ref().unwrap();
-            assert_eq!(
-                cause.message, "This argument needs a label, but it doesn't have one",
-                "failed test {i}: {program}"
-            );
-            assert_eq!(
-                cause.source_range.start(),
-                program.find("y").unwrap(),
-                "failed test {i}: {program}"
-            );
-        }
-    }
-
     #[test]
     fn test_sensible_error_when_missing_rhs_of_kw_arg() {
         for (i, program) in ["f(x, y=)"].into_iter().enumerate() {

rust/kcl-lib/src/std/args.rs

@@ -62,6 +62,7 @@ pub struct KwArgs {
     pub unlabeled: Option<Arg>,
     /// Labeled args.
     pub labeled: IndexMap<String, Arg>,
+    pub errors: Vec<Arg>,
 }
 
 impl KwArgs {

rust/kcl-lib/src/std/array.rs

@@ -88,6 +88,7 @@ async fn call_map_closure(
     let kw_args = KwArgs {
         unlabeled: Some(Arg::new(input, source_range)),
         labeled: Default::default(),
+        errors: Vec::new(),
     };
     let args = Args::new_kw(
         kw_args,
@@ -233,6 +234,7 @@ async fn call_reduce_closure(
     let kw_args = KwArgs {
         unlabeled: Some(Arg::new(elem, source_range)),
         labeled,
+        errors: Vec::new(),
     };
     let reduce_fn_args = Args::new_kw(
         kw_args,

rust/kcl-lib/src/std/patterns.rs

@@ -430,6 +430,7 @@ async fn make_transform<T: GeometryTrait>(
     let kw_args = KwArgs {
         unlabeled: Some(Arg::new(repetition_num, source_range)),
         labeled: Default::default(),
+        errors: Vec::new(),
     };
     let transform_fn_args = Args::new_kw(
         kw_args,

rust/kcl-lib/src/unparser.rs

@@ -405,9 +405,13 @@ impl CallExpressionKw {
 
 impl LabeledArg {
     fn recast(&self, options: &FormatOptions, indentation_level: usize, ctxt: ExprContext) -> String {
-        let label = &self.label.name;
+        let mut result = String::new();
-        let arg = self.arg.recast(options, indentation_level, ctxt);
+        if let Some(l) = &self.label {
-        format!("{label} = {arg}")
+            result.push_str(&l.name);
+            result.push_str(" = ");
+        }
+        result.push_str(&self.arg.recast(options, indentation_level, ctxt));
+        result
     }
 }
 

src/lang/modifyAst.ts

@@ -223,7 +223,7 @@ export function mutateKwArg(
 ): boolean | 'no-mutate' {
   for (let i = 0; i < node.arguments.length; i++) {
     const arg = node.arguments[i]
-    if (arg.label.name === label) {
+    if (arg.label?.name === label) {
       if (isLiteralArrayOrStatic(val) && isLiteralArrayOrStatic(arg.arg)) {
         node.arguments[i].arg = val
         return true
@@ -259,7 +259,7 @@ export function mutateKwArgOnly(
 ): boolean {
   for (let i = 0; i < node.arguments.length; i++) {
     const arg = node.arguments[i]
-    if (arg.label.name === label) {
+    if (arg.label?.name === label) {
       node.arguments[i].arg = val
       return true
     }
@@ -273,7 +273,7 @@ Mutates the given node by removing the labeled arguments.
 */
 export function removeKwArgs(labels: string[], node: CallExpressionKw) {
   for (const label of labels) {
-    const i = node.arguments.findIndex((la) => la.label.name === label)
+    const i = node.arguments.findIndex((la) => la.label?.name === label)
     if (i == -1) {
       continue
     }

src/lang/modifyAst/addEdgeTreatment.ts

@@ -773,7 +773,7 @@ export async function editEdgeTreatment(
 
   // find the index of an argument to update
   const index = edgeTreatmentCall.node.arguments.findIndex(
-    (arg) => arg.label.name === parameterName
+    (arg) => arg.label?.name === parameterName
   )
 
   // create a new argument with the updated value

src/lang/modifyAst/tagManagement.ts

@@ -500,7 +500,7 @@ function modifyAstWithTagForCapFace(
 
   // Check for existing tag with this parameter name
   const existingTag = callExp.node.arguments.find(
-    (arg) => arg.label.name === tagParamName
+    (arg) => arg.label?.name === tagParamName
   )
 
   if (existingTag && existingTag.arg.type === 'TagDeclarator') {

src/lang/std/sketch.ts

@@ -190,7 +190,10 @@ const commonConstraintInfoHelper = (
             if (lengthishIndex === undefined) {
               return [undefined, undefined]
             }
-            const lengthKey = callExp.arguments[lengthishIndex].label.name
+            const lengthKey = callExp.arguments[lengthishIndex].label?.name
+            if (lengthKey === undefined) {
+              return [undefined, undefined]
+            }
             const lengthVal = callExp.arguments[lengthishIndex].arg
             // Note: The order of keys here matters.
             // Always assumes the angle was the first param, and then the length followed.
@@ -1057,7 +1060,7 @@ export const tangentialArc: SketchLineHelperKw = {
     }
 
     for (const arg of callExpression.arguments) {
-      if (arg.label.name !== ARG_END_ABSOLUTE && arg.label.name !== ARG_TAG) {
+      if (arg.label?.name !== ARG_END_ABSOLUTE && arg.label?.name !== ARG_TAG) {
         console.debug(
           'Trying to edit unsupported tangentialArc keyword arguments; skipping'
         )

src/lang/std/sketchcombos.ts

@@ -1375,12 +1375,12 @@ export function removeSingleConstraint({
 
         // 1. Filter out any existing tag argument since it will be handled separately
         const filteredArgs = existingArgs.filter(
-          (arg) => arg.label.name !== ARG_TAG
+          (arg) => arg.label?.name !== ARG_TAG
         )
 
         // 2. Map through the args, replacing only the one we want to change
         const labeledArgs = filteredArgs.map((arg) => {
-          if (arg.label.name === toReplace) {
+          if (arg.label?.name === toReplace) {
             // Find the raw value to use for the argument being replaced
             const rawArgVersion = rawArgs.find(
               (a) => a.type === 'labeledArg' && a.key === toReplace
@@ -1430,7 +1430,7 @@ export function removeSingleConstraint({
         const labeledArgs = existingArgs.map((arg) => {
           // Only modify the specific argument that matches the targeted key
           if (
-            arg.label.name === targetKey &&
+            arg.label?.name === targetKey &&
             arg.arg.type === 'ArrayExpression'
           ) {
             // We're dealing with an array expression within a labeled argument
@@ -1560,7 +1560,7 @@ export function removeSingleConstraint({
             arrayInput[inputToReplace.key][inputToReplace.index] =
               rawLiteralArrayInObjectExpr
             let existingKwgForKey = kwArgInput.find(
-              (kwArg) => kwArg.label.name === currentArg.key
+              (kwArg) => kwArg.label?.name === currentArg.key
             )
             if (!existingKwgForKey) {
               existingKwgForKey = createLabeledArg(
@@ -1586,7 +1586,7 @@ export function removeSingleConstraint({
             if (!arrayInput[currentArg.key]) arrayInput[currentArg.key] = []
             arrayInput[currentArg.key][currentArg.index] = currentArgExpr
             let existingKwgForKey = kwArgInput.find(
-              (kwArg) => kwArg.label.name === currentArg.key
+              (kwArg) => kwArg.label?.name === currentArg.key
             )
             if (!existingKwgForKey) {
               existingKwgForKey = createLabeledArg(
@@ -2190,7 +2190,7 @@ export function getConstraintLevelFromSourceRange(
           }
           const arg = findKwArgAny(DETERMINING_ARGS, nodeMeta.node)
           if (arg === undefined) {
-            const argStr = nodeMeta.node.arguments.map((a) => a.label.name)
+            const argStr = nodeMeta.node.arguments.map((a) => a.label?.name)
             return new Error(
               `call to expression ${name} has unexpected args: ${argStr} `
             )

src/lang/util.ts

@@ -136,7 +136,7 @@ export function findKwArg(
   call: CallExpressionKw
 ): Expr | undefined {
   return call?.arguments?.find((arg) => {
-    return arg.label.name === label
+    return arg.label?.name === label
   })?.arg
 }
 
@@ -149,7 +149,7 @@ export function findKwArgWithIndex(
   call: CallExpressionKw
 ): { expr: Expr; argIndex: number } | undefined {
   const index = call.arguments.findIndex((arg) => {
-    return arg.label.name === label
+    return arg.label?.name === label
   })
   return index >= 0
     ? { expr: call.arguments[index].arg, argIndex: index }
@@ -164,7 +164,7 @@ export function findKwArgAny(
   call: CallExpressionKw
 ): Expr | undefined {
   return call.arguments.find((arg) => {
-    return labels.includes(arg.label.name)
+    return labels.includes(arg.label?.name || '')
   })?.arg
 }
 
@@ -176,7 +176,7 @@ export function findKwArgAnyIndex(
   call: CallExpressionKw
 ): number | undefined {
   const index = call.arguments.findIndex((arg) => {
-    return labels.includes(arg.label.name)
+    return labels.includes(arg.label?.name || '')
   })
   if (index == -1) {
     return undefined

src/lib/kclHelpers.ts

@@ -60,7 +60,7 @@ export async function retrieveArgFromPipedCallExpression(
   name: string
 ): Promise<KclCommandValue | undefined> {
   const arg = callExpression.arguments.find(
-    (a) => a.label.type === 'Identifier' && a.label.name === name
+    (a) => a.label?.type === 'Identifier' && a.label?.name === name
   )
   if (
     arg?.type === 'LabeledArg' &&

src/lib/utils.ts

@@ -41,7 +41,9 @@ export async function refreshPage(method = 'UI button') {
  * Get all labels for a keyword call expression.
  */
 export function allLabels(callExpression: CallExpressionKw): string[] {
-  return callExpression.arguments.map((a) => a.label.name)
+  return callExpression.arguments
+    .map((a) => a.label?.name)
+    .filter((a) => a !== undefined)
 }
 
 /**