Skip to content

Commit 86d2084

Browse files
l46kokcopybara-github
authored andcommitted
Fix dead code reachability logic erroneously flagging on unconditional nested rules
Port of cel-expr/cel-go#1323 PiperOrigin-RevId: 924956895
1 parent c71923c commit 86d2084

4 files changed

Lines changed: 44 additions & 13 deletions

File tree

conformance/src/test/java/dev/cel/conformance/policy/BUILD.bazel

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,5 @@ java_library(
3232

3333
cel_policy_conformance_test_java(
3434
name = "policy_conformance_tests",
35-
skip_tests = [
36-
"nested_rules_variable_shadowing",
37-
"variable_type_propagation",
38-
"unconditional_rules",
39-
],
4035
testdata = "@cel_policy//conformance:testdata",
4136
)

policy/src/main/java/dev/cel/policy/CelCompiledRule.java

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,17 @@ public boolean hasOptionalOutput() {
5252
for (CelCompiledMatch match : matches()) {
5353
if (match.result().kind().equals(CelCompiledMatch.Result.Kind.RULE)
5454
&& match.result().rule().hasOptionalOutput()) {
55-
return true;
56-
}
57-
58-
if (match.isConditionTriviallyTrue()) {
55+
// If the nested rule is unconditional, the matching may fallthrough to the next match
56+
// in this context (unwrapping the optional value from the nested rule).
57+
if (!match.isConditionTriviallyTrue()) {
58+
return true;
59+
}
60+
isOptionalOutput = true;
61+
} else if (match.isConditionTriviallyTrue()) {
5962
return false;
63+
} else {
64+
isOptionalOutput = true;
6065
}
61-
62-
isOptionalOutput = true;
6366
}
6467

6568
return isOptionalOutput;

policy/src/main/java/dev/cel/policy/CelPolicyCompilerImpl.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -249,14 +249,21 @@ private CelCompiledRule compileRuleImpl(
249249
}
250250

251251
private void checkUnreachableCode(CelCompiledRule compiledRule, CompilerContext compilerContext) {
252-
boolean ruleHasOptional = compiledRule.hasOptionalOutput();
253252
ImmutableList<CelCompiledMatch> compiledMatches = compiledRule.matches();
254253
int matchCount = compiledMatches.size();
255254
for (int i = matchCount - 1; i >= 0; i--) {
256255
CelCompiledMatch compiledMatch = compiledMatches.get(i);
257256
boolean isTriviallyTrue = compiledMatch.isConditionTriviallyTrue();
258257

259-
if (isTriviallyTrue && !ruleHasOptional && i != matchCount - 1) {
258+
// If the match is a single output or a nested rule that always returns a value, it is
259+
// exhaustive. If the condition is trivially true, then all subsequent branches are
260+
// unreachable.
261+
boolean isExhaustive =
262+
isTriviallyTrue
263+
&& (compiledMatch.result().kind().equals(Kind.OUTPUT)
264+
|| !compiledMatch.result().rule().hasOptionalOutput());
265+
266+
if (isExhaustive && i != matchCount - 1) {
260267
if (compiledMatch.result().kind().equals(Kind.OUTPUT)) {
261268
compilerContext.addIssue(
262269
compiledMatch.sourceId(),

policy/src/test/java/dev/cel/policy/CelPolicyCompilerImplTest.java

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,32 @@ public void compileYamlPolicy_withImportsOnNestedRules() throws Exception {
111111
assertThat(ast.getResultType()).isEqualTo(OptionalType.create(SimpleType.BOOL));
112112
}
113113

114+
@Test
115+
public void compileYamlPolicy_nestedRuleOptionalFallbackDivergence() throws Exception {
116+
Cel cel = newCel().toCelBuilder().addVar("input_val", SimpleType.INT).build();
117+
String policySource =
118+
"name: grandparent_policy\n"
119+
+ "rule:\n"
120+
+ " match:\n"
121+
+ " - condition: 'input_val == 2'\n" // conditional grandparent match
122+
+ " rule:\n"
123+
+ " id: parent_rule\n"
124+
+ " match:\n"
125+
+ " - condition: 'input_val == 1'\n" // conditional parent match
126+
+ " rule:\n"
127+
+ " id: nested_rule\n"
128+
+ " match:\n"
129+
+ " - condition: 'input_val == 3'\n"
130+
+ " output: 'true'\n"
131+
+ " - output: 'true'\n" // fallback (optional)
132+
+ " - output: 'true'\n";
133+
CelPolicy policy = POLICY_PARSER.parse(policySource);
134+
CelAbstractSyntaxTree ast =
135+
CelPolicyCompilerFactory.newPolicyCompiler(cel).build().compile(policy);
136+
137+
assertThat(ast.getResultType()).isEqualTo(OptionalType.create(SimpleType.BOOL));
138+
}
139+
114140
@Test
115141
public void compileYamlPolicy_containsCompilationError_throws(
116142
@TestParameter TestErrorYamlPolicy testCase) throws Exception {

0 commit comments

Comments
 (0)