From dd370a93653ad5ec3625753d058a7dd01c171dca Mon Sep 17 00:00:00 2001 From: Adam Chalmers Date: Fri, 6 Dec 2024 15:44:39 -0600 Subject: [PATCH] AST: Allow unlabeled kw args (#4686) When declaring a function, its first parameter is allowed to be prefixed with `@`. This means that when users call this function, they don't have to label this argument. Only the first parameter is allowed this prefix, no others. Part of https://github.com/KittyCAD/modeling-app/issues/4600 --- src/wasm-lib/justfile | 1 - src/wasm-lib/kcl/src/parsing/ast/types/mod.rs | 12 ++ src/wasm-lib/kcl/src/parsing/parser.rs | 50 +++++- ...t_tests__kw_function_decl_all_labeled.snap | 75 +++++++++ ...sts__kw_function_decl_first_unlabeled.snap | 76 +++++++++ src/wasm-lib/kcl/src/simulation_tests.rs | 21 +++ src/wasm-lib/kcl/src/unparser.rs | 6 +- src/wasm-lib/kcl/tests/kw_fn/ast.snap | 138 ++++++++++++++++ src/wasm-lib/kcl/tests/kw_fn/input.kcl | 5 + .../kcl/tests/kw_fn/program_memory.snap | 150 ++++++++++++++++++ 10 files changed, 527 insertions(+), 7 deletions(-) create mode 100644 src/wasm-lib/kcl/src/parsing/snapshots/kcl_lib__parsing__parser__snapshot_tests__kw_function_decl_all_labeled.snap create mode 100644 src/wasm-lib/kcl/src/parsing/snapshots/kcl_lib__parsing__parser__snapshot_tests__kw_function_decl_first_unlabeled.snap create mode 100644 src/wasm-lib/kcl/tests/kw_fn/ast.snap create mode 100644 src/wasm-lib/kcl/tests/kw_fn/input.kcl create mode 100644 src/wasm-lib/kcl/tests/kw_fn/program_memory.snap diff --git a/src/wasm-lib/justfile b/src/wasm-lib/justfile index fe45630b1..8ace3c7f9 100644 --- a/src/wasm-lib/justfile +++ b/src/wasm-lib/justfile @@ -15,7 +15,6 @@ redo-kcl-stdlib-docs: TWENTY_TWENTY=overwrite {{cnr}} -p kcl-lib kcl_test_example EXPECTORATE=overwrite {{cnr}} -p kcl-lib docs::gen_std_tests::test_generate_stdlib - # Copy a test KCL file from executor tests into a new simulation test. copy-exec-test-into-sim-test test_name: mkdir -p kcl/tests/{{test_name}} diff --git a/src/wasm-lib/kcl/src/parsing/ast/types/mod.rs b/src/wasm-lib/kcl/src/parsing/ast/types/mod.rs index 3776c641a..ba8f84eb8 100644 --- a/src/wasm-lib/kcl/src/parsing/ast/types/mod.rs +++ b/src/wasm-lib/kcl/src/parsing/ast/types/mod.rs @@ -2834,6 +2834,18 @@ impl Parameter { } } +impl From<&Parameter> for SourceRange { + fn from(p: &Parameter) -> Self { + let sr = Self::from(&p.identifier); + // If it's unlabelled, the span should start 1 char earlier than the identifier, + // to include the '@' symbol. + if !p.labeled { + return Self::new(sr.start() - 1, sr.end(), sr.module_id()); + } + sr + } +} + fn is_true(b: &bool) -> bool { *b } diff --git a/src/wasm-lib/kcl/src/parsing/parser.rs b/src/wasm-lib/kcl/src/parsing/parser.rs index 7aa65642b..2a8816dd2 100644 --- a/src/wasm-lib/kcl/src/parsing/parser.rs +++ b/src/wasm-lib/kcl/src/parsing/parser.rs @@ -2180,6 +2180,11 @@ fn question_mark(i: TokenSlice) -> PResult<()> { Ok(()) } +fn at_sign(i: TokenSlice) -> PResult<()> { + TokenType::At.parse_from(i)?; + Ok(()) +} + fn fun(i: TokenSlice) -> PResult { any.try_map(|token: Token| match token.token_type { TokenType::Keyword if token.value == "fn" => Ok(token), @@ -2252,13 +2257,15 @@ fn argument_type(i: TokenSlice) -> PResult { } struct ParamDescription { + labeled: bool, arg_name: Token, type_: std::option::Option, is_optional: bool, } fn parameter(i: TokenSlice) -> PResult { - let (arg_name, optional, _, type_) = ( + let (found_at_sign, arg_name, optional, _, type_) = ( + opt(at_sign), any.verify(|token: &Token| !matches!(token.token_type, TokenType::Brace) || token.value != ")"), opt(question_mark), opt(whitespace), @@ -2266,6 +2273,7 @@ fn parameter(i: TokenSlice) -> PResult { ) .parse_next(i)?; Ok(ParamDescription { + labeled: found_at_sign.is_none(), arg_name, type_, is_optional: optional.is_some(), @@ -2284,6 +2292,7 @@ fn parameters(i: TokenSlice) -> PResult> { .into_iter() .map( |ParamDescription { + labeled, arg_name, type_, is_optional, @@ -2299,7 +2308,7 @@ fn parameters(i: TokenSlice) -> PResult> { } else { None }, - labeled: true, + labeled, digest: None, }) }, @@ -2307,6 +2316,15 @@ fn parameters(i: TokenSlice) -> PResult> { .collect::>() .map_err(|e: CompilationError| ErrMode::Backtrack(ContextError::from(e)))?; + // Make sure the only unlabeled parameter is the first one. + if let Some(param) = params.iter().skip(1).find(|param| !param.labeled) { + let source_range = SourceRange::from(param); + return Err(ErrMode::Cut(ContextError::from(CompilationError::fatal( + source_range, + "Only the first parameter can be declared unlabeled", + )))); + } + // Make sure optional parameters are last. if let Err(e) = optional_after_required(¶ms) { return Err(ErrMode::Cut(ContextError::from(e))); @@ -3475,12 +3493,18 @@ const mySk1 = startSketchAt([0, 0])"#; } #[track_caller] - fn assert_err(p: &str, msg: &str, src: [usize; 2]) { + fn assert_err(p: &str, msg: &str, src_expected: [usize; 2]) { let result = crate::parsing::top_level_parse(p); let err = result.unwrap_errs().next().unwrap(); assert_eq!(err.message, msg); - assert_eq!(err.source_range.start(), src[0]); - assert_eq!(err.source_range.end(), src[1]); + let src_actual = [err.source_range.start(), err.source_range.end()]; + assert_eq!( + src_expected, + src_actual, + "expected error would highlight {} but it actually highlighted {}", + &p[src_expected[0]..src_expected[1]], + &p[src_actual[0]..src_actual[1]], + ); } #[track_caller] @@ -3589,6 +3613,20 @@ const secondExtrude = startSketchOn('XY') assert_err(">!", "Unexpected token: >", [0, 1]); } + #[test] + fn test_parse_unlabeled_param_not_allowed() { + assert_err( + "fn f(@x, @y) { return 1 }", + "Only the first parameter can be declared unlabeled", + [9, 11], + ); + assert_err( + "fn f(x, @y) { return 1 }", + "Only the first parameter can be declared unlabeled", + [8, 10], + ); + } + #[test] fn test_parse_z_percent_parens() { assert_err("z%)", "Unexpected token: %", [1, 2]); @@ -4478,6 +4516,8 @@ const my14 = 4 ^ 2 - 3 ^ 2 * 2 ); snapshot_test!(kw_function_unnamed_first, r#"val = foo(x, y: z)"#); snapshot_test!(kw_function_all_named, r#"val = foo(x: a, y: b)"#); + snapshot_test!(kw_function_decl_all_labeled, r#"fn foo(x, y) { return 1 }"#); + snapshot_test!(kw_function_decl_first_unlabeled, r#"fn foo(@x, y) { return 1 }"#); } #[allow(unused)] diff --git a/src/wasm-lib/kcl/src/parsing/snapshots/kcl_lib__parsing__parser__snapshot_tests__kw_function_decl_all_labeled.snap b/src/wasm-lib/kcl/src/parsing/snapshots/kcl_lib__parsing__parser__snapshot_tests__kw_function_decl_all_labeled.snap new file mode 100644 index 000000000..d7616e011 --- /dev/null +++ b/src/wasm-lib/kcl/src/parsing/snapshots/kcl_lib__parsing__parser__snapshot_tests__kw_function_decl_all_labeled.snap @@ -0,0 +1,75 @@ +--- +source: kcl/src/parsing/parser.rs +expression: actual +snapshot_kind: text +--- +{ + "body": [ + { + "declaration": { + "end": 25, + "id": { + "end": 6, + "name": "foo", + "start": 3, + "type": "Identifier" + }, + "init": { + "body": { + "body": [ + { + "argument": { + "end": 23, + "raw": "1", + "start": 22, + "type": "Literal", + "type": "Literal", + "value": 1.0 + }, + "end": 23, + "start": 15, + "type": "ReturnStatement", + "type": "ReturnStatement" + } + ], + "end": 25, + "start": 13 + }, + "end": 25, + "params": [ + { + "type": "Parameter", + "identifier": { + "end": 8, + "name": "x", + "start": 7, + "type": "Identifier" + } + }, + { + "type": "Parameter", + "identifier": { + "end": 11, + "name": "y", + "start": 10, + "type": "Identifier" + } + } + ], + "start": 6, + "type": "FunctionExpression", + "type": "FunctionExpression" + }, + "start": 3, + "type": "VariableDeclarator" + }, + "end": 25, + "kind": "fn", + "start": 0, + "type": "VariableDeclaration", + "type": "VariableDeclaration" + } + ], + "end": 25, + "start": 0 +} diff --git a/src/wasm-lib/kcl/src/parsing/snapshots/kcl_lib__parsing__parser__snapshot_tests__kw_function_decl_first_unlabeled.snap b/src/wasm-lib/kcl/src/parsing/snapshots/kcl_lib__parsing__parser__snapshot_tests__kw_function_decl_first_unlabeled.snap new file mode 100644 index 000000000..cc1a4838a --- /dev/null +++ b/src/wasm-lib/kcl/src/parsing/snapshots/kcl_lib__parsing__parser__snapshot_tests__kw_function_decl_first_unlabeled.snap @@ -0,0 +1,76 @@ +--- +source: kcl/src/parsing/parser.rs +expression: actual +snapshot_kind: text +--- +{ + "body": [ + { + "declaration": { + "end": 26, + "id": { + "end": 6, + "name": "foo", + "start": 3, + "type": "Identifier" + }, + "init": { + "body": { + "body": [ + { + "argument": { + "end": 24, + "raw": "1", + "start": 23, + "type": "Literal", + "type": "Literal", + "value": 1.0 + }, + "end": 24, + "start": 16, + "type": "ReturnStatement", + "type": "ReturnStatement" + } + ], + "end": 26, + "start": 14 + }, + "end": 26, + "params": [ + { + "type": "Parameter", + "identifier": { + "end": 9, + "name": "x", + "start": 8, + "type": "Identifier" + }, + "labeled": false + }, + { + "type": "Parameter", + "identifier": { + "end": 12, + "name": "y", + "start": 11, + "type": "Identifier" + } + } + ], + "start": 6, + "type": "FunctionExpression", + "type": "FunctionExpression" + }, + "start": 3, + "type": "VariableDeclarator" + }, + "end": 26, + "kind": "fn", + "start": 0, + "type": "VariableDeclaration", + "type": "VariableDeclaration" + } + ], + "end": 26, + "start": 0 +} diff --git a/src/wasm-lib/kcl/src/simulation_tests.rs b/src/wasm-lib/kcl/src/simulation_tests.rs index 8cd854c7d..9225dcba6 100644 --- a/src/wasm-lib/kcl/src/simulation_tests.rs +++ b/src/wasm-lib/kcl/src/simulation_tests.rs @@ -1481,3 +1481,24 @@ mod kittycad_svg { super::execute(TEST_NAME, true).await } } +mod kw_fn { + const TEST_NAME: &str = "kw_fn"; + + /// Test parsing KCL. + #[test] + fn parse() { + super::parse(TEST_NAME) + } + + /// Test that parsing and unparsing KCL produces the original KCL input. + #[test] + fn unparse() { + super::unparse(TEST_NAME) + } + + /// Test that KCL is executed correctly. + #[tokio::test(flavor = "multi_thread")] + async fn kcl_test_execute() { + super::execute(TEST_NAME, true).await + } +} diff --git a/src/wasm-lib/kcl/src/unparser.rs b/src/wasm-lib/kcl/src/unparser.rs index 16b61c50a..78ee51516 100644 --- a/src/wasm-lib/kcl/src/unparser.rs +++ b/src/wasm-lib/kcl/src/unparser.rs @@ -659,7 +659,11 @@ impl FunctionExpression { impl Parameter { pub fn recast(&self, options: &FormatOptions, indentation_level: usize) -> String { - let mut result = self.identifier.name.clone(); + let mut result = format!( + "{}{}", + if self.labeled { "" } else { "@" }, + self.identifier.name.clone() + ); if let Some(ty) = &self.type_ { result += ": "; result += &ty.recast(options, indentation_level); diff --git a/src/wasm-lib/kcl/tests/kw_fn/ast.snap b/src/wasm-lib/kcl/tests/kw_fn/ast.snap new file mode 100644 index 000000000..6d66a0f7c --- /dev/null +++ b/src/wasm-lib/kcl/tests/kw_fn/ast.snap @@ -0,0 +1,138 @@ +--- +source: kcl/src/simulation_tests.rs +description: Result of parsing kw_fn.kcl +snapshot_kind: text +--- +{ + "Ok": { + "body": [ + { + "declaration": { + "end": 35, + "id": { + "end": 12, + "name": "increment", + "start": 3, + "type": "Identifier" + }, + "init": { + "body": { + "body": [ + { + "argument": { + "end": 33, + "left": { + "end": 29, + "name": "x", + "start": 28, + "type": "Identifier", + "type": "Identifier" + }, + "operator": "+", + "right": { + "end": 33, + "raw": "1", + "start": 32, + "type": "Literal", + "type": "Literal", + "value": 1.0 + }, + "start": 28, + "type": "BinaryExpression", + "type": "BinaryExpression" + }, + "end": 33, + "start": 21, + "type": "ReturnStatement", + "type": "ReturnStatement" + } + ], + "end": 35, + "start": 17 + }, + "end": 35, + "params": [ + { + "type": "Parameter", + "identifier": { + "end": 15, + "name": "x", + "start": 14, + "type": "Identifier" + }, + "labeled": false + } + ], + "start": 12, + "type": "FunctionExpression", + "type": "FunctionExpression" + }, + "start": 3, + "type": "VariableDeclarator" + }, + "end": 35, + "kind": "fn", + "start": 0, + "type": "VariableDeclaration", + "type": "VariableDeclaration" + }, + { + "declaration": { + "end": 55, + "id": { + "end": 40, + "name": "two", + "start": 37, + "type": "Identifier" + }, + "init": { + "arguments": [ + { + "end": 54, + "raw": "1", + "start": 53, + "type": "Literal", + "type": "Literal", + "value": 1.0 + } + ], + "callee": { + "end": 52, + "name": "increment", + "start": 43, + "type": "Identifier" + }, + "end": 55, + "start": 43, + "type": "CallExpression", + "type": "CallExpression" + }, + "start": 37, + "type": "VariableDeclarator" + }, + "end": 55, + "kind": "const", + "start": 37, + "type": "VariableDeclaration", + "type": "VariableDeclaration" + } + ], + "end": 56, + "nonCodeMeta": { + "nonCodeNodes": { + "0": [ + { + "end": 37, + "start": 35, + "type": "NonCodeNode", + "value": { + "type": "newLine" + } + } + ] + }, + "startNodes": [] + }, + "start": 0 + } +} diff --git a/src/wasm-lib/kcl/tests/kw_fn/input.kcl b/src/wasm-lib/kcl/tests/kw_fn/input.kcl new file mode 100644 index 000000000..f00997a4c --- /dev/null +++ b/src/wasm-lib/kcl/tests/kw_fn/input.kcl @@ -0,0 +1,5 @@ +fn increment(@x) { + return x + 1 +} + +two = increment(1) diff --git a/src/wasm-lib/kcl/tests/kw_fn/program_memory.snap b/src/wasm-lib/kcl/tests/kw_fn/program_memory.snap new file mode 100644 index 000000000..09da46ae4 --- /dev/null +++ b/src/wasm-lib/kcl/tests/kw_fn/program_memory.snap @@ -0,0 +1,150 @@ +--- +source: kcl/src/simulation_tests.rs +description: Program memory after executing kw_fn.kcl +snapshot_kind: text +--- +{ + "environments": [ + { + "bindings": { + "HALF_TURN": { + "type": "Number", + "value": 180.0, + "__meta": [] + }, + "QUARTER_TURN": { + "type": "Number", + "value": 90.0, + "__meta": [] + }, + "THREE_QUARTER_TURN": { + "type": "Number", + "value": 270.0, + "__meta": [] + }, + "ZERO": { + "type": "Number", + "value": 0.0, + "__meta": [] + }, + "increment": { + "type": "Function", + "expression": { + "body": { + "body": [ + { + "argument": { + "end": 33, + "left": { + "end": 29, + "name": "x", + "start": 28, + "type": "Identifier", + "type": "Identifier" + }, + "operator": "+", + "right": { + "end": 33, + "raw": "1", + "start": 32, + "type": "Literal", + "type": "Literal", + "value": 1.0 + }, + "start": 28, + "type": "BinaryExpression", + "type": "BinaryExpression" + }, + "end": 33, + "start": 21, + "type": "ReturnStatement", + "type": "ReturnStatement" + } + ], + "end": 35, + "start": 17 + }, + "end": 35, + "params": [ + { + "type": "Parameter", + "identifier": { + "end": 15, + "name": "x", + "start": 14, + "type": "Identifier" + }, + "labeled": false + } + ], + "start": 12, + "type": "FunctionExpression" + }, + "memory": { + "environments": [ + { + "bindings": { + "HALF_TURN": { + "type": "Number", + "value": 180.0, + "__meta": [] + }, + "QUARTER_TURN": { + "type": "Number", + "value": 90.0, + "__meta": [] + }, + "THREE_QUARTER_TURN": { + "type": "Number", + "value": 270.0, + "__meta": [] + }, + "ZERO": { + "type": "Number", + "value": 0.0, + "__meta": [] + } + }, + "parent": null + } + ], + "currentEnv": 0, + "return": null + }, + "__meta": [ + { + "sourceRange": [ + 12, + 35, + 0 + ] + } + ] + }, + "two": { + "type": "Number", + "value": 2.0, + "__meta": [ + { + "sourceRange": [ + 53, + 54, + 0 + ] + }, + { + "sourceRange": [ + 32, + 33, + 0 + ] + } + ] + } + }, + "parent": null + } + ], + "currentEnv": 0, + "return": null +}