fix(infer): return types were never checked — 13 false accepts - #18
Merged
Merged
Conversation
`inferModule'`'s `.dfn` arm honored every param annotation but discarded `retAnnot` entirely, so every return-position type error was a FALSE ACCEPT. Verified against march (`--check`, exit 1 = reject) for: `: String` over an `Int` body, `: ()` over a non-unit body, `: Float` over an integer literal, `: Int` over a `Float` literal, a record field of the wrong type, a nominal type alias (`type Age = Int` is NOT transparent to march), a body calling a correctly-typed function at the wrong return type, and the same shape inside a nested `mod`. All eight now agree. Cannot manufacture a false reject: `retAnnot` is `some` only when the annotation is fully in fragment (the decoder forces the whole decl to `Decl.unsupported` otherwise, and maps a surface type variable to `Ty.unsupported`), so `tyToMTy` sees only ground types and cannot throw. Confirmed empirically: 0 new rejects across all 122 `specs/**/accept/*.march` corpus files and all 124 march-accepted files in `examples/` + `stdlib/`. Three `#eval`s pin the reject and both near-miss accepts (annotation agrees; annotation absent).
… scrutinees Closes Finding I2. `matchExhaustive` declined on every non-ADT scrutinee: `Pattern.lit` is not an `isModeledArmPattern` shape, so the safety net short-circuited to "exhaustive" before the scrutinee type was consulted, and `Bool` (absent from `builtinCtors`) fell through to the unknown-type carve-out. Under `cap no_panic` that was a live FALSE ACCEPT, verified against march for all four shapes: `0 -> ..; 1 -> ..` on an `Int`, `"a" -> ..; "b" -> ..` on a `String`, `1.0 -> ..; 2.0 -> ..` on a `Float`, and `true -> ..` alone on a `Bool`. Faithful to `find_missing_mc` (typecheck.ml:4363-4381): past the `has_first_wild` test, an infinite domain reports missing unconditionally and `Bool` needs both literal rows. Neither decision consults the constructor universe or the or-expansion policy, so the general-conservatism rule (which governs exactly those two RECONSTRUCTED inputs) is not in play — the only input either branch depends on is the wildcard test this checker genuinely models (`isCatchAllPattern` <-> `norm_pat`'s `SPWild`). The `Bool` branch, which does read arm shapes, keeps its own safety net (`isBoolLitPattern`). Verified no near-miss regression: `_`/bare-var catch-alls, `true | false`, guarded-arm-plus-wildcard, and user-ADT coverage all still accept; 0 rejects across all 246 march-accepted files in specs/**/accept, examples/ and stdlib/. Ten fixtures pin both directions, each labelled with march's own verdict. Finding I1 (depth: `(true, true)` over a `(Bool, Bool)`) stays open — it needs the real pattern matrix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The oracle was never checking function return types
fn f(n : Int) : String do n end— march rejects it. We accepted it.inferModule''s.dfnarm honoured parameter annotations and discardedretAnnotentirely, so every return-position type error was invisible to the oracle. Independent type checking is milestone A1/A2's entire purpose, and half of function type-checking has been absent since then — through five milestones, 334 corpus files, and several reviews.Found by an adversarial hunt targeting
Infer.lean(1,525 lines) on the grounds that it had received far less scrutiny thanCapCheck.lean(4,168 lines) while judging the majority of non-capability files.~76 probes. 13 confirmed false accepts, ZERO false rejects. 12 fixed, 1 deliberately left.
Findings
fn f(n : Int) : String do n endString, gotIntfn f(n : Int) : () do n end(), gotIntfn f() : Float do 1 endFloat, gotIntfn f() : Int do 1.5 endInt, gotFloatString, gotInttype Age = Int; fn f(x:Int) : Age do x endfn g() : String do fact(3) endString, gotIntmodBool, gotIntno_panic:match n do 0; 1 end(Int)StringFloatmatch b do true -> 1 end(Bool)(true,true)over(Bool,Bool)1–8 are the one root cause. 9–12 are a second:
Pattern.litisn't a modeled shape, so the exhaustiveness safety net short-circuited before reading the scrutinee type, andBoolisn't inbuiltinCtors.Finding 6 is worth flagging on its own — march treats
type Age = Intnominally, so anIntis not anAge. Easy to assume structural.Why both fixes are safe
Neither depends on an input the checker reconstructs — the governing rule adopted after slice (c)'s four failed exhaustiveness rounds:
retAnnotissomeonly when fully in-fragment, sotyToMTycannot throw on it.find_missing_mcdecides infinite-domain scrutinees (Int/Float/String/Char/Atom) andBooloutright, consulting neither the constructor universe nor the or-expansion policy — the two reconstructed inputs that caused trouble before.Validated against all 246 march-accepted files in
specs/**/accept,examples/, andstdlib/: zero rejects. Plus explicit near-misses — catch-alls,k as m,true | false, guarded arms, and unit/generic/Capreturns. 13 fixtures added.Builtin signature audit
Every registered builtin checked against march's effective type (not just its table entry — the
printlnbug was a table entry that turned out to be dead code).printlnis the only prelude-shadowed name. Greppedstdlib/prelude.marchfor all 17 registered builtins.print,print_int/float,*_to_string,string_length/concat,++,%,+. -. *. /.,&& || not, Num/Ord/Eq operators,root_cap,cap_narrow), each probed at a wrong argument type — all 8 such probes agree.to_string,head,int_*,float_*,record_*) throwSKIP: unbound variable→ exit 2. Safe, but a real coverage hole.Conformance
Unchanged from before the fix — the corpus does not exercise a single one of these 13 shapes. Fourth time this pattern has held; the fixtures are the only coverage.
Left unfixed, deliberately
SKIP: unbound variable:inferModule'folds decls in order, so forward references never resolve. Safe, but it removes a common shape from coverage — including one where march rejects on a wrong return type. A pre-pass binding top-leveldfnnames to fresh metavars would close it.The pattern worth noting
retAnnotwas decoded, threaded intoDecl.dfn, scanned byCapCheck, and even had a hand-written#evalexercising the unify — and the one site that mattered dropped it behind a rationalising comment.That is the same shape as the
printlnbug (an authoritative-looking builtin table that was dead code) and theELetbug (a docstring asserting march "never producesELethere", false on two counts). Three defects, one mechanism: a plausible comment standing in for a check that wasn't there. Every remaining decoded-but-unconsumed field deserves an audit.