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.

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

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

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() & ")")

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})

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)

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]()

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)

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
 """
 

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