Fix an issue with the error message not including the source location…

… and position during function dispatch PiperOrigin-RevId: 721471017
google · Jan 30, 2025 · 3c24524 · 3c24524
1 parent 4db3123
commit 3c24524
Show file tree

Hide file tree

Showing 36 changed files with 246 additions and 209 deletions.
diff --git a/bundle/src/test/java/dev/cel/bundle/CelImplTest.java b/bundle/src/test/java/dev/cel/bundle/CelImplTest.java
@@ -2063,8 +2063,8 @@ public void program_regexProgramSizeExceedsLimit_throws() throws Exception {
     assertThat(e)
         .hasMessageThat()
         .contains(
-            "evaluation error: Regex pattern exceeds allowed program size. Allowed: 6, Provided:"
-                + " 7");
+            "evaluation error at <input>:13: Regex pattern exceeds allowed program size. Allowed:"
+                + " 6, Provided: 7");
     assertThat(e.getErrorCode()).isEqualTo(CelErrorCode.INVALID_ARGUMENT);
   }
 

diff --git a/codelab/src/test/codelab/Exercise1Test.java b/codelab/src/test/codelab/Exercise1Test.java
@@ -68,6 +68,6 @@ public void evaluate_divideByZeroExpression_throwsEvaluationException() throws E
     IllegalArgumentException exception =
         assertThrows(IllegalArgumentException.class, () -> exercise1.eval(ast));
 
-    assertThat(exception).hasMessageThat().contains("evaluation error: / by zero");
+    assertThat(exception).hasMessageThat().contains("evaluation error at <input>:1: / by zero");
   }
 }
diff --git a/codelab/src/test/codelab/Exercise3Test.java b/codelab/src/test/codelab/Exercise3Test.java
@@ -57,7 +57,8 @@ public void evaluate_logicalOrFailure_throwsException(String expression) {
 
     assertThat(exception).hasMessageThat().contains("Evaluation error has occurred.");
     assertThat(exception).hasCauseThat().isInstanceOf(CelEvaluationException.class);
-    assertThat(exception).hasCauseThat().hasMessageThat().contains("evaluation error: / by zero");
+    assertThat(exception).hasCauseThat().hasMessageThat().contains("evaluation error");
+    assertThat(exception).hasCauseThat().hasMessageThat().contains("/ by zero");
   }
 
   @Test
@@ -77,7 +78,8 @@ public void evaluate_logicalAndFailure_throwsException(String expression) {
 
     assertThat(exception).hasMessageThat().contains("Evaluation error has occurred.");
     assertThat(exception).hasCauseThat().isInstanceOf(CelEvaluationException.class);
-    assertThat(exception).hasCauseThat().hasMessageThat().contains("evaluation error: / by zero");
+    assertThat(exception).hasCauseThat().hasMessageThat().contains("evaluation error");
+    assertThat(exception).hasCauseThat().hasMessageThat().contains("/ by zero");
   }
 
   @Test
@@ -98,6 +100,7 @@ public void evaluate_ternaryFailure_throwsException(String expression) {
 
     assertThat(exception).hasMessageThat().contains("Evaluation error has occurred.");
     assertThat(exception).hasCauseThat().isInstanceOf(CelEvaluationException.class);
-    assertThat(exception).hasCauseThat().hasMessageThat().contains("evaluation error: / by zero");
+    assertThat(exception).hasCauseThat().hasMessageThat().contains("evaluation error");
+    assertThat(exception).hasCauseThat().hasMessageThat().contains("/ by zero");
   }
 }
diff --git a/codelab/src/test/codelab/Exercise8Test.java b/codelab/src/test/codelab/Exercise8Test.java
@@ -128,7 +128,7 @@ public void optimize_constantFold_evaluateError() throws Exception {
         .contains("evaluation error at <input>:15: key 'referer' is not present in map.");
     assertThat(e2)
         .hasMessageThat()
-        .contains("evaluation error at <input>:0: key 'referer' is not present in map.");
+        .contains("evaluation error: key 'referer' is not present in map.");
   }
 
   @Test

diff --git a/codelab/src/test/codelab/solutions/Exercise1Test.java b/codelab/src/test/codelab/solutions/Exercise1Test.java
@@ -68,6 +68,6 @@ public void evaluate_divideByZeroExpression_throwsEvaluationException() throws E
     IllegalArgumentException exception =
         assertThrows(IllegalArgumentException.class, () -> exercise1.eval(ast));
 
