diff --git a/checker/internal/type_check_env.cc b/checker/internal/type_check_env.cc index 91bfbaafa..d856a7230 100644 --- a/checker/internal/type_check_env.cc +++ b/checker/internal/type_check_env.cc @@ -16,7 +16,6 @@ #include #include -#include #include "absl/base/nullability.h" #include "absl/status/statusor.h" @@ -134,7 +133,7 @@ absl::StatusOr> TypeCheckEnv::LookupTypeConstant( google::protobuf::Arena* absl_nonnull arena, absl::string_view name) const { CEL_ASSIGN_OR_RETURN(absl::optional type, LookupTypeName(name)); if (type.has_value()) { - return MakeVariableDecl(std::string(type->name()), TypeType(arena, *type)); + return MakeVariableDecl(type->name(), TypeType(arena, *type)); } if (name.find('.') != name.npos) { @@ -185,7 +184,7 @@ absl::StatusOr> TypeCheckEnv::LookupStructField( return absl::nullopt; } -const VariableDecl* absl_nullable VariableScope::LookupVariable( +const VariableDecl* absl_nullable VariableScope::LookupLocalVariable( absl::string_view name) const { const VariableScope* scope = this; while (scope != nullptr) { @@ -194,8 +193,7 @@ const VariableDecl* absl_nullable VariableScope::LookupVariable( } scope = scope->parent_; } - - return env_->LookupVariable(name); + return nullptr; } } // namespace cel::checker_internal diff --git a/checker/internal/type_check_env.h b/checker/internal/type_check_env.h index 0b2ad31ed..f7f81f2a9 100644 --- a/checker/internal/type_check_env.h +++ b/checker/internal/type_check_env.h @@ -42,13 +42,11 @@ class TypeCheckEnv; // Helper class for managing nested scopes and the local variables they // implicitly declare. // -// Nested scopes have a lifetime dependency on any parent scopes and the -// parent Type environment. Nested scopes should generally be managed by -// unique_ptrs. +// Nested scopes have a lifetime dependency on any parent scopes and should +// generally be managed by unique_ptrs. class VariableScope { public: - explicit VariableScope(const TypeCheckEnv& env ABSL_ATTRIBUTE_LIFETIME_BOUND) - : env_(&env), parent_(nullptr) {} + explicit VariableScope() : parent_(nullptr) {} VariableScope(const VariableScope&) = delete; VariableScope& operator=(const VariableScope&) = delete; @@ -61,18 +59,17 @@ class VariableScope { std::unique_ptr MakeNestedScope() const ABSL_ATTRIBUTE_LIFETIME_BOUND { - return absl::WrapUnique(new VariableScope(*env_, this)); + return absl::WrapUnique(new VariableScope(this)); } - const VariableDecl* absl_nullable LookupVariable( + const VariableDecl* absl_nullable LookupLocalVariable( absl::string_view name) const; private: - VariableScope(const TypeCheckEnv& env ABSL_ATTRIBUTE_LIFETIME_BOUND, - const VariableScope* parent ABSL_ATTRIBUTE_LIFETIME_BOUND) - : env_(&env), parent_(parent) {} + explicit VariableScope( + const VariableScope* parent ABSL_ATTRIBUTE_LIFETIME_BOUND) + : parent_(parent) {} - const TypeCheckEnv* absl_nonnull env_; const VariableScope* absl_nullable parent_; absl::flat_hash_map variables_; }; @@ -190,9 +187,6 @@ class TypeCheckEnv { TypeCheckEnv MakeExtendedEnvironment() const ABSL_ATTRIBUTE_LIFETIME_BOUND { return TypeCheckEnv(this); } - VariableScope MakeVariableScope() const ABSL_ATTRIBUTE_LIFETIME_BOUND { - return VariableScope(*this); - } const google::protobuf::DescriptorPool* absl_nonnull descriptor_pool() const { return descriptor_pool_.get(); diff --git a/checker/internal/type_checker_impl.cc b/checker/internal/type_checker_impl.cc index 6d77bb2e2..676c65ad2 100644 --- a/checker/internal/type_checker_impl.cc +++ b/checker/internal/type_checker_impl.cc @@ -26,6 +26,7 @@ #include "absl/container/flat_hash_set.h" #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "absl/strings/match.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_join.h" #include "absl/strings/string_view.h" @@ -234,6 +235,11 @@ class ResolveVisitor : public AstVisitorBase { bool namespace_rewrite; }; + struct AttributeResolution { + const VariableDecl* decl; + bool requires_disambiguation; + }; + ResolveVisitor(absl::string_view container, NamespaceGenerator namespace_generator, const TypeCheckEnv& env, const Ast& ast, @@ -246,7 +252,7 @@ class ResolveVisitor : public AstVisitorBase { inference_context_(&inference_context), issues_(&issues), ast_(&ast), - root_scope_(env.MakeVariableScope()), + root_scope_(), arena_(arena), current_scope_(&root_scope_) {} @@ -294,7 +300,7 @@ class ResolveVisitor : public AstVisitorBase { return functions_; } - const absl::flat_hash_map& attributes() + const absl::flat_hash_map& attributes() const { return attributes_; } @@ -344,9 +350,13 @@ class ResolveVisitor : public AstVisitorBase { absl::string_view function_name, int arg_count, bool is_receiver); - // Resolves the function call shape (i.e. the number of arguments and call - // style) for the given function call. - const VariableDecl* absl_nullable LookupIdentifier(absl::string_view name); + // Resolves a global identifier (i.e. declared in the CEL environment). + const VariableDecl* absl_nullable LookupGlobalIdentifier( + absl::string_view name); + + // Resolves a local identifier (i.e. a bind or comprehension var). + const VariableDecl* absl_nullable LookupLocalIdentifier( + absl::string_view name); // Resolves the applicable function overloads for the given function call. // @@ -476,7 +486,7 @@ class ResolveVisitor : public AstVisitorBase { // References that were resolved and may require AST rewrites. absl::flat_hash_map functions_; - absl::flat_hash_map attributes_; + absl::flat_hash_map attributes_; absl::flat_hash_map struct_types_; absl::flat_hash_map types_; @@ -967,10 +977,20 @@ void ResolveVisitor::ResolveFunctionOverloads(const Expr& expr, types_[&expr] = resolution->result_type; } -const VariableDecl* absl_nullable ResolveVisitor::LookupIdentifier( +const VariableDecl* absl_nullable ResolveVisitor::LookupLocalIdentifier( absl::string_view name) { - if (const VariableDecl* decl = current_scope_->LookupVariable(name); - decl != nullptr) { + // Note: if we see a leading dot, this shouldn't resolve to a local variable, + // but we need to check whether we need to disambiguate against a global in + // the reference map. + if (absl::StartsWith(name, ".")) { + name = name.substr(1); + } + return current_scope_->LookupLocalVariable(name); +} + +const VariableDecl* absl_nullable ResolveVisitor::LookupGlobalIdentifier( + absl::string_view name) { + if (const VariableDecl* decl = env_->LookupVariable(name); decl != nullptr) { return decl; } absl::StatusOr> constant = @@ -996,22 +1016,34 @@ const VariableDecl* absl_nullable ResolveVisitor::LookupIdentifier( void ResolveVisitor::ResolveSimpleIdentifier(const Expr& expr, absl::string_view name) { + // Local variables (comprehension, bind) are simple identifiers so we can + // skip generating the different namespace-qualified candidates. + const VariableDecl* local_decl = LookupLocalIdentifier(name); + + if (local_decl != nullptr && !absl::StartsWith(name, ".")) { + attributes_[&expr] = {local_decl, false}; + types_[&expr] = + inference_context_->InstantiateTypeParams(local_decl->type()); + return; + } + const VariableDecl* decl = nullptr; namespace_generator_.GenerateCandidates( name, [&decl, this](absl::string_view candidate) { - decl = LookupIdentifier(candidate); + decl = LookupGlobalIdentifier(candidate); // continue searching. return decl == nullptr; }); - if (decl == nullptr) { - ReportMissingReference(expr, name); - types_[&expr] = ErrorType(); + if (decl != nullptr) { + attributes_[&expr] = {decl, + /* requires_disambiguation= */ local_decl != nullptr}; + types_[&expr] = inference_context_->InstantiateTypeParams(decl->type()); return; } - attributes_[&expr] = decl; - types_[&expr] = inference_context_->InstantiateTypeParams(decl->type()); + ReportMissingReference(expr, name); + types_[&expr] = ErrorType(); } void ResolveVisitor::ResolveQualifiedIdentifier( @@ -1021,18 +1053,28 @@ void ResolveVisitor::ResolveQualifiedIdentifier( return; } - const VariableDecl* absl_nullable decl = nullptr; - int segment_index_out = -1; - namespace_generator_.GenerateCandidates( - qualifiers, [&decl, &segment_index_out, this](absl::string_view candidate, - int segment_index) { - decl = LookupIdentifier(candidate); - if (decl != nullptr) { - segment_index_out = segment_index; - return false; - } - return true; - }); + // Local variables (comprehension, bind) are simple identifiers so we can + // skip generating the different namespace-qualified candidates. + const VariableDecl* local_decl = LookupLocalIdentifier(qualifiers[0]); + const VariableDecl* decl = nullptr; + + int matched_segment_index = -1; + + if (local_decl != nullptr && !absl::StartsWith(qualifiers[0], ".")) { + decl = local_decl; + matched_segment_index = 0; + } else { + namespace_generator_.GenerateCandidates( + qualifiers, [&decl, &matched_segment_index, this]( + absl::string_view candidate, int segment_index) { + decl = LookupGlobalIdentifier(candidate); + if (decl != nullptr) { + matched_segment_index = segment_index; + return false; + } + return true; + }); + } if (decl == nullptr) { ReportMissingReference(expr, FormatCandidate(qualifiers)); @@ -1040,7 +1082,8 @@ void ResolveVisitor::ResolveQualifiedIdentifier( return; } - const int num_select_opts = qualifiers.size() - segment_index_out - 1; + const int num_select_opts = qualifiers.size() - matched_segment_index - 1; + const Expr* root = &expr; std::vector select_opts; select_opts.reserve(num_select_opts); @@ -1049,7 +1092,9 @@ void ResolveVisitor::ResolveQualifiedIdentifier( root = &root->select_expr().operand(); } - attributes_[root] = decl; + attributes_[root] = {decl, + /* requires_disambiguation= */ decl != local_decl && + local_decl != nullptr}; types_[root] = inference_context_->InstantiateTypeParams(decl->type()); // fix-up select operations that were deferred. @@ -1196,13 +1241,18 @@ class ResolveRewriter : public AstRewriterBase { bool rewritten = false; if (auto iter = visitor_.attributes().find(&expr); iter != visitor_.attributes().end()) { - const VariableDecl* decl = iter->second; + const VariableDecl* decl = iter->second.decl; auto& ast_ref = reference_map_[expr.id()]; - ast_ref.set_name(decl->name()); + std::string name = decl->name(); + if (iter->second.requires_disambiguation && + !absl::StartsWith(name, ".")) { + name = absl::StrCat(".", name); + } + ast_ref.set_name(name); if (decl->has_value()) { ast_ref.set_value(decl->value()); } - expr.mutable_ident_expr().set_name(decl->name()); + expr.mutable_ident_expr().set_name(std::move(name)); rewritten = true; } else if (auto iter = visitor_.functions().find(&expr); iter != visitor_.functions().end()) { @@ -1211,7 +1261,6 @@ class ResolveRewriter : public AstRewriterBase { auto& ast_ref = reference_map_[expr.id()]; ast_ref.set_name(decl->name()); for (const auto& overload : decl->overloads()) { - // TODO(uncreated-issue/72): narrow based on type inferences and shape. ast_ref.mutable_overload_id().push_back(overload.id()); } expr.mutable_call_expr().set_function(decl->name()); diff --git a/checker/internal/type_checker_impl_test.cc b/checker/internal/type_checker_impl_test.cc index 0f07de75e..becc740f7 100644 --- a/checker/internal/type_checker_impl_test.cc +++ b/checker/internal/type_checker_impl_test.cc @@ -65,6 +65,7 @@ using ::testing::ElementsAre; using ::testing::Eq; using ::testing::HasSubstr; using ::testing::IsEmpty; +using ::testing::Not; using ::testing::Pair; using ::testing::Property; using ::testing::SizeIs; @@ -220,6 +221,12 @@ absl::Status RegisterMinimalBuiltins(google::protobuf::Arena* absl_nonnull arena "equals", /*return_type=*/BoolType{}, TypeParamType("A"), TypeParamType("A")))); + FunctionDecl ne_op; + ne_op.set_name("_!=_"); + CEL_RETURN_IF_ERROR(ne_op.AddOverload(MakeOverloadDecl( + "not_equals", + /*return_type=*/BoolType{}, TypeParamType("A"), TypeParamType("A")))); + FunctionDecl ternary_op; ternary_op.set_name("_?_:_"); CEL_RETURN_IF_ERROR(ternary_op.AddOverload(MakeOverloadDecl( @@ -275,6 +282,7 @@ absl::Status RegisterMinimalBuiltins(google::protobuf::Arena* absl_nonnull arena env.InsertFunctionIfAbsent(std::move(gt_op)); env.InsertFunctionIfAbsent(std::move(to_int)); env.InsertFunctionIfAbsent(std::move(eq_op)); + env.InsertFunctionIfAbsent(std::move(ne_op)); env.InsertFunctionIfAbsent(std::move(ternary_op)); env.InsertFunctionIfAbsent(std::move(index_op)); env.InsertFunctionIfAbsent(std::move(to_dyn)); @@ -750,18 +758,18 @@ TEST(TypeCheckerImplTest, NestedComprehensions) { EXPECT_THAT(result.GetIssues(), IsEmpty()); } -TEST(TypeCheckerImplTest, ComprehensionVarsFollowNamespacePriorityRules) { +TEST(TypeCheckerImplTest, ComprehensionVarsShadowNamespacePriorityRules) { TypeCheckEnv env(GetSharedTestingDescriptorPool()); env.set_container("com"); google::protobuf::Arena arena; ASSERT_THAT(RegisterMinimalBuiltins(&arena, env), IsOk()); - // Namespace resolution still applies, compre var doesn't shadow com.x + // Namespace compre var shadows com.x env.InsertVariableIfAbsent(MakeVariableDecl("com.x", IntType())); TypeCheckerImpl impl(std::move(env)); ASSERT_OK_AND_ASSIGN(auto ast, - MakeTestParsedAst("['1', '2'].all(x, x == 2)")); + MakeTestParsedAst("['1', '2'].exists(x, x == '2')")); ASSERT_OK_AND_ASSIGN(ValidationResult result, impl.Check(std::move(ast))); EXPECT_TRUE(result.IsValid()); @@ -769,20 +777,19 @@ TEST(TypeCheckerImplTest, ComprehensionVarsFollowNamespacePriorityRules) { EXPECT_THAT(result.GetIssues(), IsEmpty()); ASSERT_OK_AND_ASSIGN(auto checked_ast, result.ReleaseAst()); EXPECT_THAT(checked_ast->reference_map(), - Contains(Pair(_, IsVariableReference("com.x")))); + Not(Contains(Pair(_, IsVariableReference("com.x"))))); } -TEST(TypeCheckerImplTest, ComprehensionVarsFollowQualifiedIdentPriority) { +TEST(TypeCheckerImplTest, ComprehensionVarsShadowsQualifiedIdent) { TypeCheckEnv env(GetSharedTestingDescriptorPool()); google::protobuf::Arena arena; ASSERT_THAT(RegisterMinimalBuiltins(&arena, env), IsOk()); - // Namespace resolution still applies, compre var doesn't shadow x.y env.InsertVariableIfAbsent(MakeVariableDecl("x.y", IntType())); TypeCheckerImpl impl(std::move(env)); ASSERT_OK_AND_ASSIGN(auto ast, - MakeTestParsedAst("[{'y': '2'}].all(x, x.y == 2)")); + MakeTestParsedAst("[{'y': '2'}].all(x, x.y == '2')")); ASSERT_OK_AND_ASSIGN(ValidationResult result, impl.Check(std::move(ast))); EXPECT_TRUE(result.IsValid()); @@ -790,7 +797,82 @@ TEST(TypeCheckerImplTest, ComprehensionVarsFollowQualifiedIdentPriority) { EXPECT_THAT(result.GetIssues(), IsEmpty()); ASSERT_OK_AND_ASSIGN(auto checked_ast, result.ReleaseAst()); EXPECT_THAT(checked_ast->reference_map(), - Contains(Pair(_, IsVariableReference("x.y")))); + Not(Contains(Pair(_, IsVariableReference("x.y"))))); +} + +TEST(TypeCheckerImplTest, ComprehensionVarsShadowsQualifiedIdentTypeError) { + TypeCheckEnv env(GetSharedTestingDescriptorPool()); + google::protobuf::Arena arena; + ASSERT_THAT(RegisterMinimalBuiltins(&arena, env), IsOk()); + + env.InsertVariableIfAbsent(MakeVariableDecl("x.y", IntType())); + + TypeCheckerImpl impl(std::move(env)); + ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst("[0].all(x, x.y == 0)")); + ASSERT_OK_AND_ASSIGN(ValidationResult result, impl.Check(std::move(ast))); + + EXPECT_FALSE(result.IsValid()); + + EXPECT_THAT( + result.FormatError(), + HasSubstr("type 'int' cannot be the operand of a select operation")); +} + +TEST(TypeCheckerImplTest, ComprehensionVarsDisamgiguatesQualifiedIdent) { + TypeCheckEnv env(GetSharedTestingDescriptorPool()); + google::protobuf::Arena arena; + ASSERT_THAT(RegisterMinimalBuiltins(&arena, env), IsOk()); + + env.InsertVariableIfAbsent(MakeVariableDecl("x.y", IntType())); + + TypeCheckerImpl impl(std::move(env)); + ASSERT_OK_AND_ASSIGN(auto ast, + MakeTestParsedAst("[{'y': 0}].all(x, .x.y == 2)")); + ASSERT_OK_AND_ASSIGN(ValidationResult result, impl.Check(std::move(ast))); + + EXPECT_TRUE(result.IsValid()); + + EXPECT_THAT(result.GetIssues(), IsEmpty()); + ASSERT_OK_AND_ASSIGN(auto checked_ast, result.ReleaseAst()); + EXPECT_THAT(checked_ast->reference_map(), + Contains(Pair(_, IsVariableReference(".x.y")))); +} + +TEST(TypeCheckerImplTest, ComprehensionVarsDisamgiguatesQualifiedIdentMixed) { + TypeCheckEnv env(GetSharedTestingDescriptorPool()); + google::protobuf::Arena arena; + ASSERT_THAT(RegisterMinimalBuiltins(&arena, env), IsOk()); + + env.InsertVariableIfAbsent(MakeVariableDecl("x.y", StringType())); + + TypeCheckerImpl impl(std::move(env)); + ASSERT_OK_AND_ASSIGN(auto ast, + MakeTestParsedAst("[{'y': 0}].all(x, .x.y != x.y)")); + ASSERT_OK_AND_ASSIGN(ValidationResult result, impl.Check(std::move(ast))); + + EXPECT_FALSE(result.IsValid()); + EXPECT_THAT( + result.FormatError(), + HasSubstr("no matching overload for '_!=_' applied to '(string, int)'")); +} + +TEST(TypeCheckerImplTest, ComprehensionVarsDisamgiguatesIdent) { + TypeCheckEnv env(GetSharedTestingDescriptorPool()); + google::protobuf::Arena arena; + ASSERT_THAT(RegisterMinimalBuiltins(&arena, env), IsOk()); + + env.InsertVariableIfAbsent(MakeVariableDecl("x", IntType())); + + TypeCheckerImpl impl(std::move(env)); + ASSERT_OK_AND_ASSIGN(auto ast, MakeTestParsedAst("['foo'].all(x, .x == 2)")); + ASSERT_OK_AND_ASSIGN(ValidationResult result, impl.Check(std::move(ast))); + + EXPECT_TRUE(result.IsValid()); + + EXPECT_THAT(result.GetIssues(), IsEmpty()); + ASSERT_OK_AND_ASSIGN(auto checked_ast, result.ReleaseAst()); + EXPECT_THAT(checked_ast->reference_map(), + Contains(Pair(_, IsVariableReference(".x")))); } TEST(TypeCheckerImplTest, ComprehensionVarsCyclicParamAssignability) { diff --git a/eval/compiler/flat_expr_builder.cc b/eval/compiler/flat_expr_builder.cc index 141cabdf1..4caf0c0a0 100644 --- a/eval/compiler/flat_expr_builder.cc +++ b/eval/compiler/flat_expr_builder.cc @@ -777,7 +777,7 @@ class FlatExprVisitor : public cel::AstVisitor { if (!progress_status_.ok()) { return; } - std::string path = ident_expr.name(); + absl::string_view path = ident_expr.name(); if (!ValidateOrError( !path.empty(), "Invalid expression: identifier 'name' must not be empty")) { @@ -788,12 +788,13 @@ class FlatExprVisitor : public cel::AstVisitor { // enum or type constant value. absl::optional const_value; int64_t select_root_id = -1; + std::string qualified_path; while (!namespace_stack_.empty()) { const auto& select_node = namespace_stack_.front(); // Generate path in format ".....". auto select_expr = select_node.first; - auto qualified_path = absl::StrCat(path, ".", select_node.second); + qualified_path = 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 @@ -819,14 +820,14 @@ class FlatExprVisitor : public cel::AstVisitor { if (const_value) { if (options_.max_recursion_depth != 0) { - SetRecursiveStep(CreateDirectShadowableValueStep( - std::move(path), std::move(const_value).value(), - select_root_id), - 1); + SetRecursiveStep( + CreateDirectShadowableValueStep( + path, std::move(const_value).value(), select_root_id), + 1); return; } - AddStep(CreateShadowableValueStep( - std::move(path), std::move(const_value).value(), select_root_id)); + AddStep(CreateShadowableValueStep(path, std::move(const_value).value(), + select_root_id)); return; } @@ -858,14 +859,15 @@ class FlatExprVisitor : public cel::AstVisitor { CreateDirectSlotIdentStep(ident_expr.name(), slot.slot, expr.id()), 1); } else { - AddStep(CreateIdentStepForSlot(ident_expr, slot.slot, expr.id())); + AddStep( + CreateIdentStepForSlot(ident_expr.name(), slot.slot, expr.id())); } return; } if (options_.max_recursion_depth != 0) { SetRecursiveStep(CreateDirectIdentStep(ident_expr.name(), expr.id()), 1); } else { - AddStep(CreateIdentStep(ident_expr, expr.id())); + AddStep(CreateIdentStep(ident_expr.name(), expr.id())); } } diff --git a/eval/eval/BUILD b/eval/eval/BUILD index ddbdd0729..75c965a2e 100644 --- a/eval/eval/BUILD +++ b/eval/eval/BUILD @@ -677,11 +677,11 @@ cc_test( ":ident_step", "//base:data", "//common:casting", - "//common:expr", "//common:memory", "//common:value", "//eval/public:activation", "//eval/public:cel_attribute", + "//eval/public:cel_value", "//internal:testing", "//internal:testing_descriptor_pool", "//internal:testing_message_factory", @@ -1098,6 +1098,7 @@ cc_library( "@com_google_absl//absl/memory", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", + "@com_google_absl//absl/strings:string_view", ], ) diff --git a/eval/eval/comprehension_step_test.cc b/eval/eval/comprehension_step_test.cc index f8fab6e05..681f8af4f 100644 --- a/eval/eval/comprehension_step_test.cc +++ b/eval/eval/comprehension_step_test.cc @@ -102,8 +102,7 @@ MATCHER_P(CelStringValue, val, "") { TEST_F(ListKeysStepTest, MapPartiallyUnknown) { ExecutionPath path; - IdentExpr ident = CreateIdent("var"); - auto result = CreateIdentStep(ident, 0); + auto result = CreateIdentStep("var", 0); ASSERT_OK(result); path.push_back(*std::move(result)); ComprehensionInitStep* init_step = new ComprehensionInitStep(1); @@ -141,8 +140,7 @@ TEST_F(ListKeysStepTest, MapPartiallyUnknown) { TEST_F(ListKeysStepTest, ErrorPassedThrough) { ExecutionPath path; - IdentExpr ident = CreateIdent("var"); - auto result = CreateIdentStep(ident, 0); + auto result = CreateIdentStep("var", 0); ASSERT_OK(result); path.push_back(*std::move(result)); ComprehensionInitStep* init_step = new ComprehensionInitStep(1); @@ -167,8 +165,7 @@ TEST_F(ListKeysStepTest, ErrorPassedThrough) { TEST_F(ListKeysStepTest, UnknownSetPassedThrough) { ExecutionPath path; - IdentExpr ident = CreateIdent("var"); - auto result = CreateIdentStep(ident, 0); + auto result = CreateIdentStep("var", 0); ASSERT_OK(result); path.push_back(*std::move(result)); ComprehensionInitStep* init_step = new ComprehensionInitStep(1); diff --git a/eval/eval/container_access_step_test.cc b/eval/eval/container_access_step_test.cc index ce640987d..25bf72223 100644 --- a/eval/eval/container_access_step_test.cc +++ b/eval/eval/container_access_step_test.cc @@ -81,10 +81,8 @@ CelValue EvaluateAttributeHelper( /*enable_optional_types=*/false, 3), 3)); } else { - path.push_back( - std::move(CreateIdentStep(container_expr.ident_expr(), 1).value())); - path.push_back( - std::move(CreateIdentStep(key_expr.ident_expr(), 2).value())); + path.push_back(std::move(CreateIdentStep("container", 1).value())); + path.push_back(std::move(CreateIdentStep("key", 2).value())); path.push_back(std::move(CreateContainerAccessStep(call, 3).value())); } diff --git a/eval/eval/create_list_step_test.cc b/eval/eval/create_list_step_test.cc index 139aaf612..990003823 100644 --- a/eval/eval/create_list_step_test.cc +++ b/eval/eval/create_list_step_test.cc @@ -123,7 +123,7 @@ absl::StatusOr RunExpressionWithCelValues( expr0.mutable_ident_expr().set_name(var_name); CEL_ASSIGN_OR_RETURN(auto ident_step, - CreateIdentStep(expr0.ident_expr(), expr0.id())); + CreateIdentStep(var_name, /*expr_id=*/-1)); path.push_back(std::move(ident_step)); activation.InsertValue(var_name, value); } diff --git a/eval/eval/create_map_step_test.cc b/eval/eval/create_map_step_test.cc index 1da798a79..dbc9adb5a 100644 --- a/eval/eval/create_map_step_test.cc +++ b/eval/eval/create_map_step_test.cc @@ -71,17 +71,11 @@ absl::StatusOr CreateStackMachineProgram( std::string key_name = absl::StrCat("key", index); std::string value_name = absl::StrCat("value", index); - auto& key_expr = exprs.emplace_back(); - auto& key_ident = key_expr.mutable_ident_expr(); - key_ident.set_name(key_name); CEL_ASSIGN_OR_RETURN(auto step_key, - CreateIdentStep(key_ident, exprs.back().id())); + CreateIdentStep(key_name, /*expr_id=*/-1)); - auto& value_expr = exprs.emplace_back(); - auto& value_ident = value_expr.mutable_ident_expr(); - value_ident.set_name(value_name); CEL_ASSIGN_OR_RETURN(auto step_value, - CreateIdentStep(value_ident, exprs.back().id())); + CreateIdentStep(value_name, /*expr _id=*/-1)); path.push_back(std::move(step_key)); path.push_back(std::move(step_value)); diff --git a/eval/eval/create_struct_step_test.cc b/eval/eval/create_struct_step_test.cc index 3ed934845..cd9db9bd9 100644 --- a/eval/eval/create_struct_step_test.cc +++ b/eval/eval/create_struct_step_test.cc @@ -72,11 +72,8 @@ using ::testing::Pointwise; absl::StatusOr MakeStackMachinePath(absl::string_view field) { ExecutionPath path; - Expr expr0; - auto& ident = expr0.mutable_ident_expr(); - ident.set_name("message"); - CEL_ASSIGN_OR_RETURN(auto step0, CreateIdentStep(ident, expr0.id())); + CEL_ASSIGN_OR_RETURN(auto step0, CreateIdentStep("message", /*expr_id=*/-1)); auto step1 = CreateCreateStructStep("google.api.expr.runtime.TestMessage", {std::string(field)}, diff --git a/eval/eval/function_step_test.cc b/eval/eval/function_step_test.cc index e42be944b..a599fe154 100644 --- a/eval/eval/function_step_test.cc +++ b/eval/eval/function_step_test.cc @@ -642,7 +642,7 @@ TEST_P(FunctionStepTestUnknowns, PartialUnknownHandlingTest) { ident1.set_name("param"); CallExpr call1 = SinkFunction::MakeCall(); - ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident1, GetExprId())); + ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep("param", GetExprId())); ASSERT_OK_AND_ASSIGN(auto step1, MakeTestFunctionStep(call1, registry)); path.push_back(std::move(step0)); diff --git a/eval/eval/ident_step.cc b/eval/eval/ident_step.cc index 5f4540f7b..7ec1a3031 100644 --- a/eval/eval/ident_step.cc +++ b/eval/eval/ident_step.cc @@ -11,7 +11,6 @@ #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" -#include "common/expr.h" #include "common/value.h" #include "eval/eval/attribute_trail.h" #include "eval/eval/comprehension_slots.h" @@ -39,10 +38,10 @@ class IdentStep : public ExpressionStepBase { std::string name_; }; -absl::Status LookupIdent(const std::string& name, ExecutionFrameBase& frame, +absl::Status LookupIdent(absl::string_view name, ExecutionFrameBase& frame, Value& result, AttributeTrail& attribute) { if (frame.attribute_tracking_enabled()) { - attribute = AttributeTrail(name); + attribute = AttributeTrail(std::string(name)); if (frame.missing_attribute_errors_enabled() && frame.attribute_utility().CheckForMissingAttribute(attribute)) { CEL_ASSIGN_OR_RETURN( @@ -128,7 +127,7 @@ class DirectIdentStep : public DirectExpressionStep { class DirectSlotStep : public DirectExpressionStep { public: - DirectSlotStep(std::string name, size_t slot_index, int64_t expr_id) + DirectSlotStep(absl::string_view name, size_t slot_index, int64_t expr_id) : DirectExpressionStep(expr_id), name_(std::move(name)), slot_index_(slot_index) {} @@ -159,18 +158,17 @@ std::unique_ptr CreateDirectIdentStep( std::unique_ptr CreateDirectSlotIdentStep( absl::string_view identifier, size_t slot_index, int64_t expr_id) { - return std::make_unique(std::string(identifier), slot_index, - expr_id); + return std::make_unique(identifier, slot_index, expr_id); } absl::StatusOr> CreateIdentStep( - const cel::IdentExpr& ident_expr, int64_t expr_id) { - return std::make_unique(ident_expr.name(), expr_id); + const absl::string_view name, int64_t expr_id) { + return std::make_unique(name, expr_id); } absl::StatusOr> CreateIdentStepForSlot( - const cel::IdentExpr& ident_expr, size_t slot_index, int64_t expr_id) { - return std::make_unique(ident_expr.name(), slot_index, expr_id); + const absl::string_view name, size_t slot_index, int64_t expr_id) { + return std::make_unique(name, slot_index, expr_id); } } // namespace google::api::expr::runtime diff --git a/eval/eval/ident_step.h b/eval/eval/ident_step.h index 388e2beea..d1bdde388 100644 --- a/eval/eval/ident_step.h +++ b/eval/eval/ident_step.h @@ -1,12 +1,12 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_IDENT_STEP_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_IDENT_STEP_H_ +#include #include #include #include "absl/status/statusor.h" #include "absl/strings/string_view.h" -#include "common/expr.h" #include "eval/eval/direct_expression_step.h" #include "eval/eval/evaluator_core.h" @@ -20,11 +20,11 @@ std::unique_ptr CreateDirectSlotIdentStep( // Factory method for Ident - based Execution step absl::StatusOr> CreateIdentStep( - const cel::IdentExpr& ident, int64_t expr_id); + absl::string_view name, int64_t expr_id); // Factory method for identifier that has been assigned to a slot. absl::StatusOr> CreateIdentStepForSlot( - const cel::IdentExpr& ident_expr, size_t slot_index, int64_t expr_id); + absl::string_view name, size_t slot_index, int64_t expr_id); } // namespace google::api::expr::runtime diff --git a/eval/eval/ident_step_test.cc b/eval/eval/ident_step_test.cc index 74426e65e..ce10d7d98 100644 --- a/eval/eval/ident_step_test.cc +++ b/eval/eval/ident_step_test.cc @@ -8,7 +8,6 @@ #include "absl/status/status.h" #include "base/type_provider.h" #include "common/casting.h" -#include "common/expr.h" #include "common/memory.h" #include "common/value.h" #include "eval/eval/attribute_trail.h" @@ -16,6 +15,7 @@ #include "eval/eval/evaluator_core.h" #include "eval/public/activation.h" #include "eval/public/cel_attribute.h" +#include "eval/public/cel_value.h" #include "internal/testing.h" #include "internal/testing_descriptor_pool.h" #include "internal/testing_message_factory.h" @@ -32,7 +32,6 @@ namespace { using ::absl_testing::StatusIs; using ::cel::Cast; using ::cel::ErrorValue; -using ::cel::Expr; using ::cel::InstanceOf; using ::cel::IntValue; using ::cel::MemoryManagerRef; @@ -47,11 +46,7 @@ using ::testing::HasSubstr; using ::testing::SizeIs; TEST(IdentStepTest, TestIdentStep) { - Expr expr; - auto& ident_expr = expr.mutable_ident_expr(); - ident_expr.set_name("name0"); - - ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep(ident_expr, expr.id())); + ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep("name0", /*id=*/-1)); ExecutionPath path; path.push_back(std::move(step)); @@ -77,11 +72,7 @@ TEST(IdentStepTest, TestIdentStep) { } TEST(IdentStepTest, TestIdentStepNameNotFound) { - Expr expr; - auto& ident_expr = expr.mutable_ident_expr(); - ident_expr.set_name("name0"); - - ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep(ident_expr, expr.id())); + ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep("name0", /*id=*/-1)); ExecutionPath path; path.push_back(std::move(step)); @@ -104,11 +95,7 @@ TEST(IdentStepTest, TestIdentStepNameNotFound) { } TEST(IdentStepTest, DisableMissingAttributeErrorsOK) { - Expr expr; - auto& ident_expr = expr.mutable_ident_expr(); - ident_expr.set_name("name0"); - - ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep(ident_expr, expr.id())); + ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep("name0", /*id=*/-1)); ExecutionPath path; path.push_back(std::move(step)); @@ -144,11 +131,7 @@ TEST(IdentStepTest, DisableMissingAttributeErrorsOK) { } TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) { - Expr expr; - auto& ident_expr = expr.mutable_ident_expr(); - ident_expr.set_name("name0"); - - ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep(ident_expr, expr.id())); + ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep("name0", /*expr_id=*/1)); ExecutionPath path; path.push_back(std::move(step)); @@ -188,11 +171,7 @@ TEST(IdentStepTest, TestIdentStepMissingAttributeErrors) { } TEST(IdentStepTest, TestIdentStepUnknownAttribute) { - Expr expr; - auto& ident_expr = expr.mutable_ident_expr(); - ident_expr.set_name("name0"); - - ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep(ident_expr, expr.id())); + ASSERT_OK_AND_ASSIGN(auto step, CreateIdentStep("name0", /*expr_id=*/1)); ExecutionPath path; path.push_back(std::move(step)); diff --git a/eval/eval/logic_step_test.cc b/eval/eval/logic_step_test.cc index 98aca0df3..17ca8ba0d 100644 --- a/eval/eval/logic_step_test.cc +++ b/eval/eval/logic_step_test.cc @@ -66,19 +66,11 @@ class LogicStepTest : public testing::TestWithParam { absl::Status EvaluateLogic(CelValue arg0, CelValue arg1, bool is_or, CelValue* result, bool enable_unknown) { - Expr expr0; - auto& ident_expr0 = expr0.mutable_ident_expr(); - ident_expr0.set_name("name0"); - - Expr expr1; - auto& ident_expr1 = expr1.mutable_ident_expr(); - ident_expr1.set_name("name1"); - ExecutionPath path; - CEL_ASSIGN_OR_RETURN(auto step, CreateIdentStep(ident_expr0, expr0.id())); + CEL_ASSIGN_OR_RETURN(auto step, CreateIdentStep("name0", /*expr_id=*/-1)); path.push_back(std::move(step)); - CEL_ASSIGN_OR_RETURN(step, CreateIdentStep(ident_expr1, expr1.id())); + CEL_ASSIGN_OR_RETURN(step, CreateIdentStep("name1", /*expr_id=*/-1)); path.push_back(std::move(step)); CEL_ASSIGN_OR_RETURN(step, (is_or) ? CreateOrStep(2) : CreateAndStep(2)); @@ -259,16 +251,7 @@ TEST_F(LogicStepTest, TestAndLogicUnknownHandling) { ASSERT_THAT(status, IsOk()); ASSERT_TRUE(result.IsUnknownSet()); - Expr expr0; - auto& ident_expr0 = expr0.mutable_ident_expr(); - ident_expr0.set_name("name0"); - - Expr expr1; - auto& ident_expr1 = expr1.mutable_ident_expr(); - ident_expr1.set_name("name1"); - - CelAttribute attr0(expr0.ident_expr().name(), {}), - attr1(expr1.ident_expr().name(), {}); + CelAttribute attr0("name0", {}), attr1("name1", {}); UnknownAttributeSet unknown_attr_set0({attr0}); UnknownAttributeSet unknown_attr_set1({attr1}); UnknownSet unknown_set0(unknown_attr_set0); @@ -321,16 +304,7 @@ TEST_F(LogicStepTest, TestOrLogicUnknownHandling) { ASSERT_THAT(status, IsOk()); ASSERT_TRUE(result.IsUnknownSet()); - Expr expr0; - auto& ident_expr0 = expr0.mutable_ident_expr(); - ident_expr0.set_name("name0"); - - Expr expr1; - auto& ident_expr1 = expr1.mutable_ident_expr(); - ident_expr1.set_name("name1"); - - CelAttribute attr0(expr0.ident_expr().name(), {}), - attr1(expr1.ident_expr().name(), {}); + CelAttribute attr0("name0", {}), attr1("name1", {}); UnknownAttributeSet unknown_attr_set0({attr0}); UnknownAttributeSet unknown_attr_set1({attr1}); diff --git a/eval/eval/select_step_test.cc b/eval/eval/select_step_test.cc index dbd7ef6a1..ce532eabd 100644 --- a/eval/eval/select_step_test.cc +++ b/eval/eval/select_step_test.cc @@ -130,7 +130,7 @@ class SelectStepTest : public testing::Test { auto& ident = expr0.mutable_ident_expr(); ident.set_name("target"); - CEL_ASSIGN_OR_RETURN(auto step0, CreateIdentStep(ident, expr0.id())); + CEL_ASSIGN_OR_RETURN(auto step0, CreateIdentStep(ident.name(), expr0.id())); CEL_ASSIGN_OR_RETURN( auto step1, CreateSelectStep(select, expr.id(), @@ -327,7 +327,7 @@ TEST_F(SelectStepTest, MapPresenseIsErrorTest) { auto& ident = expr0.mutable_ident_expr(); ident.set_name("target"); - ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident, expr0.id())); + ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident.name(), expr0.id())); ASSERT_OK_AND_ASSIGN( auto step1, CreateSelectStep(select_map, expr1.id(), @@ -833,7 +833,7 @@ TEST_P(SelectStepConformanceTest, CelErrorAsArgument) { auto& ident = expr0.mutable_ident_expr(); ident.set_name("message"); - ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident, expr0.id())); + ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident.name(), expr0.id())); ASSERT_OK_AND_ASSIGN( auto step1, CreateSelectStep(select, dummy_expr.id(), @@ -874,7 +874,7 @@ TEST_F(SelectStepTest, DisableMissingAttributeOK) { auto& ident = expr0.mutable_ident_expr(); ident.set_name("message"); - ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident, expr0.id())); + ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident.name(), expr0.id())); ASSERT_OK_AND_ASSIGN( auto step1, CreateSelectStep(select, dummy_expr.id(), @@ -916,7 +916,7 @@ TEST_F(SelectStepTest, UnrecoverableUnknownValueProducesError) { auto& ident = expr0.mutable_ident_expr(); ident.set_name("message"); - ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident, expr0.id())); + ASSERT_OK_AND_ASSIGN(auto step0, CreateIdentStep(ident.name(), expr0.id())); ASSERT_OK_AND_ASSIGN( auto step1, CreateSelectStep(select, dummy_expr.id(), @@ -964,7 +964,7 @@ TEST_F(SelectStepTest, UnknownPatternResolvesToUnknown) { auto& ident = expr0.mutable_ident_expr(); ident.set_name("message"); - auto step0_status = CreateIdentStep(ident, expr0.id()); + auto step0_status = CreateIdentStep(ident.name(), expr0.id()); auto step1_status = CreateSelectStep(select, dummy_expr.id(), /*enable_wrapper_type_null_unboxing=*/false); diff --git a/eval/eval/shadowable_value_step.cc b/eval/eval/shadowable_value_step.cc index 240a0d367..1ebab2f1e 100644 --- a/eval/eval/shadowable_value_step.cc +++ b/eval/eval/shadowable_value_step.cc @@ -8,6 +8,7 @@ #include "absl/memory/memory.h" #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "absl/strings/string_view.h" #include "common/value.h" #include "eval/eval/attribute_trail.h" #include "eval/eval/direct_expression_step.h" @@ -83,14 +84,14 @@ absl::Status DirectShadowableValueStep::Evaluate( } // namespace absl::StatusOr> CreateShadowableValueStep( - std::string identifier, cel::Value value, int64_t expr_id) { - return absl::make_unique(std::move(identifier), + absl::string_view name, cel::Value value, int64_t expr_id) { + return absl::make_unique(std::string(name), std::move(value), expr_id); } std::unique_ptr CreateDirectShadowableValueStep( - std::string identifier, cel::Value value, int64_t expr_id) { - return std::make_unique(std::move(identifier), + absl::string_view name, cel::Value value, int64_t expr_id) { + return std::make_unique(std::string(name), std::move(value), expr_id); } diff --git a/eval/eval/shadowable_value_step.h b/eval/eval/shadowable_value_step.h index 21c6753d5..9c386f02d 100644 --- a/eval/eval/shadowable_value_step.h +++ b/eval/eval/shadowable_value_step.h @@ -3,9 +3,9 @@ #include #include -#include #include "absl/status/statusor.h" +#include "absl/strings/string_view.h" #include "common/value.h" #include "eval/eval/direct_expression_step.h" #include "eval/eval/evaluator_core.h" @@ -16,10 +16,10 @@ namespace google::api::expr::runtime { // shadowed by an identifier of the same name within the runtime-provided // Activation. absl::StatusOr> CreateShadowableValueStep( - std::string identifier, cel::Value value, int64_t expr_id); + absl::string_view name, cel::Value value, int64_t expr_id); std::unique_ptr CreateDirectShadowableValueStep( - std::string identifier, cel::Value value, int64_t expr_id); + absl::string_view name, cel::Value value, int64_t expr_id); } // namespace google::api::expr::runtime diff --git a/eval/eval/ternary_step_test.cc b/eval/eval/ternary_step_test.cc index 4208a28a9..ff66c0308 100644 --- a/eval/eval/ternary_step_test.cc +++ b/eval/eval/ternary_step_test.cc @@ -62,30 +62,15 @@ class LogicStepTest : public testing::TestWithParam { absl::Status EvaluateLogic(CelValue arg0, CelValue arg1, CelValue arg2, CelValue* result, bool enable_unknown) { - Expr expr0; - expr0.set_id(1); - auto& ident_expr0 = expr0.mutable_ident_expr(); - ident_expr0.set_name("name0"); - - Expr expr1; - expr1.set_id(2); - auto& ident_expr1 = expr1.mutable_ident_expr(); - ident_expr1.set_name("name1"); - - Expr expr2; - expr2.set_id(3); - auto& ident_expr2 = expr2.mutable_ident_expr(); - ident_expr2.set_name("name2"); - ExecutionPath path; - CEL_ASSIGN_OR_RETURN(auto step, CreateIdentStep(ident_expr0, expr0.id())); + CEL_ASSIGN_OR_RETURN(auto step, CreateIdentStep("name0", /*expr_id=*/-1)); path.push_back(std::move(step)); - CEL_ASSIGN_OR_RETURN(step, CreateIdentStep(ident_expr1, expr1.id())); + CEL_ASSIGN_OR_RETURN(step, CreateIdentStep("name1", /*expr_id=*/-1)); path.push_back(std::move(step)); - CEL_ASSIGN_OR_RETURN(step, CreateIdentStep(ident_expr2, expr2.id())); + CEL_ASSIGN_OR_RETURN(step, CreateIdentStep("name2", /*expr_id=*/-1)); path.push_back(std::move(step)); CEL_ASSIGN_OR_RETURN(step, CreateTernaryStep(4));