Field checks for everybody (#8957)

* Field checks for JS backend

* Clean nkCall nodes with no arguments

Generating a nkEmpty in place of no arguments makes no sense form the
AST point of view and also trips up the VM codegen.

* Field checks for VM backend

* Test case for #6612

This patchset fixes #6612

* Add test case for LHS double evaluation

* Prevent LHS double-eval for JS backend

* Prevent double evaluation in VM backend

compiler/closureiters.nim

@@ -210,7 +210,7 @@ proc newUnrollFinallyAccess(ctx: var Ctx, info: TLineInfo): PNode =
 
 proc newCurExcAccess(ctx: var Ctx): PNode =
   if ctx.curExcSym.isNil:
-    ctx.curExcSym = ctx.newEnvVar(":curExc", ctx.g.callCodegenProc("getCurrentException", ctx.g.emptyNode).typ)
+    ctx.curExcSym = ctx.newEnvVar(":curExc", ctx.g.callCodegenProc("getCurrentException").typ)
   ctx.newEnvVarAccess(ctx.curExcSym)
 
 proc newState(ctx: var Ctx, n, gotoOut: PNode): int =
@@ -324,7 +324,7 @@ proc collectExceptState(ctx: var Ctx, n: PNode): PNode {.inline.} =
           assert(c[i].kind == nkType)
           let nextCond = newTree(nkCall,
             newSymNode(g.getSysMagic(c.info, "of", mOf)),
-            g.callCodegenProc("getCurrentException", ctx.g.emptyNode),
+            g.callCodegenProc("getCurrentException"),
             c[i])
           nextCond.typ = ctx.g.getSysType(c.info, tyBool)
           nextCond.info = c.info
@@ -364,7 +364,7 @@ proc addElseToExcept(ctx: var Ctx, n: PNode) =
     block: # :curExc = getCurrentException()
       branchBody.add(newTree(nkAsgn,
         ctx.newCurExcAccess(),
-        ctx.g.callCodegenProc("getCurrentException", ctx.g.emptyNode)))
+        ctx.g.callCodegenProc("getCurrentException")))
 
     block: # goto nearestFinally
       branchBody.add(newTree(nkGotoState, ctx.g.newIntLit(n.info, ctx.nearestFinally)))
@@ -803,7 +803,7 @@ proc newEndFinallyNode(ctx: var Ctx, info: TLineInfo): PNode =
   let branch = newTree(nkElifBranch, cmp, retStmt)
 
   # The C++ backend requires `getCurrentException` here.
-  let raiseStmt = newTree(nkRaiseStmt, ctx.g.callCodegenProc("getCurrentException", ctx.g.emptyNode))
+  let raiseStmt = newTree(nkRaiseStmt, ctx.g.callCodegenProc("getCurrentException"))
   raiseStmt.info = info
   let elseBranch = newTree(nkElse, raiseStmt)
 
