Skip to content

Free discarded Option and Result payloads#8

Merged
loreste merged 1 commit into
loreste:mainfrom
attahn:fix/discarded-bag-payload-drops
Jul 19, 2026
Merged

Free discarded Option and Result payloads#8
loreste merged 1 commit into
loreste:mainfrom
attahn:fix/discarded-bag-payload-drops

Conversation

@attahn

@attahn attahn commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Fresh Option and Result values now release their owned payloads when explicitly discarded. Cleanup covers strings, slices, maps, structs, nested bags, enum errors, and float results while preserving borrowed field and index values.

Focused ASan and LSan tests cover both owned cleanup and borrowed-container safety.

Tested with cargo test --bin mako (78 passed) and the focused ownership sanitizer suite.

Summary by CodeRabbit

  • New Features

    • Explicitly discarding Option and Result values now cleans up owned payloads, including nested arrays, maps, strings, and records.
    • Borrowed payloads remain intact so their containing values stay valid.
    • Debug builds warn when a discarded payload’s type cannot be resolved for cleanup.
  • Documentation

    • Added guidance for explicit discard syntax and updated soundness and roadmap documentation.
  • Tests

    • Added coverage for owned payload cleanup, borrowed values, nested structures, and leak-detection scenarios.

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Discarded bag cleanup

Layer / File(s) Summary
Source type metadata
src/codegen/mod.rs
Codegen records source-level function return and struct field types for type-directed cleanup.
Ownership-aware discard emission
src/codegen/mod.rs
Ignored Option/Result values now emit recursive cleanup for owned payloads while preserving fallback discard behavior for other values.
Regression coverage
examples/testing/*, .github/workflows/ci.yml, src/codegen/mod.rs
Tests cover owned payload shapes, borrowed bags, and an error payload free; ASan runs execute both scenarios.
Documentation updates
CHANGELOG.md, docs/*
Documentation describes explicit discard behavior, ownership handling, soundness status, and remaining type-resolution scope.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant IgnoredStatement
  participant Codegen
  participant GeneratedC
  IgnoredStatement->>Codegen: emit discarded Option/Result expression
  Codegen->>Codegen: infer payload type and ownership
  Codegen->>GeneratedC: emit recursive drop/free cleanup
  GeneratedC-->>IgnoredStatement: complete discard without fallback expression use
Loading

Possibly related PRs

  • loreste/mako#6: Both changes update ASan configuration in the CI workflow.

Suggested reviewers: loreste

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: freeing discarded Option and Result payloads.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@attahn

attahn commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)

293-296: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Restrict default GITHUB_TOKEN permissions.

The asan job uses default permissions. Following the principle of least privilege, explicitly set read-only permissions to improve workflow security.

🔒️ Proposed fix to explicitly set job permissions
   asan:
     name: ASan invalid memory access
     runs-on: ubuntu-latest
+    permissions:
+      contents: read
     steps:
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ci.yml around lines 293 - 296, Update the asan job
definition to explicitly configure its GITHUB_TOKEN permissions as read-only,
using the workflow’s permissions syntax. Keep the existing job steps and
behavior unchanged.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 293-296: Update the asan job definition to explicitly configure
its GITHUB_TOKEN permissions as read-only, using the workflow’s permissions
syntax. Keep the existing job steps and behavior unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c62c92a9-ae62-4314-b4d4-bba6b648ea20

📥 Commits

Reviewing files that changed from the base of the PR and between 97d1e49 and 5ea0f50.

📒 Files selected for processing (9)
  • .github/workflows/ci.yml
  • CHANGELOG.md
  • docs/ROADMAP.md
  • docs/SOUNDNESS.md
  • docs/STATUS.md
  • docs/howto/06-memory.md
  • examples/testing/discarded_bag_borrow_test.mko
  • examples/testing/discarded_bag_drop_test.mko
  • src/codegen/mod.rs

@attahn

attahn commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

CI is green except for a macOS timing failure in TestJoinTimeoutFlatten. The same test passed in this run under ASan, UBSan, TSan, GCC, claims, Ubuntu, and Windows, and passed on the upstream base run. I left the unrelated test unchanged; rerunning the failed job requires repository permission.

@attahn
attahn marked this pull request as ready for review July 19, 2026 22:57
@loreste
loreste merged commit 335893c into loreste:main Jul 19, 2026
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants