properly fix #10053 ; FieldDefect msg now shows discriminant value + lineinfo, in all backends (c,vm,js) (#11955)

* fix #10053 FieldError for vm

* fixup

* FieldError now also shows runtime value of discriminant

* fix field error reporting in vm

* also report culprit line info in err msg

* fix errors for newruntime 2

* fix for js

* fixup

* PRTEMP4

* works

* works

* works perfect

* refactor

* std/private/repr_impl

* suppport --gc:arc

* cleanup

* refactor

* simplify

* simplify

* simplify

* fixup

* move out compiler.vmgen.genCustom

* fixup

* fixup

* add tests

* revert compiler/debugutils.nim

* simplify reprDiscriminant

* fixup

* lib/std/private/repr_impl.nim -> lib/system/repr_impl.nim

* try to fix D20210812T165220

* honor --declaredlocs

* control toFileLineCol via --declaredlocs

compiler/astmsgs.nim

@@ -26,3 +26,14 @@ proc addDeclaredLoc*(result: var string, conf: ConfigRef; typ: PType) =
 
 proc addDeclaredLocMaybe*(result: var string, conf: ConfigRef; typ: PType) =
   if optDeclaredLocs in conf.globalOptions: addDeclaredLoc(result, conf, typ)
+
+template quoteExpr*(a: string): untyped =
+  ## can be used for quoting expressions in error msgs.
+  "'" & a & "'"
+
+proc genFieldDefect*(conf: ConfigRef, field: string, disc: PSym): string =
+  let obj = disc.owner.name.s # `types.typeToString` might be better, eg for generics
+  result = "field '$#' is not accessible for type '$#'" % [field, obj]
+  if optDeclaredLocs in conf.globalOptions:
+    result.add " [discriminant declared in $#]" % toFileLineCol(conf, disc.info)
+  result.add " using '$# = " % disc.name.s

compiler/ccgexprs.nim

