mirror of
https://github.com/nim-lang/Nim.git
synced 2026-09-01 19:33:42 +00:00
IC: sym is not a function of its argument for fields — find out, then say so
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XEF7FJvUkGKvG9LSGuEaNR
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -430,6 +430,27 @@ when defined(newIcBackend):
|
||||
## (`(nflags <flags> <symnode>)`, `(ht <type> <symnode>)`) 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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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) =
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user