Skip to content

Commit

Permalink
Mock default enabling recursive planning for identifying compatibilit…
Browse files Browse the repository at this point in the history
…y issues.

Do not merge.

PiperOrigin-RevId: 623213699
  • Loading branch information
jnthntatum authored and copybara-github committed Apr 15, 2024
1 parent 6059846 commit 22a299d
Show file tree
Hide file tree
Showing 82 changed files with 6,449 additions and 937 deletions.
10 changes: 2 additions & 8 deletions common/value.cc
Original file line number Diff line number Diff line change
Expand Up @@ -270,15 +270,12 @@ bool Value::IsZeroValue() const {
}

std::ostream& operator<<(std::ostream& out, const Value& value) {
value.AssertIsValid();
return absl::visit(
[&out](const auto& alternative) -> std::ostream& {
if constexpr (std::is_same_v<
absl::remove_cvref_t<decltype(alternative)>,
absl::monostate>) {
// In optimized builds, we do nothing. In debug builds we cannot
// reach here.
return out;
return out << "default ctor Value";
} else {
return out << alternative;
}
Expand Down Expand Up @@ -513,15 +510,12 @@ bool ValueView::IsZeroValue() const {
}

std::ostream& operator<<(std::ostream& out, ValueView value) {
value.AssertIsValid();
return absl::visit(
[&out](auto alternative) -> std::ostream& {
if constexpr (std::is_same_v<
absl::remove_cvref_t<decltype(alternative)>,
absl::monostate>) {
// In optimized builds, we do nothing. In debug builds we cannot
// reach here.
return out;
return out << "default ctor ValueView";
} else {
return out << alternative;
}
Expand Down
3 changes: 3 additions & 0 deletions common/value.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ class Value final {
: variant_((other.AssertIsValid(), other.variant_)) {}

Value& operator=(const Value& other) {
if (this == std::addressof(other)) {
return *this;
}
other.AssertIsValid();
ABSL_DCHECK(this != std::addressof(other))
<< "Value should not be copied to itself";
Expand Down
5 changes: 3 additions & 2 deletions common/value_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,14 @@ TEST(Value, GetTypeName) {
EXPECT_DEBUG_DEATH(static_cast<void>(moved_from_value.GetTypeName()), _);
}

TEST(Value, DebugStringDebugDeath) {
TEST(Value, DebugStringUinitializedValue) {
Value moved_from_value = BoolValue(true);
Value value = std::move(moved_from_value);
IS_INITIALIZED(moved_from_value);
static_cast<void>(value);
std::ostringstream out;
EXPECT_DEBUG_DEATH(static_cast<void>(out << moved_from_value), _);
out << moved_from_value;
EXPECT_EQ(out.str(), "default ctor Value");
}

TEST(Value, NativeValueIdDebugDeath) {
Expand Down
4 changes: 1 addition & 3 deletions common/value_testing.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@

namespace cel {

void PrintTo(const Value& value, std::ostream* os) {
*os << value.DebugString() << "\n";
}
void PrintTo(const Value& value, std::ostream* os) { *os << value << "\n"; }

namespace test {
namespace {
Expand Down
6 changes: 6 additions & 0 deletions conformance/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ cc_binary(
for args in [
[],
["--opt"],
["--recursive"],
]
]

Expand Down Expand Up @@ -198,6 +199,11 @@ cc_binary(
"--arena",
"--opt",
],
[
"--modern",
"--arena",
"--recursive",
],
]
]

Expand Down
10 changes: 10 additions & 0 deletions conformance/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ ABSL_FLAG(
ABSL_FLAG(bool, arena, false,
"Use arena memory manager (default: global heap ref-counted). Only "
"affects the modern implementation");
ABSL_FLAG(bool, recursive, false,
"Enable recursive plans. Depth limited to slightly more than the "
"default nesting limit.");

namespace google::api::expr::runtime {

Expand Down Expand Up @@ -159,6 +162,10 @@ class LegacyConformanceServiceImpl : public ConformanceServiceInterface {
options.constant_arena = constant_arena;
}

if (absl::GetFlag(FLAGS_recursive)) {
options.max_recursion_depth = 48;
}

std::unique_ptr<CelExpressionBuilder> builder =
CreateCelExpressionBuilder(options);
auto type_registry = builder->GetTypeRegistry();
Expand Down Expand Up @@ -270,6 +277,9 @@ class ModernConformanceServiceImpl : public ConformanceServiceInterface {
options.enable_timestamp_duration_overflow_errors = true;
options.enable_heterogeneous_equality = true;
options.enable_empty_wrapper_null_unboxing = true;
if (absl::GetFlag(FLAGS_recursive)) {
options.max_recursion_depth = 48;
}

return absl::WrapUnique(
new ModernConformanceServiceImpl(options, use_arena, optimize));
Expand Down
45 changes: 43 additions & 2 deletions eval/compiler/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,12 @@ cc_library(
"//base:ast",
"//base/ast_internal:ast_impl",
"//base/ast_internal:expr",
"//common:native_type",
"//common:value",
"//eval/eval:direct_expression_step",
"//eval/eval:evaluator_core",
"//eval/eval:trace_step",
"//internal:casts",
"//runtime:runtime_options",
"//runtime/internal:issue_collector",
"@com_google_absl//absl/algorithm:container",
Expand All @@ -40,6 +44,7 @@ cc_library(
"@com_google_absl//absl/memory",
"@com_google_absl//absl/status",
"@com_google_absl//absl/status:statusor",
"@com_google_absl//absl/types:optional",
"@com_google_absl//absl/types:variant",
],
)
Expand All @@ -51,10 +56,14 @@ cc_test(
":flat_expr_builder_extensions",
":resolver",
"//base/ast_internal:expr",
"//common:casting",
"//common:memory",
"//common:native_type",
"//common:value",
"//eval/eval:const_value_step",
"//eval/eval:direct_expression_step",
"//eval/eval:evaluator_core",
"//eval/eval:function_step",
"//internal:status_macros",
"//internal:testing",
"//runtime:function_registry",
Expand Down Expand Up @@ -83,13 +92,16 @@ cc_library(
"//base/ast_internal:ast_impl",
"//base/ast_internal:expr",
"//common:memory",
"//common:type",
"//common:value",
"//eval/eval:attribute_trail",
"//eval/eval:comprehension_step",
"//eval/eval:const_value_step",
"//eval/eval:container_access_step",
"//eval/eval:create_list_step",
"//eval/eval:create_map_step",
"//eval/eval:create_struct_step",
"//eval/eval:direct_expression_step",
"//eval/eval:evaluator_core",
"//eval/eval:function_step",
"//eval/eval:ident_step",
Expand All @@ -99,6 +111,7 @@ cc_library(
"//eval/eval:select_step",
"//eval/eval:shadowable_value_step",
"//eval/eval:ternary_step",
"//eval/eval:trace_step",
"//eval/public:ast_traverse_native",
"//eval/public:ast_visitor_native",
"//eval/public:cel_type_registry",
Expand All @@ -108,6 +121,7 @@ cc_library(
"//runtime:runtime_issue",
"//runtime:runtime_options",
"//runtime:type_registry",
"//runtime/internal:convert_constant",
"//runtime/internal:issue_collector",
"@com_google_absl//absl/algorithm:container",
"@com_google_absl//absl/base:core_headers",
Expand All @@ -119,6 +133,7 @@ cc_library(
"@com_google_absl//absl/status",
"@com_google_absl//absl/status:statusor",
"@com_google_absl//absl/strings",
"@com_google_absl//absl/types:optional",
"@com_google_absl//absl/types:span",
"@com_google_absl//absl/types:variant",
],
Expand Down Expand Up @@ -201,6 +216,7 @@ cc_test(
"//internal:status_macros",
"//internal:testing",
"//parser",
"//runtime",
"//runtime:runtime_options",
"@com_google_absl//absl/status",
"@com_google_absl//absl/strings",
Expand All @@ -220,7 +236,9 @@ cc_library(
deps = [
":flat_expr_builder",
"//base:ast",
"//common:native_type",
"//eval/eval:cel_expression_flat_impl",
"//eval/eval:direct_expression_step",
"//eval/eval:evaluator_core",
"//eval/public:cel_expression",
"//extensions/protobuf:ast_converters",
Expand All @@ -243,15 +261,32 @@ cc_test(
],
deps = [
":cel_expression_builder_flat_impl",
":constant_folding",
":regex_precompilation_optimization",
"//eval/eval:cel_expression_flat_impl",
"//eval/public:activation",
"//eval/public:builtin_func_registrar",
"//eval/public:cel_expression",
"//eval/public:cel_function",
"//eval/public:cel_value",
"//eval/public:portable_cel_function_adapter",
"//eval/public/containers:container_backed_map_impl",
"//eval/public/structs:cel_proto_wrapper",
"//eval/public/structs:protobuf_descriptor_type_provider",
"//eval/public/testing:matchers",
"//extensions:bindings_ext",
"//extensions/protobuf:memory_manager",
"//internal:status_macros",
"//internal:testing",
"//parser",
"//parser:macro",
"//runtime:runtime_options",
"@com_google_absl//absl/algorithm:container",
"@com_google_absl//absl/status",
"@com_google_cel_spec//proto/test/v1/proto3:test_all_types_cc_proto",
"@com_google_googleapis//google/api/expr/v1alpha1:checked_cc_proto",
"@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto",
"@com_google_protobuf//:protobuf",
],
)

Expand All @@ -274,6 +309,7 @@ cc_library(
"//common:memory",
"//common:value",
"//eval/eval:const_value_step",
"//eval/eval:direct_expression_step",
"//eval/eval:evaluator_core",
"//internal:status_macros",
"//runtime:activation",
Expand Down Expand Up @@ -462,14 +498,17 @@ cc_library(
"//common:native_type",
"//common:value",
"//eval/eval:compiler_constant_step",
"//eval/eval:direct_expression_step",
"//eval/eval:evaluator_core",
"//eval/eval:regex_match_step",
"//internal:casts",
"//internal:status_macros",
"@com_google_absl//absl/base:nullability",
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/status",
"@com_google_absl//absl/strings",
"@com_google_absl//absl/types:optional",
"@com_googlesource_code_re2//:re2",
],
)

Expand All @@ -485,16 +524,18 @@ cc_test(
"//base/ast_internal:ast_impl",
"//base/ast_internal:expr",
"//common:memory",
"//common:type",
"//common:value",
"//eval/eval:cel_expression_flat_impl",
"//eval/eval:evaluator_core",
"//eval/public:activation",
"//eval/public:builtin_func_registrar",
"//eval/public:cel_expression",
"//eval/public:cel_options",
"//eval/public:cel_value",
"//internal:testing",
"//parser",
"//runtime:runtime_issue",
"//runtime/internal:issue_collector",
"@com_google_absl//absl/status",
"@com_google_googleapis//google/api/expr/v1alpha1:checked_cc_proto",
"@com_google_googleapis//google/api/expr/v1alpha1:syntax_cc_proto",
"@com_google_protobuf//:protobuf",
Expand Down
10 changes: 10 additions & 0 deletions eval/compiler/cel_expression_builder_flat_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@
#include "absl/status/status.h"
#include "absl/status/statusor.h"
#include "base/ast.h"
#include "common/native_type.h"
#include "eval/eval/cel_expression_flat_impl.h"
#include "eval/eval/direct_expression_step.h"
#include "eval/eval/evaluator_core.h"
#include "eval/public/cel_expression.h"
#include "extensions/protobuf/ast_converters.h"
Expand Down Expand Up @@ -94,6 +96,14 @@ CelExpressionBuilderFlatImpl::CreateExpressionImpl(
warnings->push_back(issue.ToStatus());
}
}
if (flat_expr_builder_.options().max_recursion_depth != 0 &&
impl.path().size() > 0 &&
// mainline expression is exactly one recursive step.
impl.subexpressions().front().size() == 1 &&
impl.path().front()->GetNativeTypeId() ==
cel::NativeTypeId::For<WrappedDirectStep>()) {
return CelExpressionRecursiveImpl::Create(std::move(impl));
}

return std::make_unique<CelExpressionFlatImpl>(std::move(impl));
}
Expand Down
Loading

0 comments on commit 22a299d

Please sign in to comment.