Skip to content

Commit

Permalink
Update cel.@block implementation in C++ to handle comprehensions
Browse files Browse the repository at this point in the history 
PiperOrigin-RevId: 666893945
  • Loading branch information
@jcking @copybara-github
jcking authored and copybara-github committed Aug 26, 2024
1 parent 647ed5f commit aeefe8f
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 53 deletions.
4 changes: 2 additions & 2 deletions bazel/deps.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,10 @@ def cel_spec_deps():
url = "https://github.com/bazelbuild/rules_python/releases/download/0.33.2/rules_python-0.33.2.tar.gz",
)

CEL_SPEC_GIT_SHA = "5ed294fa64206016a37db2986dab942c80a65e4b" # Aug 16, 2024
CEL_SPEC_GIT_SHA = "f027a86d2e5bf18f796be0c4373f637a61041cde" # Aug 23, 2024
http_archive(
name = "com_google_cel_spec",
sha256 = "926abf84cde8c05ce99700caee5786bc7d8aeec77185fd669bce27df455a1215",
sha256 = "006594fa4f97819a4e4cd98404e4522f5f46ed5ac65402b354649bcc871b0cf2",
strip_prefix = "cel-spec-" + CEL_SPEC_GIT_SHA,
urls = ["https://github.com/google/cel-spec/archive/" + CEL_SPEC_GIT_SHA + ".zip"],
)
Expand Down
20 changes: 0 additions & 20 deletions conformance/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,17 +211,6 @@ _TESTS_TO_SKIP_MODERN = [

# TODO: Add missing conversion function
"conversions/bool",

# cel.@block
"block_ext/basic/multiple_macros_1",
"block_ext/basic/multiple_macros_2",
"block_ext/basic/multiple_macros_3",
"block_ext/basic/nested_macros",
"block_ext/basic/nested_macros_1",
"block_ext/basic/nested_macros_2",
"block_ext/basic/adjacent_macros",
"block_ext/basic/macro_shadowed_variable_1",
"block_ext/basic/macro_shadowed_variable_2",
]

