IC: revert the (ht . <sym>) lazy-type pin — it broke sem

The previous commit made `(ht . <sym>)` — 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XEF7FJvUkGKvG9LSGuEaNR
This commit is contained in:
Araq
2026-08-30 08:54:46 +02:00
parent 5799220c98
commit 512d2a8f26
5 changed files with 153 additions and 34 deletions

View File

@@ -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

View File

@@ -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 . <sym>)` — 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 . <sym>)` — 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

View File

@@ -460,19 +460,20 @@ when defined(newIcBackend):
# `(ht <type> <sym>)`: 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 . <sym>)` — 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 . <sym>)`, 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 . <sym>)` 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,

View File

@@ -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 =

View File

@@ -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 . <sym>)` 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 &