KCL: Fix 'cryptic' error when referencing a variable in its own declaration

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#issuecomment-2923227477

rust/kcl-lib/src/errors.rs

@@ -107,8 +107,11 @@ pub enum KclError {
     Unexpected(KclErrorDetails),
     #[error("value already defined: {0:?}")]
     ValueAlreadyDefined(KclErrorDetails),
-    #[error("undefined value: {0:?}")]
-    UndefinedValue(KclErrorDetails),
+    #[error("undefined value: {details:?}")]
+    UndefinedValue {
+        details: KclErrorDetails,
+        undefined_name: Option<String>,
+    },
     #[error("invalid expression: {0:?}")]
     InvalidExpression(KclErrorDetails),
     #[error("engine: {0:?}")]
@@ -304,7 +307,7 @@ impl miette::Diagnostic for ReportWithOutputs {
             KclError::Io(_) => "I/O",
             KclError::Unexpected(_) => "Unexpected",
             KclError::ValueAlreadyDefined(_) => "ValueAlreadyDefined",
-            KclError::UndefinedValue(_) => "UndefinedValue",
+            KclError::UndefinedValue { .. } => "UndefinedValue",
             KclError::InvalidExpression(_) => "InvalidExpression",
             KclError::Engine(_) => "Engine",
             KclError::Internal(_) => "Internal",
@@ -354,7 +357,7 @@ impl miette::Diagnostic for Report {
             KclError::Io(_) => "I/O",
             KclError::Unexpected(_) => "Unexpected",
             KclError::ValueAlreadyDefined(_) => "ValueAlreadyDefined",
-            KclError::UndefinedValue(_) => "UndefinedValue",
+            KclError::UndefinedValue { .. } => "UndefinedValue",
             KclError::InvalidExpression(_) => "InvalidExpression",
             KclError::Engine(_) => "Engine",
             KclError::Internal(_) => "Internal",
@@ -432,7 +435,7 @@ impl KclError {
             KclError::Io(_) => "i/o",
             KclError::Unexpected(_) => "unexpected",
             KclError::ValueAlreadyDefined(_) => "value already defined",
-            KclError::UndefinedValue(_) => "undefined value",
+            KclError::UndefinedValue { .. } => "undefined value",
             KclError::InvalidExpression(_) => "invalid expression",
             KclError::Engine(_) => "engine",
             KclError::Internal(_) => "internal",
@@ -449,7 +452,7 @@ impl KclError {
             KclError::Io(e) => e.source_ranges.clone(),
             KclError::Unexpected(e) => e.source_ranges.clone(),
             KclError::ValueAlreadyDefined(e) => e.source_ranges.clone(),
-            KclError::UndefinedValue(e) => e.source_ranges.clone(),
+            KclError::UndefinedValue { details, .. } => details.source_ranges.clone(),
             KclError::InvalidExpression(e) => e.source_ranges.clone(),
             KclError::Engine(e) => e.source_ranges.clone(),
             KclError::Internal(e) => e.source_ranges.clone(),
@@ -467,7 +470,7 @@ impl KclError {
             KclError::Io(e) => &e.message,
             KclError::Unexpected(e) => &e.message,
             KclError::ValueAlreadyDefined(e) => &e.message,
-            KclError::UndefinedValue(e) => &e.message,
+            KclError::UndefinedValue { details, .. } => &details.message,
             KclError::InvalidExpression(e) => &e.message,
             KclError::Engine(e) => &e.message,
             KclError::Internal(e) => &e.message,
@@ -484,7 +487,7 @@ impl KclError {
             | KclError::Io(e)
             | KclError::Unexpected(e)
             | KclError::ValueAlreadyDefined(e)
-            | KclError::UndefinedValue(e)
+            | KclError::UndefinedValue { details: e, .. }
             | KclError::InvalidExpression(e)
             | KclError::Engine(e)
             | KclError::Internal(e) => e.backtrace.clone(),
@@ -502,7 +505,7 @@ impl KclError {
             | KclError::Io(e)
             | KclError::Unexpected(e)
             | KclError::ValueAlreadyDefined(e)
-            | KclError::UndefinedValue(e)
+            | KclError::UndefinedValue { details: e, .. }
             | KclError::InvalidExpression(e)
             | KclError::Engine(e)
             | KclError::Internal(e) => {
@@ -531,7 +534,7 @@ impl KclError {
             | KclError::Io(e)
             | KclError::Unexpected(e)
             | KclError::ValueAlreadyDefined(e)
-            | KclError::UndefinedValue(e)
+            | KclError::UndefinedValue { details: e, .. }
             | KclError::InvalidExpression(e)
             | KclError::Engine(e)
             | KclError::Internal(e) => {
@@ -555,7 +558,7 @@ impl KclError {
             | KclError::Io(e)
             | KclError::Unexpected(e)
             | KclError::ValueAlreadyDefined(e)
-            | KclError::UndefinedValue(e)
+            | KclError::UndefinedValue { details: e, .. }
             | KclError::InvalidExpression(e)
             | KclError::Engine(e)
             | KclError::Internal(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::UndefinedValue(KclErrorDetails::new(
-                                        format!("{} is not defined in module", import_item.name.name),
-                                        vec![SourceRange::from(&import_item.name)],
-                                    )));
+                                    return Err(KclError::UndefinedValue {
+                                        undefined_name: None,
+                                        details: KclErrorDetails::new(
+                                            format!("{} is not defined in module", import_item.name.name),
+                                            vec![SourceRange::from(&import_item.name)],
+                                        ),
+                                    });
                                 }
 
                                 // Check that the item is allowed to be imported (in at least one namespace).
@@ -301,6 +304,7 @@ impl ExecutorContext {
 
                     let annotations = &variable_declaration.outer_attrs;
 
+                    exec_state.mod_local.being_declared = Some(variable_declaration.inner.name().to_owned());
                     let value = self
                         .execute_expr(
                             &variable_declaration.declaration.init,
@@ -310,6 +314,7 @@ impl ExecutorContext {
                             StatementKind::Declaration { name: &var_name },
                         )
                         .await?;
+                    exec_state.mod_local.being_declared = None;
                     exec_state
                         .mut_stack()
                         .add(var_name.clone(), value.clone(), source_range)?;
@@ -635,7 +640,23 @@ impl ExecutorContext {
             Expr::Literal(literal) => KclValue::from_literal((**literal).clone(), exec_state),
             Expr::TagDeclarator(tag) => tag.execute(exec_state).await?,
             Expr::Name(name) => {
-                let value = name.get_result(exec_state, self).await?.clone();
+                let being_declared = exec_state.mod_local.being_declared.clone();
+                let value = name
+                    .get_result(exec_state, self)
+                    .await
+                    .map_err(|e| match e {
+                        KclError::UndefinedValue {
+                            undefined_name,
+                            mut details,
+                        } => {
+                            if let Some(being_declared) = &being_declared{
+                                details.message = format!("You can't use `{}` because you're currently trying to define it. Use a different variable here instead.", being_declared);
+                            }
+                            KclError::UndefinedValue { details, undefined_name}
+                        }
+                        e => e,
+                    })?
+                    .clone();
                 if let KclValue::Module { value: module_id, meta } = value {
                     self.exec_module_for_result(
                         module_id,
@@ -913,10 +934,13 @@ impl Node<MemberExpression> {
                 if let Some(value) = map.get(&property) {
                     Ok(value.to_owned())
                 } else {
-                    Err(KclError::UndefinedValue(KclErrorDetails::new(
-                        format!("Property '{property}' not found in object"),
-                        vec![self.clone().into()],
-                    )))
+                    Err(KclError::UndefinedValue {
+                        undefined_name: None,
+                        details: KclErrorDetails::new(
+                            format!("Property '{property}' not found in object"),
+                            vec![self.clone().into()],
+                        ),
+                    })
                 }
             }
             (KclValue::Object { .. }, Property::String(property), true) => {
@@ -938,10 +962,13 @@ impl Node<MemberExpression> {
                 if let Some(value) = value_of_arr {
                     Ok(value.to_owned())
                 } else {
-                    Err(KclError::UndefinedValue(KclErrorDetails::new(
-                        format!("The array doesn't have any item at index {index}"),
-                        vec![self.clone().into()],
-                    )))
+                    Err(KclError::UndefinedValue {
+                        undefined_name: None,
+                        details: KclErrorDetails::new(
+                            format!("The array doesn't have any item at index {index}"),
+                            vec![self.clone().into()],
+                        ),
+                    })
                 }
             }
             // 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::UndefinedValue(KclErrorDetails::new(
-                        format!("Result of user-defined function {} is undefined", fn_name),
-                        source_ranges,
-                    ))
+                    KclError::UndefinedValue {
+                        undefined_name: None,
+                        details: KclErrorDetails::new(
+                            format!("Result of user-defined function {} is undefined", fn_name),
+                            source_ranges,
+                        ),
+                    }
                 })?;
 
                 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::UndefinedValue(KclErrorDetails::new(
-            format!("`{name}` is not defined"),
-            vec![source_range],
-        )))
+        Err(KclError::UndefinedValue {
+            undefined_name: Some(name.to_owned()),
+            details: KclErrorDetails::new(format!("`{name}` is not defined"), vec![source_range]),
+        })
     }
 
     /// Iterate over all key/value pairs in the specified environment which satisfy the provided
@@ -488,10 +488,10 @@ impl ProgramMemory {
             };
         }
 
-        Err(KclError::UndefinedValue(KclErrorDetails::new(
-            format!("`{}` is not defined", var),
-            vec![],
-        )))
+        Err(KclError::UndefinedValue {
+            undefined_name: Some(var.to_owned()),
+            details: KclErrorDetails::new(format!("`{}` is not defined", var), vec![]),
+        })
     }
 }
 

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::UndefinedValue(KclErrorDetails::new(
+        return Err(KclError::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::UndefinedValue(KclErrorDetails::new(
+        return Err(KclError::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,14 @@
+---
+source: kcl-lib/src/simulation_tests.rs
+description: Error from executing var_ref_in_own_def.kcl
+---
+KCL UndefinedValue error
+
+  × undefined value: You can't use `x` because you're currently trying to
+  │ define it. Use a different variable here instead.
+   ╭─[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)