From 512d2a8f26de765915faa2c0dac97e881757bdca Mon Sep 17 00:00:00 2001 From: Araq Date: Sun, 30 Aug 2026 08:54:46 +0200 Subject: [PATCH] =?UTF-8?q?IC:=20revert=20the=20`(ht=20.=20)`=20lazy-?= =?UTF-8?q?type=20pin=20=E2=80=94=20it=20broke=20sem?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit made `(ht . )` — a sym node the writer gave an EXPLICITLY nil type — load back with `nfLazyType`, so `ast.typ` answered `sym.typ` instead of nil. The stated reason was to remove a load-order dependence, and the direction was wrong: that nil is load-bearing. `writeSymNode` only emits the wrapper when the node's own type DIFFERED from its symbol's, so a nil there says the node genuinely had no type while the symbol had one. A type symbol used as a VALUE is exactly that shape: `newException(KeyError, ...)` passes a typedesc, whose node carries no type while the symbol carries the object type. Handing it `sym.typ` makes sem read the typedesc as an expression of the type it denotes, and `--ic:on` compilation of anything instantiating `tables.[]` dies with "only a 'ref object' can be raised". A four-line program is enough: import std/tables var t = initTable[string, int]() t["a"] = 1 echo t["a"] The load-order dependence is real but is not fixed by pinning the flag EITHER way — setting it breaks sem as above, clearing it would strip the fallback from the not-yet-loaded-stub population that `nifcBackendActive` exists to serve. Left alone deliberately, with the reasoning recorded at the site. `bnode.typ` answers the faithful nil, and the grinder excludes this one shape via `hasExplicitNilType` — narrowly, only when the cursor says nil and the AST is saying exactly the symbol's type. Why the suite did not catch it: `tests/ic` passed 39/39 throughout. The same four-line program reproduces from the scratchpad and from the repo root, and PASSES under `tests/ic` — `--skipParentCfg --skipProjCfg` makes it fail there too, so `tests/config.nims` is what masks it, most plausibly because evaluating a NimScript config runs the VM and perturbs the very load order the bug depends on. A test file under `tests/` therefore cannot guard this class, and no test is added rather than one that passes on the buggy compiler. Also in this commit, and the reason the bug was found at all: * `effectsOf` / `raisesNothing` replace the raw subscripting of `fn.typ.n` in `canRaiseImpl`, so the effect-list layout is written down in one place and the templates carry no knowledge of it. `raisesNothing` is stated as the NEGATIVE on purpose — the safe default is "can raise", so the one narrow shape that licenses dropping an exception check is the one spelled out, and an unanticipated shape falls conservative by construction. * `-d:icCanRaiseLog` logs every `canRaiseDisp` verdict keyed by name, disamb and OWNING MODULE, with the deciding branch. What "the canRaise helpers work on a `.bif`" means is that the type the decoder materialises carries the same effect list the from-source one did — a claim about the WRITER that the BNode/PNode grinder structurally cannot make, since both spellings ask the same `PType` and agree however wrong it is. The only oracle is the same program built without IC: 234 callees comparable, 0 disagreeing, 23 of them reaching the effect-list branch in both builds. Two instrumentation bugs worth recording, because both produced confident wrong numbers first: keying by name+disamb alone collided (`len.0` names a different routine per module) and reported one false disagreement; and the branch marker was a global that `canRaiseDisp` left stale on its early return, which inflated effect-list coverage from 23 to a claimed 142. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XEF7FJvUkGKvG9LSGuEaNR --- compiler/ast.nim | 70 ++++++++++++++++++++++++++++++++++++++----- compiler/ast2nif.nim | 29 ++++++++++-------- compiler/bnode.nim | 45 ++++++++++++++++++++-------- compiler/ccgcalls.nim | 15 ++++++++++ compiler/cgen.nim | 28 ++++++++++++++++- 5 files changed, 153 insertions(+), 34 deletions(-) diff --git a/compiler/ast.nim b/compiler/ast.nim index b6cef5c6ed..7476d7025c 100644 --- a/compiler/ast.nim +++ b/compiler/ast.nim @@ -1735,15 +1735,64 @@ const magicsThatCanRaise* = { # templates and `bnode.nim` instantiates them for its own node type: one source # of truth, no import cycle, and no second copy to keep in sync. # -# `fn.typ.n` below is a *type's* formal-params node, not a routine body: it is -# always fully materialised, so indexing it is not the hazard that indexing a -# body node is. +# The effect list is reached through `effectsOf` / `raisesNothing` rather than +# by subscripting `fn.typ.n`, so the templates below contain no knowledge of the +# layout and the `BNode` instantiation inherits none. `fn.typ` stays a `PType` +# in both spellings -- there is deliberately no `BType` (see `bnode.nim`) -- so +# what "works on a `.bif`" means for these two is that the type the decoder +# materialises must carry the same effect list the from-source one did. That is +# a claim about the WRITER, not about the vocabulary, and it is checked +# separately: `-d:icCanRaiseLog` logs every answer, and the same program built +# with and without `--ic:on` must produce the same verdicts. + +when defined(icCanRaiseLog): + var canRaiseBranch* = 0 + ## Which branch decided the last answer: 1 = the symbol's magic/flags, + ## 2 = `mEcho`, 3 = the EFFECT LIST reached through `effectsOf`, 4 = the + ## conservative predicate, 5 = short-circuited in `canRaiseDisp` before + ## either predicate ran, 0 = fell through. Only branch 3 reads anything + ## that had to survive a `.bif` round trip, so a differential in which no + ## callee reaches it would prove nothing about the writer — which is the + ## whole point of running the differential. See `-d:icCanRaiseLog`. + +template markCanRaiseBranch*(n: int) = + when defined(icCanRaiseLog): canRaiseBranch = n template canRaiseConservativeImpl*(fnArg: typed): bool = block: let fn = fnArg + markCanRaiseBranch 4 not (fn.kind == nkSym and fn.sym.magic notin magicsThatCanRaise) +proc effectsOf*(t: PType): PNode {.inline.} = + ## The `nkEffectList` a proc type carries as child 0 of its formal-params + ## node, with the parameters following from index 1 (`newProcType` builds it + ## that way; `cgen` reads the params back with `sonsFrom(prc.typ.n, 1)`). + ## + ## Named rather than subscripted so that the layout is written down in ONE + ## place. `.n` here is a TYPE's node, never a routine body, so it is always + ## fully materialised and `firstSon` is safe — the `nfLazyBody` hazard that + ## makes raw child access dangerous elsewhere (see `astdef.sons`) cannot reach + ## it. A proc type always has this child; `t.n` with no children is not a + ## shape the writer or sem produces, and this deliberately does not paper over + ## one appearing. + result = if t.n == nil: nil else: t.n.firstSon + +proc raisesNothing*(effects: PNode): bool = + ## Whether an effect list says DEFINITIVELY that nothing is raised: it is long + ## enough to have a raises slot at all, the slot is present, and it is empty. + ## + ## Every other shape — a list too short to carry the slot, an absent slot, a + ## non-empty one — means the effects are unspecified or non-empty, and a + ## caller must assume a raise. Stating it as the NEGATIVE is the point: the + ## safe default has to be "can raise", so the one narrow case that licenses + ## dropping an exception check is the one spelled out here, and a shape nobody + ## anticipated falls on the conservative side by construction rather than by + ## luck. + result = effects != nil and effects.len >= effectListLen and + effects[exceptionEffects] != nil and + effects[exceptionEffects].safeLen == 0 + template canRaiseImpl*(fnArg: typed): bool = block: let fn = fnArg @@ -1751,22 +1800,27 @@ template canRaiseImpl*(fnArg: typed): bool = if fn.kind == nkSym and (fn.sym.magic notin magicsThatCanRaise or {sfImportc, sfInfixCall} * fn.sym.flags == {sfImportc} or sfGeneratedOp in fn.sym.flags): + markCanRaiseBranch 1 res = false elif fn.kind == nkSym and fn.sym.magic == mEcho: + markCanRaiseBranch 2 res = true elif fn.typ != nil and fn.typ.kind == tyProc and fn.typ.n != nil: - # TODO check for n having sons? or just return false for now if not - if fn.typ.n[0].kind == nkSym: + markCanRaiseBranch 3 + let effects = effectsOf(fn.typ) + if effects.kind == nkSym: + # The historical shape: slot 0 used to be an `nkType` before the effects + # moved in (see `newProcType`). Nothing to read, so nothing licenses a + # raise. res = false else: # A proc-typed value with no explicit raises slot still has # unspecified effects, which sempass2 treats conservatively. # Codegen needs to do the same in order to keep goto-exception # checks after indirect/closure calls. - res = ((fn.typ.n[0].len < effectListLen) or - fn.typ.n[0][exceptionEffects] == nil or - fn.typ.n[0][exceptionEffects].safeLen > 0) + res = not raisesNothing(effects) else: + markCanRaiseBranch 0 res = false res diff --git a/compiler/ast2nif.nim b/compiler/ast2nif.nim index 3183b37bc2..dae74b2c87 100644 --- a/compiler/ast2nif.nim +++ b/compiler/ast2nif.nim @@ -3280,18 +3280,23 @@ proc loadNode(c: var DecodeContext; n: var Cursor; thisModule: string; s = c.loadSymStub(n, thisModule, localSyms) result = newSymNode(s, info) result.typField = typ - if typ == nil: - # `(ht . )` — an EXPLICITLY nil node type. Without this the - # node's type is LOAD-ORDER DEPENDENT: `newSymNode` above marks the - # node lazy only when the symbol was still an unloaded stub at this - # moment, so the very same `.bif` node answers `sym.typ` or `nil` - # for `n.typ` depending on whether something else happened to touch - # that symbol first. Pin it to the lazy reading — the one `newSymNode` - # exists to provide (see `nifcBackendActive` in astdef: snapshotting - # a nil leaves the node permanently typeless and the backend then - # reads `t.flags` off it) — so the answer is a property of the file, - # not of the traversal order. - result.flags.incl nfLazyType + # `(ht . )` — an EXPLICITLY nil node type — is left exactly as the + # writer meant it: NIL. The wrapper is only emitted when the node's own + # type differed from its symbol's (`writeSymNode`), so a nil here says + # the node genuinely had no type while the symbol had one, and that is + # load-bearing: a type symbol used as a VALUE (`newException(KeyError, + # ...)`) is exactly that shape, and handing it `sym.typ` makes sem read + # the typedesc as an expression of the type it denotes ("only a 'ref + # object' can be raised"). + # + # There IS a load-order dependence here — `newSymNode` above marks the + # node lazy when the symbol was still an unloaded stub, so `ast.typ` + # answers `sym.typ` for that population and `nil` for the rest — and it + # is NOT fixed by pinning the flag either way: setting it breaks sem as + # above, and clearing it would strip the fallback from the stub + # population that `nifcBackendActive` exists to serve. Left alone + # deliberately; `bnode.typ` answers the faithful `nil` and the grinder + # excludes this one shape with the reason recorded there. elif tagIs(n, symDefTagName): let info = c.infos.oldLineInfo(n.info, cursorPool(n)) let name = n.firstSon diff --git a/compiler/bnode.nim b/compiler/bnode.nim index 1521412324..36a5fc7dc6 100644 --- a/compiler/bnode.nim +++ b/compiler/bnode.nim @@ -460,19 +460,20 @@ when defined(newIcBackend): # `(ht )`: the node type is spelled out because it differed # from the symbol's at write time. result = typeAt(currentNav()[], childCursor(c)) - if result == nil: - # `(ht . )` — an EXPLICITLY nil node type, which the writer tries - # hard not to emit but does for a sym whose own type was nil. The - # loader does not read it as nil either: `newSymNode(sym, info)` runs - # first and sets `nfLazyType` whenever the symbol was still an - # unloaded stub (`typImpl == nil`), and only then is `typField` - # overwritten with this nil — so `ast.typ` falls back to `sym.typ`. - # Mirror that. (The AST's answer is strictly speaking load-order - # dependent — a sym already loaded at that moment would leave - # `nfLazyType` clear and yield nil — which is a fragility of the - # loader, not of this mirror. The grinder walks every body in the - # dependency closure and this is the branch it lands on.) - result = symTyp(n) + # A nil here is `(ht . )`, an EXPLICITLY nil node type, and it is + # answered as nil — the writer only emits the wrapper when the node's + # type differed from its symbol's, so nil means the node really had + # none. Do NOT fall back to `sym.typ`: a type symbol used as a value + # (`newException(KeyError, ...)`) is exactly this shape, and giving it + # the symbol's type makes sem read the typedesc as an expression of the + # type it denotes. That was tried, and it broke `--ic:on` compilation of + # anything instantiating `tables.[]`. + # + # `ast.typ` may still answer `sym.typ` here, because the loader's + # `nfLazyType` marking depends on whether the symbol happened to be + # loaded already (see `ast2nif`). That is a pre-existing load-order + # dependence in the AST, not a disagreement this side can resolve, and + # the grinder excludes this shape for that reason. elif name == symNodeFlagsTagName: var inner = childCursor(c) skip inner @@ -488,6 +489,24 @@ when defined(newIcBackend): else: result = nil + proc hasExplicitNilType*(n: BNode): bool = + ## Whether this is the `(ht . )` shape — a sym node the writer gave an + ## EXPLICITLY nil type — after peeling any `(nflags ...)` wrapper, which is + ## how it usually arrives. See `typ` for why nil is the faithful answer and + ## why `ast.typ` may nonetheless say otherwise. + var c = n.raw + while nifcore.kind(c) == TagLit: + let tag = c.tags.tagName(cursorTagId(c)) + if tag == symNodeFlagsTagName: + var inner = childCursor(c) + skip inner # the node flags + c = inner + elif tag == hiddenTypeTagName: + return nifcore.kind(childCursor(c)) == DotToken + else: + return false + result = false + proc rawDesc*(n: BNode): string = ## What the token stream literally says here — the NIF token kind and, for a ## tag, its name. Diagnostics only: the vocabulary above is the interface, diff --git a/compiler/ccgcalls.nim b/compiler/ccgcalls.nim index 24e0cab2ee..28819f607c 100644 --- a/compiler/ccgcalls.nim +++ b/compiler/ccgcalls.nim @@ -11,6 +11,12 @@ proc canRaiseDisp(p: BProc; n: AnyNode): bool = # we assume things like sysFatal cannot raise themselves + # 5 = "decided here, neither predicate ran". Without resetting, the marker + # keeps whatever the PREVIOUS call left in it and the early return below + # attributes this answer to a branch that did not execute — which is how the + # first run of this differential came to claim effect-list coverage it did + # not have. + markCanRaiseBranch 5 if n.kind == nkSym and {sfNeverRaises, sfImportc, sfCompilerProc} * n.sym.flags != {}: result = false elif optPanics in p.config.globalOptions or @@ -21,6 +27,15 @@ proc canRaiseDisp(p: BProc; n: AnyNode): bool = else: # we have to be *very* conservative: result = canRaiseConservative(n) + when defined(icCanRaiseLog): + # `canRaise` reads the raises spec off `fn.typ.n`, and under `--ic:on` that + # node came back from a `.bif`. Whether it came back INTACT is not something + # the `BNode`/`PNode` grinder can answer — both spellings ask the same + # `PType` and so agree however wrong it is. The only oracle is the same + # program built without IC. Log the verdict per callee; the two builds must + # produce the same one. + if n.kind == nkSym: + logCanRaise(n.sym, result) proc preventNrvo(p: BProc; dest, le, ri: PNode): bool = proc locationEscapes(p: BProc; le: PNode; inTryStmt: bool): bool = diff --git a/compiler/cgen.nim b/compiler/cgen.nim index f8463de4d8..5f2c5a6ffc 100644 --- a/compiler/cgen.nim +++ b/compiler/cgen.nim @@ -1053,6 +1053,22 @@ proc initLocExprSingleUse(p: BProc, e: PNode): TLoc = result.flags.incl lfSingleUse expr(p, e, result) +when defined(icCanRaiseLog): + import std / syncio + + proc logCanRaise(s: PSym; verdict: bool) = + ## One line per verdict, keyed by name + disamb + OWNING MODULE, and carrying + ## the magic that usually decides the answer. + ## + ## The module is not decoration: `disamb` is a per-module counter, so `len.0` + ## names a different routine in every module that has one, and a key without + ## the module reports a collision as a disagreement. NOT the itemId — that is + ## a per-build counter and would make every line differ for no reason. + let m = getModule(s) + stderr.writeLine "CANRAISE " & s.name.s & "." & $s.disamb & "." & + (if m == nil: "?" else: m.name.s) & "|" & $verdict & "|" & $s.magic & + "|b" & $canRaiseBranch + include ccgcalls, "ccgstmts.nim" proc initFrame(p: BProc, procname, filename: Rope): Rope = @@ -1593,7 +1609,17 @@ when defined(newIcBackend): let ct = c.typ let at = a.typ - if (ct == nil) != (at == nil): + # `(ht . )` is the one shape where the two spellings may legitimately + # differ: the cursor answers the faithful `nil`, while `ast.typ` answers + # `sym.typ` for whichever nodes the loader happened to mark `nfLazyType` + # (see `bnode.typ`). Excluded rather than papered over — and narrowly: only + # when the cursor says nil AND the AST is saying exactly the symbol's type. + let htNilTyp = ct == nil and at != nil and a.kind == nkSym and + a.typField == nil and a.sym != nil and at == a.sym.typ and + c.hasExplicitNilType + if htNilTyp: + discard + elif (ct == nil) != (at == nil): bail("typ nil-ness", (if ct == nil: "nil" else: $ct.kind) & " raw=" & c.rawDesc, (if at == nil: "nil" else: $at.kind) & " kind=" & $a.kind &