From 3f3693e12df9182c62c953e7252051699cd76de9 Mon Sep 17 00:00:00 2001 From: Adam Chalmers Date: Tue, 3 Jun 2025 19:05:40 -0500 Subject: [PATCH] Type ascription produces two incompatible fields (#7355) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Symptoms This code produces a big ugly confusing error in the frontend, see #7340. # Root cause I added a new test case, with an unknown type. In `ast.snap` under `body[0].declaration.init.ty` there two different `type` fields in the AST node for the type's name, and they have conflicting values Primitive and Identifier. Screenshot 2025-06-03 at 4 04 55 PM # Solution Change the `enum PrimitiveType` variant from `Named(Node)` to `Named { name: Node }` so that the fields nest differently. Now the error correctly points out to the user that the type `NotARealType` can't be found. Much better error message that shows the user the problem. # Alternative solutions Stop the duplicated JSON fields altogether. I tried this previously in https://github.com/KittyCAD/modeling-app/pull/4369 but it was very involved, and I didn't think it was worth it. Maybe I should reopen that PR and solve this properly. Closes #7340 --- rust/kcl-lib/src/execution/types.rs | 2 +- rust/kcl-lib/src/parsing/ast/digest.rs | 2 +- rust/kcl-lib/src/parsing/ast/types/mod.rs | 4 +- rust/kcl-lib/src/parsing/parser.rs | 2 +- rust/kcl-lib/src/simulation_tests.rs | 21 ++++++ .../artifact_commands.snap | 32 +++++++++ .../artifact_graph_flowchart.snap | 6 ++ .../artifact_graph_flowchart.snap.md | 3 + .../tests/ascription_unknown_type/ast.snap | 67 +++++++++++++++++++ .../execution_error.snap | 12 ++++ .../tests/ascription_unknown_type/input.kcl | 1 + .../tests/ascription_unknown_type/ops.snap | 5 ++ .../ascription_unknown_type/unparsed.snap | 5 ++ 13 files changed, 157 insertions(+), 5 deletions(-) create mode 100644 rust/kcl-lib/tests/ascription_unknown_type/artifact_commands.snap create mode 100644 rust/kcl-lib/tests/ascription_unknown_type/artifact_graph_flowchart.snap create mode 100644 rust/kcl-lib/tests/ascription_unknown_type/artifact_graph_flowchart.snap.md create mode 100644 rust/kcl-lib/tests/ascription_unknown_type/ast.snap create mode 100644 rust/kcl-lib/tests/ascription_unknown_type/execution_error.snap create mode 100644 rust/kcl-lib/tests/ascription_unknown_type/input.kcl create mode 100644 rust/kcl-lib/tests/ascription_unknown_type/ops.snap create mode 100644 rust/kcl-lib/tests/ascription_unknown_type/unparsed.snap diff --git a/rust/kcl-lib/src/execution/types.rs b/rust/kcl-lib/src/execution/types.rs index 6eb3ac2a4..27908e986 100644 --- a/rust/kcl-lib/src/execution/types.rs +++ b/rust/kcl-lib/src/execution/types.rs @@ -187,7 +187,7 @@ impl RuntimeType { }; RuntimeType::Primitive(PrimitiveType::Number(ty)) } - AstPrimitiveType::Named(name) => Self::from_alias(&name.name, exec_state, source_range)?, + AstPrimitiveType::Named { id } => Self::from_alias(&id.name, exec_state, source_range)?, AstPrimitiveType::Tag => RuntimeType::Primitive(PrimitiveType::Tag), AstPrimitiveType::ImportedGeometry => RuntimeType::Primitive(PrimitiveType::ImportedGeometry), AstPrimitiveType::Function(_) => RuntimeType::Primitive(PrimitiveType::Function), diff --git a/rust/kcl-lib/src/parsing/ast/digest.rs b/rust/kcl-lib/src/parsing/ast/digest.rs index 42a413c2f..c1099baf3 100644 --- a/rust/kcl-lib/src/parsing/ast/digest.rs +++ b/rust/kcl-lib/src/parsing/ast/digest.rs @@ -228,7 +228,7 @@ impl PrimitiveType { let mut hasher = Sha256::new(); match self { PrimitiveType::Any => hasher.update(b"any"), - PrimitiveType::Named(id) => hasher.update(id.compute_digest()), + PrimitiveType::Named { id } => hasher.update(id.compute_digest()), PrimitiveType::String => hasher.update(b"string"), PrimitiveType::Number(suffix) => hasher.update(suffix.digestable_id()), PrimitiveType::Boolean => hasher.update(b"bool"), diff --git a/rust/kcl-lib/src/parsing/ast/types/mod.rs b/rust/kcl-lib/src/parsing/ast/types/mod.rs index b9a208c3d..a01bd3309 100644 --- a/rust/kcl-lib/src/parsing/ast/types/mod.rs +++ b/rust/kcl-lib/src/parsing/ast/types/mod.rs @@ -3230,7 +3230,7 @@ pub enum PrimitiveType { /// `fn`, type of functions. Function(FunctionType), /// An identifier used as a type (not really a primitive type, but whatever). - Named(Node), + Named { id: Node }, } impl PrimitiveType { @@ -3286,7 +3286,7 @@ impl fmt::Display for PrimitiveType { } Ok(()) } - PrimitiveType::Named(n) => write!(f, "{}", n.name), + PrimitiveType::Named { id: n } => write!(f, "{}", n.name), } } } diff --git a/rust/kcl-lib/src/parsing/parser.rs b/rust/kcl-lib/src/parsing/parser.rs index f71aac3b7..ff354915a 100644 --- a/rust/kcl-lib/src/parsing/parser.rs +++ b/rust/kcl-lib/src/parsing/parser.rs @@ -2938,7 +2938,7 @@ fn primitive_type(i: &mut TokenSlice) -> ModalResult> { (identifier, opt(delimited(open_paren, uom_for_type, close_paren))).map(|(ident, suffix)| { let mut result = Node::new(PrimitiveType::Boolean, ident.start, ident.end, ident.module_id); result.inner = - PrimitiveType::primitive_from_str(&ident.name, suffix).unwrap_or(PrimitiveType::Named(ident)); + PrimitiveType::primitive_from_str(&ident.name, suffix).unwrap_or(PrimitiveType::Named { id: ident }); result }), )) diff --git a/rust/kcl-lib/src/simulation_tests.rs b/rust/kcl-lib/src/simulation_tests.rs index a834ceb4b..39d2b609a 100644 --- a/rust/kcl-lib/src/simulation_tests.rs +++ b/rust/kcl-lib/src/simulation_tests.rs @@ -3504,3 +3504,24 @@ mod var_ref_in_own_def { super::execute(TEST_NAME, true).await } } +mod ascription_unknown_type { + const TEST_NAME: &str = "ascription_unknown_type"; + + /// Test parsing KCL. + #[test] + fn parse() { + super::parse(TEST_NAME) + } + + /// Test that parsing and unparsing KCL produces the original KCL input. + #[tokio::test(flavor = "multi_thread")] + async fn unparse() { + super::unparse(TEST_NAME).await + } + + /// 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/rust/kcl-lib/tests/ascription_unknown_type/artifact_commands.snap b/rust/kcl-lib/tests/ascription_unknown_type/artifact_commands.snap new file mode 100644 index 000000000..2e21d5a35 --- /dev/null +++ b/rust/kcl-lib/tests/ascription_unknown_type/artifact_commands.snap @@ -0,0 +1,32 @@ +--- +source: kcl-lib/src/simulation_tests.rs +description: Artifact commands ascription_unknown_type.kcl +--- +[ + { + "cmdId": "[uuid]", + "range": [], + "command": { + "type": "edge_lines_visible", + "hidden": false + } + }, + { + "cmdId": "[uuid]", + "range": [], + "command": { + "type": "object_visible", + "object_id": "[uuid]", + "hidden": true + } + }, + { + "cmdId": "[uuid]", + "range": [], + "command": { + "type": "object_visible", + "object_id": "[uuid]", + "hidden": true + } + } +] diff --git a/rust/kcl-lib/tests/ascription_unknown_type/artifact_graph_flowchart.snap b/rust/kcl-lib/tests/ascription_unknown_type/artifact_graph_flowchart.snap new file mode 100644 index 000000000..4f98534a2 --- /dev/null +++ b/rust/kcl-lib/tests/ascription_unknown_type/artifact_graph_flowchart.snap @@ -0,0 +1,6 @@ +--- +source: kcl-lib/src/simulation_tests.rs +description: Artifact graph flowchart ascription_unknown_type.kcl +extension: md +snapshot_kind: binary +--- diff --git a/rust/kcl-lib/tests/ascription_unknown_type/artifact_graph_flowchart.snap.md b/rust/kcl-lib/tests/ascription_unknown_type/artifact_graph_flowchart.snap.md new file mode 100644 index 000000000..13e533509 --- /dev/null +++ b/rust/kcl-lib/tests/ascription_unknown_type/artifact_graph_flowchart.snap.md @@ -0,0 +1,3 @@ +```mermaid +flowchart LR +``` diff --git a/rust/kcl-lib/tests/ascription_unknown_type/ast.snap b/rust/kcl-lib/tests/ascription_unknown_type/ast.snap new file mode 100644 index 000000000..b4f269361 --- /dev/null +++ b/rust/kcl-lib/tests/ascription_unknown_type/ast.snap @@ -0,0 +1,67 @@ +--- +source: kcl-lib/src/simulation_tests.rs +description: Result of parsing ascription_unknown_type.kcl +--- +{ + "Ok": { + "body": [ + { + "commentStart": 0, + "declaration": { + "commentStart": 0, + "end": 0, + "id": { + "commentStart": 0, + "end": 0, + "name": "z", + "start": 0, + "type": "Identifier" + }, + "init": { + "commentStart": 0, + "end": 0, + "expr": { + "commentStart": 0, + "end": 0, + "raw": "10", + "start": 0, + "type": "Literal", + "type": "Literal", + "value": { + "value": 10.0, + "suffix": "None" + } + }, + "start": 0, + "ty": { + "commentStart": 0, + "end": 0, + "id": { + "commentStart": 0, + "end": 0, + "name": "NotARealType", + "start": 0, + "type": "Identifier" + }, + "p_type": "Named", + "start": 0, + "type": "Primitive" + }, + "type": "AscribedExpression", + "type": "AscribedExpression" + }, + "start": 0, + "type": "VariableDeclarator" + }, + "end": 0, + "kind": "const", + "start": 0, + "type": "VariableDeclaration", + "type": "VariableDeclaration" + } + ], + "commentStart": 0, + "end": 0, + "start": 0 + } +} diff --git a/rust/kcl-lib/tests/ascription_unknown_type/execution_error.snap b/rust/kcl-lib/tests/ascription_unknown_type/execution_error.snap new file mode 100644 index 000000000..88ff3370e --- /dev/null +++ b/rust/kcl-lib/tests/ascription_unknown_type/execution_error.snap @@ -0,0 +1,12 @@ +--- +source: kcl-lib/src/simulation_tests.rs +description: Error from executing ascription_unknown_type.kcl +--- +KCL Semantic error + + × semantic: Unknown type: NotARealType + ╭──── + 1 │ z = 10: NotARealType + · ─┬ + · ╰── tests/ascription_unknown_type/input.kcl + ╰──── diff --git a/rust/kcl-lib/tests/ascription_unknown_type/input.kcl b/rust/kcl-lib/tests/ascription_unknown_type/input.kcl new file mode 100644 index 000000000..5f4f8b4fa --- /dev/null +++ b/rust/kcl-lib/tests/ascription_unknown_type/input.kcl @@ -0,0 +1 @@ +z = 10: NotARealType diff --git a/rust/kcl-lib/tests/ascription_unknown_type/ops.snap b/rust/kcl-lib/tests/ascription_unknown_type/ops.snap new file mode 100644 index 000000000..408fd7a1d --- /dev/null +++ b/rust/kcl-lib/tests/ascription_unknown_type/ops.snap @@ -0,0 +1,5 @@ +--- +source: kcl-lib/src/simulation_tests.rs +description: Operations executed ascription_unknown_type.kcl +--- +[] diff --git a/rust/kcl-lib/tests/ascription_unknown_type/unparsed.snap b/rust/kcl-lib/tests/ascription_unknown_type/unparsed.snap new file mode 100644 index 000000000..708360a3a --- /dev/null +++ b/rust/kcl-lib/tests/ascription_unknown_type/unparsed.snap @@ -0,0 +1,5 @@ +--- +source: kcl-lib/src/simulation_tests.rs +description: Result of unparsing ascription_unknown_type.kcl +--- +z = 10: NotARealType