diff --git a/checker/internal/type_checker_impl.cc b/checker/internal/type_checker_impl.cc index 5e9272d10..a854e20e3 100644 --- a/checker/internal/type_checker_impl.cc +++ b/checker/internal/type_checker_impl.cc @@ -1365,6 +1365,9 @@ class ResolveRewriter : public AstRewriterBase { absl::StatusOr TypeCheckerImpl::CheckImpl( std::unique_ptr ast, google::protobuf::Arena* arena) const { + ast->mutable_reference_map().clear(); + ast->mutable_type_map().clear(); + std::optional type_arena; if (arena == nullptr) { type_arena.emplace(); diff --git a/checker/internal/type_checker_impl_test.cc b/checker/internal/type_checker_impl_test.cc index 76e1bfa13..5ed350f21 100644 --- a/checker/internal/type_checker_impl_test.cc +++ b/checker/internal/type_checker_impl_test.cc @@ -2886,6 +2886,32 @@ INSTANTIATE_TEST_SUITE_P( .error_substring = "no matching overload for '_==_'", })); +TEST(TypeCheckerImplTest, RecheckClearsStaleMetadata) { + TypeCheckEnv env(GetSharedTestingDescriptorPool()); + google::protobuf::Arena arena; + ASSERT_THAT(RegisterMinimalBuiltins(&arena, env), absl_testing::IsOk()); + + env.InsertVariableIfAbsent(MakeVariableDecl("x", IntType())); + + TypeCheckerImpl impl(std::move(env)); + ASSERT_OK_AND_ASSIGN(std::unique_ptr ast, MakeTestParsedAst("x")); + + // Artificially inject stale metadata for IDs that do not exist. + constexpr int64_t kStaleId = 9999; + ast->mutable_reference_map()[kStaleId] = + Reference("stale_ref", {}, Constant()); + ast->mutable_type_map()[kStaleId] = TypeSpec(PrimitiveType::kInt64); + + ASSERT_OK_AND_ASSIGN(ValidationResult result, impl.Check(std::move(ast))); + EXPECT_TRUE(result.IsValid()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr checked_ast, result.ReleaseAst()); + + EXPECT_EQ(checked_ast->reference_map().find(kStaleId), + checked_ast->reference_map().end()); + EXPECT_EQ(checked_ast->type_map().find(kStaleId), + checked_ast->type_map().end()); +} + } // namespace } // namespace checker_internal } // namespace cel diff --git a/policy/BUILD b/policy/BUILD index 9b7fe4202..20a5658b4 100644 --- a/policy/BUILD +++ b/policy/BUILD @@ -215,6 +215,7 @@ cc_test( "//runtime:runtime_options", "//runtime:standard_runtime_builder_factory", "//tools:cel_unparser", + "@com_google_absl//absl/container:flat_hash_set", "@com_google_absl//absl/status", "@com_google_absl//absl/status:status_matchers", "@com_google_absl//absl/status:statusor", diff --git a/policy/compiler.cc b/policy/compiler.cc index 98103dbe1..3bd0f3493 100644 --- a/policy/compiler.cc +++ b/policy/compiler.cc @@ -306,6 +306,28 @@ bool HasErrors(const policy_internal::IssueReporter& issues) { return false; } +void AdaptTypeCheckIssues(CelPolicyElementId id, const ValidationResult& r, + const CelPolicySource* absl_nullable policy_source, + policy_internal::IssueReporter& issues) { + const Source* source = r.GetSource(); + if (source == nullptr && policy_source != nullptr) { + source = policy_source->content(); + } + + for (const auto& iss : r.GetIssues()) { + std::optional offset; + if (source != nullptr) { + offset = source->GetPosition(iss.location()); + } + if (offset.has_value()) { + issues.ReportOffsetIssue(id, *offset, MapSeverity(iss.severity()), + iss.message()); + continue; + } + issues.ReportIssue(id, MapSeverity(iss.severity()), iss.message()); + } +} + // Note on lifetime safety: // // The output policy will contain references to types that are owned by the @@ -352,20 +374,7 @@ class PolicyCompiler { } void AdaptTypeCheckIssues(CelPolicyElementId id, const ValidationResult& r) { - const Source* source = r.GetSource(); - - for (const auto& iss : r.GetIssues()) { - std::optional offset; - if (source != nullptr) { - offset = source->GetPosition(iss.location()); - } - if (offset.has_value()) { - issues_.ReportOffsetIssue(id, offset.value(), - MapSeverity(iss.severity()), iss.message()); - continue; - } - issues_.ReportIssue(id, MapSeverity(iss.severity()), iss.message()); - } + cel::AdaptTypeCheckIssues(id, r, src_, issues_); } absl::StatusOr CompileOutputBlock( @@ -601,6 +610,14 @@ class FirstMatchComposer { absl::Status FirstMatchComposer::Compose() { ABSL_DCHECK(icp_.semantics() == RuleSemantics::kFirstMatch); + if (icp_.policy_source() != nullptr && + icp_.policy_source()->content() != nullptr) { + absl::Span line_offsets = + icp_.policy_source()->content()->line_offsets(); + factory_.mutable_ast().mutable_source_info().set_line_offsets( + std::vector(line_offsets.begin(), line_offsets.end())); + } + factory_.mutable_ast().mutable_root_expr() = factory_.NewCall( "cel.@block", factory_.NewList(), factory_.NewUnspecified()); auto& block_init_list = factory_.mutable_ast() @@ -627,9 +644,8 @@ absl::Status FirstMatchComposer::Compose() { CEL_ASSIGN_OR_RETURN(cel::ValidationResult result, compiler_.GetTypeChecker().Check(factory_.ast())); if (!result.IsValid()) { - for (const auto& iss : result.GetIssues()) { - issues_.ReportError(icp_.root_rule().id, iss.message()); - } + AdaptTypeCheckIssues(icp_.root_rule().id, result, icp_.policy_source(), + issues_); return absl::OkStatus(); } @@ -913,8 +929,14 @@ bool HasComprehensionParent(const NavigableAstNode& node) { // Unnester implementation. class Unnester { public: - Unnester(Ast ast, int height, policy_internal::IssueReporter& issues) - : factory_(std::move(ast)), height_(height), issues_(issues) {} + Unnester(const TypeChecker& type_checker, + const CelPolicySource* absl_nullable policy_source, Ast ast, + int height, policy_internal::IssueReporter& issues) + : type_checker_(type_checker), + policy_source_(policy_source), + factory_(std::move(ast)), + height_(height), + issues_(issues) {} // Run the unnesting. // The class cannot be reused after this is called. @@ -923,6 +945,18 @@ class Unnester { CEL_RETURN_IF_ERROR(Slice()); } CEL_RETURN_IF_ERROR(Cleanup()); + if (modified_) { + CEL_ASSIGN_OR_RETURN( + ValidationResult result, + type_checker_.Check(std::move(factory_.mutable_ast()))); + if (!result.IsValid()) { + AdaptTypeCheckIssues(-1, result, policy_source_, issues_); + return absl::InternalError("type checking failed after unnesting"); + } + CEL_ASSIGN_OR_RETURN(std::unique_ptr checked_ast, + result.ReleaseAst()); + return std::move(*checked_ast); + } return std::move(factory_.mutable_ast()); } @@ -934,9 +968,12 @@ class Unnester { void ReportErrorAtId(int64_t id, absl::string_view message); + const TypeChecker& type_checker_; + const CelPolicySource* absl_nullable policy_source_; OptimizerExprFactory factory_; int height_; policy_internal::IssueReporter& issues_; + bool modified_ = false; }; class UnnestRewriter : public AstRewriterBase { @@ -1014,6 +1051,7 @@ absl::Status Unnester::Slice() { UnnestRewriter rewriter(factory_, block_list_expr, cuts); AstRewrite(insertion_expr, rewriter); + modified_ = true; return absl::OkStatus(); } @@ -1028,11 +1066,18 @@ absl::Status Unnester::Cleanup() { return absl::InternalError("malformed AST detected during unnesting"); } if (ast.root_expr().call_expr().args()[0].list_expr().elements().empty()) { + const ExprId block_id = ast.root_expr().id(); + const ExprId list_id = ast.root_expr().call_expr().args()[0].id(); + Expr value = std::move(factory_.mutable_ast() .mutable_root_expr() .mutable_call_expr() .mutable_args()[1]); factory_.mutable_ast().mutable_root_expr() = std::move(value); + + factory_.PruneMetadata(block_id); + factory_.PruneMetadata(list_id); + modified_ = true; } return absl::OkStatus(); @@ -1093,7 +1138,8 @@ absl::StatusOr CompilePolicy( } auto ast = composer.ReleaseAst(); - Unnester unnester(std::move(*ast), options.unnesting_height_limit, + Unnester unnester(base_compiler->GetTypeChecker(), icp.policy_source(), + std::move(*ast), options.unnesting_height_limit, issues); CEL_ASSIGN_OR_RETURN(Ast unnested_ast, unnester.Unnest()); diff --git a/policy/compiler_test.cc b/policy/compiler_test.cc index 6702b5002..3586904aa 100644 --- a/policy/compiler_test.cc +++ b/policy/compiler_test.cc @@ -19,9 +19,11 @@ #include #include +#include "absl/container/flat_hash_set.h" #include "absl/status/status.h" #include "absl/status/status_matchers.h" #include "absl/status/statusor.h" +#include "absl/strings/match.h" #include "absl/strings/string_view.h" #include "common/ast.h" #include "common/ast_proto.h" @@ -114,6 +116,51 @@ absl::StatusOr> ParsePolicyFromYaml( return parse_result.ReleasePolicy(); } +const Ast* GetAstPointer(const Ast& ast) { return * } +const Ast* GetAstPointer(const std::unique_ptr& ast) { return ast.get(); } +const Ast* GetAstPointer(const Ast* ast) { return ast; } + +MATCHER(MetadataMapValid, negation ? "has stale metadata map entries" + : "has valid metadata maps") { + const Ast* ast = GetAstPointer(arg); + if (ast == nullptr) { + *result_listener << "AST is null"; + return false; + } + NavigableAst nav_ast = cel::NavigableAst::Build(ast->root_expr()); + absl::flat_hash_set valid_ids; + for (const NavigableAstNode& node : nav_ast.Root().DescendantsPostorder()) { + valid_ids.insert(node.expr()->id()); + } + + bool is_valid = true; + for (const auto& [id, _] : ast->type_map()) { + if (!valid_ids.contains(id)) { + *result_listener << "stale type_map entry for id " << id << "; "; + is_valid = false; + } + } + for (const auto& [id, _] : ast->reference_map()) { + if (!valid_ids.contains(id)) { + *result_listener << "stale reference_map entry for id " << id << "; "; + is_valid = false; + } + } + for (const auto& [id, _] : ast->source_info().positions()) { + if (!valid_ids.contains(id)) { + *result_listener << "stale source_info position for id " << id << "; "; + is_valid = false; + } + } + for (const auto& [id, _] : ast->source_info().macro_calls()) { + if (!valid_ids.contains(id)) { + *result_listener << "stale source_info macro_call for id " << id << "; "; + is_valid = false; + } + } + return is_valid; +} + TEST(CompilerTest, SmokeTest) { std::string contents; std::string test_file = @@ -1084,22 +1131,167 @@ name: cel_policy - condition: true output: '"ok"' )yaml"; - ASSERT_OK_AND_ASSIGN(auto policy, ParsePolicyFromYaml(yaml)); - ASSERT_OK_AND_ASSIGN(auto compiler, BuildTestCompiler()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr policy, + ParsePolicyFromYaml(yaml)); + ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, BuildTestCompiler()); CompilePolicyOptions options; options.unnesting_height_limit = 0; // Disabled - ASSERT_OK_AND_ASSIGN(auto result, CompilePolicy(*compiler, *policy, options)); + ASSERT_OK_AND_ASSIGN(CelPolicyValidationResult result, + CompilePolicy(*compiler, *policy, options)); ASSERT_TRUE(result.IsValid()); - ASSERT_OK_AND_ASSIGN(auto ast, result.ReleaseAst()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr ast, result.ReleaseAst()); // If cleanup ran, it should have optimized away the trivial `cel.@block`. // So the root expression should NOT be a call to `cel.@block`. // It should be just the constant `"ok"`. - auto nav_ast = cel::NavigableAst::Build(ast->root_expr()); + NavigableAst nav_ast = cel::NavigableAst::Build(ast->root_expr()); EXPECT_FALSE(nav_ast.Root().expr()->has_call_expr() && nav_ast.Root().expr()->call_expr().function() == "cel.@block"); EXPECT_TRUE(nav_ast.Root().expr()->has_const_expr()); + + // Stale metadata for cel.@block and empty list must be pruned. + for (const auto& [id, ref] : ast->reference_map()) { + EXPECT_NE(ref.name(), "cel.@block"); + } + + EXPECT_THAT(ast, MetadataMapValid()); +} + +TEST(CompilerTest, UnnestCleanupRemovesStaleMetadata) { + absl::string_view yaml = R"yaml( +name: cel_policy +rule: + id: test_rule + match: + - condition: x > 0 + output: 1 + - output: 2 +)yaml"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr policy, + ParsePolicyFromYaml(yaml)); + ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, BuildTestCompiler()); + + ASSERT_OK_AND_ASSIGN(CelPolicyValidationResult result, + CompilePolicy(*compiler, *policy)); + ASSERT_TRUE(result.IsValid()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr ast, result.ReleaseAst()); + + NavigableAst nav_ast = cel::NavigableAst::Build(ast->root_expr()); + EXPECT_FALSE(nav_ast.Root().expr()->has_call_expr() && + nav_ast.Root().expr()->call_expr().function() == "cel.@block"); + + for (const auto& [id, ref] : ast->reference_map()) { + EXPECT_NE(ref.name(), "cel.@block"); + } + + EXPECT_THAT(ast, MetadataMapValid()); } + +TEST(CompilerTest, UnnestPreservesVariablesBlock) { + absl::string_view yaml = R"yaml( +name: cel_policy +rule: + id: test_rule + variables: + - name: my_var + expression: 10 + match: + - condition: x > variables.my_var + output: 1 + - output: 2 +)yaml"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr policy, + ParsePolicyFromYaml(yaml)); + ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, BuildTestCompiler()); + + CompilePolicyOptions options; + options.unnesting_height_limit = 0; + ASSERT_OK_AND_ASSIGN(CelPolicyValidationResult result, + CompilePolicy(*compiler, *policy, options)); + ASSERT_TRUE(result.IsValid()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr ast, result.ReleaseAst()); + + NavigableAst nav_ast = cel::NavigableAst::Build(ast->root_expr()); + EXPECT_TRUE(nav_ast.Root().expr()->has_call_expr() && + nav_ast.Root().expr()->call_expr().function() == "cel.@block"); + + EXPECT_THAT(ast, MetadataMapValid()); +} + +TEST(CompilerTest, UnnestRequiredLongConditionalChain) { + absl::string_view yaml = R"yaml( +name: conditional_chain_policy +rule: + match: + - condition: x == 1 + output: 10 + - condition: x == 2 + output: 20 + - condition: x == 3 + output: 30 + - condition: x == 4 + output: 40 + - condition: x == 5 + output: 50 + - condition: x == 6 + output: 60 + - condition: x == 7 + output: 70 + - output: 0 +)yaml"; + ASSERT_OK_AND_ASSIGN(std::unique_ptr policy, + ParsePolicyFromYaml(yaml)); + ASSERT_OK_AND_ASSIGN(std::unique_ptr compiler, BuildTestCompiler()); + + CompilePolicyOptions options; + options.unnesting_height_limit = 2; + ASSERT_OK_AND_ASSIGN(CelPolicyValidationResult result, + CompilePolicy(*compiler, *policy, options)); + ASSERT_TRUE(result.IsValid()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr ast, result.ReleaseAst()); + + // Root should be cel.@block because unnesting introduced @index variables. + NavigableAst nav_ast = cel::NavigableAst::Build(ast->root_expr()); + EXPECT_TRUE(nav_ast.Root().expr()->has_call_expr() && + nav_ast.Root().expr()->call_expr().function() == "cel.@block"); + + bool found_index_var = false; + for (const NavigableAstNode& node : nav_ast.Root().DescendantsPostorder()) { + if (node.expr()->has_ident_expr() && + absl::StartsWith(node.expr()->ident_expr().name(), "@index")) { + found_index_var = true; + break; + } + } + EXPECT_TRUE(found_index_var); + + EXPECT_THAT(ast, MetadataMapValid()); + + // Also verify that the compiled unnested AST evaluates correctly. + cel::RuntimeOptions opts; + opts.enable_qualified_type_identifiers = true; + ASSERT_OK_AND_ASSIGN( + cel::RuntimeBuilder rt_builder, + cel::CreateStandardRuntimeBuilder( + cel::internal::GetSharedTestingDescriptorPool(), opts)); + ASSERT_THAT(cel::extensions::EnableOptionalTypes(rt_builder), + absl_testing::IsOk()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr runtime, + std::move(rt_builder).Build()); + + ASSERT_OK_AND_ASSIGN(std::unique_ptr program, + runtime->CreateProgram(std::move(ast))); + + google::protobuf::Arena arena; + for (int64_t test_x : {1, 3, 5, 7, 99}) { + cel::Activation activation; + activation.InsertOrAssignValue("x", cel::IntValue(test_x)); + ASSERT_OK_AND_ASSIGN(cel::Value res, program->Evaluate(&arena, activation)); + int64_t expected = (test_x >= 1 && test_x <= 7) ? test_x * 10 : 0; + EXPECT_THAT(res, IntValueIs(expected)); + } +} + } // namespace } // namespace cel diff --git a/policy/internal/optimizer_expr_factory.cc b/policy/internal/optimizer_expr_factory.cc index 6c89ae958..84bf711db 100644 --- a/policy/internal/optimizer_expr_factory.cc +++ b/policy/internal/optimizer_expr_factory.cc @@ -271,8 +271,7 @@ void OptimizerExprFactory::RecordReplacement(ExprId id, const Expr& replacement, bool keep_metadata) { auto& source_info = ast_.mutable_source_info(); if (!keep_metadata) { - source_info.mutable_positions().erase(id); - source_info.mutable_macro_calls().erase(id); + PruneMetadata(id); } for (auto& [macro_id, macro_expr] : source_info.mutable_macro_calls()) { @@ -280,6 +279,12 @@ void OptimizerExprFactory::RecordReplacement(ExprId id, const Expr& replacement, } } +void OptimizerExprFactory::PruneMetadata(ExprId id) { + SourceInfo& source_info = ast_.mutable_source_info(); + source_info.mutable_positions().erase(id); + source_info.mutable_macro_calls().erase(id); +} + Expr OptimizerExprFactory::ReportError(absl::string_view message) { ExprId id = NextId(); issues_.push_back(Issue{id, std::string(message)}); diff --git a/policy/internal/optimizer_expr_factory.h b/policy/internal/optimizer_expr_factory.h index 6f63f1485..984350f6e 100644 --- a/policy/internal/optimizer_expr_factory.h +++ b/policy/internal/optimizer_expr_factory.h @@ -73,6 +73,9 @@ class OptimizerExprFactory : protected ExprFactory { void RecordReplacement(ExprId id, const Expr& replacement, bool keep_metadata = false); + // Prunes metadata for the given ID from source info. + void PruneMetadata(ExprId id); + // Makes a copy of source metadata that is remapped to new expr Ids using // current renumberings. This is suitable for merging into the main source // info. diff --git a/policy/internal/optimizer_expr_factory_test.cc b/policy/internal/optimizer_expr_factory_test.cc index a85399411..f240bb308 100644 --- a/policy/internal/optimizer_expr_factory_test.cc +++ b/policy/internal/optimizer_expr_factory_test.cc @@ -16,6 +16,7 @@ #include #include +#include #include #include "absl/status/status_matchers.h" @@ -565,5 +566,27 @@ TEST(OptimizerExprFactory, MergeSourceInfoMacroConflict) { EXPECT_EQ(factory.issues()[0].message, "conflicting ID in macro calls merge"); } +TEST(OptimizerExprFactory, PruneMetadataSingle) { + SourceInfo base_info; + base_info.mutable_positions()[1] = 10; + base_info.mutable_positions()[2] = 20; + base_info.mutable_macro_calls()[1] = Expr(); + base_info.mutable_macro_calls()[2] = Expr(); + + Ast base_ast(Expr(), std::move(base_info)); + TestOptimizerExprFactory factory{std::move(base_ast)}; + + factory.PruneMetadata(1); + + EXPECT_EQ(factory.ast().source_info().positions().find(1), + factory.ast().source_info().positions().end()); + EXPECT_NE(factory.ast().source_info().positions().find(2), + factory.ast().source_info().positions().end()); + EXPECT_EQ(factory.ast().source_info().macro_calls().find(1), + factory.ast().source_info().macro_calls().end()); + EXPECT_NE(factory.ast().source_info().macro_calls().find(2), + factory.ast().source_info().macro_calls().end()); +} + } // namespace } // namespace cel