-    assertThat(exception).hasMessageThat().contains("evaluation error: / by zero");
+    assertThat(exception).hasMessageThat().contains("evaluation error at <input>:1: / by zero");
   }
 }
diff --git a/codelab/src/test/codelab/solutions/Exercise3Test.java b/codelab/src/test/codelab/solutions/Exercise3Test.java
@@ -57,7 +57,8 @@ public void evaluate_logicalOrFailure_throwsException(String expression) {
 
     assertThat(exception).hasMessageThat().contains("Evaluation error has occurred.");
     assertThat(exception).hasCauseThat().isInstanceOf(CelEvaluationException.class);
-    assertThat(exception).hasCauseThat().hasMessageThat().contains("evaluation error: / by zero");
+    assertThat(exception).hasCauseThat().hasMessageThat().contains("evaluation error");
+    assertThat(exception).hasCauseThat().hasMessageThat().contains("/ by zero");
   }
 
   @Test
@@ -83,7 +84,8 @@ public void evaluate_logicalAndFailure_throwsException(String expression) {
 
     assertThat(exception).hasMessageThat().contains("Evaluation error has occurred.");
     assertThat(exception).hasCauseThat().isInstanceOf(CelEvaluationException.class);
-    assertThat(exception).hasCauseThat().hasMessageThat().contains("evaluation error: / by zero");
+    assertThat(exception).hasCauseThat().hasMessageThat().contains("evaluation error");
+    assertThat(exception).hasCauseThat().hasMessageThat().contains("/ by zero");
   }
 
   @Test
@@ -108,6 +110,7 @@ public void evaluate_ternaryFailure_throwsException(String expression) {
 
     assertThat(exception).hasMessageThat().contains("Evaluation error has occurred.");
     assertThat(exception).hasCauseThat().isInstanceOf(CelEvaluationException.class);
-    assertThat(exception).hasCauseThat().hasMessageThat().contains("evaluation error: / by zero");
+    assertThat(exception).hasCauseThat().hasMessageThat().contains("evaluation error");
+    assertThat(exception).hasCauseThat().hasMessageThat().contains("/ by zero");
   }
 }
diff --git a/codelab/src/test/codelab/solutions/Exercise8Test.java b/codelab/src/test/codelab/solutions/Exercise8Test.java
@@ -128,7 +128,7 @@ public void optimize_constantFold_evaluateError() throws Exception {
         .contains("evaluation error at <input>:15: key 'referer' is not present in map.");
     assertThat(e2)
         .hasMessageThat()
-        .contains("evaluation error at <input>:0: key 'referer' is not present in map.");
+        .contains("evaluation error: key 'referer' is not present in map.");
   }
 
   @Test

diff --git a/extensions/src/test/java/dev/cel/extensions/CelListsExtensionsTest.java b/extensions/src/test/java/dev/cel/extensions/CelListsExtensionsTest.java
@@ -76,7 +76,7 @@ public void flatten_negativeDepth_throws() {
 
     assertThat(e)
         .hasMessageThat()
-        .contains("evaluation error: Function 'list_flatten_list_int' failed");
+        .contains("evaluation error at <input>:17: Function 'list_flatten_list_int' failed");
     assertThat(e).hasCauseThat().hasMessageThat().isEqualTo("Level must be non-negative");
   }
 

diff --git a/runtime/src/main/java/dev/cel/runtime/CelEvaluationExceptionBuilder.java b/runtime/src/main/java/dev/cel/runtime/CelEvaluationExceptionBuilder.java
@@ -44,9 +44,12 @@ public CelEvaluationExceptionBuilder setErrorCode(CelErrorCode errorCode) {
 
   @CanIgnoreReturnValue
   public CelEvaluationExceptionBuilder setMetadata(Metadata metadata, long exprId) {
-    this.errorLocation =
-        SafeStringFormatter.format(
-            " at %s:%d", metadata.getLocation(), metadata.getPosition(exprId));
+    if (metadata.hasPosition(exprId)) {
+      this.errorLocation =
+          SafeStringFormatter.format(
+              " at %s:%d", metadata.getLocation(), metadata.getPosition(exprId));
+    }
+
     return this;
   }
 

diff --git a/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java b/runtime/src/main/java/dev/cel/runtime/DefaultInterpreter.java
@@ -212,7 +212,7 @@ private IntermediateResult evalInternal(ExecutionFrame frame, CelExpr expr)
       } catch (RuntimeException e) {
         throw CelEvaluationExceptionBuilder.newBuilder(e.getMessage())
             .setCause(e)
-            // .setMetadata(metadata, expr.id()) TODO: Uncomment in the upcoming cl
+            .setMetadata(metadata, expr.id())
             .build();
       }
     }
