Replace tfHasRequiresInit with a more accurate mechanism

The new mechanism can deal with more complex scenarios such as
not nil field appearing in a non-default case object branch or
a field within a generic object that may depend on a when branch.

The commit also plugs another hole: the user is no longer able
to create illegal default values through seq.setLen(N).

Author: zah

compiler/ast.nim

@@ -517,11 +517,11 @@ type
     tfIterator,       # type is really an iterator, not a tyProc
     tfPartial,        # type is declared as 'partial'
     tfNotNil,         # type cannot be 'nil'
-
-    tfHasRequiresInit,# type constains a "not nil" constraint somewhere or
+    tfRequiresInit,   # type constains a "not nil" constraint somewhere or
                       # a `requiresInit` field, so the default zero init
                       # is not appropriate
-    tfRequiresInit,   # all fields of the type must be initialized
+    tfNeedsFullInit,  # object type marked with {.requiresInit.}
+                      # all fields must be initialized
     tfVarIsPtr,       # 'var' type is translated like 'ptr' even in C++ mode
     tfHasMeta,        # type contains "wildcard" sub-types such as generic params
                       # or other type classes
@@ -1397,7 +1397,7 @@ proc copyType*(t: PType, owner: PSym, keepId: bool): PType =
 proc exactReplica*(t: PType): PType = copyType(t, t.owner, true)
 
 template requiresInit*(t: PType): bool =
-  t.flags * {tfRequiresInit, tfHasRequiresInit, tfNotNil} != {}
+  t.flags * {tfRequiresInit, tfNotNil} != {}
 
 proc copySym*(s: PSym): PSym =
   result = newSym(s.kind, s.name, s.owner, s.info, s.options)
@@ -1489,12 +1489,6 @@ proc propagateToOwner*(owner, elem: PType; propagateHasAsgn = true) =
   if tfNotNil in elem.flags:
     if owner.kind in {tyGenericInst, tyGenericBody, tyGenericInvocation}:
       owner.flags.incl tfNotNil
-    elif owner.kind notin HaveTheirOwnEmpty:
-      owner.flags.incl tfHasRequiresInit
-
-  if {tfRequiresInit, tfHasRequiresInit} * elem.flags != {}:
-    if owner.kind in HaveTheirOwnEmpty: discard
-    else: owner.flags.incl tfHasRequiresInit
 
   if elem.isMetaType:
     owner.flags.incl tfHasMeta

compiler/pragmas.nim

