Turn the warning for uninitialized (result) variables into errors

7 files changed, 103 insertions, 17 deletions

diff --git a/compiler/ast.nim b/compiler/ast.nim
index a7eb2cac5..01f9ed29b 100644
--- a/compiler/ast.nim
+++ b/compiler/ast.nim
@@ -1396,6 +1396,9 @@ 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} != {}
+
 proc copySym*(s: PSym): PSym =
   result = newSym(s.kind, s.name, s.owner, s.info, s.options)
   #result.ast = nil            # BUGFIX; was: s.ast which made problems
diff --git a/compiler/lineinfos.nim b/compiler/lineinfos.nim
index f7f3b7706..585a95c80 100644
--- a/compiler/lineinfos.nim
+++ b/compiler/lineinfos.nim
@@ -23,6 +23,7 @@ type
     errGeneralParseError,
     errNewSectionExpected,
     errInvalidDirectiveX,
+    errProveInit,
     errGenerated,
     errUser,
     warnCannotOpenFile,
@@ -63,6 +64,7 @@ const
     errGeneralParseError: "general parse error",
     errNewSectionExpected: "new section expected",
     errInvalidDirectiveX: "invalid directive: '$1'",
+    errProveInit: "Cannot prove that '$1' is initialized.",
     errGenerated: "$1",
     errUser: "$1",
     warnCannotOpenFile: "cannot open '$1'",
diff --git a/compiler/semobjconstr.nim b/compiler/semobjconstr.nim
index b9931f527..7222433b3 100644
--- a/compiler/semobjconstr.nim
+++ b/compiler/semobjconstr.nim
@@ -146,8 +146,9 @@ proc fieldsPresentInInitExpr(c: PContext, fieldsRecList, initExpr: PNode): strin
 proc missingMandatoryFields(c: PContext, fieldsRecList: PNode,
                             constrCtx: ObjConstrContext): string =
   for r in directFieldsInRecList(fieldsRecList):
-    if constrCtx.requiresFullInit or sfRequiresInit in r.sym.flags or
-       {tfNotNil, tfRequiresInit, tfHasRequiresInit} * r.sym.typ.flags != {}:
+    if constrCtx.requiresFullInit or
+       sfRequiresInit in r.sym.flags or
+       r.sym.typ.requiresInit:
       let assignment = locateFieldInInitExpr(c, r.sym, constrCtx.initExpr)
       if assignment == nil:
         if result.len == 0:
diff --git a/compiler/sempass2.nim b/compiler/sempass2.nim
index 65dce3ed8..2d5e3e721 100644
--- a/compiler/sempass2.nim
+++ b/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, tfNotNil} * s.typ.flags <= {tfNotNil}:
+  if {tfRequiresInit, tfHasRequiresInit, 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)
@@ -253,8 +253,8 @@ proc useVar(a: PEffects, n: PNode) =
       # If the variable is explicitly marked as .noinit. do not emit any error
       a.init.add s.id
     elif s.id notin a.init:
-      if {tfRequiresInit, tfNotNil} * s.typ.flags != {}:
-        message(a.config, n.info, warnProveInit, s.name.s)
+      if s.typ.requiresInit:
+        localError(a.config, n.info, errProveInit, s.name.s)
       else:
         message(a.config, n.info, warnUninit, s.name.s)
       # prevent superfluous warnings about the same variable:
@@ -838,13 +838,13 @@ 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} * arg.typ.lastSon.flags != {}:
+      if arg.typ.len != 0 and {tfRequiresInit, tfHasRequiresInit} * 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!
           discard
         else:
-          message(tracked.config, arg.info, warnProveInit, $arg)
+          localError(tracked.config, arg.info, errProveInit, $arg)
 
       # check required for 'nim check':
       if n[1].typ.len > 0:
@@ -1203,12 +1203,11 @@ proc trackProc*(c: PContext; s: PSym, body: PNode) =
         createTypeBoundOps(t, typ, param.info)
 
   if not isEmptyType(s.typ[0]) and