@@ -1204,7 +1204,7 @@ proc newCatchBody(ctx: var Ctx, info: TLineInfo): PNode {.inline.} =
   block:
     result.add(newTree(nkAsgn,
       ctx.newCurExcAccess(),
-      ctx.g.callCodegenProc("getCurrentException", ctx.g.emptyNode)))
+      ctx.g.callCodegenProc("getCurrentException")))
 
 proc wrapIntoTryExcept(ctx: var Ctx, n: PNode): PNode {.inline.} =
   let setupExc = newTree(nkCall,

compiler/destroyer.nim

@@ -289,7 +289,7 @@ proc destructiveMoveSink(n: PNode; c: var Con): PNode =
   result = newNodeIT(nkStmtListExpr, n.info, n.typ)
   let bit = newSymNode dropBit(c, n.sym)
   if optMoveCheck in c.owner.options:
-    result.add callCodegenProc(c.graph, "chckMove", bit)
+    result.add callCodegenProc(c.graph, "chckMove", bit.info, bit)
   result.add newTree(nkAsgn, bit,
     newIntTypeNode(nkIntLit, 0, getSysType(c.graph, n.info, tyBool)))
   result.add n

compiler/jsgen.nim

@@ -986,13 +986,50 @@ proc genFieldAccess(p: PProc, n: PNode, r: var TCompRes) =
 
 proc genAddr(p: PProc, n: PNode, r: var TCompRes)
 
-proc genCheckedFieldAddr(p: PProc, n: PNode, r: var TCompRes) =
-  let m = if n.kind == nkHiddenAddr: n.sons[0] else: n
-  internalAssert p.config, m.kind == nkCheckedFieldExpr
-  genAddr(p, m, r) # XXX
+proc genCheckedFieldOp(p: PProc, n: PNode, addrTyp: PType, r: var TCompRes) =
+  internalAssert p.config, n.kind == nkCheckedFieldExpr
+  # nkDotExpr to access the requested field
+  let accessExpr = n[0]
+  # nkCall to check if the discriminant is valid
+  var checkExpr = n[1]
 
-proc genCheckedFieldAccess(p: PProc, n: PNode, r: var TCompRes) =
-  genFieldAccess(p, n.sons[0], r) # XXX
+  let negCheck = checkExpr[0].sym.magic == mNot
+  if negCheck:
+    checkExpr = checkExpr[^1]
+
+  # Field symbol
+  var field = accessExpr[1].sym
+  internalAssert p.config, field.kind == skField
+  if field.loc.r == nil: field.loc.r = mangleName(p.module, field)
+  # Discriminant symbol
+  let disc = checkExpr[2].sym
+  internalAssert p.config, disc.kind == skField
+  if disc.loc.r == nil: disc.loc.r = mangleName(p.module, disc)
+
+  var setx: TCompRes
+  gen(p, checkExpr[1], setx)
+
+  var obj: TCompRes
+  gen(p, accessExpr[0], obj)
+  # Avoid evaluating the LHS twice (one to read the discriminant and one to read
+  # the field)
+  let tmp = p.getTemp()
+  lineF(p, "var $1 = $2;$n", tmp, obj.res)
+
+  useMagic(p, "raiseFieldError")
+  useMagic(p, "makeNimstrLit")
+  lineF(p, "if ($1[$2.$3]$4undefined) { raiseFieldError(makeNimstrLit($5)); }$n",
+    setx.res, tmp, disc.loc.r, if negCheck: ~"!==" else: ~"===",
+    makeJSString(field.name.s))
+
+  if addrTyp != nil and mapType(p, addrTyp) == etyBaseIndex:
+    r.typ = etyBaseIndex
+    r.res = makeJSString($field.loc.r)
+    r.address = tmp
+  else:
+    r.typ = etyNone
+    r.res = "$1.$2" % [tmp, field.loc.r]
+  r.kind = resExpr
 
 proc genArrayAddr(p: PProc, n: PNode, r: var TCompRes) =
   var
@@ -1071,7 +1108,7 @@ proc genAddr(p: PProc, n: PNode, r: var TCompRes) =
         #internalError(p.config, n.info, "genAddr: 4 " & renderTree(n))
     else: internalError(p.config, n.info, "genAddr: 2")
   of nkCheckedFieldExpr:
-    genCheckedFieldAddr(p, n, r)
+    genCheckedFieldOp(p, n[0], n.typ, r)
   of nkDotExpr:
     if mapType(p, n.typ) == etyBaseIndex:
       genFieldAddr(p, n.sons[0], r)
@@ -2140,7 +2177,7 @@ proc gen(p: PProc, n: PNode, r: var TCompRes) =
   of nkDerefExpr, nkHiddenDeref: genDeref(p, n, r)
   of nkBracketExpr: genArrayAccess(p, n, r)
   of nkDotExpr: genFieldAccess(p, n, r)
-  of nkCheckedFieldExpr: genCheckedFieldAccess(p, n, r)
+  of nkCheckedFieldExpr: genCheckedFieldOp(p, n, nil, r)
   of nkObjDownConv: gen(p, n.sons[0], r)
   of nkObjUpConv: upConv(p, n, r)
   of nkCast: genCast(p, n, r)

compiler/lowerings.nim

@@ -281,15 +281,16 @@ proc genDeref*(n: PNode): PNode =
                      n.typ.skipTypes(abstractInst).sons[0])
   result.add n
 
-proc callCodegenProc*(g: ModuleGraph; name: string, arg1: PNode;
-                      arg2, arg3, optionalArgs: PNode = nil): PNode =
-  result = newNodeI(nkCall, arg1.info)
+proc callCodegenProc*(g: ModuleGraph; name: string;
+                      info: TLineInfo = unknownLineInfo();
+                      arg1, arg2, arg3, optionalArgs: PNode = nil): PNode =
+  result = newNodeI(nkCall, info)
   let sym = magicsys.getCompilerProc(g, name)
   if sym == nil:
-    localError(g.config, arg1.info, "system module needs: " & name)
+    localError(g.config, info, "system module needs: " & name)
   else:
     result.add newSymNode(sym)
