Skip to content

Update runtime to correctly handle comprehension vars shadowing root scope. #1859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
copybara-service merged 1 commit into from
Jan 21, 2026
Merged

Update runtime to correctly handle comprehension vars shadowing root scope. #1859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions eval/compiler/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,13 +167,15 @@ cc_test(
":qualified_reference_resolver",
"//base:builtins",
"//common:function_descriptor",
"//common:kind",
"//common:value",
"//eval/public:activation",
"//eval/public:builtin_func_registrar",
"//eval/public:cel_attribute",
"//eval/public:cel_builtins",
"//eval/public:cel_expr_builder_factory",
"//eval/public:cel_expression",
"//eval/public:cel_function",
"//eval/public:cel_function_adapter",
"//eval/public:cel_function_registry",
"//eval/public:cel_options",
Expand All @@ -193,12 +195,16 @@ cc_test(
"//runtime:function",
"//runtime:function_adapter",
"//runtime:runtime_options",
"//runtime:standard_functions",
"//runtime/internal:runtime_env_testing",
"@com_google_absl//absl/base:nullability",
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/log:absl_check",
"@com_google_absl//absl/status",
"@com_google_absl//absl/status:status_matchers",
"@com_google_absl//absl/status:statusor",
"@com_google_absl//absl/strings",
"@com_google_absl//absl/time",
"@com_google_absl//absl/types:span",
"@com_google_cel_spec//proto/cel/expr:checked_cc_proto",
"@com_google_cel_spec//proto/cel/expr:syntax_cc_proto",
Expand Down
96 changes: 54 additions & 42 deletions eval/compiler/flat_expr_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,6 +696,10 @@ class FlatExprVisitor : public cel::AstVisitor {
// If lazy evaluation enabled and ided as a lazy expression,
// subexpression and slot will be set.
SlotLookupResult LookupSlot(absl::string_view path) {
// If there's a leading dot, it cannot resolve to a local variable.
if (absl::StartsWith(path, ".")) {
return {-1, -1};
}
if (block_.has_value()) {
const BlockInfo& block = *block_;
if (block.in) {
Expand Down Expand Up @@ -784,27 +788,62 @@ class FlatExprVisitor : public cel::AstVisitor {
return;
}

// Check if this is a local variable first (since it should shadow most
// other interpretations).
SlotLookupResult slot = LookupSlot(path);

if (slot.subexpression >= 0) {
auto* subexpression =
program_builder_.GetExtractedSubexpression(slot.subexpression);
if (subexpression == nullptr) {
SetProgressStatusError(
absl::InternalError("bad subexpression reference"));
return;
}
if (subexpression->IsRecursive()) {
const auto& program = subexpression->recursive_program();
SetRecursiveStep(
CreateDirectLazyInitStep(slot.slot, program.step.get(), expr.id()),
program.depth + 1);
} else {
// Off by one since mainline expression will be index 0.
AddStep(
CreateLazyInitStep(slot.slot, slot.subexpression + 1, expr.id()));
}
return;
} else if (slot.slot >= 0) {
if (options_.max_recursion_depth != 0) {
SetRecursiveStep(
CreateDirectSlotIdentStep(ident_expr.name(), slot.slot, expr.id()),
1);
} else {
AddStep(
CreateIdentStepForSlot(ident_expr.name(), slot.slot, expr.id()));
}
return;
}

// Attempt to resolve a select expression as a namespaced identifier for an
// enum or type constant value.
absl::optional<cel::Value> const_value;
int64_t select_root_id = -1;
std::string qualified_path;
std::string path_candidate;

while (!namespace_stack_.empty()) {
const auto& select_node = namespace_stack_.front();
// Generate path in format "<ident>.<field 0>.<field 1>...".
auto select_expr = select_node.first;
qualified_path = absl::StrCat(path, ".", select_node.second);
const cel::Expr* select_expr = select_node.first;
path_candidate = absl::StrCat(path, ".", select_node.second);

// Attempt to find a constant enum or type value which matches the
// qualified path present in the expression. Whether the identifier
// can be resolved to a type instance depends on whether the option to
// 'enable_qualified_type_identifiers' is set to true.
const_value = resolver_.FindConstant(qualified_path, select_expr->id());
const_value = resolver_.FindConstant(path_candidate, select_expr->id());
if (const_value) {
resolved_select_expr_ = select_expr;
select_root_id = select_expr->id();
path = qualified_path;
path = path_candidate;
namespace_stack_.clear();
break;
}
Expand All @@ -818,56 +857,29 @@ class FlatExprVisitor : public cel::AstVisitor {
select_root_id = expr.id();
}

// TODO(issues/97): Need to add support for resolving packaged names at
// runtime if Parse-only. For checked, checker should have reported the
// expected interpretation.
if (const_value) {
// If the path starts with a dot, strip it.
absl::string_view name = absl::StripPrefix(path, ".");
if (options_.max_recursion_depth != 0) {
SetRecursiveStep(
CreateDirectShadowableValueStep(
path, std::move(const_value).value(), select_root_id),
name, std::move(const_value).value(), select_root_id),
1);
return;
}
AddStep(CreateShadowableValueStep(path, std::move(const_value).value(),
AddStep(CreateShadowableValueStep(name, std::move(const_value).value(),
select_root_id));
return;
}

// If this is a comprehension variable, check for the assigned slot.
SlotLookupResult slot = LookupSlot(path);

if (slot.subexpression >= 0) {
auto* subexpression =
program_builder_.GetExtractedSubexpression(slot.subexpression);
if (subexpression == nullptr) {
SetProgressStatusError(
absl::InternalError("bad subexpression reference"));
return;
}
if (subexpression->IsRecursive()) {
const auto& program = subexpression->recursive_program();
SetRecursiveStep(
CreateDirectLazyInitStep(slot.slot, program.step.get(), expr.id()),
program.depth + 1);
} else {
// Off by one since mainline expression will be index 0.
AddStep(
CreateLazyInitStep(slot.slot, slot.subexpression + 1, expr.id()));
}
return;
} else if (slot.slot >= 0) {
if (options_.max_recursion_depth != 0) {
SetRecursiveStep(
CreateDirectSlotIdentStep(ident_expr.name(), slot.slot, expr.id()),
1);
} else {
AddStep(
CreateIdentStepForSlot(ident_expr.name(), slot.slot, expr.id()));
}
return;
}
absl::string_view ident_name = absl::StripPrefix(ident_expr.name(), ".");
if (options_.max_recursion_depth != 0) {
SetRecursiveStep(CreateDirectIdentStep(ident_expr.name(), expr.id()), 1);
SetRecursiveStep(CreateDirectIdentStep(ident_name, expr.id()), 1);
} else {
AddStep(CreateIdentStep(ident_expr.name(), expr.id()));
AddStep(CreateIdentStep(ident_name, expr.id()));
}
}

Expand Down
Loading