From fe39ad5fba3ffe9ecf93c3d9aeac32854eaaf52d Mon Sep 17 00:00:00 2001 From: Araq Date: Sun, 30 Aug 2026 10:41:05 +0200 Subject: [PATCH] =?UTF-8?q?IC:=20`sym`=20is=20not=20a=20function=20of=20it?= =?UTF-8?q?s=20argument=20for=20fields=20=E2=80=94=20find=20out,=20then=20?= =?UTF-8?q?say=20so?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The goal was `aliases.isPartOf`, the deepest thing on the migration list: mutual recursion over two trees at once, reaching `sym`, `typ`, `intVal` and `isDeepConstExpr`. It does not migrate, and the reason is worth more than the migration would have been. `bnode.sym` IS NOT IDEMPOTENT for object fields. Two calls on the SAME token yield two different `skField` `PSym`s with consecutive item ids — field uses bypass the nav's memo and go to `loadFieldStub`, which mints per use because two distinct fields can share a name AND a position across types, so one shared stub would mistype one of them. `isPartOf` compares `a[1].sym.id != b[1].sym.id` to decide whether two accessor chains touch the same field, so on a cursor it answers `arNo` where the AST answers `arYes`: wrong alias analysis feeding NRVO and observable-store decisions. The grinder caught it on the first run after the migration, on a self-comparison `isPartOf(n, n)` — a shape production never passes, which is exactly why it was worth grading. The property was then confirmed directly rather than inferred: call `sym` twice on one token and print both, and the ids differ. So `aliases.nim` is reverted, and what stays is the knowledge: * `bnode.sym` says which symbols it is stable for and which it is not, what codegen actually consumes for a field instead (the name it re-navigates the reclist with, plus the position for tuples — the same tolerance the grinder applies), and why this module cannot fix it alone: a stable field identity needs the token's own position as a key, and `nifcore.Cursor` keeps that pointer private. * The grinder asserts idempotence for every NON-field sym at every node. It passes, and admitting fields makes it fail immediately — so the check states a precise boundary rather than a vague warning, and the day fields join it will be visible. `getInt` moves into a template so `bnode` can instantiate it (the `canRaiseImpl` pattern), and `astalgo.sameValue` becomes `AnyNode` and is graded — both are literal-only and unaffected by the field problem. `preventNrvo` records the OTHER kind of blocker while it is fresh: its alias analysis is fine, but the `warnObservableStores` message renders the node, and rendering is a capability the seam does not have at all. Verified: grind clean (67_857 nodes, 0 disagreements); 215/215 byte-identical `.c` against HEAD on the default path; all four build configurations compile. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XEF7FJvUkGKvG9LSGuEaNR --- compiler/ast.nim | 34 ++++++++++++++++++++++------------ compiler/astalgo.nim | 4 ++-- compiler/bnode.nim | 23 +++++++++++++++++++++++ compiler/ccgcalls.nim | 7 +++++++ compiler/ccgexprs.nim | 2 +- compiler/cgen.nim | 16 ++++++++++++++++ 6 files changed, 71 insertions(+), 15 deletions(-) diff --git a/compiler/ast.nim b/compiler/ast.nim index 7476d7025c..d3c43dc638 100644 --- a/compiler/ast.nim +++ b/compiler/ast.nim @@ -1464,18 +1464,28 @@ proc hasSubnodeWith*(n: PNode, kind: TNodeKind): bool = return true result = false -proc getInt*(a: PNode): Int128 = - case a.kind - of nkCharLit, nkUIntLit..nkUInt64Lit: - result = toInt128(cast[uint64](a.intVal)) - of nkInt8Lit..nkInt64Lit: - result = toInt128(a.intVal) - of nkIntLit: - # XXX: enable this assert - # assert a.typ.kind notin {tyChar, tyUint..tyUInt64} - result = toInt128(a.intVal) - else: - raiseRecoverableError("cannot extract number from invalid AST node") +template getIntImpl*(aArg: typed): Int128 = + ## The body of `getInt`, in a form `bnode.nim` can instantiate for a `BNode` + ## too — same reason as `canRaiseImpl`: `BNode` is defined there and that + ## module imports this one, so the shared logic has to live in a template + ## rather than an `AnyNode` proc. There is no second copy. + block: + let a = aArg + var res: Int128 + case a.kind + of nkCharLit, nkUIntLit..nkUInt64Lit: + res = toInt128(cast[uint64](a.intVal)) + of nkInt8Lit..nkInt64Lit: + res = toInt128(a.intVal) + of nkIntLit: + # XXX: enable this assert + # assert a.typ.kind notin {tyChar, tyUint..tyUInt64} + res = toInt128(a.intVal) + else: + raiseRecoverableError("cannot extract number from invalid AST node") + res + +proc getInt*(a: PNode): Int128 = getIntImpl(a) proc getInt64*(a: PNode): int64 {.deprecated: "use getInt".} = case a.kind diff --git a/compiler/astalgo.nim b/compiler/astalgo.nim index ba4396a335..978c9e0a83 100644 --- a/compiler/astalgo.nim +++ b/compiler/astalgo.nim @@ -13,7 +13,7 @@ import ast, astyaml, options, lineinfos, idents, rodutils, - msgs + msgs, bnode import std/[hashes, intsets] import std/strutils except addf @@ -100,7 +100,7 @@ proc skipConvCastAndClosure*(n: PNode): PNode = result = result[1] else: break -proc sameValue*(a, b: PNode): bool = +proc sameValue*[T: AnyNode](a, b: T): bool = result = false case a.kind of nkCharLit..nkUInt64Lit: diff --git a/compiler/bnode.nim b/compiler/bnode.nim index 776dcf562e..c2842a4458 100644 --- a/compiler/bnode.nim +++ b/compiler/bnode.nim @@ -430,6 +430,27 @@ when defined(newIcBackend): ## (`(nflags )`, `(ht )`) are peeled by ## `bodynav.symToken`, beside the code that derives the lookup key from them, ## so the two cannot drift apart. + ## + ## NOT IDEMPOTENT FOR OBJECT FIELDS, and anything built on this accessor has + ## to know it. Two calls on the SAME token yield two different `skField` + ## `PSym`s with consecutive item ids: field uses deliberately bypass the + ## nav's memo and go to `loadFieldStub`, which mints per use because two + ## distinct fields can share a name AND a position across types, so one + ## shared stub would mistype one of them (see `bodynav`). For every other + ## symbol kind the answer is stable — the nav memoises it — and `cgen`'s + ## grinder asserts that for the non-field case at every node. + ## + ## The consequence is not theoretical. A proc that reads a field sym twice + ## and compares IDENTITY is correct on a `PNode` and wrong on a `Cursor`: + ## `aliases.isPartOf` does exactly that (`a[1].sym.id != b[1].sym.id`, to + ## decide whether two accessor chains touch the same field) and so CANNOT be + ## migrated as written. What codegen actually consumes for a field is the + ## name it re-navigates the reclist with (`lookupFieldAgain`) plus, for + ## tuples, the position — which is also the tolerance the grinder applies — + ## so the fix is either to compare fields that way or to give a field token + ## a stable identity. The latter needs the token's own position as a key, + ## and `nifcore.Cursor` keeps that pointer private, so it is not something + ## this module can do alone. result = symAt(currentNav()[], n.raw) proc symTyp(n: BNode): PType = @@ -610,6 +631,8 @@ when defined(newIcBackend): # the body in a template and these two instantiate it. There is no second # copy of the logic — change the template and both spellings change. + proc getInt*(n: BNode): Int128 = getIntImpl(n) + proc canRaiseConservative*(fn: BNode): bool = canRaiseConservativeImpl(fn) proc canRaise*(fn: BNode): bool = canRaiseImpl(fn) diff --git a/compiler/ccgcalls.nim b/compiler/ccgcalls.nim index 440314509f..882cb03e8e 100644 --- a/compiler/ccgcalls.nim +++ b/compiler/ccgcalls.nim @@ -38,6 +38,13 @@ proc canRaiseDisp(p: BProc; n: AnyNode): bool = logCanRaise(n.sym, result) proc preventNrvo(p: BProc; dest, le, ri: PNode): bool = + ## STAYS on `PNode`, and the reason is a capability the seam does not have + ## rather than an accessor it is missing: the `warnObservableStores` message + ## interpolates `$le`, i.e. it RENDERS the node. Rendering is `renderer.nim` + ## reconstructing source text, which is a different job from reading a node's + ## kind/sym/type, and nothing needs it until a diagnostic does. The alias + ## analysis this calls (`isPartOf`) is already `AnyNode`, so only the message + ## is in the way. proc locationEscapes(p: BProc; le: PNode; inTryStmt: bool): bool = result = false var n = le diff --git a/compiler/ccgexprs.nim b/compiler/ccgexprs.nim index 1db60d6025..dc400dbf55 100644 --- a/compiler/ccgexprs.nim +++ b/compiler/ccgexprs.nim @@ -1950,7 +1950,7 @@ proc genObjConstr(p: BProc, e: PNode, d: var TLoc) = proc lhsDoesAlias(a, b: PNode): bool = result = false - for y in b: + for y in sons(b): if isPartOf(a, y) != arNo: return true proc genSeqConstr(p: BProc, n: PNode, d: var TLoc) = diff --git a/compiler/cgen.nim b/compiler/cgen.nim index 45d035af5b..b707a774e2 100644 --- a/compiler/cgen.nim +++ b/compiler/cgen.nim @@ -1622,6 +1622,22 @@ when defined(newIcBackend): check "isDeepConstExpr", isDeepConstExpr(n) check "stmtsContainPragma", stmtsContainPragma(n, wLinearScanEnd) check "notYetAlive", notYetAlive(n) + check "getInt", (if n.kind in nkIntLits: $getInt(n) else: "") + check "sameValue self", sameValue(n, n) + + # `sym` IS NOT A FUNCTION OF ITS ARGUMENT for object fields, so this asserts + # the property the rest of the seam quietly assumes everywhere else. Two + # calls on the SAME token mint two `skField` stubs with consecutive item + # ids (`loadFieldStub`, by design: two distinct fields can share a name and + # a position across types, so one shared stub would mistype one of them). + # Anything that reads a field sym twice and compares identity is therefore + # wrong on a cursor and right on an AST — which is exactly how the attempt + # to migrate `aliases.isPartOf` failed, and it failed LOUDLY only because + # this grinder existed. Left as a live check so the day it starts holding + # is visible. + if a.kind == nkSym and a.sym != nil and a.sym.kind != skField: + if c.sym != c.sym: + bail("sym is not idempotent", "two different PSyms", "one PSym") # `stmtsContainPragma` had to be re-derived rather than defined as # `getPragmaStmt(...) != nil`, because a `Cursor` has no nil to return (see