-    result.add arg1
+    if arg1 != nil: result.add arg1
     if arg2 != nil: result.add arg2
     if arg3 != nil: result.add arg3
     if optionalArgs != nil:
@@ -410,7 +411,8 @@ proc createWrapperProc(g: ModuleGraph; f: PNode; threadParam, argsParam: PSym;
     threadLocalBarrier = addLocalVar(g, varSection2, nil, argsParam.owner,
                                      barrier.typ, barrier)
     body.add varSection2
-    body.add callCodegenProc(g, "barrierEnter", threadLocalBarrier.newSymNode)
+    body.add callCodegenProc(g, "barrierEnter", threadLocalBarrier.info,
+      threadLocalBarrier.newSymNode)
   var threadLocalProm: PSym
   if spawnKind == srByVar:
     threadLocalProm = addLocalVar(g, varSection, nil, argsParam.owner, fv.typ, fv)
@@ -425,7 +427,8 @@ proc createWrapperProc(g: ModuleGraph; f: PNode; threadParam, argsParam: PSym;
     body.add newAsgnStmt(indirectAccess(threadLocalProm.newSymNode,
       "owner", fv.info, g.cache), threadParam.newSymNode)
 
-  body.add callCodegenProc(g, "nimArgsPassingDone", threadParam.newSymNode)
+  body.add callCodegenProc(g, "nimArgsPassingDone", threadParam.info,
+    threadParam.newSymNode)
   if spawnKind == srByVar:
     body.add newAsgnStmt(genDeref(threadLocalProm.newSymNode), call)
   elif fv != nil:
@@ -444,11 +447,13 @@ proc createWrapperProc(g: ModuleGraph; f: PNode; threadParam, argsParam: PSym;
     if barrier == nil:
       # by now 'fv' is shared and thus might have beeen overwritten! we need
       # to use the thread-local view instead:
-      body.add callCodegenProc(g, "nimFlowVarSignal", threadLocalProm.newSymNode)
+      body.add callCodegenProc(g, "nimFlowVarSignal", threadLocalProm.info,
+        threadLocalProm.newSymNode)
   else:
     body.add call
   if barrier != nil:
-    body.add callCodegenProc(g, "barrierLeave", threadLocalBarrier.newSymNode)
+    body.add callCodegenProc(g, "barrierLeave", threadLocalBarrier.info,
+      threadLocalBarrier.newSymNode)
 
   var params = newNodeI(nkFormalParams, f.info)
   params.add newNodeI(nkEmpty, f.info)
@@ -710,7 +715,8 @@ proc wrapProcForSpawn*(g: ModuleGraph; owner: PSym; spawnExpr: PNode; retType: P
     # create flowVar:
     result.add newFastAsgnStmt(fvField, callProc(spawnExpr[^1]))
     if barrier == nil:
-      result.add callCodegenProc(g, "nimFlowVarCreateSemaphore", fvField)
+      result.add callCodegenProc(g, "nimFlowVarCreateSemaphore", fvField.info,
+        fvField)
 
   elif spawnKind == srByVar:
     var field = newSym(skField, getIdent(g.cache, "fv"), owner, n.info, g.config.options)
@@ -723,7 +729,7 @@ proc wrapProcForSpawn*(g: ModuleGraph; owner: PSym; spawnExpr: PNode; retType: P
   let wrapper = createWrapperProc(g, fn, threadParam, argsParam,
                                   varSection, varInit, call,
                                   barrierAsExpr, fvAsExpr, spawnKind)
-  result.add callCodegenProc(g, "nimSpawn" & $spawnExpr.len, wrapper.newSymNode,
-                             genAddrOf(scratchObj.newSymNode), nil, spawnExpr)
+  result.add callCodegenProc(g, "nimSpawn" & $spawnExpr.len, wrapper.info,
+    wrapper.newSymNode, genAddrOf(scratchObj.newSymNode), nil, spawnExpr)
 
   if spawnKind == srFlowVar: result.add fvField

compiler/semparallel.nim

@@ -493,6 +493,6 @@ proc liftParallel*(g: ModuleGraph; owner: PSym; n: PNode): PNode =
   result = newNodeI(nkStmtList, n.info)
   generateAliasChecks(a, result)
   result.add varSection
-  result.add callCodegenProc(g, "openBarrier", barrier)
+  result.add callCodegenProc(g, "openBarrier", barrier.info, barrier)
   result.add transformSpawn(g, owner, body, barrier)
-  result.add callCodegenProc(g, "closeBarrier", barrier)
+  result.add callCodegenProc(g, "closeBarrier", barrier.info, barrier)

compiler/transf.nim

@@ -739,7 +739,7 @@ proc transformExceptBranch(c: PTransf, n: PNode): PTransNode =
     let actions = newTransNode(nkStmtListExpr, n[1], 2)
     # Generating `let exc = (excType)(getCurrentException())`
     # -> getCurrentException()
-    let excCall = PTransNode(callCodegenProc(c.graph, "getCurrentException", newNodeI(nkEmpty, n.info)))
+    let excCall = PTransNode(callCodegenProc(c.graph, "getCurrentException"))
     # -> (excType)
     let convNode = newTransNode(nkHiddenSubConv, n[1].info, 2)
     convNode[0] = PTransNode(newNodeI(nkEmpty, n.info))

compiler/vmgen.nim

@@ -562,6 +562,8 @@ proc genIndex(c: PCtx; n: PNode; arr: PType): TRegister =
   else:
     result = c.genx(n)
 
+proc genCheckedObjAccessAux(c: PCtx; n: PNode; dest: var TDest; flags: TGenFlags)
+
 proc genAsgnPatch(c: PCtx; le: PNode, value: TRegister) =
   case le.kind
   of nkBracketExpr:
@@ -570,12 +572,16 @@ proc genAsgnPatch(c: PCtx; le: PNode, value: TRegister) =
     c.gABC(le, opcWrArr, dest, idx, value)
     c.freeTemp(dest)
     c.freeTemp(idx)
-  of nkDotExpr, nkCheckedFieldExpr:
-    # XXX field checks here
-    let left = if le.kind == nkDotExpr: le else: le.sons[0]
-    let dest = c.genx(left.sons[0], {gfNode})
-    let idx = genField(c, left.sons[1])
-    c.gABC(left, opcWrObj, dest, idx, value)
+  of nkCheckedFieldExpr:
+    var objR: TDest = -1
+    genCheckedObjAccessAux(c, le, objR, {gfNode})
+    let idx = genField(c, le[0].sons[1])
+    c.gABC(le[0], opcWrObj, objR, idx, value)
+    c.freeTemp(objR)
+  of nkDotExpr:
+    let dest = c.genx(le.sons[0], {gfNode})
+    let idx = genField(c, le.sons[1])
+    c.gABC(le, opcWrObj, dest, idx, value)
     c.freeTemp(dest)
   of nkDerefExpr, nkHiddenDeref:
     let dest = c.genx(le.sons[0], {gfNode})
@@ -1419,13 +1425,19 @@ proc genAsgn(c: PCtx; le, ri: PNode; requiresCopy: bool) =
     else:
       c.preventFalseAlias(le, opcWrArr, dest, idx, tmp)
     c.freeTemp(tmp)
-  of nkDotExpr, nkCheckedFieldExpr:
-    # XXX field checks here
-    let left = if le.kind == nkDotExpr: le else: le.sons[0]
-    let dest = c.genx(left.sons[0], {gfNode})
-    let idx = genField(c, left.sons[1])
+  of nkCheckedFieldExpr:
+    var objR: TDest = -1
+    genCheckedObjAccessAux(c, le, objR, {gfNode})
+    let idx = genField(c, le[0].sons[1])
     let tmp = c.genx(ri)
-    c.preventFalseAlias(left, opcWrObj, dest, idx, tmp)
+    c.preventFalseAlias(le[0], opcWrObj, objR, idx, tmp)
+    c.freeTemp(tmp)
+    c.freeTemp(objR)
+  of nkDotExpr:
+    let dest = c.genx(le.sons[0], {gfNode})
+    let idx = genField(c, le.sons[1])
+    let tmp = c.genx(ri)
+    c.preventFalseAlias(le, opcWrObj, dest, idx, tmp)
     c.freeTemp(tmp)
   of nkDerefExpr, nkHiddenDeref:
     let dest = c.genx(le.sons[0], {gfNode})
@@ -1561,9 +1573,59 @@ proc genObjAccess(c: PCtx; n: PNode; dest: var TDest; flags: TGenFlags) =
     c.gABC(n, opcLdObj, dest, a, b)
   c.freeTemp(a)
 
+proc genCheckedObjAccessAux(c: PCtx; n: PNode; dest: var TDest; flags: TGenFlags) =
+  internalAssert c.config, n.kind == nkCheckedFieldExpr
+  # nkDotExpr to access the requested field
+  let accessExpr = n[0]
+  # nkCall to check if the discriminant is valid
+  var checkExpr = n[1]
+
+  let negCheck = checkExpr[0].sym.magic == mNot
+  if negCheck:
+    checkExpr = checkExpr[^1]
+
+  # Discriminant symbol
+  let disc = checkExpr[2]
+  internalAssert c.config, disc.sym.kind == skField
+
+  # Load the object in `dest`
+  c.gen(accessExpr[0], dest, flags)
+  # Load the discriminant
+  var discVal = c.getTemp(disc.typ)
+  c.gABC(n, opcLdObj, discVal, dest, genField(c, disc))
+  # Check if its value is contained in the supplied set
+  let setLit = c.genx(checkExpr[1])
+  var rs = c.getTemp(getSysType(c.graph, n.info, tyBool))
+  c.gABC(n, opcContainsSet, rs, setLit, discVal)
+  c.freeTemp(setLit)
+  # If the check fails let the user know
+  let L1 = c.xjmp(n, if negCheck: opcFJmp else: opcTJmp, rs)
+  c.freeTemp(rs)
+  # Not ideal but will do for the moment
+  c.gABC(n, opcQuit)
+  c.patch(L1)
+
 proc genCheckedObjAccess(c: PCtx; n: PNode; dest: var TDest; flags: TGenFlags) =
-  # XXX implement field checks!
-  genObjAccess(c, n.sons[0], dest, flags)
+  var objR: TDest = -1
+  genCheckedObjAccessAux(c, n, objR, flags)
+
+  let accessExpr = n[0]
+  # Field symbol
+  var field = accessExpr[1]
+  internalAssert c.config, field.sym.kind == skField
+
+  # Load the content now
+  if dest < 0: dest = c.getTemp(n.typ)
+  let fieldPos = genField(c, field)
+  if needsRegLoad():
+    var cc = c.getTemp(accessExpr.typ)
+    c.gABC(n, opcLdObj, cc, objR, fieldPos)
+    c.gABC(n, opcNodeToReg, dest, cc)
+    c.freeTemp(cc)
+  else:
+    c.gABC(n, opcLdObj, dest, objR, fieldPos)
+
+  c.freeTemp(objR)
 
 proc genArrAccess(c: PCtx; n: PNode; dest: var TDest; flags: TGenFlags) =
   let arrayType = n.sons[0].typ.skipTypes(abstractVarRange-{tyTypeDesc}).kind

tests/js/t6612.nim

@@ -0,0 +1,24 @@
+discard """
+  action: "run"
+"""
+
+proc fillWith(sq: var seq[int], n: int, unused: string) =
+  sq = @[n]
+
+type
+  Object = object of RootObj
+    case hasNums: bool
+    of true:
+      numbers: seq[int]
+    of false:
+      discard
+    always: seq[int]
+
+var obj = Object(hasNums: true)
+
+obj.always.fillWith(5, "unused")
+doAssert obj.always == @[5]
+
+obj.numbers.fillWith(3, "unused")
+doAssert obj.numbers == @[3]
+doAssert obj.always == @[5]

tests/js/tfieldchecks.nim

@@ -0,0 +1,48 @@
+discard """
+  output: '''
+foo
+C
+3.14
+foo
+3.14
+3.14
+'''
+"""
+
+type
+  V = enum
+    A, B, C
+  X = object
+    f0: string
+    case f1: V
+    of A: f2: string
+    of B: discard
+    of C: f3: float
+
+var obj = X(f0: "foo", f1: C, f3: 3.14)
+
+block:
+  echo obj.f0
+  echo obj.f1
+  doAssertRaises(FieldError): echo obj.f2
+  echo obj.f3
+
+block:
+  let a0 = addr(obj.f0)
+  echo a0[]
+  # let a1 = unsafeAddr(obj.f1)
+  # echo a1[]
+  doAssertRaises(FieldError):
+    let a2 = addr(obj.f2)
+    echo a2[]
+  let a3 = addr(obj.f3)
+  echo a3[]
+
+# Prevent double evaluation of LHS
+block:
+  var flag = false
+  proc wrap(x: X): X =
+    doAssert flag == false
+    flag = true
+    result = x
+  echo wrap(obj).f3

tests/vm/tcgemptycallarg.nim

@@ -0,0 +1,3 @@
+static:
+  doAssertRaises(ValueError):
+    raise newException(ValueError, "Yes")