@@ -1090,7 +1090,7 @@ proc singlePragma(c: PContext, sym: PSym, n: PNode, i: var int,
         if sym.kind == skField:
           sym.flags.incl sfRequiresInit
         elif sym.typ != nil:
-          incl(sym.typ.flags, tfRequiresInit)
+          incl(sym.typ.flags, tfNeedsFullInit)
         else:
           invalidPragma(c, it)
       of wByRef:

compiler/sem.nim

@@ -46,14 +46,15 @@ proc addParams(c: PContext, n: PNode, kind: TSymKind)
 proc maybeAddResult(c: PContext, s: PSym, n: PNode)
 proc tryExpr(c: PContext, n: PNode, flags: TExprFlags = {}): PNode
 proc activate(c: PContext, n: PNode)
-proc checkDefaultConstruction(c: PContext, typ: PType, info: TLineInfo)
 proc semQuoteAst(c: PContext, n: PNode): PNode
 proc finishMethod(c: PContext, s: PSym)
 proc evalAtCompileTime(c: PContext, n: PNode): PNode
 proc indexTypesMatch(c: PContext, f, a: PType, arg: PNode): PNode
 proc semStaticExpr(c: PContext, n: PNode): PNode
 proc semStaticType(c: PContext, childNode: PNode, prev: PType): PType
 proc semTypeOf(c: PContext; n: PNode): PNode
+proc computeRequiresInit(c: PContext, t: PType): bool
+proc defaultConstructionError(c: PContext, t: PType, info: TLineInfo)
 proc hasUnresolvedArgs(c: PContext, n: PNode): bool
 proc isArrayConstr(n: PNode): bool {.inline.} =
   result = n.kind == nkBracket and
@@ -512,6 +513,7 @@ proc myOpen(graph: ModuleGraph; module: PSym): PPassContext =
   c.semExpr = semExpr
   c.semTryExpr = tryExpr
   c.semTryConstExpr = tryConstExpr
+  c.computeRequiresInit = computeRequiresInit
   c.semOperand = semOperand
   c.semConstBoolExpr = semConstBoolExpr
   c.semOverloadedCall = semOverloadedCall

compiler/semdata.nim

@@ -103,6 +103,7 @@ type
     semExpr*: proc (c: PContext, n: PNode, flags: TExprFlags = {}): PNode {.nimcall.}
     semTryExpr*: proc (c: PContext, n: PNode, flags: TExprFlags = {}): PNode {.nimcall.}
     semTryConstExpr*: proc (c: PContext, n: PNode): PNode {.nimcall.}
+    computeRequiresInit*: proc (c: PContext, t: PType): bool {.nimcall.}
     semOperand*: proc (c: PContext, n: PNode, flags: TExprFlags = {}): PNode {.nimcall.}
     semConstBoolExpr*: proc (c: PContext, n: PNode): PNode {.nimcall.} # XXX bite the bullet
     semOverloadedCall*: proc (c: PContext, n, nOrig: PNode,

compiler/semexprs.nim

@@ -2267,13 +2267,19 @@ proc semMagic(c: PContext, n: PNode, s: PSym, flags: TExprFlags): PNode =
   of mSizeOf:
     markUsed(c, n.info, s)
     result = semSizeof(c, setMs(n, s))
+  of mSetLengthSeq:
+    result = semDirectOp(c, n, flags)
+    let seqType = result[1].typ.skipTypes({tyPtr, tyRef, # in case we had auto-dereferencing
+                                           tyVar, tyGenericInst, tyOwned, tySink,
+                                           tyAlias, tyUserTypeClassInst})
+    if seqType.kind == tySequence and seqType.base.requiresInit:
+      localError(c.config, n.info, "setLen can potentially expand the sequence, " &
+                                   "but the element type $1 doesn't have a default value.",
+                                   [typeToString(seqType.base)])
   of mDefault:
     result = semDirectOp(c, n, flags)
     c.config.internalAssert result[1].typ.kind == tyTypeDesc
-    let typ = result[1].typ.base
-    if typ.kind == tyObject:
-      checkDefaultConstruction(c, typ, n.info)
-    elif typ.kind in {tyRef, tyPtr} and tfNotNil in typ.flags:
+    if result[1].typ.base.requiresInit:
       localError(c.config, n.info, "not nil types don't have a default value")
   else:
     result = semDirectOp(c, n, flags)

compiler/semobjconstr.nim

@@ -15,7 +15,7 @@ type
   ObjConstrContext = object
     typ: PType               # The constructed type
     initExpr: PNode          # The init expression (nkObjConstr)
-    requiresFullInit: bool   # A `requiresInit` derived type will
+    needsFullInit: bool      # A `requiresInit` derived type will
                              # set this to true while visiting
                              # parent types.
     missingFields: seq[PSym] # Fields that the user failed to specify
@@ -147,7 +147,7 @@ proc fieldsPresentInInitExpr(c: PContext, fieldsRecList, initExpr: PNode): strin
 proc collectMissingFields(c: PContext, fieldsRecList: PNode,
                           constrCtx: var ObjConstrContext) =
   for r in directFieldsInRecList(fieldsRecList):
-    if constrCtx.requiresFullInit or
+    if constrCtx.needsFullInit or
        sfRequiresInit in r.sym.flags or
        r.sym.typ.requiresInit:
       let assignment = locateFieldInInitExpr(c, r.sym, constrCtx.initExpr)
@@ -323,14 +323,11 @@ proc semConstructFields(c: PContext, recNode: PNode,
   else:
     internalAssert c.config, false
 
-proc semConstructType(c: PContext, initExpr: PNode,
-                      t: PType, flags: TExprFlags): InitStatus =
+proc semConstructTypeAux(c: PContext,
+                         constrCtx: var ObjConstrContext,
+                         flags: TExprFlags): InitStatus =
   result = initUnknown
-
-  var
-    t = t
-    constrCtx = ObjConstrContext(typ: t, initExpr: initExpr,
-                                 requiresFullInit: tfRequiresInit in t.flags)
+  var t = constrCtx.typ
   while true:
     let status = semConstructFields(c, t.n, constrCtx, flags)
     mergeInitStatus(result, status)
@@ -339,18 +336,30 @@ proc semConstructType(c: PContext, initExpr: PNode,
     let base = t[0]
     if base == nil: break
     t = skipTypes(base, skipPtrs)
-    constrCtx.requiresFullInit = constrCtx.requiresFullInit or
-                                 tfRequiresInit in t.flags
-
-  # It's possible that the object was not fully initialized while
-  # specifying a .requiresInit. pragma:
-  if constrCtx.missingFields.len > 0:
-    localError(c.config, constrCtx.initExpr.info,
-      "The $1 type requires the following fields to be initialized: $2.",
-      [constrCtx.typ.sym.name.s, listSymbolNames(constrCtx.missingFields)])
-
-proc checkDefaultConstruction(c: PContext, typ: PType, info: TLineInfo) =
-  discard semConstructType(c, newNodeI(nkObjConstr, info), typ, {})
+    constrCtx.needsFullInit = constrCtx.needsFullInit or
+                              tfNeedsFullInit in t.flags
+
+proc initConstrContext(t: PType, initExpr: PNode): ObjConstrContext =
+  ObjConstrContext(typ: t, initExpr: initExpr,
+                   needsFullInit: tfNeedsFullInit in t.flags)
+
+proc computeRequiresInit(c: PContext, t: PType): bool =
+  assert t.kind == tyObject
+  var constrCtx = initConstrContext(t, newNode(nkObjConstr))
+  let initResult = semConstructTypeAux(c, constrCtx, {})
+  constrCtx.missingFields.len > 0
+
+proc defaultConstructionError(c: PContext, t: PType, info: TLineInfo) =
+  var objType = t
+  while objType.kind != tyObject:
+    objType = objType.lastSon
+    assert objType != nil
+  var constrCtx = initConstrContext(objType, newNodeI(nkObjConstr, info))
+  let initResult = semConstructTypeAux(c, constrCtx, {})
+  assert constrCtx.missingFields.len > 0
+  localError(c.config, info,
+    "The $1 type doesn't have a default value. The following fields must be initialized: $2.",
+    [typeToString(t), listSymbolNames(constrCtx.missingFields)])
 
 proc semObjConstr(c: PContext, n: PNode, flags: TExprFlags): PNode =
   var t = semTypeNode(c, n[0], nil)
@@ -376,7 +385,15 @@ proc semObjConstr(c: PContext, n: PNode, flags: TExprFlags): PNode =
   # Check if the object is fully initialized by recursively testing each
   # field (if this is a case object, initialized fields in two different
   # branches will be reported as an error):
-  let initResult = semConstructType(c, result, t, flags)
+  var constrCtx = initConstrContext(t, result)
+  let initResult = semConstructTypeAux(c, constrCtx, flags)
+
+  # It's possible that the object was not fully initialized while
+  # specifying a .requiresInit. pragma:
+  if constrCtx.missingFields.len > 0:
+    localError(c.config, result.info,
+      "The $1 type requires the following fields to be initialized: $2.",
+      [t.sym.name.s, listSymbolNames(constrCtx.missingFields)])
 
   # Since we were traversing the object fields, it's possible that
   # not all of the fields specified in the constructor was visited.

compiler/sempass2.nim

@@ -184,7 +184,7 @@ proc initVar(a: PEffects, n: PNode; volatileCheck: bool) =
 proc initVarViaNew(a: PEffects, n: PNode) =
   if n.kind != nkSym: return
   let s = n.sym
-  if {tfRequiresInit, tfHasRequiresInit, tfNotNil} * s.typ.flags <= {tfNotNil}:
+  if {tfRequiresInit, tfNotNil} * s.typ.flags <= {tfNotNil}:
     # 'x' is not nil, but that doesn't mean its "not nil" children
     # are initialized:
     initVar(a, n, volatileCheck=true)
@@ -590,7 +590,7 @@ proc trackCase(tracked: PEffects, n: PNode) =
   track(tracked, n[0])
   let oldState = tracked.init.len
   let oldFacts = tracked.guards.s.len
-  let stringCase = skipTypes(n[0].typ,
+  let stringCase = n[0].typ != nil and skipTypes(n[0].typ,
         abstractVarRange-{tyTypeDesc}).kind in {tyFloat..tyFloat128, tyString}
   let interesting = not stringCase and interestingCaseExpr(n[0]) and
         tracked.config.hasWarn(warnProveField)
@@ -838,7 +838,7 @@ proc track(tracked: PEffects, n: PNode) =
       # may not look like an assignment, but it is:
       let arg = n[1]
       initVarViaNew(tracked, arg)
-      if arg.typ.len != 0 and {tfRequiresInit, tfHasRequiresInit} * arg.typ.lastSon.flags != {}:
+      if arg.typ.len != 0 and {tfRequiresInit} * arg.typ.lastSon.flags != {}:
         if a.sym.magic == mNewSeq and n[2].kind in {nkCharLit..nkUInt64Lit} and
             n[2].intVal == 0:
           # var s: seq[notnil];  newSeq(s, 0)  is a special case!

compiler/semstmts.nim

@@ -614,8 +614,10 @@ proc semVarOrLet(c: PContext, n: PNode, symkind: TSymKind): PNode =
         else: v.typ = tup
         b[j] = newSymNode(v)
       if def.kind == nkEmpty:
-        if v.typ.kind == tyObject:
-          checkDefaultConstruction(c, v.typ, v.info)
+        let actualType = v.typ.skipTypes({tyGenericInst, tyAlias,
+                                          tyUserTypeClassInst})
+        if actualType.kind == tyObject and actualType.requiresInit:
+          defaultConstructionError(c, v.typ, v.info)
         else:
           checkNilable(c, v)
       if sfCompileTime in v.flags:

compiler/semtypes.nim

@@ -770,8 +770,6 @@ proc semRecordNodeAux(c: PContext, n: PNode, check: var IntSet, pos: var int,
       f.typ = typ
       f.position = pos
       f.options = c.config.options
-      if sfRequiresInit in f.flags:
-        rectype.flags.incl tfHasRequiresInit
       if fieldOwner != nil and
          {sfImportc, sfExportc} * fieldOwner.flags != {} and
          not hasCaseFields and f.loc.r == nil:
@@ -879,6 +877,8 @@ proc semObjectNode(c: PContext, n: PNode, prev: PType; isInheritable: bool): PTy
     pragma(c, s, n[0], typePragmas)
   if base == nil and tfInheritable notin result.flags:
     incl(result.flags, tfFinal)
+  if c.inGenericContext == 0 and computeRequiresInit(c, result):
+    result.flags.incl tfRequiresInit
 
 proc semAnyRef(c: PContext; n: PNode; kind: TTypeKind; prev: PType): PType =
   if n.len < 1:

compiler/semtypinst.nim

@@ -613,6 +613,8 @@ proc replaceTypeVarsTAux(cl: var TReplTypeVars, t: PType): PType =
 
       of tyObject, tyTuple:
         propagateFieldFlags(result, result.n)
+        if result.kind == tyObject and cl.c.computeRequiresInit(cl.c, result):
+          result.flags.incl tfRequiresInit
 
       of tyProc:
         eraseVoidParams(result)

tests/constructors/tinvalid_construction.nim

@@ -10,6 +10,9 @@ type
   TRefObj = ref object
     x: int
 
+  IllegalToConstruct = object
+    x: cstring not nil
+
   THasNotNils = object of RootObj
     a: TRefObj not nil
     b: TRefObj not nil
@@ -89,6 +92,10 @@ proc userDefinedDefault(T: typedesc): T =
 proc genericDefault(T: typedesc): T =
   result = default(T)
 
+reject IllegalToConstruct()
+reject:
+  var x: IllegalToConstruct
+
 accept TObj()
 accept TObj(choice: A)
 reject TObj(choice: A, bc: 10)  # bc is in the wrong branch
@@ -256,6 +263,111 @@ reject TNestedChoices(outerChoice: false, innerChoice: C)
 accept TNestedChoices(outerChoice: false, innerChoice: C, notnil: notNilRef)
 reject TNestedChoices(outerChoice: false, innerChoice: C, notnil: nil)
 
+# Tests involving generics and sequences:
+#
+block:
+  # This test aims to show that it's possible to instantiate and
+  # use a sequence with a requiresInit type:
+
+  var legalSeq: seq[IllegalToConstruct]
+  legalSeq.add IllegalToConstruct(x: "one")
+  var two = IllegalToConstruct(x: "two")
+  legalSeq.add two
+  var one = legalSeq[0]
+  var twoAgain = legalSeq.pop
+
+  # It's not possible to tell the sequence to create elements
+  # for us though:
+  reject:
+    var illegalSeq = newSeq[IllegalToConstruct](10)
+
+  reject:
+    var illegalSeq: seq[IllegalToConstruct]
+    newSeq(illegalSeq, 10)
+
+  reject:
+    var illegalSeq: seq[IllegalToConstruct]
+    illegalSeq.setLen 10
+
+  # You can still use newSeqOfCap to write efficient code:
+  var anotherLegalSequence = newSeqOfCap[IllegalToConstruct](10)
+  for i in 0..9:
+    anotherLegalSequence.add IllegalToConstruct(x: "x")
+
+type DefaultConstructible[yesOrNo: static[bool]] = object
+  when yesOrNo:
+    x: string
+  else:
+    x: cstring not nil
+
+block:
+  # Constructability may also depend on the generic parameters of the type:
+  accept:
+    var a: DefaultConstructible[true]
+    var b = DefaultConstructible[true]()
+    var c = DefaultConstructible[true](x: "test")
+    var d = DefaultConstructible[false](x: "test")
+
+  reject:
+    var y: DefaultConstructible[false]
+
+  reject:
+    var y = DefaultConstructible[false]()
+
+block:
+  type
+    Hash = int
+
+    HashTableSlotType = enum
+      Free    = Hash(0)
+      Deleted = Hash(1)
+      HasKey  = Hash(2)
+
+    KeyValuePair[A, B] = object
+      key: A
+      case hash: HashTableSlotType
+      of Free, Deleted:
+        discard
+      else:
+        value: B
+
+  # The above KeyValuePair is an interesting type because it
+  # may become unconstructible depending on the generic parameters:
+  accept KeyValuePair[int, string](hash: Deleted)
+  accept KeyValuePair[int, IllegalToConstruct](hash: Deleted)
+
+  accept KeyValuePair[int, string](hash: HasKey)
+  reject KeyValuePair[int, IllegalToConstruct](hash: HasKey)
+
+  # Since all the above variations don't have a non-constructible
+  # field in the default branch of the case object, we can construct
+  # such values:
+  accept KeyValuePair[int, string]()
+  accept KeyValuePair[int, IllegalToConstruct]()
+  accept KeyValuePair[DefaultConstructible[true], string]()
+  accept KeyValuePair[DefaultConstructible[true], IllegalToConstruct]()
+
+  var a: KeyValuePair[int, string]
+  var b: KeyValuePair[int, IllegalToConstruct]
+  var c: KeyValuePair[DefaultConstructible[true], string]
+  var d: KeyValuePair[DefaultConstructible[true], IllegalToConstruct]
+  var s1 = newSeq[KeyValuePair[int, IllegalToConstruct]](10)
+  var s2 = newSeq[KeyValuePair[DefaultConstructible[true], IllegalToConstruct]](10)
+
+  # But let's put the non-constructible values as keys:
+  reject KeyValuePair[IllegalToConstruct, int](hash: Deleted)
+  reject KeyValuePair[IllegalToConstruct, int]()
+
+  type IllegalPair = KeyValuePair[DefaultConstructible[false], string]
+
+  reject:
+    var x: IllegalPair
+
+  reject:
+    var s = newSeq[IllegalPair](10)
+
+# Specific issues:
+#
 block:
   # https://github.com/nim-lang/Nim/issues/11428
   type