Skip to content

fix: align mixin stab list with desugared IR order in IncludeD#6167

Open
Kamirus wants to merge 1 commit into
masterfrom
kamil/fix-include-stab-order
Open

fix: align mixin stab list with desugared IR order in IncludeD#6167
Kamirus wants to merge 1 commit into
masterfrom
kamil/fix-include-stab-order

Conversation

@Kamirus

@Kamirus Kamirus commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

build_stabs for IncludeD produced [letP_flex; imports...; mixin_decs...] while dec' emits [imports...; letP; mixin_decs...], so List.map2 stabilize stabs ds paired the letP stab with the first import dec and shifted each import stab onto the next dec.

Benign today (every stab in that segment is Some Flexible and stabilize is identity on Flexible), but a latent footgun the moment any of those stabs becomes anything other than Flexible.

Made with Cursor

`build_stabs` for `IncludeD` emitted `[letP_flex; imports...; mixin_decs...]`
while `dec'` emits `[imports...; letP; mixin_decs...]`, so the subsequent
`List.map2 stabilize stabs ds` paired the letP stab with the first import
dec (and shifted each import stab onto the next dec). Currently benign
because every stab in that segment is `Some Flexible` and `stabilize`
treats `Flexible` as identity, but it is a latent bug the moment any of
those stabs becomes anything else.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Kamirus Kamirus requested a review from a team as a code owner June 2, 2026 10:05
@Kamirus Kamirus requested a review from christoph-dfinity June 2, 2026 10:05
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Cursor AI review

👍 APPROVE — looks safe to merge

Category Assessment Details
Summary Reorders build_stabs for IncludeD so stab list matches dec' IR order [imports; letP; mixin decs] instead of [letP; imports; mixin decs].
Code Quality Minimal 4-line reorder plus an invariant comment; no new helpers.
Consistency Matches existing build_stabs / dec' split and OCaml list style in desugar.ml.
Correctness List.map2 stabilize stabs ds in build_actor now pairs each stab with the matching IR dec; mixin-dec stabs were already aligned at the tail.
Tests No .mo/.ok change needed: affected prefix stabs are hardcoded Flexible and stabilize is a no-op on them today; existing mixin run-drun tests still cover the path.
Changelog No user-visible behavior change; internal ordering fix only.

Verdict

Decision: APPROVE
Risk: Very Low
Reason: The diff only fixes a latent stab/dec misalignment in the IncludeD prefix where every stab is currently Flexible, so runtime behavior is unchanged while the invariant now matches dec'. No compiler-surface, codegen, or RTS impact warrants escalation.


Generated for commit a8c0420

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Comparing from e7c78d7 to a8c0420:
The produced WebAssembly code seems to be completely unchanged.
In terms of gas, no changes are observed in 5 tests.
In terms of size, no changes are observed in 5 tests.

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