sem: fix crash due to incorrect error handling (#1372)

## Summary

Wrap usage of erroneous symbols in quiet errors, so that error
propagation works as expected. This fixes compiler/nimsuggest crashes
when the iterable expression in `for` loops has an error. 

Fixes #1369.

## Details

### The Problem

If the iterable slot of a `for` loop is an error, `tyError` is assigned
as the forvars' type. When such forvar appears as an argument in a call
expression, errors

### The Solution

* add the `adWrappedSymError` diagnostic, which is a quiet diagnostic
  like `adWrappedError`, meaning that it's only used for error
  propagation and never reported
* move `newSymNode2` from `ast` to `sem` and change it so that it
  creates `adWrappedSymError` error nodes for symbols where the
  definition has an error
* rename `newSymNode2` to `newSymNodeOrError`
* update the few usages of `newSymNode2`
* add test for the `for`-loop-related compiler crash to the new
  `error_propagation` category

The introduction of `adWrappedSymError` is meant to be a foundational
work for changing `skError` to only represent errors (instead of both
errors and symbols whose definition has an error).

Author: zerbina

compiler/ast/ast.nim

@@ -193,32 +193,6 @@ proc newIdentNode*(ident: PIdent, info: TLineInfo): PNode =
   result.ident = ident
   result.info = info
 
-proc newSymNode2*(sym: PSym): PNode =
-  ## creates a new `nkSym` node, unless sym.kind is an skError where an nkError
-  ## is extracted from the sym and returned instead.
-  ## NB: not a `newSymNode` replacement, it's for when symbol sem fails
-  if sym.isError:
-    result = sym.ast
-  else:
-    result = newNode(nkSym)
-    result.sym = sym
-    result.typ = sym.typ
-    result.info = sym.info
-
-proc newSymNode2*(sym: PSym, info: TLineInfo): PNode =
-  ## creates a new `nkSym` node, unless sym.kind is an skError where an nkError
-  ## is extracted from the sym and returned instead. In either case sets the
-  ## node info to the one provided
-  ## NB: not a `newSymNode` replacement, it's for when symbol sem fails
-  if sym.isError:
-    result = sym.ast
-    result.info = info
-  else:
-    result = newNode(nkSym)
-    result.sym = sym
-    result.typ = sym.typ
-    result.info = info
-
 proc newSymNodeIT*(sym: PSym, info: TLineInfo, typ: PType): PNode =
   ## create a new sym node with the supplied `info` and `typ`
   result = newNodeIT(nkSym, info, typ)

compiler/ast/ast_types.nim

@@ -1070,6 +1070,7 @@ type
   AstDiagKind* = enum
     # general
     adWrappedError
+    adWrappedSymError
     adCyclicTree
     # type
     adSemTypeMismatch
@@ -1279,7 +1280,7 @@ type
     location*: TLineInfo        # TODO: `wrongNode` already has this, move to
                                 #       variant or handle in display/rendering
     case kind*: AstDiagKind
-    of adWrappedError:
+    of adWrappedError, adWrappedSymError:
       discard
     of adSemTypeMismatch,
         adSemIllegalConversion,

compiler/ast/errorhandling.nim

@@ -189,7 +189,7 @@ iterator walkErrors*(config: ConfigRef; n: PNode): PNode =
   for i in 0..<errNodes.len:
     # reverse index so we go from the innermost to outermost
     let e = errNodes[i]
-    if e.diag.kind == adWrappedError:
+    if e.diag.kind in {adWrappedError, adWrappedSymError}:
       continue
 
     assert(

compiler/front/cli_reporter.nim

@@ -3143,7 +3143,7 @@ func astDiagToLegacyReport(conf: ConfigRef, diag: PAstDiag): Report {.inline.} =
     vmRep: VMReport
 
   case diag.kind
-  of adWrappedError:
+  of adWrappedError, adWrappedSymError:
     semRep = SemReport(
         location: some diag.location,
         reportInst: diag.instLoc.toReportLineInfo,

compiler/front/msgs.nim

@@ -422,6 +422,7 @@ func astDiagToLegacyReportKind*(
   ## very broad categories and they'll no longer map to "reports".
   case diag
   of adWrappedError: rsemWrappedError
+  of adWrappedSymError: rsemWrappedError
   of adSemTypeMismatch: rsemTypeMismatch
   of adSemTypeNotAllowed: rsemTypeNotAllowed
   of adSemTIsNotAConcreteType: rsemTIsNotAConcreteType

compiler/sem/sem.nim

@@ -151,6 +151,20 @@ proc wrapErrorAndUpdate(c: ConfigRef, n: PNode, s: PSym): PNode =
   result = c.wrapError(n)
   s.ast = result
 
+proc newSymNodeOrError(c: ConfigRef, sym: PSym, info: TLineInfo): PNode =
+  ## Creates a new `nkSym` node, unless `sym` either represents an error
+  ## itself or refers to an erroneous entity. In the latter two cases, an
+  ## error node is returned.
+  ## NB: not a `newSymNode` replacement, it's for when symbol sem fails
+  if sym.isError:
+    result = sym.ast
+    result.info = info
+  elif sym.ast.isError or (sym.typ != nil and sym.typ.kind == tyError):
+    result = c.newError(newSymNode(sym, info),
+                        PAstDiag(kind: adWrappedSymError))
+  else:
+    result = newSymNode(sym, info)
+
 template semIdeForTemplateOrGenericCheck(conf, n, cursorInBody) =
   # use only for idetools support; detecting cursor in generic or template body
   # if so call `semIdeForTemplateOrGeneric` for semantic checking

compiler/sem/semexprs.nim

@@ -1631,7 +1631,7 @@ proc semSym(c: PContext, n: PNode, sym: PSym, flags: TExprFlags): PNode =
       localReport(c.config, n, reportSem rsemIllegalNimvmContext)
 
     markUsed(c, n.info, s)
-    result = newSymNode2(s, n.info)
+    result = newSymNodeOrError(c.config, s, n.info)
     # We cannot check for access to outer vars for example because it's still
     # not sure the symbol really ends up being used:
     # var len = 0 # but won't be called
@@ -1662,14 +1662,9 @@ proc semSym(c: PContext, n: PNode, sym: PSym, flags: TExprFlags): PNode =
       c.config.internalAssert s.owner != nil
     result = newSymNode(s, n.info)
   else:
-    if s.kind == skError and not s.ast.isNil and s.ast.kind == nkError:
-      # XXX: at the time of writing only `lookups.qualifiedlookup` sets up the
-      #      PSym so the error is in the ast field
-      result = s.ast
-    else:
-      let info = getCallLineInfo(n)
-      markUsed(c, info, s)
-      result = newSymNode(s, info)
+    let info = getCallLineInfo(n)
+    markUsed(c, info, s)
+    result = newSymNodeOrError(c.config, s, info)
 
 proc tryReadingGenericParam(c: PContext, n: PNode, i: PIdent, t: PType): PNode =
   case t.kind

compiler/sem/semstmts.nim

@@ -839,7 +839,7 @@ proc semNormalizedLetOrVar(c: PContext, n: PNode, symkind: TSymKind): PNode =
           c.config.newError(r, PAstDiag(kind: adSemIllegalCompileTime))
 
     if v.isError:
-      producedDecl[i] = newSymNode2(v)
+      producedDecl[i] = v.ast # ast is an error AST
       hasError = true
 
       continue # refactor: remove the need to continue
@@ -1202,7 +1202,7 @@ proc semNormalizedConst(c: PContext, n: PNode): PNode =
           localReport(c.config, defPart.info, reportSem(rsemResultShadowed))
 
     if v.isError:
-      producedDecl[i] = newSymNode2(v)
+      producedDecl[i] = v.ast # ast is an error AST
       hasError = true
 
       continue # refactor: remove the need to continue

tests/error_propagation/tfor_loop_error.nim

@@ -0,0 +1,12 @@
+discard """
+  description: '''
+    Ensure for-loop vars are usable in call expressions when the iterable slot
+    has an error.
+  '''
+  matrix: "--errorMax:100"
+  errormsg: "undeclared identifier: 'unknown'"
+  line: 11
+"""
+
+for x in unknown:
+  echo x