leave type section symbols unchanged on resem, fix overly general double semcheck for forward types (#24888)

fixes #24887 (really just this [1 line
commit](632c7b3)
would have been enough to fix the issue but it would ignore the general
problem)

When a type definition is encountered where the symbol already has a
type (not a forward type), the type is left alone (not reset to
`tyForward`) and the RHS is handled differently: The RHS is still
semchecked, but the type of the symbol is not updated, and nominal type
nodes are ignored entirely (specifically if they are the same kind as
the symbol's existing type but this restriction is not really needed).
If the existing type of the symbol is an enum and and the RHS has a
nominal enum type node, the enum fields of the existing type are added
to scope rather than creating a new type from the RHS and adding its
symbols instead.

The goal is to prevent any incompatible nominal types from being
generated during resem as in #24887. But it also restricts what macros
can do if they generate type section AST, for example if we have:

```nim
type Foo = int
```

and a macro modifies the type section while keeping the symbol node for
`Foo` like:

```nim
type Foo = float
```

Then the type of `Foo` will still remain `int`, while it previously
became `float`. While we could maybe allow this and make it so only
nominal types cannot be changed, it gets even more complex when
considering generic params and whether or not they get updated. So to
keep it as simple as possible the rule is that the symbol type does not
change, but maybe this behavior was useful for macros.

Only nominal type nodes are ignored for semchecking on the RHS, so that
cases like this do not cause a regression:

```nim
template foo(): untyped =
  proc bar() {.inject.} = discard
  int

type Foo = foo()
bar() # normally works
```

However this specific code exposed a problem with forward type handling:

---

In specific cases, when the type section is undergoing the final pass,
if the type fits some overly general criteria (it is not an object,
enum, alias or a sink type and its node is not a nominal type node), the
entire RHS is semchecked for a 2nd time as a standalone type (with `nil`
prev) and *maybe* reassigned to the new semchecked type, depending on
its type kind. (for some reason including nominal types when we excluded
them before?) This causes a redefinition error if the RHS defines a
symbol.

This code goes all the way back to the first commit and I could not find
the reason why it was there, but removing it showed a failure in
`thard_tyforward`: If a generic forward type is invoked, it is left as
an unresolved `tyGenericInvocation` on the first run. Semchecking it
again at the end turns it into a `tyGenericInst`. So my understanding is
that it exists to handle these loose forward types, but it is way too
general and there is a similar mechanism `c.skipTypes` which is supposed
to do the same thing but doesn't.

So this is no longer done, and `c.skipTypes` is revamped (and renamed):
It is now a list of types and the nodes that are supposed to evaluate to
them, such that types needing to be updated later due to containing
forward types are added to it along with their nodes. When finishing the
type section, these types are reassigned to the semchecked value of
their nodes so that the forward types in them are fully resolved. The
"reassigning" here works due to updating the data inside the type
pointer directly, and is how forward types work by themselves normally
(`tyForward` types are modified in place as `s.typ`).

For example, as mentioned before, generic invocations of forward types
are first created as `tyGenericInvocation` and need to become
`tyGenericInst` later. So they are now added to this list along with
their node. Object types with forward types as their base types also
need to be updated later to check that the base type is correct/inherit
fields from it: For this the entire object type and its node are added
to the list. Similarly, any case where whether a component type is
`tyGenericInst` or `tyGenericInvocation` matters also needs to cascade
this (`set` does presumably to check the instantiated type).

This is not complete: Generic invocations with forward types only check
that their base type is a forward type, but not any of their arguments,
which causes #16754 and #24133. The generated invocations also need to
cascade properly: `Foo[Bar[ForwardType]]` for example would see that
`Bar[ForwardType]` is a generic invocation and stay as a generic
invocation itself, but it might not queue itself to be updated later.
Even if it did, only the entire type `Foo[Bar[ForwardType]]` needs to be
queued, updating `Bar[ForwardType]` by itself would be redundant or it
would not change anything at all. But these can be done later.

Author: metagn

compiler/semdata.nim

@@ -172,7 +172,9 @@ type
     sideEffects*: Table[int, seq[(TLineInfo, PSym)]] # symbol.id index
     inUncheckedAssignSection*: int
     importModuleLookup*: Table[int, seq[int]] # (module.ident.id, [module.id])
-    skipTypes*: seq[PNode] # used to skip types between passes in type section. So far only used for inheritance, sets and generic bodies.
+    forwardTypeUpdates*: seq[(PType, PNode)]
+      # types that need to be updated in a type section
+      # due to containing forward types, and their corresponding nodes
     inTypeofContext*: int
 
     semAsgnOpr*: proc (c: PContext; n: PNode; k: TNodeKind): PNode {.nimcall.}

compiler/semstmts.nim

@@ -1460,8 +1460,13 @@ proc typeDefLeftSidePass(c: PContext, typeSection: PNode, i: int) =
   else:
     s = semIdentDef(c, name, skType)
     onDef(name.info, s)
-    s.typ = newTypeS(tyForward, c)
-    s.typ.sym = s             # process pragmas:
+    if s.typ != nil:
+      # name node is a symbol with a type already, probably in resem, don't touch it
+      discard
+    else:
+      s.typ = newTypeS(tyForward, c)
+      s.typ.sym = s
+    # process pragmas:
     if name.kind == nkPragmaExpr:
       let rewritten = applyTypeSectionPragmas(c, name[1], typeDef)
       if rewritten != nil:
@@ -1599,7 +1604,26 @@ proc typeSectionRightSidePass(c: PContext, n: PNode) =
       localError(c.config, a.info, errImplOfXexpected % s.name.s)
     if s.magic != mNone: processMagicType(c, s)
     let oldFlags = s.typ.flags
-    if a[1].kind != nkEmpty:
+    let preserveSym = s.typ != nil and s.typ.kind != tyForward and sfForward notin s.flags and
+        s.magic == mNone # magic might have received type above but still needs processing
+    if preserveSym:
+      # symbol already has a type, probably in resem, do not modify it
+      # but still semcheck the RHS to handle any defined symbols
+      # nominal type nodes are still ignored in semtypes
+      if a[1].kind != nkEmpty:
+        openScope(c)
+        pushOwner(c, s)
+        a[1] = semGenericParamList(c, a[1], nil)
+        inc c.inGenericContext
+        discard semTypeNode(c, a[2], s.typ)
+        dec c.inGenericContext
+        popOwner(c)
+        closeScope(c)
+      elif a[2].kind != nkEmpty:
+        pushOwner(c, s)
+        discard semTypeNode(c, a[2], s.typ)
+        popOwner(c)
+    elif a[1].kind != nkEmpty:
       # We have a generic type declaration here. In generic types,
       # symbol lookup needs to be done here.
       openScope(c)
@@ -1689,7 +1713,7 @@ proc typeSectionRightSidePass(c: PContext, n: PNode) =
           localError(c.config, name.info, "only a 'distinct' type can borrow `.`")
     let aa = a[2]
     if aa.kind in {nkRefTy, nkPtrTy} and aa.len == 1 and
-       aa[0].kind == nkObjectTy:
+       aa[0].kind == nkObjectTy and not preserveSym:
       # give anonymous object a dummy symbol:
       var st = s.typ
       if st.kind == tyGenericBody: st = st.typeBodyImpl
@@ -1730,9 +1754,6 @@ proc typeSectionRightSidePass(c: PContext, n: PNode) =
         obj.flags.incl sfPure
       obj.typ = objTy
       objTy.sym = obj
-  for sk in c.skipTypes:
-    discard semTypeNode(c, sk, nil)
-  c.skipTypes = @[]
 
 proc checkForMetaFields(c: PContext; n: PNode; hasError: var bool) =
   proc checkMeta(c: PContext; n: PNode; t: PType; hasError: var bool; parent: PType) =
@@ -1768,6 +1789,15 @@ proc checkForMetaFields(c: PContext; n: PNode; hasError: var bool) =
     internalAssert c.config, false
 
 proc typeSectionFinalPass(c: PContext, n: PNode) =
+  for (typ, typeNode) in c.forwardTypeUpdates:
+    # types that need to be updated due to containing forward types
+    # and their corresponding type nodes
+    # for example generic invocations of forward types end up here
+    var reified = semTypeNode(c, typeNode, nil)
+    assert reified != nil
+    assignType(typ, reified)
+    typ.itemId = reified.itemId     # same id
+  c.forwardTypeUpdates = @[]
   for i in 0..<n.len:
     var a = n[i]
     if a.kind == nkCommentStmt: continue
@@ -1794,29 +1824,15 @@ proc typeSectionFinalPass(c: PContext, n: PNode) =
       else:
         while x.kind in {nkStmtList, nkStmtListExpr} and x.len > 0:
           x = x.lastSon
-        # we need the 'safeSkipTypes' here because illegally recursive types
-        # can enter at this point, see bug #13763
-        if x.kind notin {nkObjectTy, nkDistinctTy, nkEnumTy, nkEmpty} and
-            s.typ.safeSkipTypes(abstractPtrs).kind notin {tyObject, tyEnum}:
-          # type aliases are hard:
-          var t = semTypeNode(c, x, nil)
-          assert t != nil
-          if s.typ != nil and s.typ.kind notin {tyAlias, tySink}:
-            if t.kind in {tyProc, tyGenericInst} and not t.isMetaType:
-              assignType(s.typ, t)
-              s.typ.itemId = t.itemId
-            elif t.kind in {tyObject, tyEnum, tyDistinct}:
-              assert s.typ != nil
-              assignType(s.typ, t)
-              s.typ.itemId = t.itemId     # same id
         var hasError = false
-        let baseType = s.typ.safeSkipTypes(abstractPtrs)
-        if baseType.kind in {tyObject, tyTuple} and not baseType.n.isNil and
-          (x.kind in {nkObjectTy, nkTupleTy} or
+        if x.kind in {nkObjectTy, nkTupleTy} or
            (x.kind in {nkRefTy, nkPtrTy} and x.len == 1 and
-           x[0].kind in {nkObjectTy, nkTupleTy})
-          ):
-          checkForMetaFields(c, baseType.n, hasError)
+            x[0].kind in {nkObjectTy, nkTupleTy}):
+          # we need the 'safeSkipTypes' here because illegally recursive types
+          # can enter at this point, see bug #13763
+          let baseType = s.typ.safeSkipTypes(abstractPtrs)
+          if baseType.kind in {tyObject, tyTuple} and not baseType.n.isNil:
+            checkForMetaFields(c, baseType.n, hasError)
         if not hasError:
           checkConstructedType(c.config, s.info, s.typ)
   #instAllTypeBoundOp(c, n.info)

