Merge pull request #5564 from nim-lang/fix/4556

Fix/4556

compiler/sem.nim

@@ -45,7 +45,7 @@ proc tryExpr(c: PContext, n: PNode, flags: TExprFlags = {}): PNode
 proc activate(c: PContext, n: PNode)
 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 isArrayConstr(n: PNode): bool {.inline.} =
@@ -328,6 +328,20 @@ proc semConstExpr(c: PContext, n: PNode): PNode =
     else:
       result = fixupTypeAfterEval(c, result, e)
 
+proc semExprFlagDispatched(c: PContext, n: PNode, flags: TExprFlags): PNode =
+  if efNeedStatic in flags:
+    if efPreferNilResult in flags:
+      return tryConstExpr(c, n)
+    else:
+      return semConstExpr(c, n)
+  else:
+    result = semExprWithType(c, n, flags)
+    if efPreferStatic in flags:
+      var evaluated = getConstExpr(c.module, result)
+      if evaluated != nil: return evaluated
+      evaluated = evalAtCompileTime(c, result)
+      if evaluated != nil: return evaluated
+
 include hlo, seminst, semcall
 
 when false:

compiler/semdata.nim

@@ -46,6 +46,17 @@ type
 
   TExprFlag* = enum
     efLValue, efWantIterator, efInTypeof,
+    efNeedStatic,
+      # Use this in contexts where a static value is mandatory
+    efPreferStatic,
+      # Use this in contexts where a static value could bring more
+      # information, but it's not strictly mandatory. This may become
+      # the default with implicit statics in the future.
+    efPreferNilResult,
+      # Use this if you want a certain result (e.g. static value),
+      # but you don't want to trigger a hard error. For example,
+      # you may be in position to supply a better error message
+      # to the user.
     efWantStmt, efAllowStmt, efDetermineType, efExplain,
     efAllowDestructor, efWantValue, efOperand, efNoSemCheck,
     efNoProcvarCheck, efNoEvaluateGeneric, efInCall, efFromHlo,

compiler/semexprs.nim

@@ -2097,75 +2097,7 @@ proc isTupleType(n: PNode): bool =
       return false
   return true
 
-proc checkInitialized(n: PNode, ids: IntSet, info: TLineInfo) =
-  case n.kind
-  of nkRecList:
-    for i in countup(0, sonsLen(n) - 1):
-      checkInitialized(n.sons[i], ids, info)
-  of nkRecCase:
-    if (n.sons[0].kind != nkSym): internalError(info, "checkInitialized")
-    checkInitialized(n.sons[0], ids, info)
-    when false:
-      # XXX we cannot check here, as we don't know the branch!
-      for i in countup(1, sonsLen(n) - 1):
-        case n.sons[i].kind
-        of nkOfBranch, nkElse: checkInitialized(lastSon(n.sons[i]), ids, info)
-        else: internalError(info, "checkInitialized")
-  of nkSym:
-    if {tfNotNil, tfNeedsInit} * n.sym.typ.flags != {} and
-        n.sym.name.id notin ids:
-      message(info, errGenerated, "field not initialized: " & n.sym.name.s)
-  else: internalError(info, "checkInitialized")
-
-proc semObjConstr(c: PContext, n: PNode, flags: TExprFlags): PNode =
-  var t = semTypeNode(c, n.sons[0], nil)
-  result = newNodeIT(nkObjConstr, n.info, t)
-  result.add n.sons[0]
-  t = skipTypes(t, {tyGenericInst, tyAlias})
-  if t.kind == tyRef: t = skipTypes(t.sons[0], {tyGenericInst, tyAlias})
-  if t.kind != tyObject:
-    localError(n.info, errGenerated, "object constructor needs an object type")
-    return
-  var objType = t
-  var ids = initIntSet()
-  for i in 1.. <n.len:
-    let it = n.sons[i]
-    if it.kind != nkExprColonExpr:
-      localError(n.info, errNamedExprExpected)
-      break
-    let id = considerQuotedIdent(it.sons[0], it)
-
-    if containsOrIncl(ids, id.id):
-      localError(it.info, errFieldInitTwice, id.s)
-    var e = semExprWithType(c, it.sons[1], flags*{efAllowDestructor})
-    var
-      check: PNode = nil
-      f: PSym
-    t = objType
-    while true:
-      check = nil
-      f = lookupInRecordAndBuildCheck(c, it, t.n, id, check)
-      if f != nil: break
-      if t.sons[0] == nil: break
-      t = skipTypes(t.sons[0], skipPtrs)
-    if f != nil and fieldVisible(c, f):
-      it.sons[0] = newSymNode(f)
-      e = fitNode(c, f.typ, e, it.info)
-      # small hack here in a nkObjConstr the ``nkExprColonExpr`` node can have
-      # 3 children the last being the field check
-      if check != nil:
-        check.sons[0] = it.sons[0]
-        it.add(check)
-    else:
-      localError(it.info, errUndeclaredFieldX, id.s)
-    it.sons[1] = e
-    result.add it
-    # XXX object field name check for 'case objects' if the kind is static?
-  if tfNeedsInit in objType.flags:
-    while true:
-      checkInitialized(objType.n, ids, n.info)
-      if objType.sons[0] == nil: break
-      objType = skipTypes(objType.sons[0], skipPtrs)
+include semobjconstr
 
 proc semBlock(c: PContext, n: PNode): PNode =
   result = n

