diff --git a/eval/compiler/flat_expr_builder_extensions_test.cc b/eval/compiler/flat_expr_builder_extensions_test.cc index db13f6ec6..2eae196af 100644 --- a/eval/compiler/flat_expr_builder_extensions_test.cc +++ b/eval/compiler/flat_expr_builder_extensions_test.cc @@ -57,9 +57,7 @@ using ::testing::Optional; class TestStepLogic : public ExpressionStepLogic { public: - absl::Status Evaluate(ExecutionFrame* frame) const override { - return absl::OkStatus(); - } + void Evaluate(ExecutionFrame* frame) const override {} }; std::unique_ptr MakeTestStepLogic() { diff --git a/eval/compiler/instrumentation.cc b/eval/compiler/instrumentation.cc index ea3e7ddbb..7f522c739 100644 --- a/eval/compiler/instrumentation.cc +++ b/eval/compiler/instrumentation.cc @@ -37,14 +37,17 @@ class InstrumentStep : public ExpressionStepBase { expr_id_(expr_id), instrumentation_(std::move(instrumentation)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { if (!frame->value_stack().HasEnough(1)) { - return absl::InternalError("stack underflow in instrument step."); + frame->Abort(absl::InternalError("stack underflow in instrument step.")); + return; } - return instrumentation_(expr_id_, frame->value_stack().Peek()); - - return absl::OkStatus(); + if (absl::Status status = + instrumentation_(expr_id_, frame->value_stack().Peek()); + !status.ok()) { + frame->Abort(std::move(status)); + } } private: diff --git a/eval/eval/comprehension_step.cc b/eval/eval/comprehension_step.cc index d67a7e8a6..6293f97f4 100644 --- a/eval/eval/comprehension_step.cc +++ b/eval/eval/comprehension_step.cc @@ -428,14 +428,16 @@ absl::Status ComprehensionDirectStep::Evaluate2(ExecutionFrameBase& frame, } // namespace -absl::Status ComprehensionInitStep::Evaluate(ExecutionFrame* frame) const { +void ComprehensionInitStep::Evaluate(ExecutionFrame* frame) const { if (!frame->value_stack().HasEnough(1)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); + frame->Abort(absl::InternalError("Value stack underflow")); + return; } const Value& top = frame->value_stack().Peek(); if (top.IsError() || top.IsUnknown()) { - return frame->JumpTo(error_jump_offset_); + frame->JumpToOrAbort(error_jump_offset_); + return; } if (frame->enable_unknowns() && top.IsMap()) { @@ -443,40 +445,38 @@ absl::Status ComprehensionInitStep::Evaluate(ExecutionFrame* frame) const { if (frame->attribute_utility().CheckForUnknownPartial(top_attr)) { frame->value_stack().PopAndPush( frame->attribute_utility().CreateUnknownSet(top_attr.attribute())); - return frame->JumpTo(error_jump_offset_); + frame->JumpToOrAbort(error_jump_offset_); + return; } } + absl::StatusOr iterator; switch (top.kind()) { - case ValueKind::kList: { - CEL_ASSIGN_OR_RETURN(auto iterator, top.GetList().NewIterator()); - if (has_iter2_) { - frame->iterator_stack().Push(std::move(iterator), iter_slot_, - iter2_slot_, accu_slot_); - } else { - frame->iterator_stack().Push(std::move(iterator), iter_slot_, - accu_slot_); - } - } break; - case ValueKind::kMap: { - CEL_ASSIGN_OR_RETURN(auto iterator, top.GetMap().NewIterator()); - if (has_iter2_) { - frame->iterator_stack().Push(std::move(iterator), iter_slot_, - iter2_slot_, accu_slot_); - } else { - frame->iterator_stack().Push(std::move(iterator), iter_slot_, - accu_slot_); - } - } break; + case ValueKind::kList: + iterator = top.GetList().NewIterator(); + break; + case ValueKind::kMap: + iterator = top.GetMap().NewIterator(); + break; default: // Replace with an error and jump past // ComprehensionFinishStep. frame->value_stack().PopAndPush(cel::ErrorValue::From( CreateNoMatchingOverloadError(""), frame->arena())); - return frame->JumpTo(error_jump_offset_); + frame->JumpToOrAbort(error_jump_offset_); + return; } - return absl::OkStatus(); + if (!iterator.ok()) { + frame->Abort(std::move(iterator).status()); + return; + } + if (has_iter2_) { + frame->iterator_stack().Push(*std::move(iterator), iter_slot_, iter2_slot_, + accu_slot_); + } else { + frame->iterator_stack().Push(*std::move(iterator), iter_slot_, accu_slot_); + } } void ComprehensionNextStep::Evaluate1(ExecutionFrame* frame) const { diff --git a/eval/eval/comprehension_step.h b/eval/eval/comprehension_step.h index 8ec7960db..8bac8dab3 100644 --- a/eval/eval/comprehension_step.h +++ b/eval/eval/comprehension_step.h @@ -40,7 +40,7 @@ class ComprehensionInitStep final : public ExpressionStepLogic { void set_error_jump_offset(int offset) { error_jump_offset_ = offset; } - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: const size_t iter_slot_; diff --git a/eval/eval/comprehension_step_test.cc b/eval/eval/comprehension_step_test.cc index ee8b764c1..2ffbafb1b 100644 --- a/eval/eval/comprehension_step_test.cc +++ b/eval/eval/comprehension_step_test.cc @@ -81,9 +81,8 @@ class GetListKeysResultStep : public ExpressionStepBase { public: GetListKeysResultStep() : ExpressionStepBase() {} - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { frame->value_stack().Pop(1); - return absl::OkStatus(); } }; diff --git a/eval/eval/container_access_step.cc b/eval/eval/container_access_step.cc index 8ce87ef3f..f4a33b217 100644 --- a/eval/eval/container_access_step.cc +++ b/eval/eval/container_access_step.cc @@ -289,17 +289,17 @@ class ContainerAccessStep : public ExpressionStepBase { explicit ContainerAccessStep(bool enable_optional_types) : ExpressionStepBase(), enable_optional_types_(enable_optional_types) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: bool enable_optional_types_; }; -absl::Status ContainerAccessStep::Evaluate(ExecutionFrame* frame) const { +void ContainerAccessStep::Evaluate(ExecutionFrame* frame) const { if (!frame->value_stack().HasEnough(kNumContainerAccessArguments)) { - return absl::Status( - absl::StatusCode::kInternal, - "Insufficient arguments supplied for ContainerAccess-type expression"); + frame->Abort(absl::InternalError( + "Insufficient arguments supplied for ContainerAccess-type expression")); + return; } Value result; @@ -312,8 +312,6 @@ absl::Status ContainerAccessStep::Evaluate(ExecutionFrame* frame) const { enable_optional_types_, result, result_trail); frame->value_stack().PopAndPush(kNumContainerAccessArguments, std::move(result), std::move(result_trail)); - - return absl::OkStatus(); } class DirectContainerAccessStep : public DirectExpressionStep { diff --git a/eval/eval/create_list_step.cc b/eval/eval/create_list_step.cc index 9f5483a68..65636f347 100644 --- a/eval/eval/create_list_step.cc +++ b/eval/eval/create_list_step.cc @@ -38,7 +38,7 @@ class CreateListStep : public ExpressionStepBase { CreateListStep(int list_size, absl::flat_hash_set optional_indices) : list_size_(list_size), optional_indices_(std::move(optional_indices)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: absl::Status DoEvaluate(ExecutionFrame* frame, Value* result) const; @@ -47,22 +47,24 @@ class CreateListStep : public ExpressionStepBase { absl::flat_hash_set optional_indices_; }; -absl::Status CreateListStep::Evaluate(ExecutionFrame* frame) const { +void CreateListStep::Evaluate(ExecutionFrame* frame) const { if (list_size_ < 0) { - return absl::Status(absl::StatusCode::kInternal, - "CreateListStep: list size is <0"); + frame->Abort(absl::InternalError("CreateListStep: list size is <0")); + return; } if (!frame->value_stack().HasEnough(list_size_)) { - return absl::Status(absl::StatusCode::kInternal, - "CreateListStep: stack underflow"); + frame->Abort(absl::InternalError("CreateListStep: stack underflow")); + return; } Value result; - CEL_RETURN_IF_ERROR(DoEvaluate(frame, &result)); + if (absl::Status status = DoEvaluate(frame, &result); !status.ok()) { + frame->Abort(std::move(status)); + return; + } frame->value_stack().PopAndPush(list_size_, std::move(result)); - return absl::OkStatus(); } absl::Status CreateListStep::DoEvaluate(ExecutionFrame* frame, diff --git a/eval/eval/create_map_step.cc b/eval/eval/create_map_step.cc index e90de1fc7..4b27e5e30 100644 --- a/eval/eval/create_map_step.cc +++ b/eval/eval/create_map_step.cc @@ -57,7 +57,7 @@ class CreateStructStepForMap final : public ExpressionStepBase { : entry_count_(entry_count), optional_indices_(std::move(optional_indices)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: absl::StatusOr DoEvaluate(ExecutionFrame* frame) const; @@ -118,16 +118,20 @@ absl::StatusOr CreateStructStepForMap::DoEvaluate( return std::move(*builder).Build(); } -absl::Status CreateStructStepForMap::Evaluate(ExecutionFrame* frame) const { +void CreateStructStepForMap::Evaluate(ExecutionFrame* frame) const { if (frame->value_stack().size() < 2 * entry_count_) { - return absl::InternalError("CreateStructStepForMap: stack underflow"); + frame->Abort( + absl::InternalError("CreateStructStepForMap: stack underflow")); + return; } - CEL_ASSIGN_OR_RETURN(auto result, DoEvaluate(frame)); - - frame->value_stack().PopAndPush(2 * entry_count_, std::move(result)); + absl::StatusOr result = DoEvaluate(frame); + if (!result.ok()) { + frame->Abort(std::move(result).status()); + return; + } - return absl::OkStatus(); + frame->value_stack().PopAndPush(2 * entry_count_, *std::move(result)); } class DirectCreateMapStep : public DirectExpressionStep { @@ -238,10 +242,9 @@ class MutableMapStep final : public ExpressionStepBase { public: MutableMapStep() = default; - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { frame->value_stack().Push(cel::CustomMapValue( NewMutableMapValue(frame->arena()), frame->arena())); - return absl::OkStatus(); } }; diff --git a/eval/eval/create_struct_step.cc b/eval/eval/create_struct_step.cc index a4467a736..bce2a3ea9 100644 --- a/eval/eval/create_struct_step.cc +++ b/eval/eval/create_struct_step.cc @@ -55,7 +55,7 @@ class CreateStructStepForStruct final : public ExpressionStepBase { entries_(std::move(entries)), optional_indices_(std::move(optional_indices)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: absl::StatusOr DoEvaluate(ExecutionFrame* frame) const; @@ -131,14 +131,18 @@ absl::StatusOr CreateStructStepForStruct::DoEvaluate( return std::move(*builder).Build(); } -absl::Status CreateStructStepForStruct::Evaluate(ExecutionFrame* frame) const { +void CreateStructStepForStruct::Evaluate(ExecutionFrame* frame) const { if (frame->value_stack().size() < entries_.size()) { - return absl::InternalError("CreateStructStepForStruct: stack underflow"); + frame->Abort( + absl::InternalError("CreateStructStepForStruct: stack underflow")); + return; } - CEL_ASSIGN_OR_RETURN(Value result, DoEvaluate(frame)); - frame->value_stack().PopAndPush(entries_.size(), std::move(result)); - - return absl::OkStatus(); + absl::StatusOr result = DoEvaluate(frame); + if (!result.ok()) { + frame->Abort(std::move(result).status()); + return; + } + frame->value_stack().PopAndPush(entries_.size(), *std::move(result)); } class DirectCreateStructStep : public DirectExpressionStep { diff --git a/eval/eval/equality_steps_test.cc b/eval/eval/equality_steps_test.cc index 9e90dc931..76031e169 100644 --- a/eval/eval/equality_steps_test.cc +++ b/eval/eval/equality_steps_test.cc @@ -64,9 +64,8 @@ class ValueStep : public ExpressionStepLogic, public DirectExpressionStep { value_(std::move(value)), attr_() {} - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { frame->value_stack().Push(value_, attr_); - return absl::OkStatus(); } absl::Status Evaluate(ExecutionFrameBase& frame, Value& result, diff --git a/eval/eval/evaluator_core.cc b/eval/eval/evaluator_core.cc index d4be67ba8..77f0bb30f 100644 --- a/eval/eval/evaluator_core.cc +++ b/eval/eval/evaluator_core.cc @@ -146,8 +146,7 @@ void EvaluateTernaryJumpStep(const TernaryJumpStepInfo& step, ExecutionFrame& frame) { ABSL_DCHECK(step.set) << "TernaryJumpStep did not have a value set."; if (!frame.value_stack().HasEnough(1)) { - frame.Abort( - absl::Status(absl::StatusCode::kInternal, "Value stack underflow")); + frame.Abort(absl::InternalError("TernaryJumpStep: value stack underflow")); return; } const cel::Value& condition = frame.value_stack().Peek(); @@ -216,13 +215,9 @@ void EvaluateMutableListAppendStep(ExecutionFrame& frame) { void ExpressionStep::Evaluate(ExecutionFrame* context) const { switch (header_.kind) { - case ExpressionStepKind::kGenericLogic: { - EvaluationStatus s(u_.logic->Evaluate(context)); - if (!s.ok()) { - context->Abort(std::move(s).Consume()); - } + case ExpressionStepKind::kGenericLogic: + u_.logic->Evaluate(context); break; - } case ExpressionStepKind::kIntConstant: context->value_stack().Push(cel::IntValue(u_.int_val)); break; @@ -512,12 +507,15 @@ FixedJumpStepInfo* GetIfFixedJumpStep(ExpressionStep& step) { return nullptr; } -absl::Status WrappedDirectStep::Evaluate(ExecutionFrame* frame) const { +void WrappedDirectStep::Evaluate(ExecutionFrame* frame) const { cel::Value result; AttributeTrail attribute_trail; - CEL_RETURN_IF_ERROR(impl_->Evaluate(*frame, result, attribute_trail)); + if (absl::Status status = impl_->Evaluate(*frame, result, attribute_trail); + !status.ok()) { + frame->Abort(std::move(status)); + return; + } frame->value_stack().Push(std::move(result), std::move(attribute_trail)); - return absl::OkStatus(); } } // namespace google::api::expr::runtime diff --git a/eval/eval/evaluator_core.h b/eval/eval/evaluator_core.h index 79b573b8e..8e71fcd83 100644 --- a/eval/eval/evaluator_core.h +++ b/eval/eval/evaluator_core.h @@ -398,7 +398,7 @@ class WrappedDirectStep : public ExpressionStepLogic { int64_t expr_id = -1) : impl_(std::move(impl)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; cel::NativeTypeId GetNativeTypeId() const override { return cel::NativeTypeId::For(); diff --git a/eval/eval/evaluator_core_test.cc b/eval/eval/evaluator_core_test.cc index dde70857e..c67dc8b58 100644 --- a/eval/eval/evaluator_core_test.cc +++ b/eval/eval/evaluator_core_test.cc @@ -46,9 +46,8 @@ class FakeConstExpressionStep : public ExpressionStepLogic { public: FakeConstExpressionStep() = default; - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { frame->value_stack().Push(CreateIntValue(0)); - return absl::OkStatus(); } }; @@ -58,13 +57,12 @@ class FakeIncrementExpressionStep : public ExpressionStepLogic { public: FakeIncrementExpressionStep() = default; - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { auto value = frame->value_stack().Peek(); frame->value_stack().Pop(1); EXPECT_TRUE(value->Is()); int64_t val = value.GetInt().NativeValue(); frame->value_stack().Push(CreateIntValue(val + 1)); - return absl::OkStatus(); } }; diff --git a/eval/eval/expression_step_logic.h b/eval/eval/expression_step_logic.h index 2d256636f..2b07af527 100644 --- a/eval/eval/expression_step_logic.h +++ b/eval/eval/expression_step_logic.h @@ -15,7 +15,6 @@ #ifndef THIRD_PARTY_CEL_CPP_EVAL_EVAL_EXPRESSION_STEP_LOGIC_H_ #define THIRD_PARTY_CEL_CPP_EVAL_EVAL_EXPRESSION_STEP_LOGIC_H_ -#include "absl/status/status.h" #include "common/native_type.h" namespace google::api::expr::runtime { @@ -35,7 +34,9 @@ class ExpressionStepLogic { // interface. // ExpressionStep instances can in specific cases // modify execution order(perform jumps). - virtual absl::Status Evaluate(ExecutionFrame* context) const = 0; + // + // Unrecoverable errors are reported via ExecutionFrame::Abort. + virtual void Evaluate(ExecutionFrame* frame) const = 0; // Return the type of the underlying expression step for special handling in // the planning phase. This should only be overridden by special cases, and diff --git a/eval/eval/ident_step.cc b/eval/eval/ident_step.cc index eee511a7e..e3985d183 100644 --- a/eval/eval/ident_step.cc +++ b/eval/eval/ident_step.cc @@ -32,7 +32,7 @@ class IdentStep : public ExpressionStepBase { public: explicit IdentStep(absl::string_view name) : name_(name) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: std::string name_; @@ -74,15 +74,17 @@ absl::Status LookupIdent(absl::string_view name, ExecutionFrameBase& frame, return absl::OkStatus(); } -absl::Status IdentStep::Evaluate(ExecutionFrame* frame) const { +void IdentStep::Evaluate(ExecutionFrame* frame) const { Value value; AttributeTrail attribute; - CEL_RETURN_IF_ERROR(LookupIdent(name_, *frame, value, attribute)); + if (absl::Status status = LookupIdent(name_, *frame, value, attribute); + !status.ok()) { + frame->Abort(std::move(status)); + return; + } frame->value_stack().Push(std::move(value), std::move(attribute)); - - return absl::OkStatus(); } absl::StatusOr LookupSlot( diff --git a/eval/eval/optional_or_step.cc b/eval/eval/optional_or_step.cc index 551025a7e..5ac1e8400 100644 --- a/eval/eval/optional_or_step.cc +++ b/eval/eval/optional_or_step.cc @@ -64,7 +64,7 @@ class OptionalOrStep : public ExpressionStepBase { explicit OptionalOrStep(OptionalOrKind kind) : ExpressionStepBase(), kind_(kind) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: const OptionalOrKind kind_; @@ -113,9 +113,10 @@ absl::Status EvalOptionalOr(OptionalOrKind kind, const Value& lhs, return absl::OkStatus(); } -absl::Status OptionalOrStep::Evaluate(ExecutionFrame* frame) const { +void OptionalOrStep::Evaluate(ExecutionFrame* frame) const { if (!frame->value_stack().HasEnough(2)) { - return absl::InternalError("Value stack underflow"); + frame->Abort(absl::InternalError("Value stack underflow")); + return; } absl::Span args = frame->value_stack().GetSpan(2); @@ -124,12 +125,15 @@ absl::Status OptionalOrStep::Evaluate(ExecutionFrame* frame) const { Value result; AttributeTrail result_attr; - CEL_RETURN_IF_ERROR(EvalOptionalOr(kind_, args[0], args[1], args_attr[0], - args_attr[1], result, result_attr, - frame->arena())); + if (absl::Status status = + EvalOptionalOr(kind_, args[0], args[1], args_attr[0], args_attr[1], + result, result_attr, frame->arena()); + !status.ok()) { + frame->Abort(std::move(status)); + return; + } frame->value_stack().PopAndPush(2, std::move(result), std::move(result_attr)); - return absl::OkStatus(); } class ExhaustiveDirectOptionalOrStep : public DirectExpressionStep { @@ -225,9 +229,10 @@ absl::Status DirectOptionalOrStep::Evaluate(ExecutionFrameBase& frame, } // namespace -absl::Status OptionalHasValueJumpStep::Evaluate(ExecutionFrame* frame) const { +void OptionalHasValueJumpStep::Evaluate(ExecutionFrame* frame) const { if (!frame->value_stack().HasEnough(1)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); + frame->Abort(absl::InternalError("Value stack underflow")); + return; } const Value& value = frame->value_stack().Peek(); cel::optional_ref optional_value = @@ -244,11 +249,11 @@ absl::Status OptionalHasValueJumpStep::Evaluate(ExecutionFrame* frame) const { frame->value_stack().PopAndPush(optional_value->Value()); } if (!jump_offset_.has_value()) { - return absl::Status(absl::StatusCode::kInternal, "Jump offset not set"); + frame->Abort(absl::InternalError("Jump offset not set")); + return; } - return frame->JumpTo(*jump_offset_); + frame->JumpToOrAbort(*jump_offset_); } - return absl::OkStatus(); } std::unique_ptr CreateOptionalHasValueJumpStep( diff --git a/eval/eval/optional_or_step.h b/eval/eval/optional_or_step.h index 1812c8090..f54057fa9 100644 --- a/eval/eval/optional_or_step.h +++ b/eval/eval/optional_or_step.h @@ -33,7 +33,7 @@ class OptionalHasValueJumpStep final : public ExpressionStepBase { void set_jump_offset(int offset) { jump_offset_ = offset; } - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: const bool is_or_value_; diff --git a/eval/eval/regex_match_step.cc b/eval/eval/regex_match_step.cc index abc301225..d13e4f651 100644 --- a/eval/eval/regex_match_step.cc +++ b/eval/eval/regex_match_step.cc @@ -62,23 +62,22 @@ class RegexMatchStep final : public ExpressionStepBase { explicit RegexMatchStep(std::shared_ptr re2) : ExpressionStepBase(), re2_(std::move(re2)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { if (!frame->value_stack().HasEnough(kNumRegexMatchArguments)) { - return absl::Status(absl::StatusCode::kInternal, - "Insufficient arguments supplied for regular " - "expression match"); + frame->Abort(absl::InternalError( + "Insufficient arguments supplied for regular expression match")); + return; } auto input_args = frame->value_stack().GetSpan(kNumRegexMatchArguments); const auto& subject = input_args[kRegexMatchStepSubject]; if (!subject->Is()) { - return absl::Status(absl::StatusCode::kInternal, - "First argument for regular " - "expression match must be a string"); + frame->Abort(absl::InternalError( + "First argument for regular expression match must be a string")); + return; } bool match = subject.GetString().NativeValue(MatchesVisitor{*re2_}); frame->value_stack().Pop(kNumRegexMatchArguments); frame->value_stack().Push(cel::BoolValue(match)); - return absl::OkStatus(); } private: diff --git a/eval/eval/select_step.cc b/eval/eval/select_step.cc index 528f12d0f..1d6c337d6 100644 --- a/eval/eval/select_step.cc +++ b/eval/eval/select_step.cc @@ -215,7 +215,7 @@ class SelectStep : public ExpressionStepBase { test_field_presence_(test_field_presence), enable_optional_types_(enable_optional_types) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; protected: std::string field_; @@ -224,10 +224,11 @@ class SelectStep : public ExpressionStepBase { bool enable_optional_types_; }; -absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { +void SelectStep::Evaluate(ExecutionFrame* frame) const { if (!frame->value_stack().HasEnough(1)) { - return absl::Status(absl::StatusCode::kInternal, - "No arguments supplied for Select-type expression"); + frame->Abort(absl::InternalError( + "No arguments supplied for Select-type expression")); + return; } const Value& arg = frame->value_stack().Peek(); @@ -235,7 +236,7 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { if (arg.IsUnknown() || arg.IsError()) { // Bubble up unknowns and errors. - return absl::OkStatus(); + return; } AttributeTrail result_trail; @@ -255,7 +256,7 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { frame->value_stack().PopAndPush( cel::ErrorValue::From(InvalidSelectTargetError(), frame->arena()), std::move(result_trail)); - return absl::OkStatus(); + return; } absl::optional marked_attribute_check = @@ -263,7 +264,7 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { if (marked_attribute_check.has_value()) { frame->value_stack().PopAndPush(std::move(marked_attribute_check).value(), std::move(result_trail)); - return absl::OkStatus(); + return; } Value result; @@ -273,28 +274,32 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { if (!optional_arg->HasValue()) { frame->value_stack().PopAndPush(cel::BoolValue{false}, std::move(result_trail)); - return absl::OkStatus(); + return; } optional_arg->Value(&result); target = &result; } - CEL_RETURN_IF_ERROR( - PerformHas(*target, field_, cel::StringValue::WrapUnsafe(field_), - frame->descriptor_pool(), frame->message_factory(), - frame->arena(), result)); + if (absl::Status status = + PerformHas(*target, field_, cel::StringValue::WrapUnsafe(field_), + frame->descriptor_pool(), frame->message_factory(), + frame->arena(), result); + !status.ok()) { + frame->Abort(std::move(status)); + return; + } frame->value_stack().PopAndPush(std::move(result), std::move(result_trail)); - return absl::OkStatus(); + return; } if (optional_arg) { if (!optional_arg->HasValue()) { frame->value_stack().PopAndPush(OptionalValue::None(), std::move(result_trail)); - return absl::OkStatus(); + return; } Value value; optional_arg->Value(&value); - auto status = PerformOptionalGet( + absl::Status status = PerformOptionalGet( value, field_, cel::StringValue::WrapUnsafe(field_), unboxing_option_, frame->descriptor_pool(), frame->message_factory(), frame->arena(), frame->options().enable_use_new_field_select_implementation, result); @@ -302,15 +307,18 @@ absl::Status SelectStep::Evaluate(ExecutionFrame* frame) const { result = ErrorValue::From(std::move(status), frame->arena()); } frame->value_stack().PopAndPush(std::move(result), std::move(result_trail)); - return absl::OkStatus(); + return; } - CEL_RETURN_IF_ERROR(PerformGet( - arg, field_, cel::StringValue::WrapUnsafe(field_), unboxing_option_, - frame->descriptor_pool(), frame->message_factory(), frame->arena(), - frame->options().enable_use_new_field_select_implementation, result)); + if (absl::Status status = PerformGet( + arg, field_, cel::StringValue::WrapUnsafe(field_), unboxing_option_, + frame->descriptor_pool(), frame->message_factory(), frame->arena(), + frame->options().enable_use_new_field_select_implementation, result); + !status.ok()) { + frame->Abort(std::move(status)); + return; + } frame->value_stack().PopAndPush(std::move(result), std::move(result_trail)); - return absl::OkStatus(); } class DirectSelectStep : public DirectExpressionStep { @@ -473,10 +481,11 @@ class ProtoSelectStep : public SelectStep { ABSL_DCHECK(field_descriptor_ != nullptr); } - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { if (!frame->value_stack().HasEnough(1)) { - return absl::InternalError( - "No arguments supplied for Select-type expression"); + frame->Abort(absl::InternalError( + "No arguments supplied for Select-type expression")); + return; } const Value& arg = frame->value_stack().Peek(); @@ -484,7 +493,8 @@ class ProtoSelectStep : public SelectStep { unwrapped.has_value() && SupportsCachedFieldDescriptor(*unwrapped, descriptor_, field_descriptor_)) { - return EvaluateMessageFieldGet(frame, *unwrapped); + EvaluateMessageFieldGet(frame, *unwrapped); + return; } else if (const google::protobuf::Message* legacy_message = cel::interop_internal::GetLegacyMessage(arg); frame->options().enable_use_new_field_select_implementation && @@ -494,19 +504,20 @@ class ProtoSelectStep : public SelectStep { // can minimize back and forth interop conversions. if (SupportsCachedFieldDescriptor(parsed_message, descriptor_, field_descriptor_)) { - return EvaluateMessageFieldGet(frame, legacy_message); + EvaluateMessageFieldGet(frame, legacy_message); + return; } } // If we get an unexpected value type, fall back to the generic // implementation. - return SelectStep::Evaluate(frame); + SelectStep::Evaluate(frame); } private: - absl::Status EvaluateMessageFieldGet( + void EvaluateMessageFieldGet( ExecutionFrame* frame, const cel::ParsedMessageValue& parsed_message) const; - absl::Status EvaluateMessageFieldGet( + void EvaluateMessageFieldGet( ExecutionFrame* frame, const google::protobuf::Message* absl_nonnull legacy_message) const; @@ -514,28 +525,35 @@ class ProtoSelectStep : public SelectStep { const google::protobuf::FieldDescriptor* field_descriptor_; }; -absl::Status ProtoSelectStep::EvaluateMessageFieldGet( +void ProtoSelectStep::EvaluateMessageFieldGet( ExecutionFrame* frame, const cel::ParsedMessageValue& parsed_message) const { if (CheckAttributeTrail(field_, frame)) { - return absl::OkStatus(); + return; + } + if (absl::Status status = parsed_message.GetField( + field_descriptor_, unboxing_option_, frame->descriptor_pool(), + frame->message_factory(), frame->arena(), + &frame->value_stack().Peek()); + !status.ok()) { + frame->Abort(std::move(status)); } - return parsed_message.GetField( - field_descriptor_, unboxing_option_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &frame->value_stack().Peek()); } -absl::Status ProtoSelectStep::EvaluateMessageFieldGet( +void ProtoSelectStep::EvaluateMessageFieldGet( ExecutionFrame* frame, const google::protobuf::Message* absl_nonnull legacy_message) const { ABSL_DCHECK(legacy_message != nullptr); if (CheckAttributeTrail(field_, frame)) { - return absl::OkStatus(); + return; + } + if (absl::Status status = cel::interop_internal::WrapLegacyMessageField( + legacy_message, field_descriptor_, unboxing_option_, + frame->descriptor_pool(), frame->message_factory(), frame->arena(), + &frame->value_stack().Peek()); + !status.ok()) { + frame->Abort(std::move(status)); } - return cel::interop_internal::WrapLegacyMessageField( - legacy_message, field_descriptor_, unboxing_option_, - frame->descriptor_pool(), frame->message_factory(), frame->arena(), - &frame->value_stack().Peek()); } class ProtoHasStep : public SelectStep { @@ -551,10 +569,11 @@ class ProtoHasStep : public SelectStep { ABSL_DCHECK(field_descriptor_ != nullptr); } - absl::Status Evaluate(ExecutionFrame* frame) const override { + void Evaluate(ExecutionFrame* frame) const override { if (!frame->value_stack().HasEnough(1)) { - return absl::InternalError( - "No arguments supplied for Select-type expression"); + frame->Abort(absl::InternalError( + "No arguments supplied for Select-type expression")); + return; } const Value& arg = frame->value_stack().Peek(); @@ -562,7 +581,8 @@ class ProtoHasStep : public SelectStep { unwrapped.has_value() && SupportsCachedFieldDescriptor(*unwrapped, descriptor_, field_descriptor_)) { - return EvaluateHas(frame, *unwrapped); + EvaluateHas(frame, *unwrapped); + return; } else if (const google::protobuf::Message* legacy_message = cel::interop_internal::GetLegacyMessage(arg); legacy_message != nullptr) { @@ -570,31 +590,31 @@ class ProtoHasStep : public SelectStep { cel::UnsafeParsedMessageValue(legacy_message); if (SupportsCachedFieldDescriptor(parsed_message, descriptor_, field_descriptor_)) { - return EvaluateHas(frame, parsed_message); + EvaluateHas(frame, parsed_message); + return; } } // If we get an unexpected value type, fall back to the generic // implementation. - return SelectStep::Evaluate(frame); + SelectStep::Evaluate(frame); } private: - absl::Status EvaluateHas(ExecutionFrame* frame, - const cel::ParsedMessageValue& parsed_message) const; + void EvaluateHas(ExecutionFrame* frame, + const cel::ParsedMessageValue& parsed_message) const; const google::protobuf::Descriptor* descriptor_; const google::protobuf::FieldDescriptor* field_descriptor_; }; -absl::Status ProtoHasStep::EvaluateHas( +void ProtoHasStep::EvaluateHas( ExecutionFrame* frame, const cel::ParsedMessageValue& parsed_message) const { if (CheckAttributeTrail(field_, frame)) { - return absl::OkStatus(); + return; } frame->value_stack().Peek() = BoolValue{parsed_message.HasField(field_descriptor_)}; - return absl::OkStatus(); } } // namespace diff --git a/eval/eval/shadowable_value_step.cc b/eval/eval/shadowable_value_step.cc index 0b75f7cc2..b6199ed7e 100644 --- a/eval/eval/shadowable_value_step.cc +++ b/eval/eval/shadowable_value_step.cc @@ -29,25 +29,27 @@ class ShadowableValueStep : public ExpressionStepBase { identifier_(std::move(identifier)), value_(std::move(value)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: std::string identifier_; Value value_; }; -absl::Status ShadowableValueStep::Evaluate(ExecutionFrame* frame) const { +void ShadowableValueStep::Evaluate(ExecutionFrame* frame) const { cel::Value result; - CEL_ASSIGN_OR_RETURN(auto found, - frame->modern_activation().FindVariable( - identifier_, frame->descriptor_pool(), - frame->message_factory(), frame->arena(), &result)); - if (found) { + absl::StatusOr found = frame->modern_activation().FindVariable( + identifier_, frame->descriptor_pool(), frame->message_factory(), + frame->arena(), &result); + if (!found.ok()) { + frame->Abort(std::move(found).status()); + return; + } + if (*found) { frame->value_stack().Push(std::move(result)); } else { frame->value_stack().Push(value_); } - return absl::OkStatus(); } class DirectShadowableValueStep : public DirectExpressionStep { diff --git a/eval/eval/ternary_step.cc b/eval/eval/ternary_step.cc index 97b064d22..0924b915f 100644 --- a/eval/eval/ternary_step.cc +++ b/eval/eval/ternary_step.cc @@ -128,13 +128,14 @@ class TernaryStep : public ExpressionStepBase { // Constructs FunctionStep that uses overloads specified. TernaryStep() : ExpressionStepBase() {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; }; -absl::Status TernaryStep::Evaluate(ExecutionFrame* frame) const { +void TernaryStep::Evaluate(ExecutionFrame* frame) const { // Must have 3 or more values on the stack. if (!frame->value_stack().HasEnough(3)) { - return absl::Status(absl::StatusCode::kInternal, "Value stack underflow"); + frame->Abort(absl::InternalError("Value stack underflow")); + return; } // Create Span object that contains input arguments to the function. @@ -148,13 +149,13 @@ absl::Status TernaryStep::Evaluate(ExecutionFrame* frame) const { // Check if unknown? if (condition.IsUnknown()) { frame->value_stack().Pop(2); - return absl::OkStatus(); + return; } } if (condition.IsError()) { frame->value_stack().Pop(2); - return absl::OkStatus(); + return; } cel::Value result; @@ -168,8 +169,6 @@ absl::Status TernaryStep::Evaluate(ExecutionFrame* frame) const { } frame->value_stack().PopAndPush(args.size(), std::move(result)); - - return absl::OkStatus(); } } // namespace diff --git a/extensions/select_optimization.cc b/extensions/select_optimization.cc index 60d414de3..7bca6414c 100644 --- a/extensions/select_optimization.cc +++ b/extensions/select_optimization.cc @@ -755,7 +755,7 @@ class StackMachineImpl : public ExpressionStepBase { StackMachineImpl(int expr_id, OptimizedSelectImpl impl) : ExpressionStepBase(expr_id), impl_(std::move(impl)) {} - absl::Status Evaluate(ExecutionFrame* frame) const override; + void Evaluate(ExecutionFrame* frame) const override; private: // Get the effective attribute for the optimized select expression. @@ -772,7 +772,7 @@ AttributeTrail StackMachineImpl::GetAttributeTrail( return impl_.GetAttributeTrail(attr); } -absl::Status StackMachineImpl::Evaluate(ExecutionFrame* frame) const { +void StackMachineImpl::Evaluate(ExecutionFrame* frame) const { // Default empty. AttributeTrail attribute_trail; // TODO(uncreated-issue/51): add support for variable qualifiers and string literal @@ -784,7 +784,7 @@ absl::Status StackMachineImpl::Evaluate(ExecutionFrame* frame) const { if (operand->Is() || operand->Is()) { // Just forward the error which is already top of stack. - return absl::OkStatus(); + return; } if (frame->enable_attribute_tracking()) { @@ -793,27 +793,34 @@ absl::Status StackMachineImpl::Evaluate(ExecutionFrame* frame) const { // select arguments. // TODO(uncreated-issue/51): add support variable qualifiers attribute_trail = GetAttributeTrail(frame); - CEL_ASSIGN_OR_RETURN(std::optional value, - CheckForMarkedAttributes(*frame, attribute_trail)); - if (value.has_value()) { + absl::StatusOr> value = + CheckForMarkedAttributes(*frame, attribute_trail); + if (!value.ok()) { + frame->Abort(std::move(value).status()); + return; + } + if (value->has_value()) { frame->value_stack().Pop(kStackInputs); - frame->value_stack().Push(std::move(value).value(), + frame->value_stack().Push(std::move(*value).value(), std::move(attribute_trail)); - return absl::OkStatus(); + return; } } if (!operand->Is()) { - return absl::InvalidArgumentError( - "Expected struct type for select optimization."); + frame->Abort(absl::InvalidArgumentError( + "Expected struct type for select optimization.")); + return; } - CEL_ASSIGN_OR_RETURN(Value result, - impl_.ApplySelect(*frame, operand.GetStruct())); + absl::StatusOr result = impl_.ApplySelect(*frame, operand.GetStruct()); + if (!result.ok()) { + frame->Abort(std::move(result).status()); + return; + } frame->value_stack().Pop(kStackInputs); - frame->value_stack().Push(std::move(result), std::move(attribute_trail)); - return absl::OkStatus(); + frame->value_stack().Push(*std::move(result), std::move(attribute_trail)); } class RecursiveImpl : public DirectExpressionStep {