chore: Attach function name in error message with best effects (#618)

refs: #416

Best effects only

Author: He-Pin

sjsonnet/src/sjsonnet/Evaluator.scala

@@ -116,7 +116,7 @@ class Evaluator(
           newScope.bindings(base + i) = b.args match {
             case null    => visitAsLazy(b.rhs)(newScope)
             case argSpec =>
-              new Lazy(() => visitMethod(b.rhs, argSpec, b.pos)(newScope))
+              new Lazy(() => visitMethod(b.rhs, argSpec, b.pos, b.name)(newScope))
           }
           i += 1
         }
@@ -634,9 +634,10 @@ class Evaluator(
     Error.fail(s"Field name must be string or null, not ${fieldName.prettyName}", pos)
   }
 
-  def visitMethod(rhs: Expr, params: Params, outerPos: Position)(implicit
+  def visitMethod(rhs: Expr, params: Params, outerPos: Position, name: String = null)(implicit
       scope: ValScope): Val.Func =
     new Val.Func(outerPos, scope, params) {
+      override def functionName: String = name
       def evalRhs(vs: ValScope, es: EvalScope, fs: FileScope, pos: Position): Val =
         visitExpr(rhs)(vs)
       override def evalDefault(expr: Expr, vs: ValScope, es: EvalScope): Val = visitExpr(expr)(vs)
@@ -651,7 +652,7 @@ class Evaluator(
         case null =>
           new Lazy(() => visitExpr(b.rhs)(scope))
         case argSpec =>
-          new Lazy(() => visitMethod(b.rhs, argSpec, b.pos)(scope))
+          new Lazy(() => visitMethod(b.rhs, argSpec, b.pos, b.name)(scope))
       }
       i += 1
     }

sjsonnet/src/sjsonnet/Val.scala

@@ -666,6 +666,19 @@ object Val {
 
     def evalDefault(expr: Expr, vs: ValScope, es: EvalScope): Val = null
 
+    /** Override to provide a function name for error messages. Only called on error paths. */
+    def functionName: String = null
+
+    private def functionNamePrefix: String = {
+      val name = functionName
+      if (name != null) s" $name" else ""
+    }
+
+    private def functionNameSuffix: String = {
+      val name = functionName
+      if (name != null) s" in function $name" else ""
+    }
+
     def prettyName = "function"
 
     override def exprErrorString: String = "Function"
@@ -701,18 +714,24 @@ object Val {
           while (i < namedNames.length) {
             val idx = params.paramMap.getOrElse(
               namedNames(i),
-              Error.fail(s"Function has no parameter ${namedNames(i)}", outerPos)
+              Error.fail(
+                s"Function$functionNamePrefix has no parameter ${namedNames(i)}",
+                outerPos
+              )
             )
             if (argVals(base + idx) != null)
-              Error.fail(s"binding parameter a second time: ${namedNames(i)}", outerPos)
+              Error.fail(
+                s"binding parameter a second time: ${namedNames(i)}$functionNameSuffix",
+                outerPos
+              )
             argVals(base + idx) = argsL(j)
             i += 1
             j += 1
           }
         }
         if (argsL.length > params.names.length)
           Error.fail(
-            "Too many args, function has " + params.names.length + " parameter(s)",
+            "Too many args, function" + functionNamePrefix + " has " + params.names.length + " parameter(s)",
             outerPos
           )
         if (params.names.length != argsL.length) { // Args missing -> add defaults
@@ -735,7 +754,7 @@ object Val {
           if (missing != null) {
             val plural = if (missing.size > 1) "s" else ""
             Error.fail(
-              s"Function parameter$plural ${missing.mkString(", ")} not bound in call",
+              s"Function$functionNamePrefix parameter$plural ${missing.mkString(", ")} not bound in call",
               outerPos
             )
           }
@@ -815,7 +834,7 @@ object Val {
 
   /** Superclass for standard library functions */
   abstract class Builtin(
-      val functionName: String,
+      override val functionName: String,
       paramNames: Array[String],
       defaults: Array[Expr] = null)
       extends Func(

sjsonnet/test/resources/go_test_suite/native7.jsonnet.golden

@@ -1,3 +1,3 @@
-sjsonnet.Error: Function has no parameter y
+sjsonnet.Error: Function jsonToString has no parameter y
     at [Apply].(native7.jsonnet:1:27)
 

sjsonnet/test/resources/go_test_suite/optional_args8.jsonnet.golden

@@ -1,3 +1,3 @@
-sjsonnet.Error: Function has no parameter y
+sjsonnet.Error: Function foo has no parameter y
     at [Apply].(optional_args8.jsonnet:2:4)
 

sjsonnet/test/resources/go_test_suite/std.makeArrayNamed3.jsonnet.golden

@@ -1,3 +1,3 @@
-sjsonnet.Error: Function has no parameter blahblah
+sjsonnet.Error: Function makeArray has no parameter blahblah
     at [Apply].(std.makeArrayNamed3.jsonnet:1:14)
 

sjsonnet/test/resources/test_suite/error.function_too_many_args.jsonnet.golden

@@ -1,3 +1,3 @@
-sjsonnet.Error: Too many args, function has 2 parameter(s)
+sjsonnet.Error: Too many args, function foo has 2 parameter(s)
     at [Apply3].(error.function_too_many_args.jsonnet:19:4)
 

sjsonnet/test/resources/test_suite/error.trace_one_param.jsonnet.golden

@@ -1,4 +1,4 @@
-sjsonnet.Error: Function parameter rest not bound in call
+sjsonnet.Error: Function trace parameter rest not bound in call
     at [Apply1].(error.trace_one_param.jsonnet:17:20)
     at [ValidId v].(error.trace_one_param.jsonnet:19:6)
 

sjsonnet/test/resources/test_suite/error.trace_three_param.jsonnet.golden

@@ -1,4 +1,4 @@
-sjsonnet.Error: Too many args, function has 2 parameter(s)
+sjsonnet.Error: Too many args, function trace has 2 parameter(s)
     at [Apply3].(error.trace_three_param.jsonnet:17:20)
     at [ValidId v].(error.trace_three_param.jsonnet:19:6)
 

sjsonnet/test/resources/test_suite/error.trace_zero_param.jsonnet.golden

@@ -1,4 +1,4 @@
-sjsonnet.Error: Function parameters str, rest not bound in call
+sjsonnet.Error: Function trace parameters str, rest not bound in call
     at [Apply0].(error.trace_zero_param.jsonnet:17:20)
     at [ValidId v].(error.trace_zero_param.jsonnet:19:6)
 

sjsonnet/test/src/sjsonnet/EvaluatorTests.scala

@@ -457,7 +457,7 @@ object EvaluatorTests extends TestSuite {
         )
       }
 
-      assert(ex.getMessage.contains("Function parameter y not bound in call"))
+      assert(ex.getMessage.contains("Function newParams parameter y not bound in call"))
     }
 
     test("invalidParam") {
@@ -475,7 +475,7 @@ object EvaluatorTests extends TestSuite {
         )
       }
 
-      assert(ex.getMessage.contains("Function has no parameter hello"))
+      assert(ex.getMessage.contains("Function Person has no parameter hello"))
     }
 
     test("unknownVariable") {
@@ -866,6 +866,86 @@ object EvaluatorTests extends TestSuite {
       }
     }
 
+    test("builtinErrorMessageIncludesFunctionName") {
+      // Issue #416: error messages for builtin functions should include the function name.
+
+      // Too many args
+      test("tooManyArgs") {
+        val ex = assertThrows[Exception] {
+          eval("std.length([1], [2])", useNewEvaluator = useNewEvaluator)
+        }
+        assert(ex.getMessage.contains("Too many args, function length has"))
+      }
+
+      // Parameter not bound in call (missing required arg)
+      test("missingRequiredArg") {
+        val ex = assertThrows[Exception] {
+          eval("std.substr('hello')", useNewEvaluator = useNewEvaluator)
+        }
+        assert(ex.getMessage.contains("Function substr parameter"))
+        assert(ex.getMessage.contains("not bound in call"))
+      }
+
+      // Function has no parameter (invalid named arg)
+      test("invalidNamedArg") {
+        val ex = assertThrows[Exception] {
+          eval("std.length(x=[1], noSuchParam=2)", useNewEvaluator = useNewEvaluator)
+        }
+        assert(ex.getMessage.contains("Function length has no parameter noSuchParam"))
+      }
+
+      // Binding parameter a second time
+      test("duplicateArg") {
+        val ex = assertThrows[Exception] {
+          eval("std.pow(2, x=3)", useNewEvaluator = useNewEvaluator)
+        }
+        assert(ex.getMessage.contains("binding parameter a second time: x in function pow"))
+      }
+
+      // User-defined function should include function name from local binding
+      test("userDefinedFunctionIncludesFunctionName") {
+        val ex = assertThrows[Exception] {
+          eval(
+            """local myFunc(x, y) = x + y;
+              |myFunc("a")
+            """.stripMargin,
+            useNewEvaluator = useNewEvaluator
+          )
+        }
+        assert(ex.getMessage.contains("Function myFunc parameter y not bound in call"))
+      }
+
+      // User-defined function: too many args should include function name
+      test("userDefinedTooManyArgs") {
+        val ex = assertThrows[Exception] {
+          eval(
+            """local add(x, y) = x + y;
+              |add(1, 2, 3)
+            """.stripMargin,
+            useNewEvaluator = useNewEvaluator
+          )
+        }
+        assert(ex.getMessage.contains("Too many args, function add has 2 parameter(s)"))
+      }
+
+      // Anonymous function should NOT include function name in error message
+      test("anonymousFunctionNoFunctionName") {
+        val ex = assertThrows[Exception] {
+          eval("(function(x, y) x + y)('a')", useNewEvaluator = useNewEvaluator)
+        }
+        // Should be exactly "Function parameter..." without a function name inserted
+        assert(ex.getMessage.contains("Function parameter y not bound in call"))
+      }
+
+      // Anonymous function: too many args should NOT include function name
+      test("anonymousFunctionTooManyArgs") {
+        val ex = assertThrows[Exception] {
+          eval("(function(x) x)(1, 2)", useNewEvaluator = useNewEvaluator)
+        }
+        assert(ex.getMessage.contains("Too many args, function has 1 parameter(s)"))
+      }
+    }
+
   }
   def tests: Tests = allTests(false).prefix("Evaluator") ++ allTests(true).prefix("NewEvaluator")
 }