-      ({tfRequiresInit, tfNotNil} * s.typ[0].flags != {} or
-      s.typ[0].skipTypes(abstractInst).kind == tyVar) and
-      s.kind in {skProc, skFunc, skConverter, skMethod}:
+     (s.typ[0].requiresInit or s.typ[0].skipTypes(abstractInst).kind == tyVar) and
+     s.kind in {skProc, skFunc, skConverter, skMethod}:
     var res = s.ast[resultPos].sym # get result symbol
     if res.id notin t.init:
-      message(g.config, body.info, warnProveInit, "result")
+      localError(g.config, body.info, errProveInit, "result")
   let p = s.ast[pragmasPos]
   let raisesSpec = effectSpec(p, wRaises)
   if not isNil(raisesSpec):
diff --git a/compiler/semstmts.nim b/compiler/semstmts.nim
index fc624c710..23e581a53 100644
--- a/compiler/semstmts.nim
+++ b/compiler/semstmts.nim
@@ -335,8 +335,7 @@ proc semIdentDef(c: PContext, n: PNode, kind: TSymKind): PSym =
   suggestSym(c.config, info, result, c.graph.usageSym)
 
 proc checkNilable(c: PContext; v: PSym) =
-  if {sfGlobal, sfImportc} * v.flags == {sfGlobal} and
-      {tfNotNil, tfRequiresInit} * v.typ.flags != {}:
+  if {sfGlobal, sfImportc} * v.flags == {sfGlobal} and v.typ.requiresInit:
     if v.astdef.isNil:
       message(c.config, v.info, warnProveInit, v.name.s)
     elif tfNotNil in v.typ.flags and not v.astdef.typ.isNil and tfNotNil notin v.astdef.typ.flags:
@@ -485,7 +484,7 @@ proc semVarOrLet(c: PContext, n: PNode, symkind: TSymKind): PNode =
     var a = n[i]
     if c.config.cmd == cmdIdeTools: suggestStmt(c, a)
     if a.kind == nkCommentStmt: continue
-    if a.kind notin {nkIdentDefs, nkVarTuple, nkConstDef}: illFormedAst(a, c.config)
+    if a.kind notin {nkIdentDefs, nkVarTuple}: illFormedAst(a, c.config)
     checkMinSonsLen(a, 3, c.config)
 
     var typ: PType = nil
diff --git a/compiler/semtypes.nim b/compiler/semtypes.nim
index 92e7b8bd2..1de181dbe 100644
--- a/compiler/semtypes.nim
+++ b/compiler/semtypes.nim
@@ -139,7 +139,8 @@ proc semEnum(c: PContext, n: PNode, prev: PType): PType =
     if isPure and (let conflict = strTableInclReportConflict(symbols, e); conflict != nil):
       wrongRedefinition(c, e.info, e.name.s, conflict.info)
     inc(counter)
-  if tfNotNil in e.typ.flags and not hasNull: incl(result.flags, tfRequiresInit)
+  if tfNotNil in e.typ.flags and not hasNull:
+    result.flags.incl tfRequiresInit
 
 proc semSet(c: PContext, n: PNode, prev: PType): PType =
   result = newOrPrevType(tySet, prev, c)
@@ -769,6 +770,8 @@ 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:
diff --git a/tests/constructors/tinvalid_construction.nim b/tests/constructors/tinvalid_construction.nim
index f84a18e68..46156db6c 100644
--- a/tests/constructors/tinvalid_construction.nim
+++ b/tests/constructors/tinvalid_construction.nim
@@ -32,10 +32,14 @@ type
     a {.requiresInit.}: int
     b: string
 
+  PartialRequiresInitRef = ref PartialRequiresInit
+
   FullRequiresInit {.requiresInit.} = object
     a: int
     b: int
 
+  FullRequiresInitRef = ref FullRequiresInit
+
   FullRequiresInitWithParent {.requiresInit.} = object of THasNotNils
     e: int
     d: int
@@ -72,8 +76,13 @@ var nilRef: TRefObj
 var notNilRef = TRefObj(x: 20)
 
 proc makeHasNotNils: ref THasNotNils =
-  result.a = TRefObj(x: 10)
-  result.b = TRefObj(x: 20)
+  (ref THasNotNils)(a: TRefObj(x: 10),
+                    b: TRefObj(x: 20))
+
+proc userDefinedDefault(T: typedesc): T =
+  # We'll use that to make sure the user cannot cheat
+  # with constructing requiresInit types
+  discard
 
 accept TObj()
 accept TObj(choice: A)