@@ -419,13 +419,13 @@ private IntermediateResult evalCall(ExecutionFrame frame, CelExpr expr, CelCall
         return IntermediateResult.create(attr, dispatchResult);
       } catch (CelRuntimeException ce) {
         throw CelEvaluationExceptionBuilder.newBuilder(ce)
-            // .setMetadata(metadata, expr.id()) // TODO: Uncomment in the upcoming cl
+            .setMetadata(metadata, expr.id())
             .build();
       } catch (RuntimeException e) {
         throw CelEvaluationExceptionBuilder.newBuilder(
                 "Function '%s' failed with arg(s) '%s'",
                 overload.getOverloadId(), Joiner.on(", ").join(argArray))
-            // .setMetadata(metadata, expr.id()) // TODO: Uncomment in the upcoming cl
+            .setMetadata(metadata, expr.id())
             .setCause(e)
             .build();
       }
@@ -456,7 +456,7 @@ private ResolvedOverload findOverloadOrThrow(
                         .build());
       } catch (CelRuntimeException e) {
         throw CelEvaluationExceptionBuilder.newBuilder(e)
-            // .setMetadata(metadata, expr.id()) // TODO: Uncomment in the upcoming cl
+            .setMetadata(metadata, expr.id())
             .build();
       }
     }

diff --git a/runtime/src/main/java/dev/cel/runtime/DefaultMetadata.java b/runtime/src/main/java/dev/cel/runtime/DefaultMetadata.java
@@ -43,6 +43,11 @@ public String getLocation() {
   @Override
   public int getPosition(long exprId) {
     ImmutableMap<Long, Integer> positions = ast.getSource().getPositionsMap();
-    return positions.containsKey(exprId) ? positions.get(exprId) : 0;
+    return positions.getOrDefault(exprId, 0);
+  }
+
+  @Override
+  public boolean hasPosition(long exprId) {
+    return ast.getSource().getPositionsMap().containsKey(exprId);
   }
 }
diff --git a/runtime/src/main/java/dev/cel/runtime/Metadata.java b/runtime/src/main/java/dev/cel/runtime/Metadata.java
@@ -33,4 +33,7 @@ public interface Metadata {
    * Returns the character position of the node in the source. This is a 0-based character offset.
    */
   int getPosition(long exprId);
+
+  /** Checks if a source position recorded for the provided expression id. */
+  boolean hasPosition(long exprId);
 }
diff --git a/runtime/src/test/java/dev/cel/runtime/CelEvaluationExceptionBuilderTest.java b/runtime/src/test/java/dev/cel/runtime/CelEvaluationExceptionBuilderTest.java
@@ -69,6 +69,11 @@ public String getLocation() {
                   public int getPosition(long exprId) {
                     return 10;
                   }
+
+                  @Override
+                  public boolean hasPosition(long exprId) {
+                    return true;
+                  }
                 },
                 0);
 

diff --git a/runtime/src/test/resources/arithmInt64_error.baseline b/runtime/src/test/resources/arithmInt64_error.baseline
@@ -1,41 +1,41 @@
 Source: 9223372036854775807 + 1
 =====>
 bindings: {}
-error:    evaluation error: long overflow
+error:    evaluation error at test_location:20: long overflow
 error_code:    NUMERIC_OVERFLOW
 
 Source: -9223372036854775808 - 1
 =====>
 bindings: {}
-error:    evaluation error: long overflow
+error:    evaluation error at test_location:21: long overflow
 error_code:    NUMERIC_OVERFLOW
 
 Source: -(-9223372036854775808)
 =====>
 bindings: {}
-error:    evaluation error: long overflow
+error:    evaluation error at test_location:0: long overflow
 error_code:    NUMERIC_OVERFLOW
 
 Source: 5000000000 * 5000000000
 =====>
 bindings: {}
-error:    evaluation error: long overflow
+error:    evaluation error at test_location:11: long overflow
 error_code:    NUMERIC_OVERFLOW
 
 Source: (-9223372036854775808)/-1
 =====>
 bindings: {}
-error:    evaluation error: most negative number wraps
+error:    evaluation error at test_location:22: most negative number wraps
 error_code:    NUMERIC_OVERFLOW
 
 Source: 1 / 0
 =====>
 bindings: {}