compiler/semtypes.nim

@@ -59,11 +59,31 @@ proc newConstraint(c: PContext, k: TTypeKind): PType =
   result.flags.incl tfCheckedForDestructor
   result.addSonSkipIntLit(newTypeS(k, c), c.idgen)
 
+proc skipGenericPrev(prev: PType): PType =
+  result = prev
+  if prev.kind == tyGenericBody and prev.last.kind != tyNone:
+    result = prev.last
+
+proc prevIsKind(prev: PType, kind: TTypeKind): bool {.inline.} =
+  result = prev != nil and skipGenericPrev(prev).kind == kind
+
 proc semEnum(c: PContext, n: PNode, prev: PType): PType =
   if n.len == 0: return newConstraint(c, tyEnum)
   elif n.len == 1:
     # don't create an empty tyEnum; fixes #3052
     return errorType(c)
+  if prevIsKind(prev, tyEnum):
+    # the symbol already has an enum type (likely resem), don't define a new enum
+    # but add the enum fields to scope from the original type
+    let isPure = sfPure in prev.sym.flags
+    for enumField in prev.n:
+      assert enumField.kind == nkSym
+      let e = enumField.sym
+      if not isPure:
+        addInterfaceOverloadableSymAt(c, c.currentScope, e)
+      else:
+        declarePureEnumField(c, e)
+    return prev
   var
     counter, x: BiggestInt = 0
     e: PSym = nil