_TESTS_TO_SKIP_MODERN_DASHBOARD = [
Expand Down Expand Up @@ -291,15 +280,6 @@ _TESTS_TO_SKIP_LEGACY = [
"block_ext/basic/optional_map",
"block_ext/basic/optional_map_chained",
"block_ext/basic/optional_message",
"block_ext/basic/multiple_macros_1",
"block_ext/basic/multiple_macros_2",
"block_ext/basic/multiple_macros_3",
"block_ext/basic/nested_macros",
"block_ext/basic/nested_macros_1",
"block_ext/basic/nested_macros_2",
"block_ext/basic/adjacent_macros",
"block_ext/basic/macro_shadowed_variable_1",
"block_ext/basic/macro_shadowed_variable_2",
]

_TESTS_TO_SKIP_LEGACY_DASHBOARD = [
Expand Down
46 changes: 26 additions & 20 deletions conformance/service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,19 +140,22 @@ absl::optional<cel::Expr> CelIterVarMacroExpander(
if (!IsCelNamespace(target)) {
return absl::nullopt;
}
cel::Expr& index_arg = args[0];
if (!index_arg.has_const_expr() || !index_arg.const_expr().has_int_value()) {
cel::Expr& depth_arg = args[0];
if (!depth_arg.has_const_expr() || !depth_arg.const_expr().has_int_value() ||
depth_arg.const_expr().int_value() < 0) {
return factory.ReportErrorAt(
index_arg,
"cel.iterVar requires a single non-negative int constant arg");
depth_arg, "cel.iterVar requires two non-negative int constant args");
}
int64_t index = index_arg.const_expr().int_value();
if (index < 0) {
cel::Expr& unique_arg = args[1];
if (!unique_arg.has_const_expr() ||
!unique_arg.const_expr().has_int_value() ||
unique_arg.const_expr().int_value() < 0) {
return factory.ReportErrorAt(
index_arg,
"cel.iterVar requires a single non-negative int constant arg");
unique_arg, "cel.iterVar requires two non-negative int constant args");
}
return factory.NewIdent(absl::StrCat("@c:", index));
return factory.NewIdent(
absl::StrCat("@it:", depth_arg.const_expr().int_value(), ":",
unique_arg.const_expr().int_value()));
}

absl::optional<cel::Expr> CelAccuVarMacroExpander(
Expand All @@ -161,19 +164,22 @@ absl::optional<cel::Expr> CelAccuVarMacroExpander(
if (!IsCelNamespace(target)) {
return absl::nullopt;
}
cel::Expr& index_arg = args[0];
if (!index_arg.has_const_expr() || !index_arg.const_expr().has_int_value()) {
cel::Expr& depth_arg = args[0];
if (!depth_arg.has_const_expr() || !depth_arg.const_expr().has_int_value() ||
depth_arg.const_expr().int_value() < 0) {
return factory.ReportErrorAt(
index_arg,
"cel.accuVar requires a single non-negative int constant arg");
depth_arg, "cel.accuVar requires two non-negative int constant args");
}
int64_t index = index_arg.const_expr().int_value();
if (index < 0) {
cel::Expr& unique_arg = args[1];
if (!unique_arg.has_const_expr() ||
!unique_arg.const_expr().has_int_value() ||
unique_arg.const_expr().int_value() < 0) {
return factory.ReportErrorAt(
index_arg,
"cel.accuVar requires a single non-negative int constant arg");
unique_arg, "cel.accuVar requires two non-negative int constant args");
}
return factory.NewIdent(absl::StrCat("@x:", index));
return factory.NewIdent(
absl::StrCat("@ac:", depth_arg.const_expr().int_value(), ":",
unique_arg.const_expr().int_value()));
}

absl::Status RegisterCelBlockMacros(cel::MacroRegistry& registry) {
Expand All @@ -185,11 +191,11 @@ absl::Status RegisterCelBlockMacros(cel::MacroRegistry& registry) {
CEL_RETURN_IF_ERROR(registry.RegisterMacro(index_macro));
CEL_ASSIGN_OR_RETURN(
auto iter_var_macro,
cel::Macro::Receiver("iterVar", 1, CelIterVarMacroExpander));
cel::Macro::Receiver("iterVar", 2, CelIterVarMacroExpander));
CEL_RETURN_IF_ERROR(registry.RegisterMacro(iter_var_macro));
CEL_ASSIGN_OR_RETURN(
auto accu_var_macro,
cel::Macro::Receiver("accuVar", 1, CelAccuVarMacroExpander));
cel::Macro::Receiver("accuVar", 2, CelAccuVarMacroExpander));
CEL_RETURN_IF_ERROR(registry.RegisterMacro(accu_var_macro));
return absl::OkStatus();
}
Expand Down
24 changes: 13 additions & 11 deletions eval/compiler/flat_expr_builder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -531,21 +531,21 @@ class FlatExprVisitor : public cel::AstVisitor {
size_t index;
if (!absl::SimpleAtoi(index_suffix, &index)) {
SetProgressStatusError(
issue_collector_.AddIssue(RuntimeIssue::CreateWarning(
issue_collector_.AddIssue(RuntimeIssue::CreateError(
absl::InvalidArgumentError("bad @index"))));
return {-1, -1};
}
if (index >= block.size) {
SetProgressStatusError(
issue_collector_.AddIssue(RuntimeIssue::CreateWarning(
issue_collector_.AddIssue(RuntimeIssue::CreateError(
absl::InvalidArgumentError(absl::StrCat(
"invalid @index greater than number of bindings: ",
index, " >= ", block.size)))));
return {-1, -1};
}
if (index >= block.current_index) {
SetProgressStatusError(
issue_collector_.AddIssue(RuntimeIssue::CreateWarning(
issue_collector_.AddIssue(RuntimeIssue::CreateError(
absl::InvalidArgumentError(absl::StrCat(
"@index references current or future binding: ", index,
" >= ", block.current_index)))));
Expand All @@ -554,14 +554,6 @@ class FlatExprVisitor : public cel::AstVisitor {
return {static_cast<int>(block.index + index),
block.subexpressions[index]};
}
if (absl::ConsumePrefix(&index_suffix, "@c:") ||
absl::ConsumePrefix(&index_suffix, "@x:")) {
SetProgressStatusError(issue_collector_.AddIssue(
RuntimeIssue::CreateWarning(absl::InvalidArgumentError(
"support is not yet implemented for CSE generated @c: or @x: "
"comprehension variables"))));
return {-1, -1};
}
}
}
if (!comprehension_stack_.empty()) {
Expand All @@ -588,6 +580,16 @@ class FlatExprVisitor : public cel::AstVisitor {
}
}
}
if (absl::StartsWith(path, "@it:") || absl::StartsWith(path, "@it2:") ||
absl::StartsWith(path, "@ac:")) {
// If we see a CSE generated comprehension variable that was not
// resolvable through the normal comprehension scope resolution, reject it
// now rather than surfacing errors at activation time.
SetProgressStatusError(
issue_collector_.AddIssue(RuntimeIssue::CreateError(
absl::InvalidArgumentError("out of scope reference to CSE "
"generated comprehension variable"))));
}
return {-1, -1};
}

Expand Down

0 comments on commit aeefe8f

Please # to comment.