Skip to content

Refactor remaining generic steps to report terminal errors via Frame::Abort #2302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
copybara-service merged 1 commit into from
Oct 1, 2026
Merged

Refactor remaining generic steps to report terminal errors via Frame::Abort #2302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions eval/compiler/flat_expr_builder_extensions_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ExpressionStepLogic> MakeTestStepLogic() {
Expand Down
13 changes: 8 additions & 5 deletions eval/compiler/instrumentation.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
52 changes: 26 additions & 26 deletions eval/eval/comprehension_step.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,55 +428,55 @@ 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()) {
const AttributeTrail& top_attr = frame->value_stack().PeekAttribute();
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<cel::ValueIteratorPtr> 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 <iter_range> with an error and jump past
// ComprehensionFinishStep.
frame->value_stack().PopAndPush(cel::ErrorValue::From(
CreateNoMatchingOverloadError("<iter_range>"), 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 {
Expand Down
2 changes: 1 addition & 1 deletion eval/eval/comprehension_step.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down
3 changes: 1 addition & 2 deletions eval/eval/comprehension_step_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
};

Expand Down
12 changes: 5 additions & 7 deletions eval/eval/container_access_step.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down
18 changes: 10 additions & 8 deletions eval/eval/create_list_step.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class CreateListStep : public ExpressionStepBase {
CreateListStep(int list_size, absl::flat_hash_set<int> 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;
Expand All @@ -47,22 +47,24 @@ class CreateListStep : public ExpressionStepBase {
absl::flat_hash_set<int32_t> 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,
Expand Down
21 changes: 12 additions & 9 deletions eval/eval/create_map_step.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Value> DoEvaluate(ExecutionFrame* frame) const;
Expand Down Expand Up @@ -118,16 +118,20 @@ absl::StatusOr<Value> 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<Value> 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 {
Expand Down Expand Up @@ -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();
}
};

Expand Down
18 changes: 11 additions & 7 deletions eval/eval/create_struct_step.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Value> DoEvaluate(ExecutionFrame* frame) const;
Expand Down Expand Up @@ -131,14 +131,18 @@ absl::StatusOr<Value> 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<Value> 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 {
Expand Down
3 changes: 1 addition & 2 deletions eval/eval/equality_steps_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
20 changes: 9 additions & 11 deletions eval/eval/evaluator_core.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
2 changes: 1 addition & 1 deletion eval/eval/evaluator_core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<WrappedDirectStep>();
Expand Down
6 changes: 2 additions & 4 deletions eval/eval/evaluator_core_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
};

Expand All @@ -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<IntValue>());
int64_t val = value.GetInt().NativeValue();
frame->value_stack().Push(CreateIntValue(val + 1));
return absl::OkStatus();
}
};

Expand Down
Loading
Loading