Extended side effect error messages (nim-lang#18418)

* Extended side effect error messages * Applied feedback: - refactored `markSideEffect` - refactored string interpolations - single message - skip diagnostics in `system.compiles` context Other: - started a test of diagnostic messages [ci skip] Tests aren't updated yet because messaging isn't nailed down. * - Added hints of where for side effect call locations. - Tried to clarify the reasons. * fix tests * Applied PR review feedback: - moved collection of side effects from TSym to TContext - used pragma shorthand form `.sideEffect` and `.noSideEffect` in messages - added leading '>' to structured messages for readability - changed `sempass2.markSideEffect` to a proc - replaced `system.echo` in the test to make the test compatible with Windows * Applied NEP1 formatting suggestion Co-authored-by: quantimnot <[email protected]>
PMunch · Mar 28, 2022 · e79a937 · e79a937
1 parent 7ef3c3f
commit e79a937
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 17 deletions.
diff --git a/compiler/semdata.nim b/compiler/semdata.nim
@@ -159,6 +159,7 @@ type
     exportIndirections*: HashSet[(int, int)] # (module.id, symbol.id)
     importModuleMap*: Table[int, int] # (module.id, module.id)
     lastTLineInfo*: TLineInfo
+    sideEffects*: Table[int, seq[(TLineInfo, PSym)]] # symbol.id index
 
 template config*(c: PContext): ConfigRef = c.graph.config
 

diff --git a/compiler/sempass2.nim b/compiler/sempass2.nim
@@ -10,7 +10,7 @@
 import
   intsets, ast, astalgo, msgs, renderer, magicsys, types, idents, trees,
   wordrecg, strutils, options, guards, lineinfos, semfold, semdata,
-  modulegraphs, varpartitions, typeallowed, nilcheck, errorhandling
+  modulegraphs, varpartitions, typeallowed, nilcheck, errorhandling, tables
 
 when defined(useDfa):
   import dfa
@@ -223,13 +223,21 @@ proc markGcUnsafe(a: PEffects; reason: PNode) =
         a.owner.gcUnsafetyReason = newSym(skUnknown, a.owner.name, nextSymId a.c.idgen,
                                           a.owner, reason.info, {})
 
-when true:
-  template markSideEffect(a: PEffects; reason: typed) =
-    if not a.inEnforcedNoSideEffects: a.hasSideEffect = true
-else:
-  template markSideEffect(a: PEffects; reason: typed) =
-    if not a.inEnforcedNoSideEffects: a.hasSideEffect = true
-    markGcUnsafe(a, reason)
+proc markSideEffect(a: PEffects; reason: PNode | PSym; useLoc: TLineInfo) =
+  if not a.inEnforcedNoSideEffects:
+    a.hasSideEffect = true
+    if a.owner.kind in routineKinds:
+      var sym: PSym
+      when reason is PNode:
+        if reason.kind == nkSym:
+          sym = reason.sym
+        else:
+          let kind = if reason.kind == nkHiddenDeref: skParam else: skUnknown
+          sym = newSym(kind, a.owner.name, nextSymId a.c.idgen, a.owner, reason.info, {})
+      else:
+        sym = reason
+      a.c.sideEffects.mgetOrPut(a.owner.id, @[]).add (useLoc, sym)
+    when false: markGcUnsafe(a, reason)
 
 proc listGcUnsafety(s: PSym; onlyWarning: bool; cycleCheck: var IntSet; conf: ConfigRef) =
   let u = s.gcUnsafetyReason
@@ -259,6 +267,31 @@ proc listGcUnsafety(s: PSym; onlyWarning: bool; conf: ConfigRef) =
   var cycleCheck = initIntSet()
   listGcUnsafety(s, onlyWarning, cycleCheck, conf)
 
+proc listSideEffects(result: var string; s: PSym; cycleCheck: var IntSet;
+                     conf: ConfigRef; context: PContext; indentLevel: int) =
+  template addHint(msg; lineInfo; sym; level = indentLevel) =
+    result.addf("$# $# Hint: '$#' $#\n", repeat(">", level), conf $ lineInfo, sym, msg)
+  if context.sideEffects.hasKey(s.id):
+    for (useLineInfo, u) in context.sideEffects[s.id]:
+      if u != nil and not cycleCheck.containsOrIncl(u.id):
+        case u.kind
+        of skLet, skVar:
+          addHint("accesses global state '$#'" % u.name.s, useLineInfo, s.name.s)
+          addHint("accessed by '$#'" % s.name.s, u.info, u.name.s, indentLevel + 1)
+        of routineKinds:
+          addHint("calls `.sideEffect` '$#'" % u.name.s, useLineInfo, s.name.s)
+          addHint("called by '$#'" % s.name.s, u.info, u.name.s, indentLevel + 1)
+          listSideEffects(result, u, cycleCheck, conf, context, indentLevel + 2)
+        of skParam, skForVar:
+          addHint("calls routine via hidden pointer indirection", useLineInfo, s.name.s)
+        else:
+          addHint("calls routine via pointer indirection", useLineInfo, s.name.s)
+
+proc listSideEffects(result: var string; s: PSym; conf: ConfigRef; context: PContext) =
+  var cycleCheck = initIntSet()
+  result.addf("'$#' can have side effects\n", s.name.s)
+  listSideEffects(result, s, cycleCheck, conf, context, 1)
+
 proc useVarNoInitCheck(a: PEffects; n: PNode; s: PSym) =
   if {sfGlobal, sfThread} * s.flags != {} and s.kind in {skVar, skLet} and
       s.magic != mNimvm:
@@ -267,9 +300,7 @@ proc useVarNoInitCheck(a: PEffects; n: PNode; s: PSym) =
         (tfHasGCedMem in s.typ.flags or s.typ.isGCedMem):
       #if a.config.hasWarn(warnGcUnsafe): warnAboutGcUnsafe(n)
       markGcUnsafe(a, s)
-      markSideEffect(a, s)
-    else:
-      markSideEffect(a, s)
+    markSideEffect(a, s, n.info)
   if s.owner != a.owner and s.kind in {skVar, skLet, skForVar, skResult, skParam} and
      {sfGlobal, sfThread} * s.flags == {}:
     a.isInnerProc = true
@@ -502,7 +533,7 @@ proc propagateEffects(tracked: PEffects, n: PNode, s: PSym) =
     if tracked.config.hasWarn(warnGcUnsafe): warnAboutGcUnsafe(n, tracked.config)
     markGcUnsafe(tracked, s)
   if tfNoSideEffect notin s.typ.flags:
-    markSideEffect(tracked, s)
+    markSideEffect(tracked, s, n.info)
   mergeLockLevels(tracked, n, s.getLockLevel)
 
 proc procVarCheck(n: PNode; conf: ConfigRef) =
@@ -584,15 +615,15 @@ proc trackOperandForIndirectCall(tracked: PEffects, n: PNode, paramType: PType;
         if tracked.config.hasWarn(warnGcUnsafe): warnAboutGcUnsafe(n, tracked.config)
         markGcUnsafe(tracked, a)
       elif tfNoSideEffect notin op.flags and not isOwnedProcVar(a, tracked.owner):
-        markSideEffect(tracked, a)
+        markSideEffect(tracked, a, n.info)
     else:
       mergeRaises(tracked, effectList[exceptionEffects], n)
       mergeTags(tracked, effectList[tagEffects], n)
       if notGcSafe(op):
         if tracked.config.hasWarn(warnGcUnsafe): warnAboutGcUnsafe(n, tracked.config)
         markGcUnsafe(tracked, a)
       elif tfNoSideEffect notin op.flags:
-        markSideEffect(tracked, a)
+        markSideEffect(tracked, a, n.info)
   if paramType != nil and paramType.kind in {tyVar}:
     invalidateFacts(tracked.guards, n)
     if n.kind == nkSym and isLocalVar(tracked, n.sym):
@@ -749,7 +780,7 @@ proc trackCall(tracked: PEffects; n: PNode) =
     if tfNoSideEffect notin op.flags and not importedFromC(a):
       # and it's not a recursive call:
       if not (a.kind == nkSym and a.sym == tracked.owner):
-        markSideEffect(tracked, a)
+        markSideEffect(tracked, a, n.info)
 
   # p's effects are ours too:
   var a = n[0]
@@ -771,7 +802,7 @@ proc trackCall(tracked: PEffects; n: PNode) =
         if a.sym == tracked.owner: tracked.isRecursive = true
         # even for recursive calls we need to check the lock levels (!):
         mergeLockLevels(tracked, n, a.sym.getLockLevel)
-        if sfSideEffect in a.sym.flags: markSideEffect(tracked, a)
+        if sfSideEffect in a.sym.flags: markSideEffect(tracked, a, n.info)
       else:
         mergeLockLevels(tracked, n, op.lockLevel)
       var effectList = op.n[0]
@@ -1336,13 +1367,15 @@ proc trackProc*(c: PContext; s: PSym, body: PNode) =
     effects[ensuresEffects] = ensuresSpec
 
   var mutationInfo = MutationInfo()
+  var hasMutationSideEffect = false
   if {strictFuncs, views} * c.features != {}:
     var goals: set[Goal] = {}
     if strictFuncs in c.features: goals.incl constParameters
     if views in c.features: goals.incl borrowChecking
     var partitions = computeGraphPartitions(s, body, g, goals)
     if not t.hasSideEffect and t.hasDangerousAssign:
       t.hasSideEffect = varpartitions.hasSideEffect(partitions, mutationInfo)
+      hasMutationSideEffect = t.hasSideEffect
     if views in c.features:
       checkBorrowedLocations(partitions, body, g.config)
 
@@ -1357,7 +1390,14 @@ proc trackProc*(c: PContext; s: PSym, body: PNode) =
     when false:
       listGcUnsafety(s, onlyWarning=false, g.config)
     else:
-      localError(g.config, s.info, ("'$1' can have side effects" % s.name.s) & (g.config $ mutationInfo))
+      if hasMutationSideEffect:
+        localError(g.config, s.info, "'$1' can have side effects$2" % [s.name.s, g.config $ mutationInfo])
+      elif c.compilesContextId == 0: # don't render extended diagnostic messages in `system.compiles` context
+        var msg = ""
+        listSideEffects(msg, s, g.config, t.c)
+        message(g.config, s.info, errGenerated, msg)
+      else:
+        localError(g.config, s.info, "") # simple error for `system.compiles` context
   if not t.gcUnsafe:
     s.typ.flags.incl tfGcSafe
   if not t.hasSideEffect and sfSideEffect notin s.flags:

diff --git a/tests/effects/tdiagnostic_messages.nim b/tests/effects/tdiagnostic_messages.nim
@@ -0,0 +1,37 @@
+discard """
+  nimoutFull: true
+  action: "reject"
+  cmd: "nim r --hint:Conf:off $file"
+  nimout: '''
+tdiagnostic_messages.nim(36, 6) Error: 'a' can have side effects
+> tdiagnostic_messages.nim(37, 30) Hint: 'a' calls `.sideEffect` 'callWithSideEffects'
+>> tdiagnostic_messages.nim(29, 6) Hint: 'callWithSideEffects' called by 'a'
+>>> tdiagnostic_messages.nim(31, 34) Hint: 'callWithSideEffects' calls `.sideEffect` 'indirectCallViaVarParam'
+>>>> tdiagnostic_messages.nim(25, 6) Hint: 'indirectCallViaVarParam' called by 'callWithSideEffects'
+>>>>> tdiagnostic_messages.nim(26, 7) Hint: 'indirectCallViaVarParam' calls routine via hidden pointer indirection
+>>> tdiagnostic_messages.nim(32, 33) Hint: 'callWithSideEffects' calls `.sideEffect` 'indirectCallViaPointer'
+>>>> tdiagnostic_messages.nim(27, 6) Hint: 'indirectCallViaPointer' called by 'callWithSideEffects'
+>>>>> tdiagnostic_messages.nim(28, 32) Hint: 'indirectCallViaPointer' calls routine via pointer indirection
+>>> tdiagnostic_messages.nim(33, 10) Hint: 'callWithSideEffects' calls `.sideEffect` 'myEcho'
+>>>> tdiagnostic_messages.nim(24, 6) Hint: 'myEcho' called by 'callWithSideEffects'
+>>> tdiagnostic_messages.nim(34, 3) Hint: 'callWithSideEffects' accesses global state 'globalVar'
+>>>> tdiagnostic_messages.nim(23, 5) Hint: 'globalVar' accessed by 'callWithSideEffects'
+
+'''
+"""
+
+var globalVar = 0
+proc myEcho(a: string) {.sideEffect.} = discard
+proc indirectCallViaVarParam(call: var proc(): int {.nimcall.}): int =
+  call()
+proc indirectCallViaPointer(call: pointer): int =
+  cast[ptr proc(): int](call)[]()
+proc callWithSideEffects(): int =
+  var p = proc (): int {.nimcall.} = 0
+  discard indirectCallViaVarParam(p)
+  discard indirectCallViaPointer(addr p)
+  myEcho ""
+  globalVar
+
+func a: int =
+  discard callWithSideEffects()