Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Update cel.@block implementation in C++ to handle comprehensions #879

Merged
merged 1 commit into from
Aug 26, 2024
Merged

Update cel.@block implementation in C++ to handle comprehensions #879

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
4 changes: 2 additions & 2 deletions bazel/deps.bzl
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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