From f5d9e7a207383e45cd1dd8a17f8e754a14df71cf Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Mon, 15 Jun 2026 23:33:16 +0200 Subject: [PATCH] IC: bugfixes (#25914) --- compiler/ast2nif.nim | 31 ++++++++++++++++++++++++++++++- compiler/msgs.nim | 17 +++++++++++++++-- compiler/options.nim | 7 ++++++- compiler/semdata.nim | 19 ++++++++++++------- compiler/sighashes.nim | 15 +++++++++++++++ koch.nim | 3 ++- tests/ic/mconvbase.nim | 16 ++++++++++++++++ tests/ic/mconvmid.nim | 13 +++++++++++++ tests/ic/msighashstable.nim | 20 ++++++++++++++++++++ tests/ic/tconverterreexport.nim | 18 ++++++++++++++++++ tests/ic/tsighashstable.nim | 22 +++++++++++++++++++++- 11 files changed, 168 insertions(+), 13 deletions(-) create mode 100644 tests/ic/mconvbase.nim create mode 100644 tests/ic/mconvmid.nim create mode 100644 tests/ic/tconverterreexport.nim diff --git a/compiler/ast2nif.nim b/compiler/ast2nif.nim index 5a8ed60ab0..4323be7cdc 100644 --- a/compiler/ast2nif.nim +++ b/compiler/ast2nif.nim @@ -25,6 +25,15 @@ import "../dist/nimony/src/models" / nifindex_tags import typekeys import ic / [enum2nif] +const SysModuleSuffix* = "@sys" + ## Reserved module-suffix sentinel for module-less magic singleton types — the + ## `nil` type is created via `newSysType` with the graph idgen, whose `module` + ## can be `-1` (e.g. during VM const-eval before a real module is current), so + ## its `uniqueId.module` is unresolvable. Such a type has no fields and an + ## identity that is fully captured by its kind, so we serialize it with this + ## sentinel and reconstruct it on load (see `createTypeStub`) without ever + ## touching a `.nif` file. A real `moduleSuffix` never starts with '@'. + proc typeToNifSym(typ: PType; config: ConfigRef): string = # NOTE: uniqueId is the serialization identity and is unique per instance — # `exactReplica` keeps only itemId shared with its original (see ast.nim) @@ -34,7 +43,10 @@ proc typeToNifSym(typ: PType; config: ConfigRef): string = result.add '.' result.addInt typ.uniqueId.item result.add '.' - result.add modname(typ.uniqueId.module, config) + if typ.uniqueId.module < 0: + result.add SysModuleSuffix + else: + result.add modname(typ.uniqueId.module, config) proc icNifTypeName*(typ: PType; config: ConfigRef): string = ## The serialized NIF name of a type, recorded next to RTTI data @@ -1527,6 +1539,19 @@ proc loadNode(c: var DecodeContext; n: var Cursor; thisModule: string; proc loadSymFromCursor(c: var DecodeContext; s: PSym; n: var Cursor; thisModule: string; localSyms: var Table[string, PSym]) +proc reconstructSysType(c: var DecodeContext; name: string; k: int; itemVal: int32): PType = + ## Rebuild a module-less magic singleton (see `SysModuleSuffix`) from its kind + ## alone — it has no fields and no `.nif` to load. Cached in `c.types` so all + ## references in this decode context share one instance. + result = c.types.getOrDefault(name)[0] + if result == nil: + let id = itemId(-1'i32, itemVal) + result = PType(itemId: id, uniqueId: id, kind: TTypeKind(k), state: Complete) + if TTypeKind(k) == tyNil: + result.sizeImpl = c.infos.config.target.ptrSize + result.alignImpl = int16 c.infos.config.target.ptrSize + c.types[name] = (result, NifIndexEntry()) + proc tryCreateTypeStub(c: var DecodeContext; t: SymId): PType = ## Like `createTypeStub` but returns nil instead of raising when the type has ## no offset in its module index (used by the best-effort `(offer …)` loader). @@ -1546,6 +1571,8 @@ proc tryCreateTypeStub(c: var DecodeContext; t: SymId): PType = inc i if i < name.len and name[i] == '.': inc i let suffix = name.substr(i) + if suffix == SysModuleSuffix: + return reconstructSysType(c, name, k, itemVal) let id = itemId(moduleId(c, suffix).int32, itemVal) let ii = addr c.mods[id.module.FileIndex].index let offs = ii[].getOrDefault(name) @@ -1571,6 +1598,8 @@ proc createTypeStub(c: var DecodeContext; t: SymId): PType = inc i if i < name.len and name[i] == '.': inc i let suffix = name.substr(i) + if suffix == SysModuleSuffix: + return reconstructSysType(c, name, k, itemVal) let id = itemId(moduleId(c, suffix).int32, itemVal) let ii = addr c.mods[id.module.FileIndex].index let offs = ii[].getOrDefault(name) diff --git a/compiler/msgs.nim b/compiler/msgs.nim index c092ac8841..88c17cb440 100644 --- a/compiler/msgs.nim +++ b/compiler/msgs.nim @@ -125,12 +125,25 @@ proc fileInfoIdx*(conf: ConfigRef; filename: AbsoluteFile): FileIndex = var dummy: bool = false result = fileInfoIdx(conf, filename, dummy) +proc expandOrPseudo(filename: string): AbsoluteFile = + # `expandFilename` raises OSError when the path does not exist on disk. That is + # fine for a real source path, but a macro can legitimately set a node's + # line-info file to a name that has no file behind it — e.g. the `???` sentinel + # produced by `toFilename` for a NIF-loaded node whose `fileIndex` is unknown + # (FileIndex(-1)). Falling back to the raw name lets the `AbsoluteFile` overload + # register it as a pseudo-path (like `command line`/`stdin`) instead of crashing + # the whole `nim m` child with an unhandled OSError. + try: + result = AbsoluteFile expandFilename(filename) + except OSError: + result = AbsoluteFile filename + proc fileInfoIdx*(conf: ConfigRef; filename: RelativeFile; isKnownFile: var bool): FileIndex = - fileInfoIdx(conf, AbsoluteFile expandFilename(filename.string), isKnownFile) + fileInfoIdx(conf, expandOrPseudo(filename.string), isKnownFile) proc fileInfoIdx*(conf: ConfigRef; filename: RelativeFile): FileIndex = var dummy: bool = false - fileInfoIdx(conf, AbsoluteFile expandFilename(filename.string), dummy) + fileInfoIdx(conf, expandOrPseudo(filename.string), dummy) proc registerNifSuffix*(conf: ConfigRef; suffix: string; isKnownFile: var bool): FileIndex = result = conf.m.filenameToIndexTbl.getOrDefault(suffix, InvalidFileIdx) diff --git a/compiler/options.nim b/compiler/options.nim index 8559caf0e3..825addc610 100644 --- a/compiler/options.nim +++ b/compiler/options.nim @@ -29,7 +29,7 @@ const nimEnableCovariance* = defined(nimEnableCovariance) - icFormatVersion* = "5" + icFormatVersion* = "6" ## Version of the IC cache format (the sem-NIF module layout written by ## ast2nif.nim plus the iface/impl/edges side files). Bump it whenever ## that layout changes: `commandIc` wipes a nimcache whose `ic.version` @@ -49,6 +49,11 @@ const ## cdef directives with an always-present extern declaration, so the ## per-module merge stage can assign them a single owner; old `.c.nif` ## artifacts lack the wrappers. + ## v6: `signatureHash`/`hashType` of a builtin type class (`object`, `tuple`, + ## `proc`, ...) no longer mixes in the placeholder son's process-local type + ## id, so its hash is stable across the NIF boundary (was breaking + ## nim-serialization's auto-serialization lookup under IC). The sem-NIF + ## macrocache entries and baked generic-instance bodies hold the old hashes. type # please make sure we have under 32 options # (improves code efficiency a lot!) diff --git a/compiler/semdata.nim b/compiler/semdata.nim index a15bc0ef5a..5cdcc18be4 100644 --- a/compiler/semdata.nim +++ b/compiler/semdata.nim @@ -396,16 +396,21 @@ proc addConverter*(c: PContext, conv: PSym) = assert conv != nil if inclSym(c.converters, conv): add(c.graph.ifaces[c.module.position].converters, conv) + # Record for IC: the loader rebuilds Iface.converters from the NIF's + # (repconverter ...) entries (moduleFromNifFile). This must capture not only + # converters DEFINED in this module (addConverterDef) but also ones IMPORTED + # from another module here (importer.addUnnamedIt re-adds a re-exported + # module's converters via this proc). Otherwise a loaded module's + # re-exported converters were invisible to importers and implicit + # conversions silently stopped matching at a consumer that reaches the + # converter only through this module's re-export chain (e.g. faststreams' + # `InputStreamHandle -> InputStream` via ssz_serialization, breaking + # `SSZ.decode`/`encode`). `inclSym` guards against duplicate log entries. + c.graph.opsLog.add LogEntry(kind: ConverterEntry, module: c.module.position, + key: "", sym: conv) proc addConverterDef*(c: PContext, conv: PSym) = addConverter(c, conv) - # record the definition for IC: the loader rebuilds Iface.converters from - # the NIF's (repconverter ...) entries (moduleFromNifFile); without the log - # entry a loaded module's converters were invisible to importers and - # implicit conversions silently stopped matching (e.g. faststreams' - # InputStreamHandle -> InputStream at toml_serialization call sites) - c.graph.opsLog.add LogEntry(kind: ConverterEntry, module: c.module.position, - key: "", sym: conv) proc addPureEnum*(c: PContext, e: PSym) = assert e != nil diff --git a/compiler/sighashes.nim b/compiler/sighashes.nim index fdb089d7f4..d6db0b0fa0 100644 --- a/compiler/sighashes.nim +++ b/compiler/sighashes.nim @@ -335,6 +335,21 @@ proc hashType(c: var MD5Context, t: PType; flags: set[ConsiderFlag]; conf: Confi c &= char(t.kind) c.hashType(t.indexType, flags-{CoIgnoreRange}+{CoIgnoreRangeInArray}, conf) c.hashType(t.elementType, flags-{CoIgnoreRange}, conf) + of tyBuiltInTypeClass: + # A builtin type class (`object`, `tuple`, `proc`, `ref`, `seq`, ...) is + # identified solely by the *kind* of its single placeholder son plus a few + # flags/callConv (see `sameType`). That son is a fresh, field-less, sym-less + # type, so the generic `else` below would recurse into it and hash its + # process-local `t.id` — unstable across the NIF boundary. nim-serialization + # keys auto-serialization on `signatureHash(object)`/`tuple`/... and missed + # under IC because the registering and consuming modules minted different + # placeholder ids. Hash the class identity that `sameType` actually compares. + c &= char(t.kind) + let elem = t.elementType + c &= char(elem.kind) + for f in eqTypeFlags * elem.flags: c &= char(ord(f)) + if elem.kind == tyProc and tfExplicitCallConv in elem.flags: + c &= char(elem.callConv) else: c &= char(t.kind) for a in t.kids: c.hashType(a, flags, conf) diff --git a/koch.nim b/koch.nim index 809ddee868..a7333ba468 100644 --- a/koch.nim +++ b/koch.nim @@ -617,7 +617,8 @@ proc runIcTestFile(inp: string) = # on a sibling helper (`timp` -> `myimp`, `tcompiletimeglobal` -> `mctglobal`), # which exercises the NIF import/load path the single-file tests do not. const icSuite = ["thallo", "tconverter", "timp", "tmiscs", "tparseutils", - "tcompiletimeglobal", "tsighashstable", "tpureenum", "tgenericoffer"] + "tcompiletimeglobal", "tsighashstable", "tpureenum", "tgenericoffer", + "tconverterreexport"] proc icTest(args: string) = temp("") diff --git a/tests/ic/mconvbase.nim b/tests/ic/mconvbase.nim new file mode 100644 index 0000000000..8b26149753 --- /dev/null +++ b/tests/ic/mconvbase.nim @@ -0,0 +1,16 @@ +# Helper for tconverterreexport.nim: defines a converter that a consumer reaches +# only through a re-export chain (mconvbase <- mconvmid <- test). Models +# faststreams' `implicitDeref: InputStreamHandle -> InputStream`. +type + Handle* = object + x*: int + Real* = object + x*: int + +converter toReal*(h: Handle): Real = Real(x: h.x) + +type Reader* = object + v: int + +func init*(T: type Reader, r: Real): Reader = Reader(v: r.x + 100) +func readVal*(r: Reader): int = r.v diff --git a/tests/ic/mconvmid.nim b/tests/ic/mconvmid.nim new file mode 100644 index 0000000000..4c013af0c0 --- /dev/null +++ b/tests/ic/mconvmid.nim @@ -0,0 +1,13 @@ +# Helper for tconverterreexport.nim: re-exports mconvbase and provides a +# `mixin`-using template whose expansion in the consumer relies on the +# re-exported `toReal` converter (Handle -> Real) for `init`. Mirrors +# ssz_serialization re-exporting serialization/faststreams and its `decode` +# template needing the implicit stream conversion at `init(Reader, stream)`. +import mconvbase +export mconvbase + +template decode*(): auto = + mixin init, readVal + var h = Handle(x: 5) + var r = init(Reader, h) # requires the re-exported converter toReal + readVal(r) diff --git a/tests/ic/msighashstable.nim b/tests/ic/msighashstable.nim index d5ce85d18a..c423916236 100644 --- a/tests/ic/msighashstable.nim +++ b/tests/ic/msighashstable.nim @@ -36,13 +36,33 @@ func getAuto*(F: type DefaultFlavor, T: distinct type): bool {.compileTime.} = let table = F.getTable() table.hasKey(sig) +func tcOrMember*(F: type DefaultFlavor, TC: distinct type, TM: distinct type): bool {.compileTime.} = + ## Is `TM` registered, or its parent type class `TC`? Models + ## `typeClassOrMemberAutoSerialize` — used to auto-serialize any `object`/`tuple`. + if F.getAuto(TM): return true + if F.getAuto(TC): return true + false + template autoCheck*(F: distinct type, T: distinct type, body) = when not F.getAuto(T): {.error: "auto serialization not enabled for `" & typetraits.name(T) & "`".} else: body +template autoCheckTC*(F: distinct type, TC: distinct type, M: distinct type, body) = + when not F.tcOrMember(TC, M): + {.error: "auto serialization not enabled for `" & typetraits.name(M) & + "` of typeclass `" & typetraits.name(TC) & "`".} + else: + body + static: setAuto(DefaultFlavor, string) setAuto(DefaultFlavor, SomeInteger) setAuto(DefaultFlavor, seq) + # Builtin *type classes*: `object`/`tuple` are `tyBuiltInTypeClass`, whose + # `signatureHash` must not mix in the placeholder son's process-local type id + # or the lookup misses across the NIF boundary (the real nim-serialization bug + # surfaced by Nimbus: `MultiAddress`/`RequestId` "of typeclass `object`"). + setAuto(DefaultFlavor, object) + setAuto(DefaultFlavor, tuple) diff --git a/tests/ic/tconverterreexport.nim b/tests/ic/tconverterreexport.nim new file mode 100644 index 0000000000..7265110496 --- /dev/null +++ b/tests/ic/tconverterreexport.nim @@ -0,0 +1,18 @@ +discard """ +output: '''105''' +""" + +# Regression test: a converter imported into a module and re-exported from it +# (mconvbase's `toReal` re-exported by mconvmid) must survive NIF serialization +# in the re-exporting module's interface, so a consumer that reaches the +# converter only through that re-export chain can still apply it implicitly. +# +# Before the fix only converters DEFINED in a module were logged for IC +# (`addConverterDef`); converters merely imported/re-exported (`addConverter` +# from `importer.addUnnamedIt`) were not, so under `nim ic` the re-exported +# converter was invisible to importers and `init(Reader, Handle)` failed with a +# type mismatch (the real-world symptom: faststreams `InputStreamHandle -> +# InputStream` dropped, breaking `SSZ.decode`/`encode` in Nimbus). + +import mconvmid +echo decode() diff --git a/tests/ic/tsighashstable.nim b/tests/ic/tsighashstable.nim index 11c20240d9..e32cdf21cd 100644 --- a/tests/ic/tsighashstable.nim +++ b/tests/ic/tsighashstable.nim @@ -1,7 +1,9 @@ discard """ output: '''ok string ok int -ok seq''' +ok seq +ok object +ok tuple''' """ # Regression test: `signatureHash(T)` must be stable across the NIF boundary so @@ -28,6 +30,24 @@ proc writeSeq[T](F: type DefaultFlavor, v: seq[T]) = autoCheck(F, seq): echo "ok seq" +type + Msg = object + a: int + b: string + +# `object`/`tuple` are `tyBuiltInTypeClass`: a concrete type is auto-serialized +# by looking up its type class. The hash of the bare class keyword must match +# between msighashstable's registration and this lookup. +proc writeObj[T: object](F: type DefaultFlavor, v: T) = + autoCheckTC(F, object, typeof(v)): + echo "ok object" + +proc writeTup[T: tuple](F: type DefaultFlavor, v: T) = + autoCheckTC(F, tuple, typeof(v)): + echo "ok tuple" + writeStr(DefaultFlavor, "hi") writeInt(DefaultFlavor, 42) writeSeq(DefaultFlavor, @[1, 2, 3]) +writeObj(DefaultFlavor, Msg(a: 1, b: "x")) +writeTup(DefaultFlavor, (1, "x"))