diff --git a/rust/kcl-lib/src/execution/exec_ast.rs b/rust/kcl-lib/src/execution/exec_ast.rs index 677b1f7ce..023f9d243 100644 --- a/rust/kcl-lib/src/execution/exec_ast.rs +++ b/rust/kcl-lib/src/execution/exec_ast.rs @@ -1295,13 +1295,20 @@ impl Node { // 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 { 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}"); + } + } + } } diff --git a/rust/kcl-lib/src/parsing/ast/digest.rs b/rust/kcl-lib/src/parsing/ast/digest.rs index 78590ad67..9f3f22642 100644 --- a/rust/kcl-lib/src/parsing/ast/digest.rs +++ b/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()); } }); diff --git a/rust/kcl-lib/src/parsing/ast/types/mod.rs b/rust/kcl-lib/src/parsing/ast/types/mod.rs index 4f5f9db34..fe0348fe1 100644 --- a/rust/kcl-lib/src/parsing/ast/types/mod.rs +++ b/rust/kcl-lib/src/parsing/ast/types/mod.rs @@ -460,10 +460,12 @@ impl Node { 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" { - // Get the value of the argument. - if let Expr::Literal(literal) = &arg.arg { - add_color(literal); + 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); + } } } } @@ -1872,7 +1874,7 @@ pub struct CallExpressionKw { #[ts(export)] #[serde(tag = "type")] pub struct LabeledArg { - pub label: Node, + pub label: Option>, 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) { diff --git a/rust/kcl-lib/src/parsing/parser.rs b/rust/kcl-lib/src/parsing/parser.rs index 86b4858a6..668b207a0 100644 --- a/rust/kcl-lib/src/parsing/parser.rs +++ b/rust/kcl-lib/src/parsing/parser.rs @@ -2714,13 +2714,18 @@ fn pipe_sep(i: &mut TokenSlice) -> PResult<()> { } fn labeled_argument(i: &mut TokenSlice) -> PResult { - separated_pair( - terminated(nameable_identifier, opt(whitespace)), - terminated(one_of((TokenType::Operator, "=")), opt(whitespace)), + ( + opt(( + terminated(nameable_identifier, opt(whitespace)), + terminated(one_of((TokenType::Operator, "=")), opt(whitespace)), + )), expression, ) - .map(|(label, arg)| LabeledArg { label, arg }) - .parse_next(i) + .map(|(label, arg)| LabeledArg { + label: label.map(|(l, _)| l), + arg, + }) + .parse_next(i) } /// A type of a function argument. @@ -3040,6 +3045,7 @@ fn fn_call_kw(i: &mut TokenSlice) -> PResult> { return Ok(result); } + #[derive(Debug)] #[allow(clippy::large_enum_variant)] enum ArgPlace { NonCode(Node), @@ -3068,24 +3074,17 @@ fn fn_call_kw(i: &mut TokenSlice) -> PResult> { } ArgPlace::UnlabeledArg(arg) => { let followed_by_equals = peek((opt(whitespace), equals)).parse_next(i).is_ok(); - let err = if followed_by_equals { - ErrMode::Cut( + if followed_by_equals { + 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( - CompilationError::fatal( - SourceRange::from(arg), - "This argument needs a label, but it doesn't have one", - ) - .into(), - ) - }; - return Err(err); + args.push(LabeledArg { label: None, arg }); + } } } Ok((args, non_code_nodes)) @@ -3098,7 +3097,9 @@ fn fn_call_kw(i: &mut TokenSlice) -> PResult> { // 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() { diff --git a/rust/kcl-lib/src/std/args.rs b/rust/kcl-lib/src/std/args.rs index 2bdf4af05..7daf2018f 100644 --- a/rust/kcl-lib/src/std/args.rs +++ b/rust/kcl-lib/src/std/args.rs @@ -62,6 +62,7 @@ pub struct KwArgs { pub unlabeled: Option, /// Labeled args. pub labeled: IndexMap, + pub errors: Vec, } impl KwArgs { diff --git a/rust/kcl-lib/src/std/array.rs b/rust/kcl-lib/src/std/array.rs index 2c68589c1..77b285b2d 100644 --- a/rust/kcl-lib/src/std/array.rs +++ b/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, diff --git a/rust/kcl-lib/src/std/patterns.rs b/rust/kcl-lib/src/std/patterns.rs index 4884ec854..f95ff7662 100644 --- a/rust/kcl-lib/src/std/patterns.rs +++ b/rust/kcl-lib/src/std/patterns.rs @@ -430,6 +430,7 @@ async fn make_transform( 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, diff --git a/rust/kcl-lib/src/unparser.rs b/rust/kcl-lib/src/unparser.rs index 14fcc8dfc..b4055ff83 100644 --- a/rust/kcl-lib/src/unparser.rs +++ b/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 arg = self.arg.recast(options, indentation_level, ctxt); - format!("{label} = {arg}") + let mut result = String::new(); + if let Some(l) = &self.label { + result.push_str(&l.name); + result.push_str(" = "); + } + result.push_str(&self.arg.recast(options, indentation_level, ctxt)); + result } } diff --git a/src/lang/modifyAst.ts b/src/lang/modifyAst.ts index 636bcc6e4..c9279faa4 100644 --- a/src/lang/modifyAst.ts +++ b/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 } diff --git a/src/lang/modifyAst/addEdgeTreatment.ts b/src/lang/modifyAst/addEdgeTreatment.ts index 8deafa1ea..9d2e6af45 100644 --- a/src/lang/modifyAst/addEdgeTreatment.ts +++ b/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 diff --git a/src/lang/modifyAst/tagManagement.ts b/src/lang/modifyAst/tagManagement.ts index ab14a4c31..b2c6cabaf 100644 --- a/src/lang/modifyAst/tagManagement.ts +++ b/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') { diff --git a/src/lang/std/sketch.ts b/src/lang/std/sketch.ts index 2b690e14f..f257063fd 100644 --- a/src/lang/std/sketch.ts +++ b/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' ) diff --git a/src/lang/std/sketchcombos.ts b/src/lang/std/sketchcombos.ts index 506827f71..a70097fa6 100644 --- a/src/lang/std/sketchcombos.ts +++ b/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} ` ) diff --git a/src/lang/util.ts b/src/lang/util.ts index 40342802c..eb67fc69c 100644 --- a/src/lang/util.ts +++ b/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 diff --git a/src/lib/kclHelpers.ts b/src/lib/kclHelpers.ts index 9980dfbec..284ce10d0 100644 --- a/src/lib/kclHelpers.ts +++ b/src/lib/kclHelpers.ts @@ -60,7 +60,7 @@ export async function retrieveArgFromPipedCallExpression( name: string ): Promise { 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' && diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 2fed50c17..14b4c0b46 100644 --- a/src/lib/utils.ts +++ b/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) } /**