Sitelet https://github.com/starkware-libs/cairo/pull/10007
Skip to content

fix(lowering): propagate destruct-impl error in closure lowering - #10007

Merged
orizi merged 1 commit into
mainfrom
orizi/05-29-fix_lowering_propagate_destruct-impl_error_in_closure_lowering
May 31, 2026
Merged

fix(lowering): propagate destruct-impl error in closure lowering#10007
orizi merged 1 commit into
mainfrom
orizi/05-29-fix_lowering_propagate_destruct-impl_error_in_closure_lowering

Conversation

@orizi

@orizi orizi commented May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

Propagates the error returned by add_capture_destruct_impl instead of silently discarding it. The result was previously bound to _ and ignored; it is now mapped to LoweringFlowError::Failed and propagated with ?.

Type of change

Please check one:

  • Bug fix (fixes incorrect behavior)
  • New feature
  • Performance improvement
  • Documentation change with concrete technical impact
  • Style, wording, formatting, or typo-only change

Why is this change needed?

add_capture_destruct_impl can return an error, but the previous code discarded it with let _ = .... This meant that failures during closure capture destructor registration were silently swallowed, potentially allowing lowering to continue in an invalid state.

What was the behavior or documentation before?

Errors from add_capture_destruct_impl were ignored entirely, so any failure in that call had no effect on the lowering pipeline.

What is the behavior or documentation after?

Errors from add_capture_destruct_impl are now mapped to LoweringFlowError::Failed and propagated via ?, ensuring that failures are correctly surfaced during closure lowering.

Related issue or discussion (if any)

Additional context

This is a correctness fix in the closure lowering path. The change is minimal but important for ensuring error handling is not accidentally bypassed.

@orizi
fix(lowering): propagate destruct-impl error in closure lowering
8c48467 
  lower_expr_closure discarded the Maybe<()> from add_capture_destruct_impl
  via `let _ =`, so a DB/inference failure (which already emits a diagnostic
  via `?` internally) was silently swallowed and lowering continued as if the
  capture's destruct impl had been generated. Propagate it with
  .map_err(LoweringFlowError::Failed)? to match every other add_* call site.

  Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@reviewable-StarkWare

reviewable-StarkWare commented May 29, 2026

Copy link
Copy Markdown

This change is Reviewable

@orizi Graphite App

orizi commented May 29, 2026

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

eytan-starkware
eytan-starkware approved these changes May 31, 2026
View reviewed changes

@eytan-starkware eytan-starkware left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@eytan-starkware reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on TomerStarkware).

@orizi
orizi marked this pull request as ready for review May 31, 2026 07:28
@orizi
orizi added this pull request to the merge queue May 31, 2026
@cursor

cursor Bot commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Summary

Low Risk
Small correctness fix in error handling only; no change to successful lowering behavior.

Overview
Closure lowering now treats failures from add_capture_destruct_impl like other setup steps in the same path: the result is no longer discarded with let _ = ..., and errors are mapped to LoweringFlowError::Failed and propagated with ?.

That registration step can fail while generating Drop/panic_destruct lowering for captured variables; previously those failures were ignored and lowering could continue without the expected generated destruct impl.

Reviewed by Cursor Bugbot for commit 8c48467. Bugbot is set up for automated code reviews on this repo. Configure here.

Merged via the queue into main with commit 5878bd8 May 31, 2026
54 checks passed
@orizi
orizi deleted the orizi/05-29-fix_lowering_propagate_destruct-impl_error_in_closure_lowering branch May 31, 2026 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eytan-starkware eytan-starkware eytan-starkware approved these changes
@TomerStarkware TomerStarkware Awaiting requested review from TomerStarkware

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@orizi @reviewable-StarkWare @eytan-starkware