Fix argument error to point to the arg at the call site (#4533)

* Fix argument error to point to the arg at the call site

* Change MemoryFunction to accept Vec<Arg> instead of Vec<KclValue>

* Rename variable to be clearer

* Fix one more argument error message

* Clean up from PR feedback

src/wasm-lib/kcl/src/ast/types/execute.rs

@@ -11,7 +11,7 @@ use crate::{
         BodyType, ExecState, ExecutorContext, KclValue, Metadata, SourceRange, StatementKind, TagEngineInfo,
         TagIdentifier,
     },
-    std::FunctionKind,
+    std::{args::Arg, FunctionKind},
 };
 use async_recursion::async_recursion;
 
@@ -361,16 +361,17 @@ impl Node<CallExpression> {
     pub async fn execute(&self, exec_state: &mut ExecState, ctx: &ExecutorContext) -> Result<KclValue, KclError> {
         let fn_name = &self.callee.name;
 
-        let mut fn_args: Vec<KclValue> = Vec::with_capacity(self.arguments.len());
+        let mut fn_args: Vec<Arg> = Vec::with_capacity(self.arguments.len());
 
-        for arg in &self.arguments {
+        for arg_expr in &self.arguments {
             let metadata = Metadata {
-                source_range: SourceRange::from(arg),
+                source_range: SourceRange::from(arg_expr),
             };
-            let result = ctx
+            let value = ctx
-                .execute_expr(arg, exec_state, &metadata, StatementKind::Expression)
+                .execute_expr(arg_expr, exec_state, &metadata, StatementKind::Expression)
                 .await?;
-            fn_args.push(result);
+            let arg = Arg::new(value, SourceRange::from(arg_expr));
+            fn_args.push(arg);
         }
 
         match ctx.stdlib.get_either(&self.callee.name) {
@@ -470,14 +471,18 @@ impl Node<CallExpression> {
                 for (index, param) in required_params.iter().enumerate() {
                     fn_memory.add(
                         &param.identifier.name,
-                        fn_args.get(index).unwrap().clone(),
+                        fn_args.get(index).unwrap().value.clone(),
                         param.identifier.clone().into(),
                     )?;
                 }
                 // Add the optional arguments to the memory.
                 for (index, param) in optional_params.iter().enumerate() {
                     if let Some(arg) = fn_args.get(index + required_params.len()) {
-                        fn_memory.add(&param.identifier.name, arg.clone(), param.identifier.clone().into())?;
+                        fn_memory.add(
+                            &param.identifier.name,
+                            arg.value.clone(),
+                            param.identifier.clone().into(),
+                        )?;
                     } else {
                         fn_memory.add(
                             &param.identifier.name,

src/wasm-lib/kcl/src/executor.rs

@@ -33,7 +33,7 @@ use crate::{
     errors::{KclError, KclErrorDetails},
     fs::{FileManager, FileSystem},
     settings::types::UnitLength,
-    std::StdLib,
+    std::{args::Arg, StdLib},
     Program,
 };
 
@@ -740,7 +740,7 @@ impl std::hash::Hash for TagIdentifier {
 
 pub type MemoryFunction =
     fn(
-        s: Vec<KclValue>,
+        s: Vec<Arg>,
         memory: ProgramMemory,
         expression: crate::ast::types::BoxNode<FunctionExpression>,
         metadata: Vec<Metadata>,
@@ -1032,6 +1032,11 @@ impl SourceRange {
         Self([start, end, module_id.as_usize()])
     }
 
+    /// A source range that doesn't correspond to any source code.
+    pub fn synthetic() -> Self {
+        Self::default()
+    }
+
     /// Get the start of the range.
     pub fn start(&self) -> usize {
         self.0[0]
@@ -2203,7 +2208,7 @@ impl ExecutorContext {
 /// Returns Err if too few/too many arguments were given for the function.
 fn assign_args_to_params(
     function_expression: NodeRef<'_, FunctionExpression>,
-    args: Vec<KclValue>,
+    args: Vec<Arg>,
     mut fn_memory: ProgramMemory,
 ) -> Result<ProgramMemory, KclError> {
     let num_args = function_expression.number_of_args();
@@ -2229,7 +2234,7 @@ fn assign_args_to_params(
     for (index, param) in function_expression.params.iter().enumerate() {
         if let Some(arg) = args.get(index) {
             // Argument was provided.
-            fn_memory.add(&param.identifier.name, arg.clone(), (&param.identifier).into())?;
+            fn_memory.add(&param.identifier.name, arg.value.clone(), (&param.identifier).into())?;
         } else {
             // Argument was not provided.
             if param.optional {
@@ -2257,7 +2262,7 @@ fn assign_args_to_params(
 }
 
 pub(crate) async fn call_user_defined_function(
-    args: Vec<KclValue>,
+    args: Vec<Arg>,
     memory: &ProgramMemory,
     function_expression: NodeRef<'_, FunctionExpression>,
     exec_state: &mut ExecState,
@@ -3156,6 +3161,7 @@ let w = f() + f()
                 return_type: None,
                 digest: None,
             });
+            let args = args.into_iter().map(Arg::synthetic).collect();
             let actual = assign_args_to_params(func_expr, args, ProgramMemory::new());
             assert_eq!(
                 actual, expected,

src/wasm-lib/kcl/src/function_param.rs

@@ -6,6 +6,7 @@ use crate::{
     executor::{
         call_user_defined_function, ExecState, ExecutorContext, KclValue, MemoryFunction, Metadata, ProgramMemory,
     },
+    std::args::Arg,
 };
 
 /// A function being used as a parameter into a stdlib function.  This is a
@@ -19,7 +20,7 @@ pub struct FunctionParam<'a> {
 }
 
 impl<'a> FunctionParam<'a> {
-    pub async fn call(&self, exec_state: &mut ExecState, args: Vec<KclValue>) -> Result<Option<KclValue>, KclError> {
+    pub async fn call(&self, exec_state: &mut ExecState, args: Vec<Arg>) -> Result<Option<KclValue>, KclError> {
         if let Some(inner) = self.inner {
             inner(
                 args,

src/wasm-lib/kcl/src/kcl_value.rs

@@ -9,7 +9,7 @@ use crate::{
     errors::KclErrorDetails,
     exec::{ProgramMemory, Sketch},
     executor::{Face, ImportedGeometry, MemoryFunction, Metadata, Plane, SketchSet, Solid, SolidSet, TagIdentifier},
-    std::FnAsArg,
+    std::{args::Arg, FnAsArg},
     ExecState, ExecutorContext, KclError, SourceRange,
 };
 
@@ -452,7 +452,7 @@ impl KclValue {
     /// If it's not a function, return Err.
     pub async fn call_fn(
         &self,
-        args: Vec<KclValue>,
+        args: Vec<Arg>,
         exec_state: &mut ExecState,
         ctx: ExecutorContext,
     ) -> Result<Option<KclValue>, KclError> {

src/wasm-lib/kcl/src/simulation_tests.rs

@@ -413,6 +413,36 @@ mod add_lots {
         super::execute(TEST_NAME, false).await
     }
 }
+mod argument_error {
+    //! The argument error points to the problematic argument in the call site,
+    //! not the function definition that the variable points to.
+
+    const TEST_NAME: &str = "argument_error";
+
+    /// Test tokenizing KCL.
+    #[test]
+    fn tokenize() {
+        super::tokenize(TEST_NAME)
+    }
+
+    /// 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, false).await
+    }
+}
 mod array_elem_push {
     const TEST_NAME: &str = "array_elem_push";
 

src/wasm-lib/kcl/src/std/args.rs

@@ -16,15 +16,40 @@ use crate::{
 
 use super::shapes::PolygonType;
 
+#[derive(Debug, Clone)]
+pub struct Arg {
+    /// The evaluated argument.
+    pub value: KclValue,
+    /// The source range of the unevaluated argument.
+    pub source_range: SourceRange,
+}
+
+impl Arg {
+    pub fn new(value: KclValue, source_range: SourceRange) -> Self {
+        Self { value, source_range }
+    }
+
+    pub fn synthetic(value: KclValue) -> Self {
+        Self {
+            value,
+            source_range: SourceRange::synthetic(),
+        }
+    }
+
+    pub fn source_ranges(&self) -> Vec<SourceRange> {
+        vec![self.source_range]
+    }
+}
+
 #[derive(Debug, Clone)]
 pub struct Args {
-    pub args: Vec<KclValue>,
+    pub args: Vec<Arg>,
     pub source_range: SourceRange,
     pub ctx: ExecutorContext,
 }
 
 impl Args {
-    pub fn new(args: Vec<KclValue>, source_range: SourceRange, ctx: ExecutorContext) -> Self {
+    pub fn new(args: Vec<Arg>, source_range: SourceRange, ctx: ExecutorContext) -> Self {
         Self {
             args,
             source_range,
@@ -244,10 +269,10 @@ impl Args {
             .args
             .iter()
             .map(|arg| {
-                let Some(num) = f64::from_kcl_val(arg) else {
+                let Some(num) = f64::from_kcl_val(&arg.value) else {
                     return Err(KclError::Semantic(KclErrorDetails {
-                        source_ranges: arg.metadata().iter().map(|x| x.source_range).collect(),
+                        source_ranges: arg.source_ranges(),
-                        message: format!("Expected a number but found {}", arg.human_friendly_type()),
+                        message: format!("Expected a number but found {}", arg.value.human_friendly_type()),
                     }));
                 };
                 Ok(num)
@@ -509,10 +534,10 @@ impl<'a> FromArgs<'a> for Vec<KclValue> {
                 source_ranges: vec![args.source_range],
             }));
         };
-        let KclValue::Array { value: array, meta: _ } = arg else {
+        let KclValue::Array { value: array, meta: _ } = &arg.value else {
-            let message = format!("Expected an array but found {}", arg.human_friendly_type());
+            let message = format!("Expected an array but found {}", arg.value.human_friendly_type());
             return Err(KclError::Type(KclErrorDetails {
-                source_ranges: arg.metadata().into_iter().map(|m| m.source_range).collect(),
+                source_ranges: arg.source_ranges(),
                 message,
             }));
         };
@@ -531,14 +556,14 @@ where
                 source_ranges: vec![args.source_range],
             }));
         };
-        let Some(val) = T::from_kcl_val(arg) else {
+        let Some(val) = T::from_kcl_val(&arg.value) else {
             return Err(KclError::Semantic(KclErrorDetails {
                 message: format!(
                     "Argument at index {i} was supposed to be type {} but found {}",
                     type_name::<T>(),
-                    arg.human_friendly_type()
+                    arg.value.human_friendly_type()
                 ),
-                source_ranges: vec![args.source_range],
+                source_ranges: arg.source_ranges(),
             }));
         };
         Ok(val)
@@ -551,17 +576,17 @@ where
 {
     fn from_args(args: &'a Args, i: usize) -> Result<Self, KclError> {
         let Some(arg) = args.args.get(i) else { return Ok(None) };
-        if crate::ast::types::KclNone::from_kcl_val(arg).is_some() {
+        if crate::ast::types::KclNone::from_kcl_val(&arg.value).is_some() {
             return Ok(None);
         }
-        let Some(val) = T::from_kcl_val(arg) else {
+        let Some(val) = T::from_kcl_val(&arg.value) else {
             return Err(KclError::Semantic(KclErrorDetails {
                 message: format!(
                     "Argument at index {i} was supposed to be type Option<{}> but found {}",
                     type_name::<T>(),
-                    arg.human_friendly_type()
+                    arg.value.human_friendly_type()
                 ),
-                source_ranges: vec![args.source_range],
+                source_ranges: arg.source_ranges(),
             }));
         };
         Ok(Some(val))

src/wasm-lib/kcl/src/std/array.rs

@@ -1,6 +1,9 @@
 use derive_docs::stdlib;
 
-use super::{args::FromArgs, Args, FnAsArg};
+use super::{
+    args::{Arg, FromArgs},
+    Args, FnAsArg,
+};
 use crate::{
     errors::{KclError, KclErrorDetails},
     executor::{ExecState, KclValue, SourceRange},
@@ -75,7 +78,7 @@ async fn call_map_closure<'a>(
     source_range: SourceRange,
     exec_state: &mut ExecState,
 ) -> Result<KclValue, KclError> {
-    let output = map_fn.call(exec_state, vec![input]).await?;
+    let output = map_fn.call(exec_state, vec![Arg::synthetic(input)]).await?;
     let source_ranges = vec![source_range];
     let output = output.ok_or_else(|| {
         KclError::Semantic(KclErrorDetails {
@@ -202,7 +205,7 @@ async fn call_reduce_closure<'a>(
     exec_state: &mut ExecState,
 ) -> Result<KclValue, KclError> {
     // Call the reduce fn for this repetition.
-    let reduce_fn_args = vec![elem, start];
+    let reduce_fn_args = vec![Arg::synthetic(elem), Arg::synthetic(start)];
     let transform_fn_return = reduce_fn.call(exec_state, reduce_fn_args).await?;
 
     // Unpack the returned transform object.

src/wasm-lib/kcl/src/std/patterns.rs

@@ -22,6 +22,8 @@ use crate::{
     std::{types::Uint, Args},
 };
 
+use super::args::Arg;
+
 const MUST_HAVE_ONE_INSTANCE: &str = "There must be at least 1 instance of your geometry";
 
 /// Data for a linear pattern on a 2D sketch.
@@ -381,7 +383,7 @@ async fn make_transform<'a>(
         value: i.into(),
         meta: vec![source_range.into()],
     };
-    let transform_fn_args = vec![repetition_num];
+    let transform_fn_args = vec![Arg::synthetic(repetition_num)];
     let transform_fn_return = transform_function.call(exec_state, transform_fn_args).await?;
 
     // Unpack the returned transform object.

src/wasm-lib/kcl/tests/argument_error/ast.snap

@@ -0,0 +1,137 @@
+---
+source: kcl/src/simulation_tests.rs
+description: Result of parsing argument_error.kcl
+---
+{
+  "Ok": {
+    "body": [
+      {
+        "declarations": [
+          {
+            "end": 28,
+            "id": {
+              "end": 4,
+              "name": "f",
+              "start": 3,
+              "type": "Identifier"
+            },
+            "init": {
+              "body": {
+                "body": [
+                  {
+                    "argument": {
+                      "end": 26,
+                      "raw": "5",
+                      "start": 25,
+                      "type": "Literal",
+                      "type": "Literal",
+                      "value": 5
+                    },
+                    "end": 26,
+                    "start": 18,
+                    "type": "ReturnStatement",
+                    "type": "ReturnStatement"
+                  }
+                ],
+                "end": 28,
+                "start": 14
+              },
+              "end": 28,
+              "params": [
+                {
+                  "type": "Parameter",
+                  "identifier": {
+                    "end": 9,
+                    "name": "i",
+                    "start": 8,
+                    "type": "Identifier"
+                  },
+                  "optional": false
+                }
+              ],
+              "start": 7,
+              "type": "FunctionExpression",
+              "type": "FunctionExpression"
+            },
+            "start": 3,
+            "type": "VariableDeclarator"
+          }
+        ],
+        "end": 28,
+        "kind": "fn",
+        "start": 0,
+        "type": "VariableDeclaration",
+        "type": "VariableDeclaration"
+      },
+      {
+        "end": 44,
+        "expression": {
+          "arguments": [
+            {
+              "end": 35,
+              "name": "f",
+              "start": 34,
+              "type": "Identifier",
+              "type": "Identifier"
+            },
+            {
+              "elements": [
+                {
+                  "end": 39,
+                  "raw": "0",
+                  "start": 38,
+                  "type": "Literal",
+                  "type": "Literal",
+                  "value": 0
+                },
+                {
+                  "end": 42,
+                  "raw": "1",
+                  "start": 41,
+                  "type": "Literal",
+                  "type": "Literal",
+                  "value": 1
+                }
+              ],
+              "end": 43,
+              "start": 37,
+              "type": "ArrayExpression",
+              "type": "ArrayExpression"
+            }
+          ],
+          "callee": {
+            "end": 33,
+            "name": "map",
+            "start": 30,
+            "type": "Identifier"
+          },
+          "end": 44,
+          "optional": false,
+          "start": 30,
+          "type": "CallExpression",
+          "type": "CallExpression"
+        },
+        "start": 30,
+        "type": "ExpressionStatement",
+        "type": "ExpressionStatement"
+      }
+    ],
+    "end": 45,
+    "nonCodeMeta": {
+      "nonCodeNodes": {
+        "0": [
+          {
+            "end": 30,
+            "start": 28,
+            "type": "NonCodeNode",
+            "value": {
+              "type": "newLine"
+            }
+          }
+        ]
+      },
+      "startNodes": []
+    },
+    "start": 0
+  }
+}

src/wasm-lib/kcl/tests/argument_error/execution_error.snap

@@ -0,0 +1,5 @@
+---
+source: kcl/src/simulation_tests.rs
+description: Error from executing argument_error.kcl
+---
+type: KclErrorDetails { source_ranges: [SourceRange([34, 35, 0])], message: "Expected an array but found Function" }

src/wasm-lib/kcl/tests/argument_error/input.kcl

@@ -0,0 +1,5 @@
+fn f = (i) => {
+  return 5
+}
+
+map(f, [0, 1])

src/wasm-lib/kcl/tests/argument_error/tokens.snap

@@ -0,0 +1,206 @@
+---
+source: kcl/src/simulation_tests.rs
+description: Result of tokenizing argument_error.kcl
+---
+{
+  "Ok": [
+    {
+      "type": "keyword",
+      "start": 0,
+      "end": 2,
+      "value": "fn"
+    },
+    {
+      "type": "whitespace",
+      "start": 2,
+      "end": 3,
+      "value": " "
+    },
+    {
+      "type": "word",
+      "start": 3,
+      "end": 4,
+      "value": "f"
+    },
+    {
+      "type": "whitespace",
+      "start": 4,
+      "end": 5,
+      "value": " "
+    },
+    {
+      "type": "operator",
+      "start": 5,
+      "end": 6,
+      "value": "="
+    },
+    {
+      "type": "whitespace",
+      "start": 6,
+      "end": 7,
+      "value": " "
+    },
+    {
+      "type": "brace",
+      "start": 7,
+      "end": 8,
+      "value": "("
+    },
+    {
+      "type": "word",
+      "start": 8,
+      "end": 9,
+      "value": "i"
+    },
+    {
+      "type": "brace",
+      "start": 9,
+      "end": 10,
+      "value": ")"
+    },
+    {
+      "type": "whitespace",
+      "start": 10,
+      "end": 11,
+      "value": " "
+    },
+    {
+      "type": "operator",
+      "start": 11,
+      "end": 13,
+      "value": "=>"
+    },
+    {
+      "type": "whitespace",
+      "start": 13,
+      "end": 14,
+      "value": " "
+    },
+    {
+      "type": "brace",
+      "start": 14,
+      "end": 15,
+      "value": "{"
+    },
+    {
+      "type": "whitespace",
+      "start": 15,
+      "end": 18,
+      "value": "\n  "
+    },
+    {
+      "type": "keyword",
+      "start": 18,
+      "end": 24,
+      "value": "return"
+    },
+    {
+      "type": "whitespace",
+      "start": 24,
+      "end": 25,
+      "value": " "
+    },
+    {
+      "type": "number",
+      "start": 25,
+      "end": 26,
+      "value": "5"
+    },
+    {
+      "type": "whitespace",
+      "start": 26,
+      "end": 27,
+      "value": "\n"
+    },
+    {
+      "type": "brace",
+      "start": 27,
+      "end": 28,
+      "value": "}"
+    },
+    {
+      "type": "whitespace",
+      "start": 28,
+      "end": 30,
+      "value": "\n\n"
+    },
+    {
+      "type": "word",
+      "start": 30,
+      "end": 33,
+      "value": "map"
+    },
+    {
+      "type": "brace",
+      "start": 33,
+      "end": 34,
+      "value": "("
+    },
+    {
+      "type": "word",
+      "start": 34,
+      "end": 35,
+      "value": "f"
+    },
+    {
+      "type": "comma",
+      "start": 35,
+      "end": 36,
+      "value": ","
+    },
+    {
+      "type": "whitespace",
+      "start": 36,
+      "end": 37,
+      "value": " "
+    },
+    {
+      "type": "brace",
+      "start": 37,
+      "end": 38,
+      "value": "["
+    },
+    {
+      "type": "number",
+      "start": 38,
+      "end": 39,
+      "value": "0"
+    },
+    {
+      "type": "comma",
+      "start": 39,
+      "end": 40,
+      "value": ","
+    },
+    {
+      "type": "whitespace",
+      "start": 40,
+      "end": 41,
+      "value": " "
+    },
+    {
+      "type": "number",
+      "start": 41,
+      "end": 42,
+      "value": "1"
+    },
+    {
+      "type": "brace",
+      "start": 42,
+      "end": 43,
+      "value": "]"
+    },
+    {
+      "type": "brace",
+      "start": 43,
+      "end": 44,
+      "value": ")"
+    },
+    {
+      "type": "whitespace",
+      "start": 44,
+      "end": 45,
+      "value": "\n"
+    }
+  ]
+}

src/wasm-lib/tests/executor/main.rs

@@ -866,7 +866,7 @@ part = rectShape([0, 0], 20, 20)
     assert!(result.is_err());
     assert_eq!(
         result.err().unwrap().to_string(),
-        r#"semantic: KclErrorDetails { source_ranges: [SourceRange([863, 912, 0])], message: "Argument at index 0 was supposed to be type kcl_lib::std::shapes::CircleData but found string (text)" }"#,
+        r#"semantic: KclErrorDetails { source_ranges: [SourceRange([870, 874, 0])], message: "Argument at index 0 was supposed to be type kcl_lib::std::shapes::CircleData but found string (text)" }"#,
     );
 }
 
@@ -1351,7 +1351,7 @@ secondSketch = startSketchOn(part001, '')
     assert!(result.is_err());
     assert_eq!(
         result.err().unwrap().to_string(),
-        r#"semantic: KclErrorDetails { source_ranges: [SourceRange([260, 286, 0])], message: "Argument at index 1 was supposed to be type Option<kcl_lib::std::sketch::FaceTag> but found string (text)" }"#
+        r#"semantic: KclErrorDetails { source_ranges: [SourceRange([283, 285, 0])], message: "Argument at index 1 was supposed to be type Option<kcl_lib::std::sketch::FaceTag> but found string (text)" }"#
     );
 }
 
@@ -1983,7 +1983,7 @@ someFunction('INVALID')
     assert!(result.is_err());
     assert_eq!(
         result.err().unwrap().to_string(),
-        r#"semantic: KclErrorDetails { source_ranges: [SourceRange([37, 61, 0]), SourceRange([65, 88, 0])], message: "Argument at index 0 was supposed to be type kcl_lib::std::sketch::SketchData but found string (text)" }"#
+        r#"semantic: KclErrorDetails { source_ranges: [SourceRange([51, 60, 0]), SourceRange([65, 88, 0])], message: "Argument at index 0 was supposed to be type kcl_lib::std::sketch::SketchData but found string (text)" }"#
     );
 }
 
@@ -2004,7 +2004,7 @@ someFunction('INVALID')
     assert!(result.is_err());
     assert_eq!(
         result.err().unwrap().to_string(),
-        r#"semantic: KclErrorDetails { source_ranges: [SourceRange([89, 114, 0]), SourceRange([126, 155, 0]), SourceRange([159, 182, 0])], message: "Argument at index 0 was supposed to be type kcl_lib::std::sketch::SketchData but found string (text)" }"#
+        r#"semantic: KclErrorDetails { source_ranges: [SourceRange([103, 113, 0]), SourceRange([126, 155, 0]), SourceRange([159, 182, 0])], message: "Argument at index 0 was supposed to be type kcl_lib::std::sketch::SketchData but found string (text)" }"#
     );
 }