Skip to content

Commit 2dfc0f9

Browse files
committed
Apply the review batch: unify the report with codegen, record verdicts and bugs
All four reviews cleared the borrow-elision flip (soundness by residual equivalence per surface, adversarial with no reproduced miscompile, efficiency at about 3.5 percent of compile time with a do-not-optimize list, simplification with applied findings). Record the verdicts in RC_ELISION. The report drift the simplification review confirmed is fixed at the root: analyze_rc_elision now consumes the same elidable_moves and parameter modes codegen uses, so the report counts what is actually emitted and elided. The old report claimed 78 movable on the http server where codegen elided 35, and the truthful numbers are 35 moves plus 46 borrowed call positions needing no retain at all. The unification also exposed a real refinement item, recorded: a function-local variable carries a return-path drop plus an unreachable fall-through drop, so the single-drop guard blocks every function-local move today, and moves fire only for temporaries and top-level bindings. Also applied: the stranded loop_ranges and explain doc comments moved to their functions, dependency edges deduplicated in a set, the shared borrowed_param_ids helper replacing the lower_program closure and main's special case, HashSet imported once, the report built from one function list, and the move tests rewritten to pin true elision behavior (top-level shapes with single drops, write_str instead of the print desugar whose str call is itself an Owned position). Record Bug 15 (a function referencing a top-level binding segfaults, pre-existing) and Bug 16 (explicit finalizer call runs the body twice, a semantics decision) in KNOWN_BUGS.
1 parent a5be148 commit 2dfc0f9

5 files changed

Lines changed: 198 additions & 128 deletions

File tree

plans/INLINING.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,3 +55,8 @@ transform, so nothing here bets on the interpreter.
5555
aligned view stays honest across splices.
5656
- Unrolling scope: small constant trip counts and hot parse loops first, driven by the profiling
5757
counters rather than guessed.
58+
- Re-measure the body-size-quadratic analyses when inlining lands. The elision analyses and the
59+
pre-existing `loop_depths` scale in function-body size, which inlining multiplies. The efficiency
60+
review measured them harmless at today's sizes and recorded fix directions, with `loop_depths`
61+
about ten times costlier than the new code on the same shapes, so it trips first. The do-not-do
62+
verdicts on those quadratics expire when bodies grow past tens of thousands of instructions.

plans/KNOWN_BUGS.md

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,39 @@ declared without a return-type annotation (its inferred return is indeterminate)
7171
loose typing the dedicated paths had, narrowed here rather than fully closed. Found by the arc 1a
7272
adversarial review, reconfirmed as the residual channel by the arc 1b and Bug 12 reviews.
7373

74+
## Bug 15 (OPEN, serious): a function referencing a top-level `let` variable segfaults
75+
76+
A top-level binding used inside a function compiles without diagnostics and then faults at runtime:
77+
78+
let g: List[Int] = list()
79+
def add(x: Int) -> Int:
80+
push(g, x)
81+
return len(g)
82+
let n = add(5)
83+
84+
runs to `VM error: SegmentationFault`. Top-level `let` bindings live in main's frame, a function
85+
body runs in its own frame, and nothing bridges the two, so the function reads a register that
86+
holds unrelated data. Either module-level bindings should be real globals reachable from functions
87+
(the global-cell machinery the prelude scheduler uses exists but is not wired to user bindings), or
88+
capturing a top-level binding in a function should be a descriptive compile error until they are.
89+
Silent wrong-frame access is the worst of the options. Found pre-existing by the borrow-elision
90+
adversarial review (identical on builds with and without elision).
91+
92+
## Bug 16 (OPEN, decision): calling a `Delete` finalizer directly runs its body twice
93+
94+
`r.delete()` called explicitly runs the finalizer body at the call, and the runtime runs it again
95+
when the reference count reaches zero. Before borrow elision the explicit call instead leaked the
96+
object silently (the caller's argument retain was never balanced, so the count never reached zero
97+
and the runtime finalize never happened), which was also wrong. Borrow elision balanced the books
98+
(the finalizer's `self` classifies Borrowed, the caller keeps ownership) and the object is now
99+
correctly freed, exposing the double-run. No memory unsafety either way, the second run executes on
100+
a still-valid object before the free, but a resource finalizer running twice is a hazard (a double
101+
close). Decision needed: forbid calling a finalizer directly (a descriptive compile error, the
102+
lifecycle hook is the runtime's to invoke), or mark the object finalized at the explicit call so
103+
the runtime skips its run, or pin the double-run as documented behavior. The compile-error option
104+
is the cheapest reliable answer. Found by the borrow-elision soundness review, which traced both
105+
builds' counts.
106+
74107
## Bug 14 (FIXED): `continue` and `break` skipped drops of references created in the loop body
75108

76109
A `continue` inside a `while` jumped to the next iteration without releasing reference-typed values

plans/RC_ELISION.md

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,4 +187,26 @@ assumption, and the melding is three layers, from safest to most optimized:
187187
at the owner's scope end rather than at the consuming call, the documented borrow timing.
188188
3. **Refine.** Alias-aware escape inside callees, a task-entry wrapper so spawn targets can borrow,
189189
dominance-based move elision, return-value moves, and the orphan-temp direct free that attacks
190-
the per-request allocation count.
190+
the per-request allocation count. Added by the report unification below: a variable local to a
191+
function with a return carries two drops (the return-path drop plus the unreachable fall-through
192+
drop), so the single-drop guard blocks every function-local move today, and moves fire only for
193+
temporaries and top-level bindings. The dominance refinement, or dead fall-through drop
194+
elimination, unlocks the function-local class.
195+
196+
## Step 2 review outcomes
197+
198+
Four parallel reviews cleared the flip. The soundness review proved residual equivalence per
199+
consumer surface against the pre-flip build (including a borrowed value surviving a mid-call
200+
green-thread suspension) and traced the one behavioral divergence: an explicit `r.delete()` call,
201+
where the old build silently leaked and the new build frees correctly but runs the finalizer body
202+
twice, recorded as the Bug 16 decision in KNOWN_BUGS. The adversarial review reproduced no
203+
miscompile across escape laundering, payload extraction, spawn tricks, deep chains, and split
204+
generic instantiations, and found pre-existing Bug 15 (a function referencing a top-level binding
205+
segfaults). The efficiency review measured the analyses at about 3.5 percent of compile time with
206+
an explicit do-not-optimize list, whose quadratic entries expire if inlining grows body sizes past
207+
tens of thousands of instructions (INLINING.md). The simplification review's findings were applied:
208+
the report now consumes the same `elidable_moves` and parameter modes codegen uses, so it counts
209+
what is actually emitted and elided and cannot drift (the drift it caught was real, the old report
210+
claimed 78 movable where codegen elided 35, and the truthful http numbers are 35 moves plus 46
211+
borrowed call positions), plus the stranded doc comments, the deduplicated dependency edges, and
212+
the shared `borrowed_param_ids` helper.

0 commit comments

Comments
 (0)