Skip to content

fix(codegen): point the module-global Ptr<Shape> denial at #10803, not closed #7109 - #10804

Closed
proggeramlug wants to merge 2 commits into
mainfrom
fix/stale-denial-issue-pointer
Closed

proggeramlug wants to merge 2 commits into
mainfrom
fix/stale-denial-issue-pointer

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

The optimiser report's denial message for "a module-level binding can never take a canonical slot" cited #7109. That is a different mechanism, and it is closed.

MODULE_GLOBAL_ISSUE in crates/perry-codegen/src/expr/slot_rep.rs is the issue number surfaced to users when a module-level binding is denied a canonical slot. #7109 is the module-init / program-entry context gate — a distinct rule, which MODULE_INIT_CONTEXT a few lines below still cites correctly. #7109 is closed, and so is #10774, which lifted that gate.

So a reader who followed the denial's own pointer landed on a closed issue about something else, and could reasonably conclude the module-global class had already been handled.

That is not hypothetical. One optimisation pass recorded module-global storage as "less important" on exactly that reading. A separate campaign spent a day repeating "#7109 is the blocker" on inherited belief before checking the issue state. The pointer now goes to #10803Ptr<Shape> is denied to three storage classes (module globals, function parameters, and locals escaping into a module global) — with a comment recording why the old one was wrong, so the correction isn't silently reverted later.

Scope, deliberately narrow

Only the live user-facing pointer for the module-global storage class moved. Left unchanged:

Why the two were separable, demonstrated rather than asserted

the_context_gate_is_reported_when_every_value_rule_passed asserts issue == Some("#7109") for MODULE_INIT_CONTEXT. It passes unchanged after this edit — which is the evidence that the module-global pointer and the context-gate pointer were genuinely independent, rather than my having moved one and broken the other.

cargo test -p perry-codegen --lib slot_rep: 9 passed, 0 failed. cargo check -p perry-codegen clean.

Found during cross-session review while handing the module-global lane to another campaign; the three denial classes are that campaign's #10803.

Summary by CodeRabbit

  • Documentation
    • Corrected the tracking reference for module-global storage denial issues.
    • Clarified the distinction between module-global storage handling and module-initialization context handling.
    • Added changelog coverage for the corrected reference and related test coverage.

@coderabbitai

coderabbitai Bot commented Sep 20, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 8c1de582-4b72-4981-bf4f-c35472f04dcf

📥 Commits

Reviewing files that changed from the base of the PR and between b9ba951 and f2f5d2f.

📒 Files selected for processing (2)
  • changelog.d/10804-stale-denial-issue-pointer.md
  • crates/perry-codegen/src/expr/slot_rep.rs

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The module-global denial pointer changes from issue #7109 to #10803. Documentation explains that #7109 remains associated with the module-init context gate.

Changes

Issue Pointer Correction

Layer / File(s) Summary
Update module-global issue reference
crates/perry-codegen/src/expr/slot_rep.rs, changelog.d/10804-stale-denial-issue-pointer.md
MODULE_GLOBAL_ISSUE now references #10803. The documentation distinguishes it from the unchanged MODULE_INIT_CONTEXT reference to #7109.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix · Severity of issue fixed: Low

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: updating the module-global Ptr denial pointer from closed issue #7109 to #10803.
Description check ✅ Passed The description explains the change, scope, rationale, affected identifiers, related issues, and verification results. It does not use the repository template headings or checklist, but it provides th…
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 …
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.
✨ Finishing Touches 💡 1
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

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.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

e2e-scoped and lint are both pre-existing, not this branch.

e2e-scoped: every_dispatch_entry_has_manifest_counterpart (crates/perry-codegen/tests/manifest_consistency.rs:50) fails with "API_MANIFEST is missing 15 entry/entries that exist in NATIVE_MODULE_TABLE"net::prependListener, net::pipe, net::unpipe, net::writable/readable, net::_writableState/_readableState, http::httpVersionMajor/Minor, http::complete, http::rawHeaders and http::__get_rawHeaders. This branch changes exactly one string constant in perry-codegen/src/expr/slot_rep.rs and cannot add or remove NATIVE_MODULE_TABLE rows. The remedy the test itself names is adding the rows to crates/perry-api-manifest/src/entries.rs.

lint: the known-red "Public benchmark evidence freshness" step, left red deliberately.

Every other suite in e2e-scoped reports 0 failures; the job's exit 1 comes solely from that one manifest-drift test. Branch is level with main at b9ba951ff8, so this is not base drift either.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed via merge train 243 (#10838) as v0.5.1622ba303c1f5f.

This should make compiler-output-regression green for the first time in weeks. Its structural verdicts were already fixed as of v0.5.1618 (failed_workloads: []); the only thing left was the 2 × TimeoutExpired residual recorded when #10782 was first closed — the cold build moving from the probe's budget into the linking compile's.

The pre-warm found something my design missed: there are two cold builds, not one. The perry-auto-<hash> directory name excludes the cross-feature set while the build stamp inside it includes it, and loop_bound_semantics is the one gated workload with no *_traced budget — so it wants the other feature set, hits a stamp mismatch, and re-runs cargo. A single-warm step would have left exactly one gate to absorb a cold build, and the red would have looked unchanged.

Both liveness assertions are in and both are load-bearing: the presence check (a perry-auto-*/ with a build stamp and a non-empty archive) and the verification compile that must land under half the 300 s budget. The second is the one that actually discriminates — presence can pass on a stale directory.

A gate caught the new step, and that is the gate working. compiler_output_step_liveness.py pins the job's post-build step inventory and refused the addition. Registered in COMPILER_SUBJECTS rather than exempted, since the pre-warm depends on the build and already carries the matching guard; the gate now passes at 10 subjects and its own self-test still passes at 8 cases, which I checked separately because a registry edit that broke the checker would be worse than the drift it hides.

Worth stating why it has to be listed rather than exempted: the pre-warm carries its own assertions and can fail, so it is a subject. If it were ever silently removed, every gate below would quietly inherit the timeout it exists to prevent — and the failure would look exactly like the red it just fixed.

On provenance: this work came from an agent that died on an authentication error after committing but before pushing. I recovered the commit, rebased it twice as main moved beneath it, and pushed. Since it never reached lint, I verified the workflow change here instead — test.yml parses at 28 jobs, the pre-warm sits at step 10 with all seven gates at 11–17, and ci_plan.py --self-test passes.

#10804 rode along: six areas, 197 fixtures weighted to its Ptr<Shape> surface, zero unexplained regressions.

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.

1 participant