Skip to content

Commit dd61d62

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

File tree

8 files changed

+157
-16
lines changed

8 files changed

+157
-16
lines changed

.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: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -451,17 +451,29 @@ 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+
// Appears to be a field selection on a local.
470+
// Return null instead of attempting to resolve qualified name at the root scope
471+
return dotIndex > 0 ? null : decl;
472+
}
461473
}
462474

463475
for (String cand : container.resolveCandidateNames(name)) {
464-
decl = tryLookupCelIdent(cand);
476+
CelIdentDecl decl = tryLookupCelIdent(cand);
465477
if (decl != null) {
466478
return decl;
467479
}
@@ -503,7 +515,13 @@ public Env add(String name, Type type) {
503515
return null;
504516
}
505517

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

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

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -237,15 +237,18 @@ 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+
String refName = maybeDisambiguate(ident.name(), decl.name());
245+
246+
if (!refName.equals(ident.name())) {
244247
// Overwrite the identifier with its fully qualified name.
245-
expr = replaceIdentSubtree(expr, decl.name());
248+
expr = replaceIdentSubtree(expr, refName);
246249
}
247250
env.setType(expr, decl.type());
248-
env.setRef(expr, makeReference(decl));
251+
env.setRef(expr, makeReference(refName, decl));
249252
return expr;
250253
}
251254

@@ -260,13 +263,15 @@ private CelExpr visit(CelExpr expr, CelExpr.CelSelect select) {
260263
env.reportError(expr.id(), getPosition(expr), "expression does not select a field");
261264
env.setType(expr, SimpleType.BOOL);
262265
} else {
266+
String refName = maybeDisambiguate(qname, decl.name());
267+
263268
if (namespacedDeclarations) {
264269
// Rewrite the node to be a variable reference to the resolved fully-qualified
265270
// variable name.
266-
expr = replaceIdentSubtree(expr, decl.name());
271+
expr = replaceIdentSubtree(expr, refName);
267272
}
268273
env.setType(expr, decl.type());
269-
env.setRef(expr, makeReference(decl));
274+
env.setRef(expr, makeReference(refName, decl));
270275
}
271276
return expr;
272277
}
@@ -595,14 +600,32 @@ private CelExpr visit(CelExpr expr, CelExpr.CelComprehension compre) {
595600
return expr;
596601
}
597602

598-
private CelReference makeReference(CelIdentDecl decl) {
599-
CelReference.Builder ref = CelReference.newBuilder().setName(decl.name());
603+
private CelReference makeReference(String name, CelIdentDecl decl) {
604+
CelReference.Builder ref = CelReference.newBuilder().setName(name);
600605
if (decl.constant().isPresent()) {
601606
ref.setValue(decl.constant().get());
602607
}
603608
return ref.build();
604609
}
605610

611+
/**
612+
* Returns the reference name, prefixed with a leading dot only if disambiguation is needed.
613+
* Disambiguation is needed when: the original name had a leading dot, and there's a local
614+
* variable that would shadow the resolved name.
615+
*/
616+
private String maybeDisambiguate(String originalName, String resolvedName) {
617+
if (!originalName.startsWith(".")) {
618+
return resolvedName;
619+
}
620+
String simpleName = originalName.substring(1);
621+
int dotIndex = simpleName.indexOf('.');
622+
String localName = dotIndex > 0 ? simpleName.substring(0, dotIndex) : simpleName;
623+
if (env.tryLookupCelIdentFromLocalScopes(localName) != null) {
624+
return "." + resolvedName;
625+
}
626+
return resolvedName;
627+
}
628+
606629
private OverloadResolution resolveOverload(
607630
long callExprId,
608631
int position,

checker/src/test/java/dev/cel/checker/ExprCheckerTest.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,10 @@ public void referenceTypeRelative() throws Exception {
199199
public void referenceTypeAbsolute() throws Exception {
200200
source = ".cel.expr.conformance.proto3.TestAllTypes";
201201
runTest();
202+
203+
declareVariable("app.config", SimpleType.INT);
204+
source = "[0].exists(app, .app.config == 1)";
205+
runTest();
202206
}
203207

204208
@Test
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,36 @@
11
Source: .cel.expr.conformance.proto3.TestAllTypes
22
=====>
33
cel.expr.conformance.proto3.TestAllTypes~type(cel.expr.conformance.proto3.TestAllTypes)^cel.expr.conformance.proto3.TestAllTypes
4+
5+
Source: [0].exists(app, .app.config == 1)
6+
declare app.config {
7+
value int
8+
}
9+
=====>
10+
__comprehension__(
11+
// Variable
12+
app,
13+
// Target
14+
[
15+
0~int
16+
]~list(int),
17+
// Accumulator
18+
@result,
19+
// Init
20+
false~bool,
21+
// LoopCondition
22+
@not_strictly_false(
23+
!_(
24+
@result~bool^@result
25+
)~bool^logical_not
26+
)~bool^not_strictly_false,
27+
// LoopStep
28+
_||_(
29+
@result~bool^@result,
30+
_==_(
31+
.app.config~int^.app.config,
32+
1~int
33+
)~bool^equals
34+
)~bool^logical_or,
35+
// Result
36+
@result~bool^@result)~bool

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)