diff --git a/rust/kcl-lib/src/errors.rs b/rust/kcl-lib/src/errors.rs index b142d2978..3067e0854 100644 --- a/rust/kcl-lib/src/errors.rs +++ b/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, + }, #[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) => { diff --git a/rust/kcl-lib/src/execution/exec_ast.rs b/rust/kcl-lib/src/execution/exec_ast.rs index 503a200ed..c03e6c14f 100644 --- a/rust/kcl-lib/src/execution/exec_ast.rs +++ b/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 { 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 { 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. diff --git a/rust/kcl-lib/src/execution/fn_call.rs b/rust/kcl-lib/src/execution/fn_call.rs index d6e1e5a5b..e7094bc55 100644 --- a/rust/kcl-lib/src/execution/fn_call.rs +++ b/rust/kcl-lib/src/execution/fn_call.rs @@ -318,10 +318,13 @@ impl Node { 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) diff --git a/rust/kcl-lib/src/execution/memory.rs b/rust/kcl-lib/src/execution/memory.rs index cd7eac226..223c56fa0 100644 --- a/rust/kcl-lib/src/execution/memory.rs +++ b/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![]), + }) } } diff --git a/rust/kcl-lib/src/execution/state.rs b/rust/kcl-lib/src/execution/state.rs index d9b69b7d9..49687bb72 100644 --- a/rust/kcl-lib/src/execution/state.rs +++ b/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, + /// 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, /// Identifiers that have been exported from the current module. pub module_exports: Vec, /// 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, diff --git a/rust/kcl-lib/src/simulation_tests.rs b/rust/kcl-lib/src/simulation_tests.rs index 6e4d0855c..a834ceb4b 100644 --- a/rust/kcl-lib/src/simulation_tests.rs +++ b/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 + } +} diff --git a/rust/kcl-lib/src/std/csg.rs b/rust/kcl-lib/src/std/csg.rs index 300d1e9e2..3118281e9 100644 --- a/rust/kcl-lib/src/std/csg.rs +++ b/rust/kcl-lib/src/std/csg.rs @@ -23,7 +23,7 @@ pub async fn union(exec_state: &mut ExecState, args: Args) -> Result = 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 = 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], ))); diff --git a/rust/kcl-lib/tests/var_ref_in_own_def/artifact_commands.snap b/rust/kcl-lib/tests/var_ref_in_own_def/artifact_commands.snap new file mode 100644 index 000000000..05a1a5fc1 --- /dev/null +++ b/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 + } + } +] diff --git a/rust/kcl-lib/tests/var_ref_in_own_def/artifact_graph_flowchart.snap b/rust/kcl-lib/tests/var_ref_in_own_def/artifact_graph_flowchart.snap new file mode 100644 index 000000000..cdb15ba8c --- /dev/null +++ b/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 +--- diff --git a/rust/kcl-lib/tests/var_ref_in_own_def/artifact_graph_flowchart.snap.md b/rust/kcl-lib/tests/var_ref_in_own_def/artifact_graph_flowchart.snap.md new file mode 100644 index 000000000..13e533509 --- /dev/null +++ b/rust/kcl-lib/tests/var_ref_in_own_def/artifact_graph_flowchart.snap.md @@ -0,0 +1,3 @@ +```mermaid +flowchart LR +``` diff --git a/rust/kcl-lib/tests/var_ref_in_own_def/ast.snap b/rust/kcl-lib/tests/var_ref_in_own_def/ast.snap new file mode 100644 index 000000000..ad36e1f7b --- /dev/null +++ b/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 + } +} diff --git a/rust/kcl-lib/tests/var_ref_in_own_def/execution_error.snap b/rust/kcl-lib/tests/var_ref_in_own_def/execution_error.snap new file mode 100644 index 000000000..02a017bc0 --- /dev/null +++ b/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 + ╰──── diff --git a/rust/kcl-lib/tests/var_ref_in_own_def/input.kcl b/rust/kcl-lib/tests/var_ref_in_own_def/input.kcl new file mode 100644 index 000000000..dcbbe4244 --- /dev/null +++ b/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) diff --git a/rust/kcl-lib/tests/var_ref_in_own_def/ops.snap b/rust/kcl-lib/tests/var_ref_in_own_def/ops.snap new file mode 100644 index 000000000..809df3700 --- /dev/null +++ b/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 +--- +[] diff --git a/rust/kcl-lib/tests/var_ref_in_own_def/unparsed.snap b/rust/kcl-lib/tests/var_ref_in_own_def/unparsed.snap new file mode 100644 index 000000000..07d9eee61 --- /dev/null +++ b/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)