diff --git a/eval/compiler/BUILD b/eval/compiler/BUILD index c5a97ae57..481072ee8 100644 --- a/eval/compiler/BUILD +++ b/eval/compiler/BUILD @@ -167,6 +167,7 @@ cc_test( ":qualified_reference_resolver", "//base:builtins", "//common:function_descriptor", + "//common:kind", "//common:value", "//eval/public:activation", "//eval/public:builtin_func_registrar", @@ -174,6 +175,7 @@ cc_test( "//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", @@ -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", diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index 4caf0c0a0..a0fd427bd 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -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) { @@ -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 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 ".....". - 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; } @@ -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())); } } diff --git a/eval/compiler/flat_expr_builder_test.cc b/eval/compiler/flat_expr_builder_test.cc index 0dba74a29..2b705398a 100644 --- a/eval/compiler/flat_expr_builder_test.cc +++ b/eval/compiler/flat_expr_builder_test.cc @@ -29,13 +29,17 @@ #include "google/protobuf/descriptor.pb.h" #include "absl/base/nullability.h" #include "absl/container/flat_hash_map.h" +#include "absl/log/absl_check.h" #include "absl/status/status.h" #include "absl/status/status_matchers.h" +#include "absl/status/statusor.h" #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" +#include "absl/time/time.h" #include "absl/types/span.h" #include "base/builtins.h" #include "common/function_descriptor.h" +#include "common/kind.h" #include "common/value.h" #include "eval/compiler/cel_expression_builder_flat_impl.h" #include "eval/compiler/constant_folding.h" @@ -46,6 +50,7 @@ #include "eval/public/cel_builtins.h" #include "eval/public/cel_expr_builder_factory.h" #include "eval/public/cel_expression.h" +#include "eval/public/cel_function.h" #include "eval/public/cel_function_adapter.h" #include "eval/public/cel_function_registry.h" #include "eval/public/cel_options.h" @@ -66,6 +71,7 @@ #include "runtime/function_adapter.h" #include "runtime/internal/runtime_env_testing.h" #include "runtime/runtime_options.h" +#include "runtime/standard_functions.h" #include "cel/expr/conformance/proto3/test_all_types.pb.h" #include "google/protobuf/arena.h" #include "google/protobuf/descriptor.h" @@ -1483,92 +1489,223 @@ TEST(FlatExprBuilderTest, ContainerStringFormat) { } } -void EvalExpressionWithEnum(absl::string_view enum_name, - absl::string_view container, CelValue* result) { - TestMessage message; - - Expr expr; - SourceInfo source_info; - - std::vector enum_name_parts = absl::StrSplit(enum_name, '.'); - Expr* cur_expr = &expr; - - for (int i = enum_name_parts.size() - 1; i > 0; i--) { - auto select_expr = cur_expr->mutable_select_expr(); - select_expr->set_field(enum_name_parts[i]); - cur_expr = select_expr->mutable_operand(); - } - - cur_expr->mutable_ident_expr()->set_name(enum_name_parts[0]); +// Builder with google.api.expr.runtime.TestMessage and TestEnum types +// linked in and the standard functions registered. +CelExpressionBuilderFlatImpl BuilderForNameResolutionTest( + absl::string_view container) { + cel::RuntimeOptions options; + options.enable_qualified_type_identifiers = true; - CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv()); + CelExpressionBuilderFlatImpl builder(NewTestingRuntimeEnv(), options); builder.GetTypeRegistry()->Register(TestMessage::TestEnum_descriptor()); builder.GetTypeRegistry()->Register(TestEnum_descriptor()); builder.set_container(std::string(container)); - ASSERT_OK_AND_ASSIGN(auto cel_expr, - builder.CreateExpression(&expr, &source_info)); + ABSL_CHECK_OK(cel::RegisterStandardFunctions( + builder.GetRegistry()->InternalGetRegistry(), options)); + return builder; +} +TEST(FlatExprBuilderTest, ShortEnumResolution) { google::protobuf::Arena arena; + CelExpressionBuilderFlatImpl builder = + BuilderForNameResolutionTest("google.api.expr.runtime.TestMessage"); + + ASSERT_OK_AND_ASSIGN(ParsedExpr expr, + parser::Parse("TestMessage.TestEnum.TEST_ENUM_1")); + ASSERT_OK_AND_ASSIGN(auto cel_expr, builder.CreateExpression( + &expr.expr(), &expr.source_info())); + Activation activation; - auto eval = cel_expr->Evaluate(activation, &arena); - ASSERT_THAT(eval, IsOk()); - *result = eval.value(); -} -TEST(FlatExprBuilderTest, ShortEnumResolution) { - CelValue result; - // Test resolution of ".". - ASSERT_NO_FATAL_FAILURE(EvalExpressionWithEnum( - "TestEnum.TEST_ENUM_1", "google.api.expr.runtime.TestMessage", &result)); + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr->Evaluate(activation, &arena)); + ASSERT_TRUE(result.IsInt64()); EXPECT_THAT(result.Int64OrDie(), Eq(TestMessage::TEST_ENUM_1)); } +TEST(FlatExprBuilderTest, EnumResolutionHonorsLeadingDot) { + google::protobuf::Arena arena; + CelExpressionBuilderFlatImpl builder = + BuilderForNameResolutionTest("google.api.expr.runtime"); + + // Leading dot disables container resolution. + ASSERT_OK_AND_ASSIGN(ParsedExpr expr, + parser::Parse(".TestMessage.TestEnum.TEST_ENUM_1")); + ASSERT_OK_AND_ASSIGN(auto cel_expr, builder.CreateExpression( + &expr.expr(), &expr.source_info())); + + Activation activation; + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr->Evaluate(activation, &arena)); + ASSERT_TRUE(result.IsError()); + EXPECT_THAT( + result.ErrorOrDie()->message(), + HasSubstr("No value with name \"TestMessage\" found in Activation")); +} + +TEST(FlatExprBuilderTest, EnumResolutionComprehensionShadowing) { + google::protobuf::Arena arena; + CelExpressionBuilderFlatImpl builder = + BuilderForNameResolutionTest("google.api.expr.runtime"); + + // Prefer the interpretation that it's a comprehension var if there's a + // collision. + ASSERT_OK_AND_ASSIGN( + ParsedExpr expr, + parser::Parse("[{'TestEnum': {'TEST_ENUM_1': 42}}].map(TestMessage, " + "TestMessage.TestEnum.TEST_ENUM_1)[0] == 42")); + ASSERT_OK_AND_ASSIGN(auto cel_expr, builder.CreateExpression( + &expr.expr(), &expr.source_info())); + + Activation activation; + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr->Evaluate(activation, &arena)); + ASSERT_TRUE(result.IsBool()); + EXPECT_TRUE(result.BoolOrDie()); +} + +TEST(FlatExprBuilderTest, EnumResolutionComprehensionShadowingLeadingDot) { + google::protobuf::Arena arena; + CelExpressionBuilderFlatImpl builder = + BuilderForNameResolutionTest("google.api.expr.runtime"); + + // Prefer the interpretation that it's a comprehension var if there's a + // collision. + ASSERT_OK_AND_ASSIGN( + ParsedExpr expr, + parser::Parse("[0].map(google, " + ".google.api.expr.runtime.TestMessage.TestEnum.TEST_ENUM_1)" + "[0] == TestMessage.TestEnum.TEST_ENUM_1")); + ASSERT_OK_AND_ASSIGN(auto cel_expr, builder.CreateExpression( + &expr.expr(), &expr.source_info())); + + Activation activation; + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr->Evaluate(activation, &arena)); + ASSERT_TRUE(result.IsBool()); + EXPECT_TRUE(result.BoolOrDie()); +} + TEST(FlatExprBuilderTest, FullEnumNameWithContainerResolution) { - CelValue result; + google::protobuf::Arena arena; + CelExpressionBuilderFlatImpl builder = + BuilderForNameResolutionTest("very.random.Namespace"); + // Fully qualified name should work. - ASSERT_NO_FATAL_FAILURE(EvalExpressionWithEnum( - "google.api.expr.runtime.TestMessage.TestEnum.TEST_ENUM_1", - "very.random.Namespace", &result)); + ASSERT_OK_AND_ASSIGN( + ParsedExpr expr, + parser::Parse( + "google.api.expr.runtime.TestMessage.TestEnum.TEST_ENUM_1")); + ASSERT_OK_AND_ASSIGN(auto cel_expr, builder.CreateExpression( + &expr.expr(), &expr.source_info())); + + Activation activation; + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr->Evaluate(activation, &arena)); ASSERT_TRUE(result.IsInt64()); EXPECT_THAT(result.Int64OrDie(), Eq(TestMessage::TEST_ENUM_1)); } TEST(FlatExprBuilderTest, SameShortNameEnumResolution) { - CelValue result; + google::protobuf::Arena arena; // This precondition validates that // TestMessage::TestEnum::TEST_ENUM1 and TestEnum::TEST_ENUM1 are compiled and // linked in and their values are different. ASSERT_TRUE(static_cast(TestEnum::TEST_ENUM_1) != static_cast(TestMessage::TEST_ENUM_1)); - ASSERT_NO_FATAL_FAILURE(EvalExpressionWithEnum( - "TestEnum.TEST_ENUM_1", "google.api.expr.runtime.TestMessage", &result)); - ASSERT_TRUE(result.IsInt64()); - EXPECT_THAT(result.Int64OrDie(), Eq(TestMessage::TEST_ENUM_1)); + + { + CelExpressionBuilderFlatImpl builder = + BuilderForNameResolutionTest("google.api.expr.runtime.TestMessage"); + ASSERT_OK_AND_ASSIGN(ParsedExpr expr, + parser::Parse("TestEnum.TEST_ENUM_1")); + ASSERT_OK_AND_ASSIGN(auto cel_expr, builder.CreateExpression( + &expr.expr(), &expr.source_info())); + Activation activation; + ASSERT_OK_AND_ASSIGN(CelValue result, + cel_expr->Evaluate(activation, &arena)); + ASSERT_TRUE(result.IsInt64()); + EXPECT_THAT(result.Int64OrDie(), Eq(TestMessage::TEST_ENUM_1)); + } // TEST_ENUM3 is present in google.api.expr.runtime.TestEnum, is absent in // google.api.expr.runtime.TestMessage.TestEnum. - ASSERT_NO_FATAL_FAILURE(EvalExpressionWithEnum( - "TestEnum.TEST_ENUM_3", "google.api.expr.runtime.TestMessage", &result)); - ASSERT_TRUE(result.IsInt64()); - EXPECT_THAT(result.Int64OrDie(), Eq(TestEnum::TEST_ENUM_3)); + { + CelExpressionBuilderFlatImpl builder = + BuilderForNameResolutionTest("google.api.expr.runtime.TestMessage"); + ASSERT_OK_AND_ASSIGN(ParsedExpr expr, + parser::Parse("TestEnum.TEST_ENUM_3")); + ASSERT_OK_AND_ASSIGN(auto cel_expr, builder.CreateExpression( + &expr.expr(), &expr.source_info())); + Activation activation; + ASSERT_OK_AND_ASSIGN(CelValue result, + cel_expr->Evaluate(activation, &arena)); + ASSERT_TRUE(result.IsInt64()); + EXPECT_THAT(result.Int64OrDie(), Eq(TestEnum::TEST_ENUM_3)); + } - ASSERT_NO_FATAL_FAILURE(EvalExpressionWithEnum( - "TestEnum.TEST_ENUM_1", "google.api.expr.runtime", &result)); - ASSERT_TRUE(result.IsInt64()); - EXPECT_THAT(result.Int64OrDie(), Eq(TestEnum::TEST_ENUM_1)); + { + CelExpressionBuilderFlatImpl builder = + BuilderForNameResolutionTest("google.api.expr.runtime"); + ASSERT_OK_AND_ASSIGN(ParsedExpr expr, + parser::Parse("TestEnum.TEST_ENUM_1")); + ASSERT_OK_AND_ASSIGN(auto cel_expr, builder.CreateExpression( + &expr.expr(), &expr.source_info())); + Activation activation; + ASSERT_OK_AND_ASSIGN(CelValue result, + cel_expr->Evaluate(activation, &arena)); + ASSERT_TRUE(result.IsInt64()); + EXPECT_THAT(result.Int64OrDie(), Eq(TestEnum::TEST_ENUM_1)); + } } TEST(FlatExprBuilderTest, PartialQualifiedEnumResolution) { - CelValue result; - ASSERT_NO_FATAL_FAILURE(EvalExpressionWithEnum( - "runtime.TestMessage.TestEnum.TEST_ENUM_1", "google.api.expr", &result)); + google::protobuf::Arena arena; + CelExpressionBuilderFlatImpl builder = + BuilderForNameResolutionTest("google.api.expr"); + + ASSERT_OK_AND_ASSIGN( + ParsedExpr expr, + parser::Parse("runtime.TestMessage.TestEnum.TEST_ENUM_1")); + ASSERT_OK_AND_ASSIGN(auto cel_expr, builder.CreateExpression( + &expr.expr(), &expr.source_info())); + + Activation activation; + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr->Evaluate(activation, &arena)); ASSERT_TRUE(result.IsInt64()); EXPECT_THAT(result.Int64OrDie(), Eq(TestMessage::TEST_ENUM_1)); } +TEST(FlatExprBuilderTest, NameCollisionWithComprehensionVar) { + google::protobuf::Arena arena; + CelExpressionBuilderFlatImpl builder = BuilderForNameResolutionTest("google"); + + ASSERT_OK_AND_ASSIGN(ParsedExpr expr, parser::Parse("[0].map(x, x)[0]")); + ASSERT_OK_AND_ASSIGN(auto cel_expr, builder.CreateExpression( + &expr.expr(), &expr.source_info())); + + Activation activation; + activation.InsertValue("x", CelValue::CreateInt64(1)); + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr->Evaluate(activation, &arena)); + + ASSERT_TRUE(result.IsInt64()); + EXPECT_THAT(result.Int64OrDie(), Eq(0)); +} + +TEST(FlatExprBuilderTest, NameCollisionWithComprehensionVarLeadingDot) { + google::protobuf::Arena arena; + CelExpressionBuilderFlatImpl builder = BuilderForNameResolutionTest("google"); + + ASSERT_OK_AND_ASSIGN(ParsedExpr expr, parser::Parse("[0].map(x, .x)[0]")); + ASSERT_OK_AND_ASSIGN(auto cel_expr, builder.CreateExpression( + &expr.expr(), &expr.source_info())); + + Activation activation; + activation.InsertValue("x", CelValue::CreateInt64(1)); + ASSERT_OK_AND_ASSIGN(CelValue result, cel_expr->Evaluate(activation, &arena)); + + ASSERT_TRUE(result.IsInt64()); + EXPECT_THAT(result.Int64OrDie(), Eq(1)); +} + TEST(FlatExprBuilderTest, MapFieldPresence) { Expr expr; SourceInfo source_info;