@@ -18,6 +18,8 @@ proc getNullValueAuxT(p: BProc; orig, t: PType; obj, constOrNil: PNode,
 
 # -------------------------- constant expressions ------------------------
 
+proc rdSetElemLoc(conf: ConfigRef; a: TLoc, typ: PType): Rope
+
 proc int64Literal(i: BiggestInt): Rope =
   if i > low(int64):
     result = "IL64($1)" % [rope(i)]
@@ -873,16 +875,34 @@ proc genFieldCheck(p: BProc, e: PNode, obj: Rope, field: PSym) =
     v.r.add(".")
     v.r.add(disc.sym.loc.r)
     genInExprAux(p, it, u, v, test)
-    let msg = genFieldDefect(field, disc.sym)
+    var msg = ""
+    if optDeclaredLocs in p.config.globalOptions:
+      # xxx this should be controlled by a separate flag, and
+      # used for other similar defects so that location information is shown
+      # even without the expensive `--stacktrace`; binary size could be optimized
+      # by encoding the file names separately from `file(line:col)`, essentially
+      # passing around `TLineInfo` + the set of files in the project.
+      msg.add toFileLineCol(p.config, e.info) & " "
+    msg.add genFieldDefect(p.config, field.name.s, disc.sym)
     let strLit = genStringLiteral(p.module, newStrNode(nkStrLit, msg))
-    if op.magic == mNot:
+
-      linefmt(p, cpsStmts,
+    ## discriminant check
-              "if ($1){ #raiseFieldError($2); $3}$n",
+    template fun(code) = linefmt(p, cpsStmts, code, [rdLoc(test)])
-              [rdLoc(test), strLit, raiseInstr(p)])
+    if op.magic == mNot: fun("if ($1) ") else: fun("if (!($1)) ")
+
+    ## call raiseFieldError2 on failure
+    let discIndex = rdSetElemLoc(p.config, v, u.t)
+    if optTinyRtti in p.config.globalOptions:
+      # not sure how to use `genEnumToStr` here
+      const code = "{ #raiseFieldError2($1, (NI)$3); $2} $n"
+      linefmt(p, cpsStmts, code, [strLit, raiseInstr(p), discIndex])
     else:
-      linefmt(p, cpsStmts,
+      # complication needed for signed types
-              "if (!($1)){ #raiseFieldError($2); $3}$n",
+      let first = p.config.firstOrd(disc.sym.typ)
-              [rdLoc(test), strLit, raiseInstr(p)])
+      let firstLit = int64Literal(cast[int](first))
+      let discName = genTypeInfo(p.config, p.module, disc.sym.typ, e.info)
+      const code = "{ #raiseFieldError2($1, #reprDiscriminant(((NI)$3) + (NI)$4, $5)); $2} $n"
+      linefmt(p, cpsStmts, code, [strLit, raiseInstr(p), discIndex, firstLit, discName])
 
 proc genCheckedRecordField(p: BProc, e: PNode, d: var TLoc) =
   assert e[0].kind == nkDotExpr
@@ -1565,10 +1585,7 @@ proc genNewFinalize(p: BProc, e: PNode) =
   initLocExpr(p, e[1], a)
   initLocExpr(p, e[2], f)
   initLoc(b, locExpr, a.lode, OnHeap)
-  if optTinyRtti in p.config.globalOptions:
+  ti = genTypeInfo(p.config, p.module, refType, e.info)
-    ti = genTypeInfoV2(p.module, refType, e.info)
-  else:
-    ti = genTypeInfoV1(p.module, refType, e.info)
   p.module.s[cfsTypeInit3].addf("$1->finalizer = (void*)$2;$n", [ti, rdLoc(f)])
   b.r = ropecg(p.module, "($1) #newObj($2, sizeof($3))", [
       getTypeDesc(p.module, refType),

compiler/ccgtypes.nim

@@ -1514,3 +1514,9 @@ proc genTypeInfoV1(m: BModule, t: PType; info: TLineInfo): Rope =
 
 proc genTypeSection(m: BModule, n: PNode) =
   discard
+
+proc genTypeInfo*(config: ConfigRef, m: BModule, t: PType; info: TLineInfo): Rope =
+  if optTinyRtti in config.globalOptions:
+    result = genTypeInfoV2(m, t, info)
+  else:
+    result = genTypeInfoV1(m, t, info)

compiler/cgen.nim

@@ -15,7 +15,7 @@ import
   ccgutils, os, ropes, math, passes, wordrecg, treetab, cgmeth,
   rodutils, renderer, cgendata, aliases,
   lowerings, tables, sets, ndi, lineinfos, pathutils, transf,
-  injectdestructors
+  injectdestructors, astmsgs
 
 when not defined(leanCompiler):
   import spawn, semparallel

compiler/jsgen.nim

@@ -33,7 +33,7 @@ import
   nversion, msgs, idents, types,
   ropes, passes, ccgutils, wordrecg, renderer,
   cgmeth, lowerings, sighashes, modulegraphs, lineinfos, rodutils,
-  transf, injectdestructors, sourcemap
+  transf, injectdestructors, sourcemap, astmsgs
 
 import json, sets, math, tables, intsets, strutils
 
@@ -1208,12 +1208,13 @@ proc genCheckedFieldOp(p: PProc, n: PNode, addrTyp: PType, r: var TCompRes) =
   let tmp = p.getTemp()
   lineF(p, "var $1 = $2;$n", tmp, obj.res)
 
-  useMagic(p, "raiseFieldError")
+  useMagic(p, "raiseFieldError2")
   useMagic(p, "makeNimstrLit")
-  let msg = genFieldDefect(field, disc)
+  useMagic(p, "reprDiscriminant") # no need to offset by firstOrd unlike for cgen
-  lineF(p, "if ($1[$2.$3]$4undefined) { raiseFieldError(makeNimstrLit($5)); }$n",
+  let msg = genFieldDefect(p.config, field.name.s, disc)
+  lineF(p, "if ($1[$2.$3]$4undefined) { raiseFieldError2(makeNimstrLit($5), reprDiscriminant($2.$3, $6)); }$n",
     setx.res, tmp, disc.loc.r, if negCheck: ~"!==" else: ~"===",
-    makeJSString(msg))
+    makeJSString(msg), genTypeInfo(p, disc.typ))
 
   if addrTyp != nil and mapType(p, addrTyp) == etyBaseIndex:
     r.typ = etyBaseIndex

compiler/pragmas.nim

@@ -699,7 +699,7 @@ proc typeBorrow(c: PContext; sym: PSym, n: PNode) =
   incl(sym.typ.flags, tfBorrowDot)
 
 proc markCompilerProc(c: PContext; s: PSym) =
-  # minor hack ahead: FlowVar is the only generic .compilerProc type which
+  # minor hack ahead: FlowVar is the only generic .compilerproc type which
   # should not have an external name set:
   if s.kind != skType or s.name.s != "FlowVar":
     makeExternExport(c, s, "$1", s.info)

compiler/renderer.nim

@@ -1758,15 +1758,3 @@ proc getTokSym*(r: TSrcGen): PSym =
     result = r.tokens[r.idx-1].sym
   else:
     result = nil
-
-proc quoteExpr*(a: string): string {.inline.} =
-  ## can be used for quoting expressions in error msgs.
-  "'" & a & "'"
-
-proc genFieldDefect*(field: PSym, disc: PSym): string =
-  ## this needs to be in a module accessible by jsgen, ccgexprs, and vm to
-  ## provide this error msg FieldDefect; msgs would be better but it does not
-  ## import ast
-  result = field.name.s.quoteExpr & " is not accessible using discriminant " &
-    disc.name.s.quoteExpr & " of type " &
-    disc.owner.name.s.quoteExpr

compiler/vm.nim

@@ -1472,7 +1472,10 @@ proc rawExecute(c: PCtx, start: int, tos: PStackFrame): TFullReg =
       else:
         return TFullReg(kind: rkNone)
     of opcInvalidField:
-      stackTrace(c, tos, pc, errFieldXNotFound & regs[ra].node.strVal)
+      let msg = regs[ra].node.strVal
+      let disc = regs[instr.regB].regToNode
+      let msg2 = formatFieldDefect(msg, $disc)
+      stackTrace(c, tos, pc, msg2)
     of opcSetLenStr:
       decodeB(rkNode)
       #createStrKeepNode regs[ra]

compiler/vmgen.nim

@@ -31,7 +31,7 @@ import tables
 
 import
   strutils, ast, types, msgs, renderer, vmdef,
-  intsets, magicsys, options, lowerings, lineinfos, transf
+  intsets, magicsys, options, lowerings, lineinfos, transf, astmsgs
 
 from modulegraphs import getBody
 
@@ -1742,12 +1742,14 @@ proc genCheckedObjAccessAux(c: PCtx; n: PNode; dest: var TDest; flags: TGenFlags
   let lab1 = c.xjmp(n, if negCheck: opcFJmp else: opcTJmp, rs)
   c.freeTemp(rs)
   let strType = getSysType(c.graph, n.info, tyString)
-  var fieldNameRegister: TDest = c.getTemp(strType)
+  var msgReg: TDest = c.getTemp(strType)
-  let strLit = newStrNode($accessExpr[1], accessExpr[1].info)
+  let fieldName = $accessExpr[1]
+  let msg = genFieldDefect(c.config, fieldName, disc.sym)
+  let strLit = newStrNode(msg, accessExpr[1].info)
   strLit.typ = strType
-  c.genLit(strLit, fieldNameRegister)
+  c.genLit(strLit, msgReg)
-  c.gABC(n, opcInvalidField, fieldNameRegister)
+  c.gABC(n, opcInvalidField, msgReg, discVal)
-  c.freeTemp(fieldNameRegister)
+  c.freeTemp(msgReg)
   c.patch(lab1)
 
 proc genCheckedObjAccess(c: PCtx; n: PNode; dest: var TDest; flags: TGenFlags) =

lib/system/chcks.nim

@@ -26,8 +26,18 @@ proc raiseIndexError() {.compilerproc, noinline.} =
   sysFatal(IndexDefect, "index out of bounds")
 
 proc raiseFieldError(f: string) {.compilerproc, noinline.} =
+  ## remove after bootstrap > 1.5.1
   sysFatal(FieldDefect, f)
 
+when defined(gcdestructors):
+  proc raiseFieldError2(f: string, discVal: int) {.compilerproc, noinline.} =
+    ## raised when field is inaccessible given runtime value of discriminant
+    sysFatal(FieldError, f & $discVal & "'")
+else:
+  proc raiseFieldError2(f: string, discVal: string) {.compilerproc, noinline.} =
+    ## raised when field is inaccessible given runtime value of discriminant
+    sysFatal(FieldError, formatFieldDefect(f, discVal))
+
 proc raiseRangeErrorI(i, a, b: BiggestInt) {.compilerproc, noinline.} =
   when defined(standalone):
     sysFatal(RangeDefect, "value out of range")

lib/system/indexerrors.nim

@@ -1,4 +1,5 @@
 # imported by other modules, unlike helpers.nim which is included
+# xxx this is now included instead of imported, we should import instead
 
 template formatErrorIndexBound*[T](i, a, b: T): string =
   when defined(standalone):
@@ -9,3 +10,6 @@ template formatErrorIndexBound*[T](i, a, b: T): string =
 
 template formatErrorIndexBound*[T](i, n: T): string =
   formatErrorIndexBound(i, 0, n)
+
+template formatFieldDefect*(f, discVal): string =
+  f & discVal & "'"

lib/system/jssys.nim

@@ -171,8 +171,8 @@ proc raiseRangeError() {.compilerproc, noreturn.} =
 proc raiseIndexError(i, a, b: int) {.compilerproc, noreturn.} =
   raise newException(IndexDefect, formatErrorIndexBound(int(i), int(a), int(b)))
 
-proc raiseFieldError(f: string) {.compilerproc, noreturn.} =
+proc raiseFieldError2(f: string, discVal: string) {.compilerproc, noreturn.} =
-  raise newException(FieldDefect, f)
+  raise newException(FieldDefect, formatFieldDefect(f, discVal))
 
 proc setConstr() {.varargs, asmNoStackFrame, compilerproc.} =
   asm """

lib/system/repr.nim

@@ -72,6 +72,8 @@ proc reprEnum(e: int, typ: PNimType): string {.compilerRtl.} =
 
   result = $e & " (invalid data!)"
 
+include system/repr_impl
+
 type
   PByteArray = ptr UncheckedArray[byte] # array[0xffff, byte]
 

lib/system/repr_impl.nim

@@ -0,0 +1,15 @@
+#[
+other APIs common to system/repr and system/reprjs could be refactored here, eg:
+* reprChar
+* reprBool
+* reprStr
+
+Another possibility in future work would be to have a single include file instead
+of system/repr and system/reprjs, and use `when defined(js)` inside it.
+]#
+
+proc reprDiscriminant*(e: int, typ: PNimType): string {.compilerRtl.} =
+  case typ.kind
+  of tyEnum: reprEnum(e, typ)
+  of tyBool: $(e != 0)
+  else: $e

lib/system/repr_v2.nim

@@ -1,3 +1,5 @@
+include system/inclrtl
+
 proc isNamedTuple(T: typedesc): bool {.magic: "TypeTrait".}
   ## imported from typetraits
 
@@ -66,6 +68,11 @@ proc repr*[Enum: enum](x: Enum): string {.magic: "EnumToStr", noSideEffect.}
   ## If a `repr` operator for a concrete enumeration is provided, this is
   ## used instead. (In other words: *Overwriting* is possible.)
 
+proc reprDiscriminant*(e: int): string {.compilerproc.} =
+  # repr and reprjs can use `PNimType` to symbolize `e`; making this work here
+  # would require a way to pass the set of enum stringified values to cgen.
+  $e
+
 proc repr*(p: pointer): string =
   ## repr of pointer as its hexadecimal value
   if p == nil:

lib/system/reprjs.nim

@@ -30,6 +30,8 @@ proc reprEnum(e: int, typ: PNimType): string {.compilerRtl.} =
   else:
     result = $e & " (invalid data!)"
 
+include system/repr_impl
+
 proc reprChar(x: char): string {.compilerRtl.} =
   result = "\'"
   case x

tests/misc/mfield_defect.nim

@@ -0,0 +1,30 @@
+#[
+ran from trunner
+]#
+
+
+
+
+
+
+# line 10
+type Kind = enum k0, k1, k2, k3, k4
+
+type Foo = object
+  case kind: Kind
+  of k0: f0: int
+  of k1: f1: int
+  of k2: f2: int
+  of k3: f3: int
+  of k4: f4: int
+
+proc main()=
+  var foo = Foo(kind: k3, f3: 3)
+  let s1 = foo.f3
+  doAssert s1 == 3
+  let s2 = foo.f2
+
+when defined case1:
+  static: main()
+when defined case2:
+  main()

tests/misc/trunner.nim

@@ -37,9 +37,9 @@ proc runNimCmd(file, options = "", rtarg = ""): auto =
     echo cmd
     echo result[0] & "\n" & $result[1]
 
-proc runNimCmdChk(file, options = "", rtarg = ""): string =
+proc runNimCmdChk(file, options = "", rtarg = "", status = 0): string =
-  let (ret, status) = runNimCmd(file, options, rtarg = rtarg)
+  let (ret, status2) = runNimCmd(file, options, rtarg = rtarg)
-  doAssert status == 0, $(file, options) & "\n" & ret
+  doAssert status2 == status, $(file, options, status, status2) & "\n" & ret
   ret
 
 proc genShellCmd(filename: string): string =
@@ -376,5 +376,15 @@ mused3.nim(13, 8) Warning: imported and not used: 'mused3b' [UnusedImport]
 mused3.nim(75, 10) Hint: duplicate import of 'mused3a'; previous import here: mused3.nim(74, 10) [DuplicateModuleImport]
 """
 
+  block: # FieldDefect
+    proc fn(opt: string, expected: string) =
+      let output = runNimCmdChk("misc/mfield_defect.nim", fmt"-r --warning:all:off --declaredlocs {opt}", status = 1)
+      doAssert expected in output, opt & "\noutput:\n" & output & "expected:\n" & expected
+    fn("-d:case1"): """mfield_defect.nim(25, 15) Error: field 'f2' is not accessible for type 'Foo' [discriminant declared in mfield_defect.nim(14, 8)] using 'kind = k3'"""
+    fn("-d:case2 --gc:refc"): """mfield_defect.nim(25, 15) field 'f2' is not accessible for type 'Foo' [discriminant declared in mfield_defect.nim(14, 8)] using 'kind = k3'"""
+    fn("-d:case1 -b:js"): """mfield_defect.nim(25, 15) Error: field 'f2' is not accessible for type 'Foo' [discriminant declared in mfield_defect.nim(14, 8)] using 'kind = k3'"""
+    fn("-d:case2 -b:js"): """field 'f2' is not accessible for type 'Foo' [discriminant declared in mfield_defect.nim(14, 8)] using 'kind = k3'"""
+    # 3 instead of k3, because of lack of RTTI
+    fn("-d:case2 --gc:arc"): """mfield_defect.nim(25, 15) field 'f2' is not accessible for type 'Foo' [discriminant declared in mfield_defect.nim(14, 8)] using 'kind = 3'"""
 else:
   discard # only during debugging, tests added here will run with `-d:nimTestsTrunnerDebugging` enabled