KCL: Fix 'cryptic' error when referencing a variable in its own declaration (#7325)

Previously, `x = cos(x)` would just say "`x` is undefined". Now it says that `x` cannot be referenced in its own definition, try using a different variable instead.

To do this, I've added a new `Option<String>` field to the mod-local executor context, tracking the current variable declaration. This means cloning some strings, implying a small performance hit. I think it's fine, for the better diagnostics.

In the future we could refactor this to use a &str or store variable labels in stack-allocated strings like docs.rs/compact_str or something.

Closes https://github.com/KittyCAD/modeling-app/issues/6072

rust/kcl-lib/src/errors.rs

@@ -108,7 +108,10 @@ pub enum KclError {
     #[error("value already defined: {details:?}")]
     ValueAlreadyDefined { details: KclErrorDetails },
     #[error("undefined value: {details:?}")]
-    UndefinedValue { details: KclErrorDetails },
+    UndefinedValue {
+        details: KclErrorDetails,
+        name: Option<String>,
+    },
     #[error("invalid expression: {details:?}")]
     InvalidExpression { details: KclErrorDetails },
     #[error("engine: {details:?}")]
@@ -451,8 +454,8 @@ impl KclError {
         KclError::Lexical { details }
     }
 
-    pub fn new_undefined_value(details: KclErrorDetails) -> KclError {
+    pub fn new_undefined_value(details: KclErrorDetails, name: Option<String>) -> KclError {
-        KclError::UndefinedValue { details }
+        KclError::UndefinedValue { details, name }
     }
 
     pub fn new_type(details: KclErrorDetails) -> KclError {
@@ -491,7 +494,7 @@ impl KclError {
             KclError::Io { details: e } => e.source_ranges.clone(),
             KclError::Unexpected { details: e } => e.source_ranges.clone(),
             KclError::ValueAlreadyDefined { details: e } => e.source_ranges.clone(),
-            KclError::UndefinedValue { details: e } => e.source_ranges.clone(),
+            KclError::UndefinedValue { details: e, .. } => e.source_ranges.clone(),
             KclError::InvalidExpression { details: e } => e.source_ranges.clone(),
             KclError::Engine { details: e } => e.source_ranges.clone(),
             KclError::Internal { details: e } => e.source_ranges.clone(),
@@ -509,7 +512,7 @@ impl KclError {
             KclError::Io { details: e } => &e.message,
             KclError::Unexpected { details: e } => &e.message,
             KclError::ValueAlreadyDefined { details: e } => &e.message,
-            KclError::UndefinedValue { details: e } => &e.message,
+            KclError::UndefinedValue { details: e, .. } => &e.message,
             KclError::InvalidExpression { details: e } => &e.message,
             KclError::Engine { details: e } => &e.message,
             KclError::Internal { details: e } => &e.message,
@@ -526,7 +529,7 @@ impl KclError {
             | KclError::Io { details: e }
             | KclError::Unexpected { details: e }
             | KclError::ValueAlreadyDefined { details: e }
-            | KclError::UndefinedValue { details: e }
+            | KclError::UndefinedValue { details: e, .. }
             | KclError::InvalidExpression { details: e }
             | KclError::Engine { details: e }
             | KclError::Internal { details: e } => e.backtrace.clone(),
@@ -544,7 +547,7 @@ impl KclError {
             | KclError::Io { details: e }
             | KclError::Unexpected { details: e }
             | KclError::ValueAlreadyDefined { details: e }
-            | KclError::UndefinedValue { details: e }
+            | KclError::UndefinedValue { details: e, .. }
             | KclError::InvalidExpression { details: e }
             | KclError::Engine { details: e }
             | KclError::Internal { details: e } => {
@@ -573,7 +576,7 @@ impl KclError {
             | KclError::Io { details: e }
             | KclError::Unexpected { details: e }
             | KclError::ValueAlreadyDefined { details: e }
-            | KclError::UndefinedValue { details: e }
+            | KclError::UndefinedValue { details: e, .. }
             | KclError::InvalidExpression { details: e }
             | KclError::Engine { details: e }
             | KclError::Internal { details: e } => {
@@ -597,7 +600,7 @@ impl KclError {
             | KclError::Io { details: e }
             | KclError::Unexpected { details: e }
             | KclError::ValueAlreadyDefined { details: e }
-            | KclError::UndefinedValue { details: e }
+            | KclError::UndefinedValue { details: e, .. }
             | KclError::InvalidExpression { details: e }
             | KclError::Engine { details: e }
             | KclError::Internal { details: e } => {

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

@@ -164,10 +164,13 @@ impl ExecutorContext {
                                 let mut mod_value = mem.get_from(&mod_name, env_ref, import_item.into(), 0).cloned();
 
                                 if value.is_err() && ty.is_err() && mod_value.is_err() {
-                                    return Err(KclError::new_undefined_value(KclErrorDetails::new(
+                                    return Err(KclError::new_undefined_value(
-                                        format!("{} is not defined in module", import_item.name.name),
+                                        KclErrorDetails::new(
-                                        vec![SourceRange::from(&import_item.name)],
+                                            format!("{} is not defined in module", import_item.name.name),
-                                    )));
+                                            vec![SourceRange::from(&import_item.name)],
+                                        ),
+                                        None,
+                                    ));
                                 }
 
                                 // Check that the item is allowed to be imported (in at least one namespace).
@@ -301,7 +304,10 @@ impl ExecutorContext {
 
                     let annotations = &variable_declaration.outer_attrs;
 
-                    let value = self
+                    // During the evaluation of the variable's LHS, set context that this is all happening inside a variable
+                    // declaration, for the given name. This helps improve user-facing error messages.
+                    exec_state.mod_local.being_declared = Some(variable_declaration.inner.name().to_owned());
+                    let rhs_result = self
                         .execute_expr(
                             &variable_declaration.declaration.init,
                             exec_state,
@@ -309,10 +315,14 @@ impl ExecutorContext {
                             annotations,
                             StatementKind::Declaration { name: &var_name },
                         )
-                        .await?;
+                        .await;
+                    // Declaration over, so unset this context.
+                    exec_state.mod_local.being_declared = None;
+                    let rhs = rhs_result?;
+
                     exec_state
                         .mut_stack()
-                        .add(var_name.clone(), value.clone(), source_range)?;
+                        .add(var_name.clone(), rhs.clone(), source_range)?;
 
                     // Track exports.
                     if let ItemVisibility::Export = variable_declaration.visibility {
@@ -326,7 +336,7 @@ impl ExecutorContext {
                         }
                     }
                     // Variable declaration can be the return value of a module.
-                    last_expr = matches!(body_type, BodyType::Root).then_some(value);
+                    last_expr = matches!(body_type, BodyType::Root).then_some(rhs);
                 }
                 BodyItem::TypeDeclaration(ty) => {
                     let metadata = Metadata::from(&**ty);
@@ -913,10 +923,13 @@ impl Node<MemberExpression> {
                 if let Some(value) = map.get(&property) {
                     Ok(value.to_owned())
                 } else {
-                    Err(KclError::new_undefined_value(KclErrorDetails::new(
+                    Err(KclError::new_undefined_value(
-                        format!("Property '{property}' not found in object"),
+                        KclErrorDetails::new(
-                        vec![self.clone().into()],
+                            format!("Property '{property}' not found in object"),
-                    )))
+                            vec![self.clone().into()],
+                        ),
+                        None,
+                    ))
                 }
             }
             (KclValue::Object { .. }, Property::String(property), true) => {
@@ -938,10 +951,13 @@ impl Node<MemberExpression> {
                 if let Some(value) = value_of_arr {
                     Ok(value.to_owned())
                 } else {
-                    Err(KclError::new_undefined_value(KclErrorDetails::new(
+                    Err(KclError::new_undefined_value(
-                        format!("The array doesn't have any item at index {index}"),
+                        KclErrorDetails::new(
-                        vec![self.clone().into()],
+                            format!("The array doesn't have any item at index {index}"),
-                    )))
+                            vec![self.clone().into()],
+                        ),
+                        None,
+                    ))
                 }
             }
             // Singletons and single-element arrays should be interchangeable, but only indexing by 0 should work.

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

@@ -318,10 +318,13 @@ impl Node<CallExpressionKw> {
                     if let KclValue::Function { meta, .. } = func {
                         source_ranges = meta.iter().map(|m| m.source_range).collect();
                     };
-                    KclError::new_undefined_value(KclErrorDetails::new(
+                    KclError::new_undefined_value(
-                        format!("Result of user-defined function {} is undefined", fn_name),
+                        KclErrorDetails::new(
-                        source_ranges,
+                            format!("Result of user-defined function {} is undefined", fn_name),
-                    ))
+                            source_ranges,
+                        ),
+                        None,
+                    )
                 })?;
 
                 Ok(result)

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

@@ -367,10 +367,10 @@ impl ProgramMemory {
 
         let name = var.trim_start_matches(TYPE_PREFIX).trim_start_matches(MODULE_PREFIX);
 
-        Err(KclError::new_undefined_value(KclErrorDetails::new(
+        Err(KclError::new_undefined_value(
-            format!("`{name}` is not defined"),
+            KclErrorDetails::new(format!("`{name}` is not defined"), vec![source_range]),
-            vec![source_range],
+            Some(name.to_owned()),
-        )))
+        ))
     }
 
     /// Iterate over all key/value pairs in the specified environment which satisfy the provided
@@ -488,10 +488,10 @@ impl ProgramMemory {
             };
         }
 
-        Err(KclError::new_undefined_value(KclErrorDetails::new(
+        Err(KclError::new_undefined_value(
-            format!("`{}` is not defined", var),
+            KclErrorDetails::new(format!("`{}` is not defined", var), vec![]),
-            vec![],
+            Some(var.to_owned()),
-        )))
+        ))
     }
 }
 

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

@@ -80,6 +80,11 @@ pub(super) struct ModuleState {
     /// The current value of the pipe operator returned from the previous
     /// expression.  If we're not currently in a pipeline, this will be None.
     pub pipe_value: Option<KclValue>,
+    /// The closest variable declaration being executed in any parent node in the AST.
+    /// This is used to provide better error messages, e.g. noticing when the user is trying
+    /// to use the variable `length` inside the RHS of its own definition, like `length = tan(length)`.
+    /// TODO: Make this a reference.
+    pub being_declared: Option<String>,
     /// Identifiers that have been exported from the current module.
     pub module_exports: Vec<String>,
     /// Settings specified from annotations.
@@ -342,6 +347,7 @@ impl ModuleState {
             id_generator: IdGenerator::new(module_id),
             stack: memory.new_stack(),
             pipe_value: Default::default(),
+            being_declared: Default::default(),
             module_exports: Default::default(),
             explicit_length_units: false,
             path,

rust/kcl-lib/src/simulation_tests.rs

@@ -3483,3 +3483,24 @@ mod spheres {
         super::execute(TEST_NAME, true).await
     }
 }
+mod var_ref_in_own_def {
+    const TEST_NAME: &str = "var_ref_in_own_def";
+
+    /// 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
+    }
+}

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

@@ -23,7 +23,7 @@ pub async fn union(exec_state: &mut ExecState, args: Args) -> Result<KclValue, K
     let tolerance: Option<TyF64> = args.get_kw_arg_opt_typed("tolerance", &RuntimeType::length(), exec_state)?;
 
     if solids.len() < 2 {
-        return Err(KclError::new_undefined_value(KclErrorDetails::new(
+        return Err(KclError::new_semantic(KclErrorDetails::new(
             "At least two solids are required for a union operation.".to_string(),
             vec![args.source_range],
         )));
@@ -88,7 +88,7 @@ pub async fn intersect(exec_state: &mut ExecState, args: Args) -> Result<KclValu
     let tolerance: Option<TyF64> = args.get_kw_arg_opt_typed("tolerance", &RuntimeType::length(), exec_state)?;
 
     if solids.len() < 2 {
-        return Err(KclError::new_undefined_value(KclErrorDetails::new(
+        return Err(KclError::new_semantic(KclErrorDetails::new(
             "At least two solids are required for an intersect operation.".to_string(),
             vec![args.source_range],
         )));

rust/kcl-lib/tests/var_ref_in_own_def/artifact_commands.snap

@@ -0,0 +1,32 @@
+---
+source: kcl-lib/src/simulation_tests.rs
+description: Artifact commands var_ref_in_own_def.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
+    }
+  }
+]

rust/kcl-lib/tests/var_ref_in_own_def/artifact_graph_flowchart.snap

@@ -0,0 +1,6 @@
+---
+source: kcl-lib/src/simulation_tests.rs
+description: Artifact graph flowchart var_ref_in_own_def.kcl
+extension: md
+snapshot_kind: binary
+---

rust/kcl-lib/tests/var_ref_in_own_def/artifact_graph_flowchart.snap.md

@@ -0,0 +1,3 @@
+```mermaid
+flowchart LR
+```

rust/kcl-lib/tests/var_ref_in_own_def/ast.snap

@@ -0,0 +1,75 @@
+---
+source: kcl-lib/src/simulation_tests.rs
+description: Result of parsing var_ref_in_own_def.kcl
+---
+{
+  "Ok": {
+    "body": [
+      {
+        "commentStart": 0,
+        "declaration": {
+          "commentStart": 0,
+          "end": 0,
+          "id": {
+            "commentStart": 0,
+            "end": 0,
+            "name": "x",
+            "start": 0,
+            "type": "Identifier"
+          },
+          "init": {
+            "callee": {
+              "abs_path": false,
+              "commentStart": 0,
+              "end": 0,
+              "name": {
+                "commentStart": 0,
+                "end": 0,
+                "name": "cos",
+                "start": 0,
+                "type": "Identifier"
+              },
+              "path": [],
+              "start": 0,
+              "type": "Name"
+            },
+            "commentStart": 0,
+            "end": 0,
+            "start": 0,
+            "type": "CallExpressionKw",
+            "type": "CallExpressionKw",
+            "unlabeled": {
+              "abs_path": false,
+              "commentStart": 0,
+              "end": 0,
+              "name": {
+                "commentStart": 0,
+                "end": 0,
+                "name": "x",
+                "start": 0,
+                "type": "Identifier"
+              },
+              "path": [],
+              "start": 0,
+              "type": "Name",
+              "type": "Name"
+            }
+          },
+          "start": 0,
+          "type": "VariableDeclarator"
+        },
+        "end": 0,
+        "kind": "const",
+        "preComments": [
+          "// This won't work, because `x` is being referenced in its own definition."
+        ],
+        "start": 0,
+        "type": "VariableDeclaration",
+        "type": "VariableDeclaration"
+      }
+    ],
+    "commentStart": 0,
+    "end": 0,
+    "start": 0
+  }
+}

rust/kcl-lib/tests/var_ref_in_own_def/execution_error.snap

@@ -0,0 +1,13 @@
+---
+source: kcl-lib/src/simulation_tests.rs
+description: Error from executing var_ref_in_own_def.kcl
+---
+KCL UndefinedValue error
+
+  × undefined value: `x` is not defined
+   ╭─[2:9]
+ 1 │ // This won't work, because `x` is being referenced in its own definition.
+ 2 │ x = cos(x)
+   ·         ┬
+   ·         ╰── tests/var_ref_in_own_def/input.kcl
+   ╰────

rust/kcl-lib/tests/var_ref_in_own_def/input.kcl

@@ -0,0 +1,2 @@
+// This won't work, because `x` is being referenced in its own definition.
+x = cos(x)

rust/kcl-lib/tests/var_ref_in_own_def/ops.snap

@@ -0,0 +1,5 @@
+---
+source: kcl-lib/src/simulation_tests.rs
+description: Operations executed var_ref_in_own_def.kcl
+---
+[]

rust/kcl-lib/tests/var_ref_in_own_def/unparsed.snap

@@ -0,0 +1,6 @@
+---
+source: kcl-lib/src/simulation_tests.rs
+description: Result of unparsing var_ref_in_own_def.kcl
+---
+// This won't work, because `x` is being referenced in its own definition.
+x = cos(x)