Merge pull request #103 from google/cpp-sync

Export of internal changes

Author: kyessenov

base/status_macros.h

@@ -67,8 +67,4 @@ inline To down_cast(From* f) {         // so we only accept pointers
 #define EXPECT_OK(expression) EXPECT_TRUE(expression.ok())
 #endif
 
-#if !defined(CHECK_OK)
-#define CHECK_OK(expression) assert(expression.ok())
-#endif
-
 #endif  // THIRD_PARTY_CEL_CPP_BASE_STATUS_MACROS_H_

common/escaping.cc

@@ -294,7 +294,7 @@ absl::optional<std::string> unescape(const std::string& s, bool is_bytes) {
   }
   value = value.substr(1, n - 2);
   // If there is nothing to escape, then return.
-  if (is_raw_literal || (value.find("\\") == std::string::npos)) {
+  if (is_raw_literal || (value.find('\\') == std::string::npos)) {
     return value;
   }
 

conformance/BUILD

@@ -53,7 +53,6 @@ cc_binary(
     testonly = 1,
     srcs = ["server.cc"],
     deps = [
-        "//base:status_macros",
         "//eval/public:builtin_func_registrar",
         "//eval/public:cel_expr_builder_factory",
         "//eval/public:transform_utility",
@@ -88,22 +87,14 @@ cc_binary(
             # TODO(issues/94): Missing timestamp conversion functions (type / string)
             "--skip_test=timestamps/timestamp_conversions/toType_timestamp,toString_timestamp",
             "--skip_test=timestamps/duration_conversions/toType_duration,toString_duration",
-            # TODO(issues/78): Missing bytes() conversion functions
-            "--skip_test=conversions/bytes",
-            # TODO(issues/79): Missing double() conversion functions
-            "--skip_test=conversions/double",
-            # TODO(issues/80): Missing dyn() conversion functions
-            "--skip_test=conversions/dyn/dyn_heterogeneous_list",
-            # TODO(issues/81): Conversion functions for int() which can be
-            # uncommented when the spec changes to truncation rather than
-            # rounding.
+            # TODO(issues/81): Conversion functions for int(), uint() which can be
+            # uncommented when the spec changes to truncation rather than rounding.
             "--skip_test=conversions/int/double_nearest,double_nearest_neg,double_half_away_neg,double_half_away_pos",
+            "--skip_test=conversions/uint/double_nearest,double_nearest_int,double_half_away",
             # TODO(issues/82): Unexpected behavior when converting invalid bytes to string.
             "--skip_test=conversions/string/bytes_invalid",
             # TODO(issues/83): Missing type() conversion functions
             "--skip_test=conversions/type",
-            # TODO(issues/84): Missing uint() conversion functions
-            "--skip_test=conversions/uint",
             # TODO(issues/96): Well-known type conversion support.
             "--skip_test=proto2/literal_wellknown",
             "--skip_test=proto3/literal_wellknown",
@@ -116,13 +107,11 @@ cc_binary(
             "--skip_test=namespace/namespace/self_eval_container_lookup_unchecked",
             "--skip_test=basic/self_eval_nonzeroish/self_eval_bytes_invalid_utf8",
             # Requires heteregenous equality spec clarification
-            "--skip_test=comparisons/eq_literal/eq_bytes",
-            "--skip_test=comparisons/ne_literal/not_ne_bytes",
             "--skip_test=comparisons/in_list_literal/elem_in_mixed_type_list_error",
             "--skip_test=comparisons/in_map_literal/key_in_mixed_key_type_map_error",
             "--skip_test=fields/in/singleton",
             # Requires qualified bindings error message relaxation
-            "--skip_test=fields/qualified_identifier_resolution/int64_field_select_unsupported,list_field_select_unsupported,map_key_null,qualified_identifier_resolution_unchecked",
+            "--skip_test=fields/qualified_identifier_resolution/qualified_identifier_resolution_unchecked",
             "--skip_test=string/size/one_unicode,unicode",
             "--skip_test=string/bytes_concat/left_unit",
             # TODO(issues/85): The exists one macro should not short-circuit false.

conformance/server.cc

@@ -23,7 +23,6 @@
 #include "parser/parser.h"
 #include "proto/test/v1/proto2/test_all_types.pb.h"
 #include "proto/test/v1/proto3/test_all_types.pb.h"
-#include "base/status_macros.h"
 
 
 using absl::Status;
@@ -41,17 +40,20 @@ namespace runtime {
 
 class ConformanceServiceImpl {
  public:
-  ConformanceServiceImpl(std::unique_ptr<CelExpressionBuilder> builder)
+  explicit ConformanceServiceImpl(std::unique_ptr<CelExpressionBuilder> builder)
       : builder_(std::move(builder)),
         proto2Tests_(&google::api::expr::test::v1::proto2::TestAllTypes::
                          default_instance()),
         proto3Tests_(&google::api::expr::test::v1::proto3::TestAllTypes::
                          default_instance()) {}
 
-  Status Parse(const v1alpha1::ParseRequest* request,
-               v1alpha1::ParseResponse* response) {
+  void Parse(const v1alpha1::ParseRequest* request,
+             v1alpha1::ParseResponse* response) {
     if (request->cel_source().empty()) {
-      return Status(StatusCode::kInvalidArgument, "No source code.");
+      auto issue = response->add_issues();
+      issue->set_message("No source code");
+      issue->set_code(google::rpc::Code::INVALID_ARGUMENT);
+      return;
     }
     auto parse_status = parser::Parse(request->cel_source(), "");
     if (!parse_status.ok()) {
@@ -63,16 +65,17 @@ class ConformanceServiceImpl {
       (out).MergeFrom(parse_status.value());
       response->mutable_parsed_expr()->CopyFrom(out);
     }
-    return absl::OkStatus();
   }
 
-  Status Check(const v1alpha1::CheckRequest* request,
-               v1alpha1::CheckResponse* response) {
-    return Status(StatusCode::kUnimplemented, "Check is not supported");
+  void Check(const v1alpha1::CheckRequest* request,
+             v1alpha1::CheckResponse* response) {
+    auto issue = response->add_issues();
+    issue->set_message("Check is not supported");
+    issue->set_code(google::rpc::Code::UNIMPLEMENTED);
   }
 
-  Status Eval(const v1alpha1::EvalRequest* request,
-              v1alpha1::EvalResponse* response) {
+  void Eval(const v1alpha1::EvalRequest* request,
+            v1alpha1::EvalResponse* response) {
     const v1alpha1::Expr* expr = nullptr;
     if (request->has_parsed_expr()) {
       expr = &request->parsed_expr().expr();
@@ -87,8 +90,10 @@ class ConformanceServiceImpl {
     auto cel_expression_status = builder_->CreateExpression(&out, &source_info);
 
     if (!cel_expression_status.ok()) {
-      return Status(StatusCode::kInternal,
-                    std::string(cel_expression_status.status().message()));
+      auto issue = response->add_issues();
+      issue->set_message(cel_expression_status.status().ToString());
+      issue->set_code(google::rpc::Code::INTERNAL);
+      return;
     }
 
     auto cel_expression = std::move(cel_expression_status.value());
@@ -100,7 +105,10 @@ class ConformanceServiceImpl {
       (*import_value).MergeFrom(pair.second.value());
       auto import_status = ValueToCelValue(*import_value, &arena);
       if (!import_status.ok()) {
-        return Status(StatusCode::kInternal, import_status.status().ToString());
+        auto issue = response->add_issues();
+        issue->set_message(import_status.status().ToString());
+        issue->set_code(google::rpc::Code::INTERNAL);
+        return;
       }
       activation.InsertValue(pair.first, import_status.value());
     }
@@ -111,7 +119,7 @@ class ConformanceServiceImpl {
            ->mutable_error()
            ->add_errors()
            ->mutable_message() = eval_status.status().ToString();
-      return absl::OkStatus();
+      return;
     }
 
     CelValue result = eval_status.value();
@@ -124,12 +132,14 @@ class ConformanceServiceImpl {
       google::api::expr::v1alpha1::Value export_value;
       auto export_status = CelValueToValue(result, &export_value);
       if (!export_status.ok()) {
-        return Status(StatusCode::kInternal, export_status.ToString());
+        auto issue = response->add_issues();
+        issue->set_message(export_status.ToString());
+        issue->set_code(google::rpc::Code::INTERNAL);
+        return;
       }
       auto* result_value = response->mutable_result()->mutable_value();
       (*result_value).MergeFrom(export_value);
     }
-    return absl::OkStatus();
   }
 
  private:
@@ -178,15 +188,23 @@ int RunServer(bool optimize) {
     if (cmd == "parse") {
       v1alpha1::ParseRequest request;
       v1alpha1::ParseResponse response;
-      CHECK_OK(JsonStringToMessage(input, &request));
-      CHECK_OK(service.Parse(&request, &response));
-      CHECK_OK(MessageToJsonString(response, &output));
+      if (!JsonStringToMessage(input, &request).ok()) {
+        std::cerr << "Failed to parse JSON" << std::endl;
+      }
+      service.Parse(&request, &response);
+      if (!MessageToJsonString(response, &output).ok()) {
+        std::cerr << "Failed to convert to JSON" << std::endl;
+      }
     } else if (cmd == "eval") {
       v1alpha1::EvalRequest request;
       v1alpha1::EvalResponse response;
-      CHECK_OK(JsonStringToMessage(input, &request));
-      CHECK_OK(service.Eval(&request, &response));
-      CHECK_OK(MessageToJsonString(response, &output));
+      if (!JsonStringToMessage(input, &request).ok()) {
+        std::cerr << "Failed to parse JSON" << std::endl;
+      }
+      service.Eval(&request, &response);
+      if (!MessageToJsonString(response, &output).ok()) {
+        std::cerr << "Failed to convert to JSON" << std::endl;
+      }
     } else if (cmd.empty()) {
       return 0;
     } else {

eval/compiler/flat_expr_builder.cc

@@ -1,5 +1,7 @@
 #include "eval/compiler/flat_expr_builder.h"
 
+#include <memory>
+
 #include "google/api/expr/v1alpha1/checked.pb.h"
 #include "stack"
 #include "absl/container/node_hash_map.h"
@@ -658,8 +660,8 @@ void ComprehensionVisitor::PreVisit(const Expr*) {
 
 void ComprehensionVisitor::PostVisitArg(int arg_num, const Expr* expr) {
   const Comprehension* comprehension = &expr->comprehension_expr();
-  const auto accu_var = comprehension->accu_var();
-  const auto iter_var = comprehension->iter_var();
+  const auto& accu_var = comprehension->accu_var();
+  const auto& iter_var = comprehension->iter_var();
   // TODO(issues/20): Consider refactoring the comprehension prologue step.
   switch (arg_num) {
     case ITER_RANGE: {
@@ -732,7 +734,7 @@ FlatExprBuilder::CreateExpressionImpl(
 
   const Expr* effective_expr = expr;
   // transformed expression preserving expression IDs
-  Expr rewrite_buffer;
+  std::unique_ptr<Expr> rewrite_buffer = nullptr;
   // TODO(issues/98): A type checker may perform these rewrites, but there
   // currently  isn't a signal to expose that in an expression. If that becomes
   // available, we can skip the reference resolve step here if it's already
@@ -745,19 +747,19 @@ FlatExprBuilder::CreateExpressionImpl(
       return rewritten.status();
     }
     if (rewritten.value().has_value()) {
-      rewrite_buffer = std::move(rewritten)->value();
-      effective_expr = &rewrite_buffer;
+      rewrite_buffer =
+          std::make_unique<Expr>(std::move(rewritten).value().value());
+      effective_expr = rewrite_buffer.get();
     }
     // TODO(issues/99): we could setup a check step here that confirms all of
     // references are defined before actually evaluating.
   }
 
+  Expr const_fold_buffer;
   if (constant_folding_) {
-    Expr buffer;
     FoldConstants(*effective_expr, *this->GetRegistry(), constant_arena_,
-                  idents, &buffer);
-    rewrite_buffer = std::move(buffer);
-    effective_expr = &rewrite_buffer;
+                  idents, &const_fold_buffer);
+    effective_expr = &const_fold_buffer;
   }
 
   std::set<std::string> iter_variable_names;
@@ -776,7 +778,8 @@ FlatExprBuilder::CreateExpressionImpl(
       absl::make_unique<CelExpressionFlatImpl>(
           expr, std::move(execution_path), comprehension_max_iterations_,
           std::move(iter_variable_names), enable_unknowns_,
-          enable_unknown_function_results_, enable_missing_attribute_errors_);
+          enable_unknown_function_results_, enable_missing_attribute_errors_,
+          std::move(rewrite_buffer));
 
   if (warnings != nullptr) {
     *warnings = std::move(warnings_builder).warnings();

eval/compiler/flat_expr_builder_test.cc

@@ -614,6 +614,60 @@ TEST(FlatExprBuilderTest, CheckedExprActivationMissesReferences) {
   EXPECT_FALSE(result.BoolOrDie());
 }
 
+TEST(FlatExprBuilderTest, CheckedExprWithReferenceMapAndConstantFolding) {
+  CheckedExpr expr;
+  // {`var1`: 'hello'}
+  google::protobuf::TextFormat::ParseFromString(R"(
+    reference_map {
+      key: 3
+      value {
+        name: "var1"
+        value {
+          int64_value: 1
+        }
+      }
+    }
+    expr {
+      id: 1
+      struct_expr {
+        entries {
+          id: 2
+          map_key {
+            id: 3
+            ident_expr {
+              name: "var1"
+            }
+          }
+          value {
+            id: 4
+            const_expr {
+              string_value: "hello"
+            }
+          }
+        }
+      }
+    })",
+                                      &expr);
+
+  FlatExprBuilder builder;
+  google::protobuf::Arena arena;
+  builder.set_constant_folding(true, &arena);
+  ASSERT_OK(RegisterBuiltinFunctions(builder.GetRegistry()));
+  auto build_status = builder.CreateExpression(&expr);
+  ASSERT_OK(build_status);
+
+  auto cel_expr = std::move(build_status.value());
+
+  Activation activation;
+  auto result_or = cel_expr->Evaluate(activation, &arena);
+  ASSERT_OK(result_or);
+  CelValue result = result_or.value();
+  ASSERT_TRUE(result.IsMap());
+  auto m = result.MapOrDie();
+  auto v = (*m)[CelValue::CreateInt64(1L)];
+  EXPECT_THAT(v.value().StringOrDie().value(), Eq("hello"));
+}
+
 TEST(FlatExprBuilderTest, ComprehensionWorksForError) {
   Expr expr;
   // {}[0].all(x, x) should evaluate OK but return an error value

eval/eval/evaluator_core.h

@@ -34,6 +34,8 @@ namespace runtime {
 // Forward declaration of ExecutionFrame, to resolve circular dependency.
 class ExecutionFrame;
 
+using Expr = google::api::expr::v1alpha1::Expr;
+
 // Class Expression represents single execution step.
 class ExpressionStep {
  public:
@@ -67,7 +69,7 @@ using ExecutionPath = std::vector<std::unique_ptr<const ExpressionStep>>;
 // stack as Span<>.
 class ValueStack {
  public:
-  ValueStack(size_t max_size) : current_size_(0) {
+  explicit ValueStack(size_t max_size) : current_size_(0) {
     stack_.resize(max_size);
     attribute_stack_.resize(max_size);
   }
@@ -336,8 +338,10 @@ class CelExpressionFlatImpl : public CelExpression {
                         std::set<std::string> iter_variable_names,
                         bool enable_unknowns = false,
                         bool enable_unknown_function_results = false,
-                        bool enable_missing_attribute_errors = false)
-      : path_(std::move(path)),
+                        bool enable_missing_attribute_errors = false,
+                        std::unique_ptr<Expr> rewritten_expr = nullptr)
+      : rewritten_expr_(std::move(rewritten_expr)),
+        path_(std::move(path)),
         max_iterations_(max_iterations),
         iter_variable_names_(std::move(iter_variable_names)),
         enable_unknowns_(enable_unknowns),
@@ -372,6 +376,8 @@ class CelExpressionFlatImpl : public CelExpression {
                                  CelEvaluationListener callback) const override;
 
  private:
+  // Maintain lifecycle of a modified expression.
+  std::unique_ptr<Expr> rewritten_expr_;
   const ExecutionPath path_;
   const int max_iterations_;
   const std::set<std::string> iter_variable_names_;