@@ -197,7 +217,7 @@ proc semSet(c: PContext, n: PNode, prev: PType): PType =
     if base.kind in {tyGenericInst, tyAlias, tySink}: base = skipModifier(base)
     if base.kind notin {tyGenericParam, tyGenericInvocation}:
       if base.kind == tyForward:
-        c.skipTypes.add n
+        c.forwardTypeUpdates.add (base, n[1])
       elif not isOrdinalType(base, allowEnumWithHoles = true):
         localError(c.config, n.info, errOrdinalTypeExpected % typeToString(base, preferDesc))
       elif lengthOrd(c.config, base) > MaxSetElements:
@@ -307,6 +327,9 @@ proc addSonSkipIntLitChecked(c: PContext; father, son: PType; it: PNode, id: IdG
 
 proc semDistinct(c: PContext, n: PNode, prev: PType): PType =
   if n.len == 0: return newConstraint(c, tyDistinct)
+  if prevIsKind(prev, tyDistinct):
+    # the symbol already has a distinct type (likely resem), don't create a new type
+    return skipGenericPrev(prev)
   result = newOrPrevType(tyDistinct, prev, c)
   addSonSkipIntLitChecked(c, result, semTypeNode(c, n[0], nil), n[0], c.idgen)
   if n.len > 1: result.n = n[1]
@@ -994,11 +1017,15 @@ proc semObjectNode(c: PContext, n: PNode, prev: PType; flags: TTypeFlags): PType
   result = nil
   if n.len == 0:
     return newConstraint(c, tyObject)
+  if prevIsKind(prev, tyObject) and sfForward notin prev.sym.flags:
+    # the symbol already has an object type (likely resem), don't create a new type
+    return skipGenericPrev(prev)
   var check = initIntSet()
   var pos = 0
   var base, realBase: PType = nil
   # n[0] contains the pragmas (if any). We process these later...
   checkSonsLen(n, 3, c.config)
+  var needsForwardUpdate = false
   if n[1].kind != nkEmpty:
     realBase = semTypeNode(c, n[1][0], nil)
     base = skipTypesOrNil(realBase, skipPtrs)
@@ -1020,7 +1047,7 @@ proc semObjectNode(c: PContext, n: PNode, prev: PType; flags: TTypeFlags): PType
             return newType(tyError, c.idgen, result.owner)
 
       elif concreteBase.kind == tyForward:
-        c.skipTypes.add n #we retry in the final pass
+        needsForwardUpdate = true
       else:
         if concreteBase.kind != tyError:
           localError(c.config, n[1].info, "inheritance only works with non-final objects; " &
@@ -1030,6 +1057,10 @@ proc semObjectNode(c: PContext, n: PNode, prev: PType; flags: TTypeFlags): PType
         realBase = nil
   if n.kind != nkObjectTy: internalError(c.config, n.info, "semObjectNode")
   result = newOrPrevType(tyObject, prev, c)
+  if needsForwardUpdate:
+    # if the inherited object is a forward type,
+    # the entire object needs to be checked again
+    c.forwardTypeUpdates.add (result, n) #we retry in the final pass
   rawAddSon(result, realBase)
   if realBase == nil and tfInheritable in flags:
     result.flags.incl tfInheritable
@@ -1056,6 +1087,9 @@ proc semAnyRef(c: PContext; n: PNode; kind: TTypeKind; prev: PType): PType =
   if n.len < 1:
     result = newConstraint(c, kind)
   else:
+    if prevIsKind(prev, kind) and tfRefsAnonObj in prev.skipTypes({tyGenericBody}).flags:
+      # the symbol already has an object type (likely resem), don't create a new type
+      return skipGenericPrev(prev)
     let isCall = int ord(n.kind in nkCallKinds+{nkBracketExpr})
     let n = if n[0].kind == nkBracket: n[0] else: n
     checkMinSonsLen(n, 1, c.config)
@@ -1660,6 +1694,7 @@ proc semGeneric(c: PContext, n: PNode, s: PSym, prev: PType): PType =
     for i in 1..<n.len:
       var elem = semGenericParamInInvocation(c, n[i])
       addToResult(elem, true)
+    c.forwardTypeUpdates.add (result, n)
     return
   elif t.kind != tyGenericBody:
     # we likely got code of the form TypeA[TypeB] where TypeA is
@@ -1708,7 +1743,7 @@ proc semGeneric(c: PContext, n: PNode, s: PSym, prev: PType): PType =
         localError(c.config, n.info, errCannotInstantiateX % s.name.s)
         result = newOrPrevType(tyError, prev, c)
       elif containsGenericInvocationWithForward(n[0]):
-        c.skipTypes.add n #fixes 1500
+        c.forwardTypeUpdates.add (result, n) #fixes 1500
       else:
         result = instGenericContainer(c, n.info, result,
                                       allowMetaTypes = false)

compiler/types.nim

@@ -526,7 +526,7 @@ proc typeToString(typ: PType, prefer: TPreferedDesc = preferName): string =
     let t = typ
     if t == nil: return
     if prefer in preferToResolveSymbols and t.sym != nil and
-         sfAnon notin t.sym.flags and t.kind != tySequence:
+         sfAnon notin t.sym.flags and t.kind notin {tySequence, tyInferred}:
       if t.kind == tyInt and isIntLit(t):
         if prefer == preferInlayHint:
           result = t.sym.name.s

nimsuggest/tests/ttype_highlight.nim

@@ -20,8 +20,4 @@ highlight;;skType;;4;;33;;3
 highlight;;skType;;5;;13;;1
 highlight;;skType;;6;;25;;5
 highlight;;skType;;6;;34;;3
-highlight;;skType;;2;;10;;3
-highlight;;skType;;3;;11;;3
-highlight;;skType;;4;;33;;3
-highlight;;skType;;6;;34;;3
 """

tests/types/tresemtypesection.nim

@@ -0,0 +1,55 @@
+discard """
+  output: '''
+NONE
+a
+NONE
+a
+'''
+"""
+
+# issue #24887
+
+macro foo(x: typed) =
+  result = x
+
+foo:
+  type
+    Flags64 = distinct uint64
+
+  const NONE = Flags64(0'u64)
+  const MAX: Flags64 = Flags64(uint64.high)
+
+  proc `$`(x: Flags64): string =
+    case x:
+    of NONE:
+      return "NONE"
+    of MAX:
+      return "MAX"
+    else:
+      return "UNKNOWN"
+
+  let okay = Flags64(128'u64)
+
+  echo $NONE
+  type Foo = ref object
+    x: int
+  discard Foo(x: 123)
+  type Enum = enum a, b, c
+  echo a
+  type Bar[T] = object
+    x: T
+  discard Bar[int](x: 123)
+  discard Bar[string](x: "abc")
+
+  # regression test:
+  template templ(): untyped =
+    proc injected() {.inject.} = discard
+    int
+
+  type TestInject = templ()
+  var x1: TestInject
+  injected() # normally works
+echo $NONE
+echo a
+var x2: TestInject
+injected()