From 7973ccbbd106c0670114befee44c5a21e6ab9786 Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Wed, 9 Sep 2026 21:24:47 +0200 Subject: [PATCH] compiler: derived type flags are exempt from IC (#26198) `propagateToOwner` aborted with ast.nim(1314, 9) `mask <= o2.flags` IC bug: sealed type missing propagated flags whenever a consumer module propagated `tfHasAsgn`/`tfHasOwned` into a type another module had already sealed. The assert encoded the hope that a producer's derivation is complete at seal time. It is not: these flags are DERIVED -- a fixpoint over kind, MM config, elements and the attached-op table -- so a consumer can legitimately discover one later. The trigger in the wild is a generic alias, `Channel[TMsg] {.gcsafe.} = RawChannel`, whose object only acquires `tfHasAsgn` at the first `Channel[T]` instantiation, in another module and (under IC) another process. Any program that opens a channel crashed the compiler under `nim ic` as soon as a `=copy` hook was declared for the alias (nim-lang/Nim#26182). Partition the flags instead. `derivedTypeFlags` names the bookkeeping bits -- tfHasAsgn, tfHasOwned, tfHasGCedMem, tfCheckedForDestructor -- and `ast.inclDerived` / `ast.exclDerived` write exactly those without the `Sealed` assert. The exemption is sound: none of them is in `eqTypeFlags`, the `typekeys` content key or the NIF name, so they cannot rename a type, move it in the cache or change `sameType`; and `ast2nif.writeType` emits a `SymUse` for any type the current module does not own, so a consumer's write can never reach a NIF. They stay serialized, so a reload starts from the producer's derivation and a consumer only ever adds to it. `tfGenericHasDestructor` is deliberately NOT in the set: it is an alias for `tfExplicitCallConv`, and exempting it would exempt a real proc-type property. This also gives the four existing `flagsImpl` writes a name. They wrote the raw field precisely to dodge this assert -- liftdestructors' one carried `# ^ XXX Breaks IC!` -- and are now `inclDerived`/`exclDerived` calls, so the exemption is typed, asserted against `derivedTypeFlags`, and greppable. `-d:icDerivedBarrier` reports every derived-flag write onto a sealed type; a full `nim ic` build of a channel program produces 25, of which 24 are the `tfCheckedForDestructor` writes that were already happening silently. --- compiler/ast.nim | 41 +++++++++++++++++++++++++++------- compiler/astdef.nim | 20 +++++++++++++++++ compiler/injectdestructors.nim | 2 +- compiler/liftdestructors.nim | 10 +++++---- compiler/semdata.nim | 4 ++-- compiler/semfold.nim | 2 +- 6 files changed, 63 insertions(+), 16 deletions(-) diff --git a/compiler/ast.nim b/compiler/ast.nim index 7877d291cb..a7818b7711 100644 --- a/compiler/ast.nim +++ b/compiler/ast.nim @@ -459,6 +459,29 @@ proc excl*(t: PType; flags: set[TTypeFlag]) {.inline.} = if t.state == Partial: loadType(t) t.flagsImpl.excl(flags) +proc inclDerived*(t: PType; flags: TTypeFlags) {.inline.} = + ## Add DERIVED bookkeeping flags (see `derivedTypeFlags`). Unlike `incl` this + ## is allowed on a `Sealed` type: the bit is not part of the type's identity, + ## its NIF name or its content key, and a consumer never rewrites a foreign + ## type's def (`ast2nif.writeType` emits a `SymUse` for anything it does not + ## own), so the write cannot reach any NIF. It is process-local bookkeeping + ## that a consumer re-derives from data the producer already serialized. + assert flags <= derivedTypeFlags + if t.state == Partial: loadType(t) + when defined(icDerivedBarrier): + if t.state == Sealed and not (flags <= t.flagsImpl): + echo "[derived-barrier] +", flags - t.flagsImpl, " on sealed ", t.kind, + (if t.symImpl != nil: "/" & t.symImpl.name.s else: ""), + " @", t.itemId.module, ".", t.itemId.item + t.flagsImpl.incl(flags) + +proc exclDerived*(t: PType; flags: TTypeFlags) {.inline.} = + ## Counterpart of `inclDerived`. Same reasoning; `tfCheckedForDestructor` is + ## genuinely cleared again (`injectdestructors`), so this is not monotone. + assert flags <= derivedTypeFlags + if t.state == Partial: loadType(t) + t.flagsImpl.excl(flags) + proc typ*(n: PNode): lent PType {.inline.} = result = n.typField if result == nil and nfLazyType in n.flags: @@ -1308,13 +1331,15 @@ proc propagateToOwner*(owner, elem: PType; propagateHasAsgn = true) = let o2 = owner.skipTypes({tyGenericInst, tyAlias, tySink}) if o2.kind in {tyTuple, tyObject, tyArray, tySequence, tyString, tySet, tyDistinct}: - if o2.state == Sealed: - # During the original compilation, propagateToOwner set tfHasAsgn/tfHasOwned on the type before it was sealed - # On IC reload, the sealed type already has those flags - assert mask <= o2.flags, "IC bug: sealed type missing propagated flags" - else: - o2.incl mask - owner.incl mask + # `o2` may be `Sealed`: these are DERIVED flags, and a consumer can reach + # a foreign type here that the producer sealed without them. The classic + # case is a generic alias -- `Channel[TMsg] = RawChannel` -- where + # `normalizeTypeHook` leaves `tfHasAsgn` on the `tyAlias` and the object + # behind it only acquires it at the first instantiation, in another + # module and, under IC, another process. Adding the bit is sound (see + # `inclDerived`); asserting here just crashed on legal code. + o2.inclDerived mask + owner.inclDerived mask if owner.kind notin {tyProc, tyGenericInst, tyGenericBody, tyGenericInvocation, tyPtr}: @@ -1322,7 +1347,7 @@ proc propagateToOwner*(owner, elem: PType; propagateHasAsgn = true) = if elemB.isGCedMem or tfHasGCedMem in elemB.flags: # for simplicity, we propagate this flag even to generics. We then # ensure this doesn't bite us in sempass2. - owner.incl tfHasGCedMem + owner.inclDerived {tfHasGCedMem} proc rawAddSon*(father, son: PType; propagateHasAsgn = true) = ensureMutable father diff --git a/compiler/astdef.nim b/compiler/astdef.nim index 341e98a2f1..5d0682b5cb 100644 --- a/compiler/astdef.nim +++ b/compiler/astdef.nim @@ -473,6 +473,26 @@ const ## tyGenericBody where an instance has a generated destructor skError* = skUnknown +const + derivedTypeFlags* = {tfHasAsgn, tfHasOwned, tfHasGCedMem, tfCheckedForDestructor} + ## Codegen/lifting BOOKKEEPING bits, as opposed to the flags that make a + ## type what it is. They are *derived*: a fixpoint over the type's own kind, + ## the memory management config, its elements and the attached-op table -- + ## never something the source said. They are in none of `eqTypeFlags`, the + ## `typekeys` content key or the NIF name, so changing one cannot rename a + ## type, move it in the cache, or alter `sameType`. + ## + ## Because they are derived rather than declared, a consumer module can + ## legitimately discover one *after* the defining module sealed the type + ## (the classic case: an alias carries `tfHasAsgn` into the NIF but the + ## object behind it only gets it at the first generic instantiation, which + ## happens in another module -- and, under IC, another process). Writing + ## one is therefore exempt from the `Sealed` assert: see `ast.inclDerived`. + ## + ## NOT in this set even though it looks like it belongs: + ## `tfGenericHasDestructor`, which is an ALIAS for `tfExplicitCallConv`. + ## Exempting it would exempt a real proc-type property from the seal. + var eqTypeFlags* = {tfIterator, tfNotNil, tfVarIsPtr, tfGcSafe, tfNoSideEffect, tfIsOutParam} ## type flags that are essential for type equality. diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim index 4a17dd7015..14418a4e80 100644 --- a/compiler/injectdestructors.nim +++ b/compiler/injectdestructors.nim @@ -242,7 +242,7 @@ proc genOp(c: var Con; t: PType; kind: TTypeAttachedOp; dest, ri: PNode): PNode # closure-env identity resolves via `attachedOps[itemId]`/env-erased typeKey, # env objects load complete, and atomicRefOp's type-erased path covers any # still-incomplete env (so the lift never walks a nil field). - excl t.flagsImpl, tfCheckedForDestructor + t.exclDerived {tfCheckedForDestructor} createTypeBoundOps(c.graph, nil, t, dest.info, c.idgen) op = getAttachedOp(c.graph, t, kind) if op == nil: diff --git a/compiler/liftdestructors.nim b/compiler/liftdestructors.nim index 4e2063888a..07e0a8e613 100644 --- a/compiler/liftdestructors.nim +++ b/compiler/liftdestructors.nim @@ -1463,7 +1463,7 @@ proc createTypeBoundOps(g: ModuleGraph; c: PContext; orig: PType; info: TLineInf ## The later 'injectdestructors' pass depends on it. if orig == nil or {tfCheckedForDestructor, tfHasMeta} * orig.flags != {}: return # IC: review this solution again later - incl orig.flagsImpl, tfCheckedForDestructor + orig.inclDerived {tfCheckedForDestructor} # for user defined generic destructors: let origRoot = genericRoot(orig) if origRoot != nil: @@ -1513,6 +1513,8 @@ proc createTypeBoundOps(g: ModuleGraph; c: PContext; orig: PType; info: TLineInf if not isTrivial(getAttachedOp(g, orig, attachedDestructor)): #or not isTrivial(orig.assignment) or # not isTrivial(orig.sink): - # IC: review this solution again later - orig.flagsImpl.incl tfHasAsgn - # ^ XXX Breaks IC! + # A hook was lifted for `orig` in THIS module, which may be a module that + # merely uses the type. `inclDerived` is the sanctioned way to record that + # on a possibly-`Sealed` foreign type (it used to write `flagsImpl` behind + # the accessor's back precisely to dodge the seal assert). + orig.inclDerived {tfHasAsgn} diff --git a/compiler/semdata.nim b/compiler/semdata.nim index 50b33d48a1..53dddb447f 100644 --- a/compiler/semdata.nim +++ b/compiler/semdata.nim @@ -486,7 +486,7 @@ proc makeVarType*(c: PContext, baseType: PType; kind = tyVar): PType = proc makeTypeSymNode*(c: PContext, typ: PType, info: TLineInfo): PNode = let typedesc = newTypeS(tyTypeDesc, c) - incl typedesc.flagsImpl, tfCheckedForDestructor + typedesc.inclDerived {tfCheckedForDestructor} internalAssert(c.config, typ != nil) typedesc.addSonSkipIntLit(typ, c.idgen) let sym = newSym(skType, c.cache.idAnon, c.idgen, getCurrOwner(c), info, @@ -565,7 +565,7 @@ template rangeHasUnresolvedStatic*(t: PType): bool = proc errorType*(c: PContext): PType = ## creates a type representing an error state result = newTypeS(tyError, c) - result.flagsImpl.incl tfCheckedForDestructor + result.inclDerived {tfCheckedForDestructor} proc errorNode*(c: PContext, n: PNode): PNode = result = newNodeI(nkEmpty, n.info) diff --git a/compiler/semfold.nim b/compiler/semfold.nim index 51e5043a39..a07462783c 100644 --- a/compiler/semfold.nim +++ b/compiler/semfold.nim @@ -24,7 +24,7 @@ when defined(nimPreviewSlimSystem): proc errorType*(g: ModuleGraph): PType = ## creates a type representing an error state result = newType(tyError, g.idgen, g.owners[^1]) - result.flagsImpl.incl tfCheckedForDestructor + result.inclDerived {tfCheckedForDestructor} proc getIntLitTypeG(g: ModuleGraph; literal: PNode; idgen: IdGenerator): PType = # we cache some common integer literal types for performance: