From 6edb07091dafd4f0214c4aa2df8fc862d03e0a99 Mon Sep 17 00:00:00 2001 From: Zahary Karadjov Date: Sat, 18 Mar 2017 01:44:51 +0200 Subject: [PATCH 1/5] fix #4556 This implements a number of new safety checks and error messages when object constructors are used: In case objects: * the compiler will prevent you from initializing fields in conflicting branches * When a field from a particular branch is initialized, the compiler will demand that the discriminator field is also supplied with a maching compile-time value In all objects: * When the "requiresInit" pragma is applied to a type, all fields of the type must be initialized when object construction is used. The code will be simplified in a follow up commit. --- compiler/semdata.nim | 2 +- compiler/semexprs.nim | 219 +++++++++++++++++++++++++++++++----------- 2 files changed, 165 insertions(+), 56 deletions(-) diff --git a/compiler/semdata.nim b/compiler/semdata.nim index 023b85802..5c2427401 100644 --- a/compiler/semdata.nim +++ b/compiler/semdata.nim @@ -45,7 +45,7 @@ type inst*: PInstantiation TExprFlag* = enum - efLValue, efWantIterator, efInTypeof, + efLValue, efWantIterator, efWantStatic, efInTypeof, efWantStmt, efAllowStmt, efDetermineType, efExplain, efAllowDestructor, efWantValue, efOperand, efNoSemCheck, efNoProcvarCheck, efNoEvaluateGeneric, efInCall, efFromHlo, diff --git a/compiler/semexprs.nim b/compiler/semexprs.nim index 5f9263645..032b146d3 100644 --- a/compiler/semexprs.nim +++ b/compiler/semexprs.nim @@ -2097,25 +2097,137 @@ proc isTupleType(n: PNode): bool = return false return true -proc checkInitialized(n: PNode, ids: IntSet, info: TLineInfo) = - case n.kind +type + InitializationResult = 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 + + InitStatus = object + status: InitializationResult + missingFields: seq[int] + conflictingFields: seq[int] + assumptions: seq[(int, PNode)] # field and expected value + +proc mergeInitStatus(existing: var InitializationResult, + newStatus: InitializationResult) = + case newStatus + of initConflict: + existing = initConflict + 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 semConstrField(c: PContext, flags: TExprFlags, + field: PSym, initExpr: PNode): PNode = + let fieldId = field.name.id + + for i in 1 .. = 0 and selectedBranch < recNode.len - 1: + let discriminatorVal = semConstrField(c, flags + {efWantStatic}, + discriminator.sym, initExpr) + if discriminatorVal == nil: + localError(initExpr.info, "discrimantor not const") + elif not discriminatorVal.matchesAnyCaseInNkOf(recNode[selectedBranch]): + localError(initExpr.info, "wrong branch taken") + else: + result.status = initNone + let f = semConstrField(c, flags, discriminator.sym, initExpr) + mergeInitStatus(result.status, if f != nil: initFull else: initNone) + 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") + let e = semConstrField(c, flags, recNode.sym, initExpr) + result.status = if e != nil: initFull else: initNone + + else: + internalAssert false proc semObjConstr(c: PContext, n: PNode, flags: TExprFlags): PNode = var t = semTypeNode(c, n.sons[0], nil) @@ -2126,46 +2238,43 @@ proc semObjConstr(c: PContext, n: PNode, flags: TExprFlags): PNode = 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.. Date: Sat, 18 Mar 2017 02:01:52 +0200 Subject: [PATCH 2/5] News items for previous commit --- doc/manual/stmts.txt | 6 +++--- web/news/e031_version_0_16_2.rst | 8 +++++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/doc/manual/stmts.txt b/doc/manual/stmts.txt index fa1cac8e1..5668b8cc2 100644 --- a/doc/manual/stmts.txt +++ b/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 diff --git a/web/news/e031_version_0_16_2.rst b/web/news/e031_version_0_16_2.rst index a15e715d6..fd12bb822 100644 --- a/web/news/e031_version_0_16_2.rst +++ b/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 ----------------- From 564c0acae20419133f3fadd17be4bae42dd408d9 Mon Sep 17 00:00:00 2001 From: Zahary Karadjov Date: Sat, 18 Mar 2017 17:22:26 +0200 Subject: [PATCH 3/5] cleaned up the code and implemented proper error messages --- compiler/sem.nim | 16 +- compiler/semdata.nim | 15 +- compiler/semexprs.nim | 259 ++++++++++++++++++-------- tests/notnil/tnotnil_in_objconstr.nim | 2 +- 4 files changed, 209 insertions(+), 83 deletions(-) diff --git a/compiler/sem.nim b/compiler/sem.nim index 57b87e0bb..ab3c5cec8 100644 --- a/compiler/sem.nim +++ b/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: diff --git a/compiler/semdata.nim b/compiler/semdata.nim index 5c2427401..3abfeac21 100644 --- a/compiler/semdata.nim +++ b/compiler/semdata.nim @@ -45,8 +45,19 @@ type inst*: PInstantiation TExprFlag* = enum - efLValue, efWantIterator, efWantStatic, efInTypeof, - efWantStmt, efAllowStmt, efDetermineType, efExplain, + 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, exExplain, efAllowDestructor, efWantValue, efOperand, efNoSemCheck, efNoProcvarCheck, efNoEvaluateGeneric, efInCall, efFromHlo, diff --git a/compiler/semexprs.nim b/compiler/semexprs.nim index 032b146d3..f80f67160 100644 --- a/compiler/semexprs.nim +++ b/compiler/semexprs.nim @@ -2098,24 +2098,17 @@ proc isTupleType(n: PNode): bool = return true type - InitializationResult = enum + 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 - InitStatus = object - status: InitializationResult - missingFields: seq[int] - conflictingFields: seq[int] - assumptions: seq[(int, PNode)] # field and expected value - -proc mergeInitStatus(existing: var InitializationResult, - newStatus: InitializationResult) = +proc mergeInitStatus(existing: var InitStatus, newStatus: InitStatus) = case newStatus of initConflict: - existing = initConflict + existing = newStatus of initPartial: if existing in {initUnknown, initFull, initNone}: existing = initPartial @@ -2132,103 +2125,211 @@ proc mergeInitStatus(existing: var InitializationResult, 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 .. = 0 and selectedBranch < recNode.len - 1: - let discriminatorVal = semConstrField(c, flags + {efWantStatic}, - discriminator.sym, initExpr) + if selectedBranch != -1: + let branchNode = recNode[selectedBranch] + let flags = flags*{efAllowDestructor} + {efNeedStatic, efPreferNilResult} + let discriminatorVal = semConstrField(c, flags, + discriminator.sym, initExpr) if discriminatorVal == nil: - localError(initExpr.info, "discrimantor not const") - elif not discriminatorVal.matchesAnyCaseInNkOf(recNode[selectedBranch]): - localError(initExpr.info, "wrong branch taken") + let fields = fieldsPresentInBranch(selectedBranch) + localError(initExpr.info, + "the discriminator '$1' appearing in the construction of a case " & + "object must be a compile-time value in order to prove that it's " & + "initialize field(s) $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.status = initNone - let f = semConstrField(c, flags, discriminator.sym, initExpr) - mergeInitStatus(result.status, if f != nil: initFull else: initNone) + 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 .. Date: Sat, 18 Mar 2017 20:21:36 +0200 Subject: [PATCH 4/5] object construction: test cases and manual additions --- compiler/semexprs.nim | 18 ++- doc/manual/types.txt | 4 +- tests/constructors/tinvalid_construction.nim | 122 +++++++++++++++++++ 3 files changed, 133 insertions(+), 11 deletions(-) create mode 100644 tests/constructors/tinvalid_construction.nim diff --git a/compiler/semexprs.nim b/compiler/semexprs.nim index f80f67160..5a71896ef 100644 --- a/compiler/semexprs.nim +++ b/compiler/semexprs.nim @@ -2146,7 +2146,8 @@ proc semConstrField(c: PContext, flags: TExprFlags, return var initValue = semExprFlagDispatched(c, assignment[1], flags) - initValue = fitNode(c, field.typ, initValue, assignment.info) + if initValue != nil: + initValue = fitNode(c, field.typ, initValue, assignment.info) assignment.sons[0] = newSymNode(field) assignment.sons[1] = initValue assignment.flags.incl nfSem @@ -2252,9 +2253,8 @@ proc semConstructFields(c: PContext, recNode: PNode, if discriminatorVal == nil: let fields = fieldsPresentInBranch(selectedBranch) localError(initExpr.info, - "the discriminator '$1' appearing in the construction of a case " & - "object must be a compile-time value in order to prove that it's " & - "initialize field(s) $2.", + "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: @@ -2344,17 +2344,15 @@ proc semObjConstr(c: PContext, n: PNode, flags: TExprFlags): PNode = # field (if this is a case object, initialized fields in two different # branches will be reported as an error): let initResult = semContructType(c, t, n, flags) - if initResult == initConflict: - localError(n.info, - "invalid object construction. " & - "fields from conflicting case branches have been initialized.") - return # 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: - message(n.info, warnUser, + # 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") diff --git a/doc/manual/types.txt b/doc/manual/types.txt index 57e086558..e6875f2df 100644 --- a/doc/manual/types.txt +++ b/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 diff --git a/tests/constructors/tinvalid_construction.nim b/tests/constructors/tinvalid_construction.nim new file mode 100644 index 000000000..bb3b1bebb --- /dev/null +++ b/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) + From 34c34cb49b63b04ddb2b0b2680210da742a4545d Mon Sep 17 00:00:00 2001 From: Zahary Karadjov Date: Thu, 6 Apr 2017 00:44:46 +0300 Subject: [PATCH 5/5] move the object construction logic to a separate file --- compiler/semdata.nim | 2 +- compiler/semexprs.nim | 278 +----------------------------------- compiler/semobjconstr.nim | 292 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 294 insertions(+), 278 deletions(-) create mode 100644 compiler/semobjconstr.nim diff --git a/compiler/semdata.nim b/compiler/semdata.nim index 3abfeac21..8f2c802de 100644 --- a/compiler/semdata.nim +++ b/compiler/semdata.nim @@ -57,7 +57,7 @@ type # 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, exExplain, + efWantStmt, efAllowStmt, efDetermineType, efExplain, efAllowDestructor, efWantValue, efOperand, efNoSemCheck, efNoProcvarCheck, efNoEvaluateGeneric, efInCall, efFromHlo, diff --git a/compiler/semexprs.nim b/compiler/semexprs.nim index 5a71896ef..316cf55c8 100644 --- a/compiler/semexprs.nim +++ b/compiler/semexprs.nim @@ -2097,283 +2097,7 @@ proc isTupleType(n: PNode): bool = return false return true -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 ..