compiler/semobjconstr.nim

@@ -0,0 +1,292 @@
+#
+#
+#           The Nim Compiler
+#        (c) Copyright 2015 Nim Contributors
+#
+#    See the file "copying.txt", included in this
+#    distribution, for details about the copyright.
+#
+
+## This module implements Nim's object construction rules.
+
+# included from sem.nim
+
+type
+  InitStatus = enum
+    initUnknown
+    initFull     # All  of the fields have been initialized
+    initPartial  # Some of the fields have been initialized
+    initNone     # None of the fields have been initialized
+    initConflict # Fields from different branches have been initialized
+
+proc mergeInitStatus(existing: var InitStatus, newStatus: InitStatus) =
+  case newStatus
+  of initConflict:
+    existing = newStatus
+  of initPartial:
+    if existing in {initUnknown, initFull, initNone}:
+      existing = initPartial
+  of initNone:
+    if existing == initUnknown:
+      existing = initNone
+    elif existing == initFull:
+      existing = initPartial
+  of initFull:
+    if existing == initUnknown:
+      existing = initFull
+    elif existing == initNone:
+      existing = initPartial
+  of initUnknown:
+    discard
+
+proc locateFieldInInitExpr(field: PSym, initExpr: PNode): PNode =
+  # Returns the assignment nkExprColonExpr node or nil
+  let fieldId = field.name.id
+  for i in 1 .. <initExpr.len:
+    let assignment = initExpr[i]
+    internalAssert assignment.kind == nkExprColonExpr
+
+    if fieldId == considerQuotedIdent(assignment[0]).id:
+      return assignment
+
+proc semConstrField(c: PContext, flags: TExprFlags,
+                    field: PSym, initExpr: PNode): PNode =
+  let assignment = locateFieldInInitExpr(field, initExpr)
+  if assignment != nil:
+    if nfSem in assignment.flags: return assignment[1]
+    if not fieldVisible(c, field):
+      localError(initExpr.info,
+        "the field '$1' is not accessible.", [field.name.s])
+      return
+
+    var initValue = semExprFlagDispatched(c, assignment[1], flags)
+    if initValue != nil:
+      initValue = fitNode(c, field.typ, initValue, assignment.info)
+    assignment.sons[0] = newSymNode(field)
+    assignment.sons[1] = initValue
+    assignment.flags.incl nfSem
+    return initValue
+
+proc caseBranchMatchesExpr(branch, matched: PNode): bool =
+  for i in 0 .. (branch.len - 2):
+    if exprStructuralEquivalent(branch[i], matched):
+      return true
+
+  return false
+
+proc pickCaseBranch(caseExpr, matched: PNode): PNode =
+  # XXX: Perhaps this proc already exists somewhere
+  let endsWithElse = caseExpr{-1}.kind == nkElse
+  for i in 1 .. caseExpr.len - 1 - int(endsWithElse):
+    if caseExpr[i].caseBranchMatchesExpr(matched):
+      return caseExpr[i]
+
+  if endsWithElse:
+    return caseExpr{-1}
+
+iterator directFieldsInRecList(recList: PNode): PNode =
+  # XXX: We can remove this case by making all nkOfBranch nodes
+  # regular. Currently, they try to avoid using nkRecList if they
+  # include only a single field
+  if recList.kind == nkSym:
+    yield recList
+  else:
+    internalAssert recList.kind == nkRecList
+    for field in recList:
+      if field.kind != nkSym: continue
+      yield field
+
+template quoteStr(s: string): string = "'" & s & "'"
+
+proc fieldsPresentInInitExpr(fieldsRecList, initExpr: PNode): string =
+  result = ""
+  for field in directFieldsInRecList(fieldsRecList):
+    let assignment = locateFieldInInitExpr(field.sym, initExpr)
+    if assignment != nil:
+      if result.len != 0: result.add ", "
+      result.add field.sym.name.s.quoteStr
+
+proc missingMandatoryFields(fieldsRecList, initExpr: PNode): string =
+  for r in directFieldsInRecList(fieldsRecList):
+    if {tfNotNil, tfNeedsInit} * r.sym.typ.flags != {}:
+      let assignment = locateFieldInInitExpr(r.sym, initExpr)
+      if assignment == nil:
+        if result == nil:
+          result = r.sym.name.s
+        else:
+          result.add ", "
+          result.add r.sym.name.s
+
+proc checkForMissingFields(recList, initExpr: PNode) =
+  let missing = missingMandatoryFields(recList, initExpr)
+  if missing != nil:
+    localError(initExpr.info, "fields not initialized: $1.", [missing])
+
+proc semConstructFields(c: PContext, recNode: PNode,
+                        initExpr: PNode, flags: TExprFlags): InitStatus =
+  result = initUnknown
+
+  case recNode.kind
+  of nkRecList:
+    for field in recNode:
+      let status = semConstructFields(c, field, initExpr, flags)
+      mergeInitStatus(result, status)
+
+  of nkRecCase:
+    template fieldsPresentInBranch(branchIdx: int): string =
+      fieldsPresentInInitExpr(recNode[branchIdx]{-1}, initExpr)
+
+    template checkMissingFields(branchNode: PNode) =
+      checkForMissingFields(branchNode{-1}, initExpr)
+
+    let discriminator = recNode.sons[0];
+    internalAssert discriminator.kind == nkSym
+    var selectedBranch = -1
+
+    for i in 1 .. <recNode.len:
+      let innerRecords = recNode[i]{-1}
+      let status = semConstructFields(c, innerRecords, initExpr, flags)
+      if status notin {initNone, initUnknown}:
+        mergeInitStatus(result, status)
+        if selectedBranch != -1:
+          let prevFields = fieldsPresentInBranch(selectedBranch)
+          let currentFields = fieldsPresentInBranch(i)
+          localError(initExpr.info,
+            "The fields ($1) and ($2) cannot be initialized together, " &
+            "because they are from conflicting branches in the case object.",
+            [prevFields, currentFields])
+          result = initConflict
+        else:
+          selectedBranch = i
+
+    if selectedBranch != -1:
+      let branchNode = recNode[selectedBranch]
+      let flags = flags*{efAllowDestructor} + {efNeedStatic, efPreferNilResult}
+      let discriminatorVal = semConstrField(c, flags,
+                                            discriminator.sym, initExpr)
+      if discriminatorVal == nil:
+        let fields = fieldsPresentInBranch(selectedBranch)
+        localError(initExpr.info,
+          "you must provide a compile-time value for the discriminator '$1' " &
+          "in order to prove that it's safe to initialize $2.",
+          [discriminator.sym.name.s, fields])
+        mergeInitStatus(result, initNone)
+      else:
+        let discriminatorVal = discriminatorVal.skipHidden
+
+        template wrongBranchError(i) =
+          let fields = fieldsPresentInBranch(i)
+          localError(initExpr.info,
+            "a case selecting discriminator '$1' with value '$2' " &
+            "appears in the object construction, but the field(s) $3 " &
+            "are in conflict with this value.",
+            [discriminator.sym.name.s, discriminatorVal.renderTree, fields])
+
+        if branchNode.kind != nkElse:
+          if not branchNode.caseBranchMatchesExpr(discriminatorVal):
+            wrongBranchError(selectedBranch)
+        else:
+          # With an else clause, check that all other branches don't match:
+          for i in 1 .. (recNode.len - 2):
+            if recNode[i].caseBranchMatchesExpr(discriminatorVal):
+              wrongBranchError(i)
+              break
+
+      # When a branch is selected with a partial match, some of the fields
+      # that were not initialized may be mandatory. We must check for this:
+      if result == initPartial:
+        checkMissingFields branchNode
+
+    else:
+      result = initNone
+      let discriminatorVal = semConstrField(c, flags + {efPreferStatic},
+                                            discriminator.sym, initExpr)
+      if discriminatorVal == nil:
+        # None of the branches were explicitly selected by the user and no
+        # value was given to the discrimator. We can assume that it will be
+        # initialized to zero and this will select a particular branch as
+        # a result:
+        let matchedBranch = recNode.pickCaseBranch newIntLit(0)
+        checkMissingFields matchedBranch
+      else:
+        result = initPartial
+        if discriminatorVal.kind == nkIntLit:
+          # When the discriminator is a compile-time value, we also know
+          # which brach will be selected:
+          let matchedBranch = recNode.pickCaseBranch discriminatorVal
+          if matchedBranch != nil: checkMissingFields matchedBranch
+        else:
+          # All bets are off. If any of the branches has a mandatory
+          # fields we must produce an error:
+          for i in 1 .. <recNode.len: checkMissingFields recNode[i]
+
+  of nkSym:
+    let field = recNode.sym
+    let e = semConstrField(c, flags, field, initExpr)
+    result = if e != nil: initFull else: initNone
+
+  else:
+    internalAssert false
+
+proc semConstructType(c: PContext, initExpr: PNode,
+                      t: PType, flags: TExprFlags): InitStatus =
+  var t = t
+  result = initUnknown
+
+  while true:
+    let status = semConstructFields(c, t.n, initExpr, flags)
+    mergeInitStatus(result, status)
+
+    if status in {initPartial, initNone, initUnknown}:
+      checkForMissingFields t.n, initExpr
+
+    let base = t.sons[0]
+    if base == nil: break
+    t = skipTypes(base, skipPtrs)
+
+proc semObjConstr(c: PContext, n: PNode, flags: TExprFlags): PNode =
+  var t = semTypeNode(c, n.sons[0], nil)
+  result = newNodeIT(nkObjConstr, n.info, t)
+  for child in n: result.add child
+  
+  t = skipTypes(t, {tyGenericInst, tyAlias})
+  if t.kind == tyRef: t = skipTypes(t.sons[0], {tyGenericInst, tyAlias})
+  if t.kind != tyObject:
+    localError(n.info, errGenerated, "object constructor needs an object type")
+    return
+
+  # 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)
+
+  # It's possible that the object was not fully initialized while
+  # specifying a .requiresInit. pragma.
+  # XXX: Turn this into an error in the next release
+  if tfNeedsInit in t.flags and initResult != initFull:
+    # XXX: Disable this warning for now, because tfNeedsInit is propagated
+    # too aggressively from fields to object types (and this is not correct
+    # in case objects)
+    when false: message(n.info, warnUser,
+      "object type uses the 'requiresInit' pragma, but not all fields " &
+      "have been initialized. future versions of Nim will treat this as " &
+      "an error")
+
+  # Since we were traversing the object fields, it's possible that
+  # not all of the fields specified in the constructor was visited.
+  # We'll check for such fields here:
+  for i in 1.. <result.len:
+    let field = result[i]
+    if nfSem notin field.flags:
+      let id = considerQuotedIdent(field[0])
+      # This node was not processed. There are two possible reasons:
+      # 1) It was shadowed by a field with the same name on the left
+      for j in 1 .. <i:
+        let prevId = considerQuotedIdent(result[j][0])
+        if prevId.id == id.id:
+          localError(field.info, errFieldInitTwice, id.s)
+          return
+      # 2) No such field exists in the constructed type
+      localError(field.info, errUndeclaredFieldX, id.s)
+      return
+

