From ce9a4ed124d798d0287a62e4700a32f1d15878c9 Mon Sep 17 00:00:00 2001 From: Zahary Karadjov Date: Mon, 30 Mar 2020 18:56:03 +0300 Subject: 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). --- compiler/ast.nim | 14 +--- compiler/pragmas.nim | 2 +- compiler/sem.nim | 4 +- compiler/semdata.nim | 1 + compiler/semexprs.nim | 14 +++- compiler/semobjconstr.nim | 61 +++++++++------ compiler/sempass2.nim | 6 +- compiler/semstmts.nim | 6 +- compiler/semtypes.nim | 4 +- compiler/semtypinst.nim | 2 + tests/constructors/tinvalid_construction.nim | 112 +++++++++++++++++++++++++++ 11 files changed, 181 insertions(+), 45 deletions(-) diff --git a/compiler/ast.nim b/compiler/ast.nim index 01f9ed29b..f4f680811 100644 --- a/compiler/ast.nim +++ b/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 diff --git a/compiler/pragmas.nim b/compiler/pragmas.nim index 604ba063f..100ce1560 100644 --- a/compiler/pragmas.nim +++ b/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: diff --git a/compiler/sem.nim b/compiler/sem.nim index 07b765f88..471a30099 100644 --- a/compiler/sem.nim +++ b/compiler/sem.nim @@ -46,7 +46,6 @@ 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 @@ -54,6 +53,8 @@ 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 diff --git a/compiler/semdata.nim b/compiler/semdata.nim index 7fa671722..fbc9762f3 100644 --- a/compiler/semdata.nim +++ b/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, diff --git a/compiler/semexprs.nim b/compiler/semexprs.nim index d920b0f25..56de00d56 100644 --- a/compiler/semexprs.nim +++ b/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) diff --git a/compiler/semobjconstr.nim b/compiler/semobjconstr.nim index 9d39dd5f8..eb0589601 100644 --- a/compiler/semobjconstr.nim +++ b/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. diff --git a/compiler/sempass2.nim b/compiler/sempass2.nim index 40b3b15ce..79d969fbb 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, 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! diff --git a/compiler/semstmts.nim b/compiler/semstmts.nim index 23e581a53..215d554fb 100644 --- a/compiler/semstmts.nim +++ b/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: diff --git a/compiler/semtypes.nim b/compiler/semtypes.nim index 39df93b62..21b95d3f6 100644 --- a/compiler/semtypes.nim +++ b/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: diff --git a/compiler/semtypinst.nim b/compiler/semtypinst.nim index 5aec3eed9..08ea85708 100644 --- a/compiler/semtypinst.nim +++ b/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) diff --git a/tests/constructors/tinvalid_construction.nim b/tests/constructors/tinvalid_construction.nim index 6716bfb45..e98b530bb 100644 --- a/tests/constructors/tinvalid_construction.nim +++ b/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 -- cgit 1.4.1-2-gfad0 22:51:06 +0100 Many async optimisations.' href='/ahoang/Nim/commit/lib/pure/asynchttpserver.nim?h=devel&id=cf5c8a204e76ff9ed5edb6ec0ebe3b97ee90f553'>cf5c8a204 ^ 
374706b1c ^



cf5c8a204 ^




















374706b1c ^

cf5c8a204 ^



374706b1c ^

cf5c8a204 ^















cd70bba33 ^


2dff5ef71 ^

cd70bba33 ^













374706b1c ^

4f5f98f0b ^


cd70bba33 ^

7ebbc0957 ^




cd70bba33 ^

62e454f41 ^












1
2
3
4
5
6
7
8
9
10

11
12

13

14

15
16

17

18
19
20

21

22

23

24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53

54

55

56
57
58
59
60
61
62

63
64
65
66
67
68
69
70
71

72
73
74

75
76
77
78
79
80
81
82
83
84
85

86

87
88
89
90
91
92
93
94
95

96
97
98

99

100
101

102

103
104
105
106
107
108
109

110
111
112
113
114
115
116
117

118
119

120

121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138

139

140
141
142

143
144
145

146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165

166

167
168
169

170

171
172
173
174
175
176
177
178
179
180
181
182
183
184
185

186
187

188

189
190
191
192
193
194
195
196
197
198
199
200
201

202

203
204

205

206
207
208
209

210

211
212
213
214
215
216
217
218
219
220
221
222