-error:    evaluation error: / by zero
+error:    evaluation error at test_location:2: / by zero
 error_code:    DIVIDE_BY_ZERO
 
 Source: 1 % 0
 =====>
 bindings: {}
-error:    evaluation error: / by zero
-error_code:    DIVIDE_BY_ZERO
+error:    evaluation error at test_location:2: / by zero
+error_code:    DIVIDE_BY_ZERO
diff --git a/runtime/src/test/resources/arithmUInt64_error.baseline b/runtime/src/test/resources/arithmUInt64_error.baseline
@@ -1,29 +1,29 @@
 Source: 18446744073709551615u + 1u
 =====>
 bindings: {}
-error:    evaluation error: range overflow on unsigned addition
+error:    evaluation error at test_location:22: range overflow on unsigned addition
 error_code:    NUMERIC_OVERFLOW
 
 Source: 0u - 1u
 =====>
 bindings: {}
-error:    evaluation error: unsigned subtraction underflow
+error:    evaluation error at test_location:3: unsigned subtraction underflow
 error_code:    NUMERIC_OVERFLOW
 
 Source: 5000000000u * 5000000000u
 =====>
 bindings: {}
-error:    evaluation error: multiply out of unsigned integer range
+error:    evaluation error at test_location:12: multiply out of unsigned integer range
 error_code:    NUMERIC_OVERFLOW
 
 Source: 1u / 0u
 =====>
 bindings: {}
-error:    evaluation error: / by zero
+error:    evaluation error at test_location:3: / by zero
 error_code:    DIVIDE_BY_ZERO
 
 Source: 1u % 0u
 =====>
 bindings: {}
-error:    evaluation error: / by zero
-error_code:    DIVIDE_BY_ZERO
+error:    evaluation error at test_location:3: / by zero
+error_code:    DIVIDE_BY_ZERO
diff --git a/runtime/src/test/resources/boolConversions.baseline b/runtime/src/test/resources/boolConversions.baseline
@@ -11,16 +11,4 @@ result:   true
 Source: bool('false') || bool('FALSE') || bool('False') || bool('f') || bool('0')
 =====>
 bindings: {}
-result:   false
-
-Source: bool('TrUe')
-=====>
-bindings: {}
-error:    evaluation error: Type conversion error from 'string' to 'bool': [TrUe]
-error_code:    BAD_FORMAT
-
-Source: bool('FaLsE')
-=====>
-bindings: {}
-error:    evaluation error: Type conversion error from 'string' to 'bool': [FaLsE]
-error_code:    BAD_FORMAT
+result:   false
diff --git a/runtime/src/test/resources/boolConversions_error.baseline b/runtime/src/test/resources/boolConversions_error.baseline
@@ -0,0 +1,11 @@
+Source: bool('TrUe')
+=====>
+bindings: {}
+error:    evaluation error at test_location:4: Type conversion error from 'string' to 'bool': [TrUe]
+error_code:    BAD_FORMAT
+
+Source: bool('FaLsE')
+=====>
+bindings: {}
+error:    evaluation error at test_location:4: Type conversion error from 'string' to 'bool': [FaLsE]
+error_code:    BAD_FORMAT
diff --git a/runtime/src/test/resources/booleans.baseline b/runtime/src/test/resources/booleans.baseline
@@ -41,54 +41,6 @@ declare y {
 bindings: {y=0}
 result:   true
 
-Source: 1 / y == 1 || false
-declare x {
-  value bool
-}
-declare y {
-  value int
-}
-=====>
-bindings: {y=0}
-error:    evaluation error: / by zero
-error_code:    DIVIDE_BY_ZERO
-
-Source: false || 1 / y == 1
-declare x {
-  value bool
-}
-declare y {
-  value int
-}
-=====>
-bindings: {y=0}
-error:    evaluation error: / by zero
-error_code:    DIVIDE_BY_ZERO
-
-Source: 1 / y == 1 && true
-declare x {
-  value bool
-}
-declare y {
-  value int
-}
-=====>
-bindings: {y=0}
-error:    evaluation error: / by zero
-error_code:    DIVIDE_BY_ZERO
-
-Source: true && 1 / y == 1
-declare x {
-  value bool
-}
-declare y {
-  value int
-}
-=====>
-bindings: {y=0}
-error:    evaluation error: / by zero
-error_code:    DIVIDE_BY_ZERO
-
 Source: 1 / y == 1 && false
 declare x {
   value bool
@@ -274,5 +226,4 @@ declare y {
 }
 =====>
 bindings: {}
-result:   true
-
+result:   true