doc/manual/stmts.txt

@@ -130,9 +130,9 @@ If a proc is annotated with the ``noinit`` pragma this refers to its implicit
 
 
 The implicit initialization can be also prevented by the `requiresInit`:idx:
-type pragma. The compiler requires an explicit initialization then. However
-it does a `control flow analysis`:idx: to prove the variable has been
-initialized and does not rely on syntactic properties:
+type pragma. The compiler requires an explicit initialization for the object
+and all of its fields. However it does a `control flow analysis`:idx: to prove
+the variable has been initialized and does not rely on syntactic properties:
 
 .. code-block:: nim
   type

doc/manual/types.txt

@@ -694,7 +694,9 @@ the ``case`` statement: The branches in a ``case`` section may be indented too.
 In the example the ``kind`` field is called the `discriminator`:idx:\: For
 safety its address cannot be taken and assignments to it are restricted: The
 new value must not lead to a change of the active object branch. For an object
-branch switch ``system.reset`` has to be used.
+branch switch ``system.reset`` has to be used. Also, when the fields of a
+particular branch are specified during object construction, the correct value
+for the discriminator must be supplied at compile-time.
 
 
 Set type

tests/constructors/tinvalid_construction.nim

@@ -0,0 +1,122 @@
+template accept(x) =
+  static: assert compiles(x)
+
+template reject(x) =
+  static: assert(not compiles(x))
+
+type
+  TRefObj = ref object
+    x: int
+
+  THasNotNils = object of TObject
+    a: TRefObj not nil
+    b: TRefObj not nil
+    c: TRefObj
+
+  THasNotNilsRef = ref THasNotNils
+
+  TChoice = enum A, B, C, D, E, F
+
+  TBaseHasNotNils = object of THasNotNils
+    case choice: TChoice
+    of A:
+      moreNotNils: THasNotNils
+    of B:
+      indirectNotNils: ref THasNotNils
+    else:
+      discard
+
+  TObj = object
+    case choice: TChoice
+    of A:
+      a: int
+    of B, C:
+      bc: int
+    of D:
+      d: TRefObj
+    of E:
+      e1: TRefObj
+      e2: int
+    else:
+      f: string
+
+  TNestedChoices = object
+    case outerChoice: bool
+    of true:
+      truthy: int
+    else:
+      case innerChoice: TChoice
+      of A:
+        a: int
+      of B:
+        b: int
+      else:
+        notnil: TRefObj not nil
+
+var x = D
+var nilRef: TRefObj
+var notNilRef = TRefObj(x: 20)
+
+proc makeHasNotNils: ref THasNotNils =
+  result.a = TRefObj(x: 10)
+  result.b = TRefObj(x: 20)
+
+accept TObj()
+accept TObj(choice: A)
+reject TObj(choice: A, bc: 10)  # bc is in the wrong branch
+accept TObj(choice: B, bc: 20)
+reject TObj(a: 10)              # branch selected without providing discriminator
+reject TObj(choice: x, a: 10)   # the discrimantor must be a compile-time value when a branch is selected
+accept TObj(choice: x)          # it's OK to use run-time value when a branch is not selected
+accept TObj(choice: F, f: "")   # match an else clause
+reject TObj(f: "")              # the discriminator must still be provided for an else clause
+reject TObj(a: 10, f: "")       # conflicting fields
+accept TObj(choice: E, e1: TRefObj(x: 10), e2: 10)
+
+accept THasNotNils(a: notNilRef, b: notNilRef, c: nilRef)
+# XXX: the "not nil" logic in the compiler is not strong enough to catch this one yet:
+# reject THasNotNils(a: notNilRef, b: nilRef, c: nilRef)
+reject THasNotNils(b: notNilRef, c: notNilRef)              # there is a missing not nil field
+reject THasNotNils()                                        # again, missing fields
+accept THasNotNils(a: notNilRef, b: notNilRef)              # it's OK to omit a non-mandatory field
+
+# missing not nils in base
+reject TBaseHasNotNils()
+
+# once you take care of them, it's ok
+accept TBaseHasNotNils(a: notNilRef, b: notNilRef, choice: D)
+
+# this one is tricky!
+# it has to be rejected, because choice gets value A by default (0) and this means
+# that the THasNotNils field will be active (and it will demand more initialized fields).
+reject TBaseHasNotNils(a: notNilRef, b: notNilRef)
+
+# you can select a branch without mandatory fields
+accept TBaseHasNotNils(a: notNilRef, b: notNilRef, choice: B)
+accept TBaseHasNotNils(a: notNilRef, b: notNilRef, choice: B, indirectNotNils: nil)
+
+# but once you select a branch with mandatory fields, you must specify them
+reject TBaseHasNotNils(a: notNilRef, b: notNilRef, choice: A)
+reject TBaseHasNotNils(a: notNilRef, b: notNilRef, choice: A, indirectNotNils: nil)
+reject TBaseHasNotNils(a: notNilRef, b: notNilRef, choice: A, moreNotNils: THasNotNils())
+accept TBaseHasNotNils(a: notNilRef, b: notNilRef, choice: A, moreNotNils: THasNotNils(a: notNilRef, b: notNilRef))
+
+# all rules apply to sub-objects as well
+accept TBaseHasNotNils(a: notNilRef, b: notNilRef, choice: B, indirectNotNils: makeHasNotNils())
+reject TBaseHasNotNils(a: notNilRef, b: notNilRef, choice: B, indirectNotNils: THasNotNilsRef())
+accept TBaseHasNotNils(a: notNilRef, b: notNilRef, choice: B, indirectNotNils: THasNotNilsRef(a: notNilRef, b: notNilRef))
+
+# this will be accepted, because the false outer branch will be taken and the inner A branch
+accept TNestedChoices()
+
+# 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
+reject TNestedChoices(outerChoice: true,  innerChoice: A) # XXX: The error message is not very good here
+
+accept TNestedChoices(outerChoice: false, innerChoice: B)
+
+reject TNestedChoices(outerChoice: false, innerChoice: C)
+accept TNestedChoices(outerChoice: false, innerChoice: C, notnil: notNilRef)
+reject TNestedChoices(outerChoice: false, innerChoice: C, notnil: nil)
+

tests/notnil/tnotnil_in_objconstr.nim

@@ -1,5 +1,5 @@
 discard """
-  errormsg: "field not initialized: bar"
+  errormsg: "fields not initialized: bar"
   line: "13"
 """
 

web/news/e031_version_0_16_2.rst

@@ -49,7 +49,13 @@ Changes affecting backwards compatibility
   instead of signed integers.
 - In Nim identifiers en-dash (Unicode point U+2013) is not an alias for the
   underscore anymore. Use underscores and fix your programming font instead.
-
+- When the ``requiresInit`` pragma is applied to a record type, future versions
+  of Nim will also require you to initialize all the fields of the type during
+  object construction. For now, only a warning will be produced.
+- The Object construction syntax now performs a number of additional safety
+  checks. When fields within case objects are initialiazed, the compiler will
+  now demand that the respective discriminator field has a matching known
+  compile-time value.
 
 Library Additions
 -----------------