@@ -125,7 +134,17 @@ accept PartialRequiresInit(a: 10, b: "x")
 accept PartialRequiresInit(a: 20)
 reject PartialRequiresInit(b: "x")
 reject PartialRequiresInit()
+accept PartialRequiresInitRef(a: 10, b: "x")
+accept PartialRequiresInitRef(a: 20)
+reject PartialRequiresInitRef(b: "x")
+reject PartialRequiresInitRef()
+accept((ref PartialRequiresInit)(a: 10, b: "x"))
+accept((ref PartialRequiresInit)(a: 20))
+reject((ref PartialRequiresInit)(b: "x"))
+reject((ref PartialRequiresInit)())
+
 reject default(PartialRequiresInit)
+reject userDefinedDefault(PartialRequiresInit)
 reject:
   var obj: PartialRequiresInit
 
@@ -133,7 +152,17 @@ accept FullRequiresInit(a: 10, b: 20)
 reject FullRequiresInit(a: 10)
 reject FullRequiresInit(b: 20)
 reject FullRequiresInit()
+accept FullRequiresInitRef(a: 10, b: 20)
+reject FullRequiresInitRef(a: 10)
+reject FullRequiresInitRef(b: 20)
+reject FullRequiresInitRef()
+accept((ref FullRequiresInit)(a: 10, b: 20))
+reject((ref FullRequiresInit)(a: 10))
+reject((ref FullRequiresInit)(b: 20))
+reject((ref FullRequiresInit)())
+
 reject default(FullRequiresInit)
+reject userDefinedDefault(FullRequiresInit)
 reject:
   var obj: FullRequiresInit
 
@@ -144,6 +173,7 @@ reject FullRequiresInitWithParent(a: notNilRef, b: notNilRef, e: 10, d: 20)   #
 reject FullRequiresInitWithParent(a: notNilRef, b: notNilRef, c: nil, e: 10)  # d should not be missing
 reject FullRequiresInitWithParent()
 reject default(FullRequiresInitWithParent)
+reject userDefinedDefault(FullRequiresInitWithParent)
 reject:
   var obj: FullRequiresInitWithParent
 
@@ -153,6 +183,55 @@ accept default(TNestedChoices)
 accept:
   var obj: TNestedChoices
 
+reject:
+  # This proc is illegal, because it tries to produce
+  # a default object of a type that requires initialization:
+  proc defaultHasNotNils: THasNotNils =
+    discard
+
+reject:
+  # You cannot cheat by using the result variable to specify
+  # only some of the fields
+  proc invalidPartialTHasNotNils: THasNotNils =
+    result.c = nilRef
+
+reject:
+  # The same applies for requiresInit types
+  proc invalidPartialRequiersInit: PartialRequiresInit =
+    result.b = "x"
+
+# All code paths must return a value when the result requires initialization:
+reject:
+  proc ifWithoutAnElse: THasNotNils =
+    if stdin.readLine == "":
+      return THasNotNils(a: notNilRef, b: notNilRef, c: nilRef)
+
+accept:
+  # All code paths must return a value when the result requires initialization:
+  proc wellFormedIf: THasNotNils =
+    if stdin.readLine == "":
+      return THasNotNils(a: notNilRef, b: notNilRef, c: nilRef)
+    else:
+      return THasNotNIls(a: notNilRef, b: notNilRef)
+
+reject:
+  proc caseWithoutAllCasesCovered: FullRequiresInit =
+    # Please note that these is no else branch here:
+    case stdin.readLine
+    of "x":
+      return FullRequiresInit(a: 10, b: 20)
+    of "y":
+      return FullRequiresInit(a: 30, b: 40)
+
+accept:
+  proc wellFormedCase: FullRequiresInit =
+    case stdin.readLine
+    of "x":
+      result = FullRequiresInit(a: 10, b: 20)
+    else:
+      # Mixing result and return is fine:
+      return FullRequiresInit(a: 30, b: 40)
+
 # but if we supply a run-time value for the inner branch, the compiler won't be able to prove
 # that the notnil field was initialized
 reject TNestedChoices(outerChoice: false, innerChoice: x) # XXX: The error message is not very good here