Skip to content

Commit 38c1f2b

Browse files
l46kokcopybara-github
authored andcommitted
Fix type-checker and runtime to properly resolve global escaped identifiers
PiperOrigin-RevId: 856445002
1 parent 777d089 commit 38c1f2b

7 files changed

Lines changed: 115 additions & 17 deletions

File tree

.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,3 +31,5 @@ mvn-artifacts
3131

3232
*.swp
3333
*.lock
34+
.eclipse
35+
.vscode

checker/src/main/java/dev/cel/checker/Env.java

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -451,17 +451,28 @@ public Env add(String name, Type type) {
451451
* until the root package is reached. If {@code container} starts with {@code .}, the resolution
452452
* is in the root container only.
453453
*
454-
* <p>Returns {@code null} if the function cannot be found.
454+
* <p>Returns {@code null} if the ident cannot be found.
455455
*/
456456
public @Nullable CelIdentDecl tryLookupCelIdent(CelContainer container, String name) {
457-
// Attempt to find the decl with just the ident name to account for shadowed variables
458-
CelIdentDecl decl = tryLookupCelIdentFromLocalScopes(name);
459-
if (decl != null) {
460-
return decl;
457+
// A name with a leading '.' always resolves in the root scope, bypassing local scopes.
458+
if (!name.startsWith(".")) {
459+
// Check if this is a qualified ident, or a field selection.
460+
String simpleName = name;
461+
int dotIndex = name.indexOf('.');
462+
if (dotIndex > 0) {
463+
simpleName = name.substring(0, dotIndex);
464+
}
465+
466+
// Attempt to find the decl with just the ident name to account for shadowed variables.
467+
CelIdentDecl decl = tryLookupCelIdentFromLocalScopes(simpleName);
468+
if (decl != null) {
469+
// Null signals field selection
470+
return dotIndex > 0 ? null : decl;
471+
}
461472
}
462473

463474
for (String cand : container.resolveCandidateNames(name)) {
464-
decl = tryLookupCelIdent(cand);
475+
CelIdentDecl decl = tryLookupCelIdent(cand);
465476
if (decl != null) {
466477
return decl;
467478
}
@@ -503,7 +514,13 @@ public Env add(String name, Type type) {
503514
return null;
504515
}
505516

506-
private @Nullable CelIdentDecl tryLookupCelIdentFromLocalScopes(String name) {
517+
/**
518+
* Lookup a local identifier by name. This searches only comprehension scopes, bypassing standard
519+
* environment or user-defined environment.
520+
*
521+
* <p>Returns {@code null} if not found in local scopes.
522+
*/
523+
@Nullable CelIdentDecl tryLookupCelIdentFromLocalScopes(String name) {
507524
int firstUserSpaceScope = 2;
508525
// Iterate from the top of the stack down to the first local scope.
509526
// Note that:

checker/src/main/java/dev/cel/checker/ExprChecker.java

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -237,15 +237,22 @@ private CelExpr visit(CelExpr expr, CelExpr.CelIdent ident) {
237237
if (decl.equals(Env.ERROR_IDENT_DECL)) {
238238
// error reported
239239
env.setType(expr, SimpleType.ERROR);
240-
env.setRef(expr, makeReference(decl));
240+
env.setRef(expr, makeReference(decl.name(), decl));
241241
return expr;
242242
}
243-
if (!decl.name().equals(ident.name())) {
243+
244+
// Preserve leading dot to signal runtime to bypass local scopes.
245+
String refName = decl.name();
246+
if (ident.name().startsWith(".")) {
247+
refName = "." + refName;
248+
}
249+
250+
if (!refName.equals(ident.name())) {
244251
// Overwrite the identifier with its fully qualified name.
245-
expr = replaceIdentSubtree(expr, decl.name());
252+
expr = replaceIdentSubtree(expr, refName);
246253
}
247254
env.setType(expr, decl.type());
248-
env.setRef(expr, makeReference(decl));
255+
env.setRef(expr, makeReference(refName, decl));
249256
return expr;
250257
}
251258

@@ -260,13 +267,24 @@ private CelExpr visit(CelExpr expr, CelExpr.CelSelect select) {
260267
env.reportError(expr.id(), getPosition(expr), "expression does not select a field");
261268
env.setType(expr, SimpleType.BOOL);
262269
} else {
270+
// Preserve leading dot to signal runtime to bypass local scopes.
271+
String refName = decl.name();
272+
if (qname.startsWith(".")) {
273+
refName = "." + refName;
274+
}
275+
263276
if (namespacedDeclarations) {
264277
// Rewrite the node to be a variable reference to the resolved fully-qualified
265278
// variable name.
266-
expr = replaceIdentSubtree(expr, decl.name());
279+
expr = replaceIdentSubtree(expr, refName);
267280
}
268281
env.setType(expr, decl.type());
269-
env.setRef(expr, makeReference(decl));
282+
// Build reference with the (potentially prefixed) name, preserving constant value.
283+
CelReference.Builder refBuilder = CelReference.newBuilder().setName(refName);
284+
if (decl.constant().isPresent()) {
285+
refBuilder.setValue(decl.constant().get());
286+
}
287+
env.setRef(expr, refBuilder.build());
270288
}
271289
return expr;
272290
}
@@ -595,8 +613,8 @@ private CelExpr visit(CelExpr expr, CelExpr.CelComprehension compre) {
595613
return expr;
596614
}
597615

598-
private CelReference makeReference(CelIdentDecl decl) {
599-
CelReference.Builder ref = CelReference.newBuilder().setName(decl.name());
616+
private CelReference makeReference(String name, CelIdentDecl decl) {
617+
CelReference.Builder ref = CelReference.newBuilder().setName(name);
600618
if (decl.constant().isPresent()) {
601619
ref.setValue(decl.constant().get());
602620
}
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
Source: .cel.expr.conformance.proto3.TestAllTypes
22
=====>
3-
cel.expr.conformance.proto3.TestAllTypes~type(cel.expr.conformance.proto3.TestAllTypes)^cel.expr.conformance.proto3.TestAllTypes
3+
.cel.expr.conformance.proto3.TestAllTypes~type(cel.expr.conformance.proto3.TestAllTypes)^.cel.expr.conformance.proto3.TestAllTypes

runtime/src/main/java/dev/cel/runtime/RuntimeUnknownResolver.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,11 @@ Optional<AccumulatedUnknowns> maybePartialUnknown(CelAttribute attribute) {
9898

9999
/** Resolve a simple name to a value. */
100100
DefaultInterpreter.IntermediateResult resolveSimpleName(String name, Long exprId) {
101+
// Strip leading dot if present (for global disambiguation).
102+
if (name.startsWith(".")) {
103+
name = name.substring(1);
104+
}
105+
101106
CelAttribute attr = CelAttribute.EMPTY;
102107

103108
if (attributeTrackingEnabled) {
@@ -154,6 +159,10 @@ private ScopedResolver(
154159

155160
@Override
156161
DefaultInterpreter.IntermediateResult resolveSimpleName(String name, Long exprId) {
162+
// A name with a leading '.' always resolves in the root scope
163+
if (name.startsWith(".")) {
164+
return parent.resolveSimpleName(name, exprId);
165+
}
157166
DefaultInterpreter.IntermediateResult result = lazyEvalResultCache.get(name);
158167
if (result != null) {
159168
return copyIfMutable(result);

runtime/src/test/resources/comprehension.baseline

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,4 +19,36 @@ declare com.x {
1919
}
2020
=====>
2121
bindings: {com.x=1}
22-
result: true
22+
result: true
23+
24+
Source: [{'z': 0}].exists(y, y.z == 0)
25+
declare cel.example.y {
26+
value int
27+
}
28+
=====>
29+
bindings: {cel.example.y={z=1}}
30+
result: true
31+
32+
Source: [{'z': 0}].exists(y, y.z == 0 && .y.z == 1)
33+
declare y.z {
34+
value int
35+
}
36+
=====>
37+
bindings: {y.z=1}
38+
result: true
39+
40+
Source: [0].exists(x, x == 0 && .x == 1)
41+
declare x {
42+
value int
43+
}
44+
=====>
45+
bindings: {x=1}
46+
result: true
47+
48+
Source: [0].exists(x, [x+1].exists(x, x == .x))
49+
declare x {
50+
value int
51+
}
52+
=====>
53+
bindings: {x=1}
54+
result: true

testing/src/main/java/dev/cel/testing/BaseInterpreterTest.java

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -863,6 +863,26 @@ public void comprehension() throws Exception {
863863
container = CelContainer.ofName("com");
864864
source = "[0].exists(x, x == 0)";
865865
runTest(ImmutableMap.of("com.x", 1));
866+
867+
clearAllDeclarations();
868+
declareVariable("cel.example.y", SimpleType.INT);
869+
container = CelContainer.ofName("cel.example");
870+
source = "[{'z': 0}].exists(y, y.z == 0)";
871+
runTest(ImmutableMap.of("cel.example.y", ImmutableMap.of("z", 1)));
872+
873+
clearAllDeclarations();
874+
declareVariable("y.z", SimpleType.INT);
875+
container = CelContainer.ofName("y");
876+
source = "[{'z': 0}].exists(y, y.z == 0 && .y.z == 1)";
877+
runTest(ImmutableMap.of("y.z", 1));
878+
879+
clearAllDeclarations();
880+
declareVariable("x", SimpleType.INT);
881+
source = "[0].exists(x, x == 0 && .x == 1)";
882+
runTest(ImmutableMap.of("x", 1));
883+
884+
source = "[0].exists(x, [x+1].exists(x, x == .x))";
885+
runTest(ImmutableMap.of("x", 1));
866886
}
867887

868888
@Test

0 commit comments

Comments
 (0)