Repository navigation
A def's body goes to --verdict through its lowered form once, so its annotations move with its binders under η, rebuild and let inlining (#1665, #1669) - #1666
Conversation
…ers, so --verdict checks what bend2 checks (bendlang#1665)
Lorenzobattistela
left a comment
There was a problem hiding this comment.
Thanks for this. The diagnosis in #1665 is clear, and the test covers all four η sites. I merged this onto main 8f86f73 and ran the gates on the mini cluster: repo 54/54, test 1658/1658, perf 124/124, ping 62/62. The safe gate shows 1079 agree vs 1078 on main (the +1 is eta_levels), with 0 false rejects. The new test fails --verdict on main and checks here, and each of its four defs does the same on its own. I'm requesting changes for the points below.
- The same level mismatch is still reachable outside η. When a let-bound λ is passed to a self-call, safe.ts copies it in at the call site at a different depth. Its annotations still name n at the let's depth, and there that level is g itself, so --verdict loops:
import Base
type Fact<-P: Type> is Type:
Fact{proof: P}
def refl(+n: Nat) -> Fact<{n == n : Nat}>:
Fact{{==}}
def walk(c: Nat, keeps: @+n: Nat -> {n == n : Nat}) -> Nat:
match c:
case Zero{}:
0n
case Succ{p}:
g : @+n: Nat -> {n == n : Nat} = n => {\{Fact: q => q} : Fact<{n == n : Nat}> -> {n == n : Nat}}(refl(n))
walk(p, g)
def main() -> Nat:
walk(3n, n => {==})It runs and prints 0n, but bend let_inline.bend --verdict fails with "Error: the machine stack overflowed" on both main and this PR. This isn't a regression, but it is the same bug class, and the PR doesn't cover it. Storing subst(x.v[j], s.d) where let_term keeps the value makes it check.
- A possible fix at the source, offered as an idea; I haven't looked into it much.
arm()turns each def body back into closures, while the annotations still name variables by the level they were checked at. Templates already go throughsubst(t0, 0, …)at that point, which may be why only non-template defs break. I tried sending every body through thatsubst, withetaleft as on main:
let t = subst(t0, 0, (o) => o.$ === "Ref" && ps !== null ? ps.get(o.k as Name) ?? undefined : undefined);With that change, eta_levels and the let probe above both check, and the --verdict sweep over tests/ matches main. safe.ts comes to 23,983 ttok. It might also cover rebuild (#1669 / #1670), but I haven't checked that. The costs I saw are about 8% more CPU over the suite and a different .bendtt for about 8 tests, which all still verify. There could be cases I'm missing, so please treat this as something to evaluate, not a drop-in replacement.
-
etanow lowers and re-raises the whole term on every η step. A leaf of an n-ary function type is expanded n times, each over a growing App spine, which makes it O(n·|t|). Fordef big() -> Fn(): h([0n, …, 1999n]), whereFn()is a 400-ary function type,-otakes 18.2s here vs 1.4s on main (200-ary: 4.2s vs 1.4s), with identical output. The extra recursion also lowers the stack limit: a 3800-element literal under an η-expanded leaf now overflows, while main still emits it (both fail at 4000). -
Minor:
eta's newdmust be the depth bend2 checkedtat, not the depth it is used at, but nothing says so, and the PR removes the only comment explaining why ("bend2's annotations name a variable by its level"). A short comment onetawould help. Relatedly,closed_val'sfnow readss.c[o.i]for everyVar, which is only correct whiles.c.length === s.d. The old range guard made that explicit.
…annotations move with its binders under η, rebuild and let inlining (bendlang#1665, bendlang#1669)
|
Thanks for the review and for the idea in 2. I took it, with one change, in bafce16 (main 64230dc merged in). The PR is now one line in
|
Fixes #1665. Fixes #1669. This follows @Lorenzobattistela's suggestion in review: fix the levels once, where
armreads a def's body, instead of at each step that moves a term. It supersedes #1670, whose test is here.Cause
bend2 wraps each node it checks in its type, as a cell (
InferandCheckinbend.ts).armraises a def's body withterm_higher, which keeps a cell as it is. So a binder in the body becomes a closure, but an annotation still names it by the level bend2 checked it at. Any step that puts part of the body under binders bend2 didn't check it under leaves those annotations at the old levels:etaputs a term under a new λ (--verdictmismatch: an η-expanded argument keeps old binder levels in its inner annotations (BoolwhereNatis expected) #1665).rebuildputs a λ's body under a binder per field (--verdictmismatch: a λ rebuilt as a match keeps its variable's old level in its annotations (NatwherePis expected) #1669).let_termbuilds a let's value at the self-call it is passed to, at the call's depth (the case from the review).A template's body already goes through
subst(t0, 0, …), which lowers and raises it. The issue's term in a def with a~parameter checks on main.Fix
armlowers and raises every other def's body once,term_higher(term_lower(t0)). Lowering forces each cell, so an annotation names the body's binders as the body's own uses do, and raising binds both.eta,rebuildandlet_termare as on main.The review's line sends every body through
subst. The output is the same, butsubst's copy recurses deeper per node than lowering does. With it,-ooverflows the stack on a def whose body is a 3,800-element list literal, with or without η. Without the copy, the limit is main's. Here 4,000 elements pass and 4,500 overflow, on both, and a chain of 4,000 lets passes on both with the same-o. The exact limit depends on the machine's stack.Checks
With main 64230dc merged in, Linux x86_64, kernel built from main's
bendtt.leanwith Lean 4.34.0:eta_levels(the four η sites),rebuild_levels(--verdictmismatch: a λ rebuilt as a match keeps its variable's old level in its annotations (NatwherePis expected) #1669: a type with two constructors, one with no fields, and a λ inside the body) andlet_levels(the review's let) each fail--verdicton main and check here. Each prints its#|value when run and from the JS and C builds, and--checkupexits 0. Lowering at depth 1 instead of 0 fails all three.--verdictgives the same exit code and output as main: 1,137 check, 623 fail.-odiffers in 15 files, all through three Base defs (String.cmp.group,Map.get.leaf,Str.split.fin). There, a variable that main passes through a match's convoy is now a copied binder,λ+x. Each of the three defs verifies in the 9 of those files that check. The other 6 fail on@unsafebefore the kernel, as on main.-ois 5 to 9% slower ondefs_12800, 10 to 15% ongenerics_3200, and 14 to 21% onproofs_3200(3.2 s on main, 3.8 s here).--verdictonproofs_3200goes from 4.0 s to 4.7 s. Most of it is the lowering itself, which forces the cell at each node and lowers its type: 0.71 s of the 1.94 ssafe_booktakes onproofs_3200. Withsubston every body,-otakes 1.8, 2.6, and 6.5 s on those three. Plain runs and--check-onlydon't change, so the perf gate's checker benches don't either. Over the 1,760-file sweep, total CPU goes up 1 to 2%.-otakes 2.7 s, the same as main, with the same output.bun gates/repo.ts: 54 / 54.safe.tsgoes from 23,992 to 24,000 ttok, at its cap.tsc: 11 errors, the same as main.Limits
safe.tsis at its cap, so this adds no comment atarm. The three test headers say why the body is lowered.A fix at each site instead (η in one pass per leaf, plus
rebuildandlet_term) would keep--verdictnear main's cost. The first version of this PR fixed only η that way and was within 3% of main on these benches. With all three sites, it doesn't fit under the cap today.