From 9c87f2cb4bc03df4102b1a8b1c11ff92adf75364 Mon Sep 17 00:00:00 2001 From: metagn Date: Sat, 16 Nov 2024 12:48:01 +0300 Subject: [PATCH] always reinstantiate nominal values of generic instantiations (#24425) fixes #22479, fixes #24374, depends on #24429 and #24430 When instantiating generic types which directly have nominal types (object, distinct, ref/ptr object but not enums[^1]) as their values, the nominal type is now copied (in the case of ref objects, its child as well) so that it receives a fresh ID and `typeInst` field. Previously this only happened if it contained any generic types in its structure, as is the case for all other types. This solves #22479 and #24374 by virtue of the IDs being unique, which is what destructors check for. Technically types containing generic param fields work for the same reason. There is also the benefit that the `typeInst` field is correct. However issues like #22445 aren't solved because the compiler still uses structural object equality checks for inheritance etc. which could be removed in a later PR. Also fixes a pre-existing issue where destructors bound to object types with generic fields would not error when attempting to define a user destructor after the fact, but the error message doesn't show where the implicit destructor was created now since it was only created for another instance. To do this, a type flag is used that marks the generic type symbol when a generic instance has a destructor created. Reusing `tfCheckedForDestructor` for this doesn't work. Maybe there is a nicer design that isn't an overreliance on the ID mechanism, but the shortcomings of `tyGenericInst` are too ingrained in the compiler to use for this. I thought about maybe adding something like `tyNominalGenericInst`, but it's really much easier if the nominal type itself directly contains the information of its generic parameters, or at least its "symbol", which the design is heading towards. [^1]: See [this test](https://github.com/nim-lang/Nim/blob/21420d8b0976dc034feb90ab2878ae0dd63121ae/lib/std/enumutils.nim#L102) in enumutils. The field symbols `b0`/`b1` always have the uninstantiated type `B` because enum fields don't expect to be generic, so no generic instance of `B` matches its own symbols. Wouldn't expect anyone to use generic enums but maybe someone does. (cherry picked from commit 05c74d684481733c507dd18a7da8a296289ca32d) --- compiler/ast.nim | 2 ++ compiler/liftdestructors.nim | 4 +++ compiler/semstmts.nim | 31 ++++++++++++----- compiler/semtypinst.nim | 18 ++++++---- compiler/types.nim | 10 ++++++ tests/arc/tphantomgeneric1.nim | 19 +++++++++++ tests/arc/tphantomgeneric2.nim | 33 +++++++++++++++++++ tests/destructor/tinvalid_rebind.nim | 2 +- tests/destructor/tinvalid_rebind_nonempty.nim | 16 +++++++++ 9 files changed, 118 insertions(+), 17 deletions(-) create mode 100644 tests/arc/tphantomgeneric1.nim create mode 100644 tests/arc/tphantomgeneric2.nim create mode 100644 tests/destructor/tinvalid_rebind_nonempty.nim diff --git a/compiler/ast.nim b/compiler/ast.nim index 2f6c272320..cc3f4dcd99 100644 --- a/compiler/ast.nim +++ b/compiler/ast.nim @@ -447,6 +447,8 @@ const tfReturnsNew* = tfInheritable tfNonConstExpr* = tfExplicitCallConv ## tyFromExpr where the expression shouldn't be evaluated as a static value + tfGenericHasDestructor* = tfExplicitCallConv + ## tyGenericBody where an instance has a generated destructor skError* = skUnknown var diff --git a/compiler/liftdestructors.nim b/compiler/liftdestructors.nim index 69d457a566..6aa2634401 100644 --- a/compiler/liftdestructors.nim +++ b/compiler/liftdestructors.nim @@ -1276,6 +1276,10 @@ 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 incl orig.flags, tfCheckedForDestructor + # for user defined generic destructors: + let origRoot = genericRoot(orig) + if origRoot != nil: + incl origRoot.flags, tfGenericHasDestructor let skipped = orig.skipTypes({tyGenericInst, tyAlias, tySink}) if isEmptyContainer(skipped) or skipped.kind == tyStatic: return diff --git a/compiler/semstmts.nim b/compiler/semstmts.nim index 5ca9c1f983..101f382e2b 100644 --- a/compiler/semstmts.nim +++ b/compiler/semstmts.nim @@ -2035,14 +2035,27 @@ proc canonType(c: PContext, t: PType): PType = else: result = t -proc prevDestructor(c: PContext; prevOp: PSym; obj: PType; info: TLineInfo) = - var msg = "cannot bind another '" & prevOp.name.s & "' to: " & typeToString(obj) - if sfOverridden notin prevOp.flags: +proc prevDestructor(c: PContext; op: TTypeAttachedOp; prevOp: PSym; obj: PType; info: TLineInfo) = + var msg = "cannot bind another '" & AttachedOpToStr[op] & "' to: " & typeToString(obj) + if prevOp == nil: + # happens if the destructor was implicitly constructed for a specific instance, + # not the entire generic type + msg.add "; previous declaration was constructed implicitly" + elif sfOverridden notin prevOp.flags: msg.add "; previous declaration was constructed here implicitly: " & (c.config $ prevOp.info) else: msg.add "; previous declaration was here: " & (c.config $ prevOp.info) localError(c.config, info, errGenerated, msg) +proc checkedForDestructor(t: PType): bool = + if tfCheckedForDestructor in t.flags: + return true + # maybe another instance was instantiated, marking the generic root: + let root = genericRoot(t) + if root != nil and tfGenericHasDestructor in root.flags: + return true + result = false + proc whereToBindTypeHook(c: PContext; t: PType): PType = result = t while true: @@ -2076,10 +2089,10 @@ proc bindDupHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp) = let ao = getAttachedOp(c.graph, obj, op) if ao == s: discard "forward declared destructor" - elif ao.isNil and tfCheckedForDestructor notin obj.flags: + elif ao.isNil and not checkedForDestructor(obj): setAttachedOp(c.graph, c.module.position, obj, op, s) else: - prevDestructor(c, ao, obj, n.info) + prevDestructor(c, op, ao, obj, n.info) noError = true if obj.owner.getModule != s.getModule: localError(c.config, n.info, errGenerated, @@ -2123,10 +2136,10 @@ proc bindTypeHook(c: PContext; s: PSym; n: PNode; op: TTypeAttachedOp) = let ao = getAttachedOp(c.graph, obj, op) if ao == s: discard "forward declared destructor" - elif ao.isNil and tfCheckedForDestructor notin obj.flags: + elif ao.isNil and not checkedForDestructor(obj): setAttachedOp(c.graph, c.module.position, obj, op, s) else: - prevDestructor(c, ao, obj, n.info) + prevDestructor(c, op, ao, obj, n.info) noError = true if obj.owner.getModule != s.getModule: localError(c.config, n.info, errGenerated, @@ -2217,10 +2230,10 @@ proc semOverride(c: PContext, s: PSym, n: PNode) = let ao = getAttachedOp(c.graph, obj, k) if ao == s: discard "forward declared op" - elif ao.isNil and tfCheckedForDestructor notin obj.flags: + elif ao.isNil and not checkedForDestructor(obj): setAttachedOp(c.graph, c.module.position, obj, k, s) else: - prevDestructor(c, ao, obj, n.info) + prevDestructor(c, k, ao, obj, n.info) if obj.owner.getModule != s.getModule: localError(c.config, n.info, errGenerated, "type bound operation `" & name & "` can be defined only in the same module with its type (" & obj.typeToString() & ")") diff --git a/compiler/semtypinst.nim b/compiler/semtypinst.nim index 2aca2c7a4b..217488cb8d 100644 --- a/compiler/semtypinst.nim +++ b/compiler/semtypinst.nim @@ -80,7 +80,7 @@ type owner*: PSym # where this instantiation comes from recursionLimit: int -proc replaceTypeVarsTAux(cl: var TReplTypeVars, t: PType): PType +proc replaceTypeVarsTAux(cl: var TReplTypeVars, t: PType, isInstValue = false): PType proc replaceTypeVarsS(cl: var TReplTypeVars, s: PSym, t: PType): PSym proc replaceTypeVarsN*(cl: var TReplTypeVars, n: PNode; start=0; expectedType: PType = nil): PNode @@ -95,8 +95,8 @@ template checkMetaInvariants(cl: TReplTypeVars, t: PType) = # noop code debug t writeStackTrace() -proc replaceTypeVarsT*(cl: var TReplTypeVars, t: PType): PType = - result = replaceTypeVarsTAux(cl, t) +proc replaceTypeVarsT*(cl: var TReplTypeVars, t: PType, isInstValue = false): PType = + result = replaceTypeVarsTAux(cl, t, isInstValue) checkMetaInvariants(cl, result) proc prepareNode*(cl: var TReplTypeVars, n: PNode): PNode = @@ -481,7 +481,7 @@ proc handleGenericInvocation(cl: var TReplTypeVars, t: PType): PType = return let bbody = last body - var newbody = replaceTypeVarsT(cl, bbody) + var newbody = replaceTypeVarsT(cl, bbody, isInstValue = true) cl.skipTypedesc = oldSkipTypedesc newbody.flags = newbody.flags + (t.flags + body.flags - tfInstClearedFlags) result.flags = result.flags + newbody.flags - tfInstClearedFlags @@ -578,7 +578,7 @@ proc propagateFieldFlags(t: PType, n: PNode) = propagateFieldFlags(t, son) else: discard -proc replaceTypeVarsTAux(cl: var TReplTypeVars, t: PType): PType = +proc replaceTypeVarsTAux(cl: var TReplTypeVars, t: PType, isInstValue = false): PType = template bailout = if (t.sym == nil) or (t.sym != nil and sfGeneratedType in t.sym.flags): # In the first case 't.sym' can be 'nil' if the type is a ref/ptr, see @@ -712,13 +712,17 @@ proc replaceTypeVarsTAux(cl: var TReplTypeVars, t: PType): PType = propagateToOwner(result, result.last) else: - if containsGenericType(t): + if containsGenericType(t) or + # nominal types as direct generic instantiation values + # are re-instantiated even if they don't contain generic fields + (isInstValue and (t.kind in {tyDistinct, tyObject} or isRefPtrObject(t))): #if not cl.allowMetaTypes: bailout() result = instCopyType(cl, t) result.size = -1 # needs to be recomputed #if not cl.allowMetaTypes: cl.localCache[t.itemId] = result + let propagateInstValue = isInstValue and isRefPtrObject(t) for i, resulti in result.ikids: if resulti != nil: @@ -728,7 +732,7 @@ proc replaceTypeVarsTAux(cl: var TReplTypeVars, t: PType): PType = typeToString(result[i], preferDesc) & "' inside of type definition: '" & t.owner.name.s & "'; Maybe generic arguments are missing?") - var r = replaceTypeVarsT(cl, resulti) + var r = replaceTypeVarsT(cl, resulti, isInstValue = propagateInstValue) if result.kind == tyObject: # carefully coded to not skip the precious tyGenericInst: let r2 = r.skipTypes({tyAlias, tySink, tyOwned}) diff --git a/compiler/types.nim b/compiler/types.nim index bf32daa056..66df653a2f 100644 --- a/compiler/types.nim +++ b/compiler/types.nim @@ -1309,12 +1309,22 @@ proc sameTypeAux(x, y: PType, c: var TSameTypeClosure): bool = withoutShallowFlags: ifFastObjectTypeCheckFailed(a, b): cycleCheck() + if a.typeInst != nil and b.typeInst != nil: + # this is required because of `ref object`s, + # the value of their dereferences are not wrapped in `tyGenericInst`, + # so we need to check the generic parameters here + for ff, aa in underspecifiedPairs(a.typeInst, b.typeInst, 1, -1): + if not sameTypeAux(ff, aa, c): return false + # XXX should be removed in favor of above lines, + # structural equality is wrong in general: result = sameObjectStructures(a, b, c) and sameFlags(a, b) of tyDistinct: cycleCheck() if c.cmp == dcEq: if sameFlags(a, b): ifFastObjectTypeCheckFailed(a, b): + # XXX should be removed in favor of checking generic params, + # structural equality is wrong in general: result = sameTypeAux(a.elementType, b.elementType, c) else: result = sameTypeAux(a.elementType, b.elementType, c) and sameFlags(a, b) diff --git a/tests/arc/tphantomgeneric1.nim b/tests/arc/tphantomgeneric1.nim new file mode 100644 index 0000000000..8ca36053d3 --- /dev/null +++ b/tests/arc/tphantomgeneric1.nim @@ -0,0 +1,19 @@ +discard """ + output: ''' +int +float +''' +""" + +# issue #22479 + +type Obj[T] = object + +proc `=destroy`[T](self: var Obj[T]) = + echo T + +block: + let intObj = Obj[int]() + +block: + let floatObj = Obj[float]() diff --git a/tests/arc/tphantomgeneric2.nim b/tests/arc/tphantomgeneric2.nim new file mode 100644 index 0000000000..f4023db43f --- /dev/null +++ b/tests/arc/tphantomgeneric2.nim @@ -0,0 +1,33 @@ +discard """ + output: ''' +created Phantom[system.float] with value 1 +created Phantom[system.string] with value 2 +created Phantom[system.byte] with value 3 +destroyed Phantom[system.byte] with value 3 +destroyed Phantom[system.string] with value 2 +destroyed Phantom[system.float] with value 1 +''' +""" + +# issue #24374 + +type Phantom[T] = object + value: int + # markerField: T + +proc initPhantom[T](value: int): Phantom[T] = + doAssert value >= 0 + echo "created " & $Phantom[T] & " with value " & $value + result = Phantom[T](value: value) + +proc `=wasMoved`[T](x: var Phantom[T]) = + x.value = -1 + +proc `=destroy`[T](x: Phantom[T]) = + if x.value >= 0: + echo "destroyed " & $Phantom[T] & " with value " & $x.value + +let + x = initPhantom[float](1) + y = initPhantom[string](2) + z = initPhantom[byte](3) diff --git a/tests/destructor/tinvalid_rebind.nim b/tests/destructor/tinvalid_rebind.nim index 0f15c8f9e7..1dca8e993d 100644 --- a/tests/destructor/tinvalid_rebind.nim +++ b/tests/destructor/tinvalid_rebind.nim @@ -1,7 +1,7 @@ discard """ joinable: false cmd: "nim check $file" -errormsg: "cannot bind another '=destroy' to: Foo; previous declaration was constructed here implicitly: tinvalid_rebind.nim(12, 7)" +errormsg: "cannot bind another '=destroy' to: Foo; previous declaration was constructed implicitly" line: 14 """ diff --git a/tests/destructor/tinvalid_rebind_nonempty.nim b/tests/destructor/tinvalid_rebind_nonempty.nim new file mode 100644 index 0000000000..ffa886700b --- /dev/null +++ b/tests/destructor/tinvalid_rebind_nonempty.nim @@ -0,0 +1,16 @@ +discard """ +joinable: false +cmd: "nim check $file" +errormsg: "cannot bind another '=destroy' to: Foo; previous declaration was constructed implicitly" +line: 15 +""" + +type + Foo[T] = object + x: T + +proc main = + var f: Foo[int] + +proc `=destroy`[T](f: var Foo[T]) = + discard