side-effect computation now done in the proper pass; fixes #4254

compiler/semexprs.nim

@@ -701,9 +701,6 @@ proc semOverloadedCallAnalyseEffects(c: PContext, n: PNode, nOrig: PNode,
         # error correction, prevents endless for loop elimination in transf.
         # See bug #2051:
         result.sons[0] = newSymNode(errorSym(c, n))
-      if sfNoSideEffect notin callee.flags:
-        if {sfImportc, sfSideEffect} * callee.flags != {}:
-          incl(c.p.owner.flags, sfSideEffect)
 
 proc semObjConstr(c: PContext, n: PNode, flags: TExprFlags): PNode
 
@@ -804,9 +801,6 @@ proc semIndirectOp(c: PContext, n: PNode, flags: TExprFlags): PNode =
     else:
       result = m.call
       instGenericConvertersSons(c, result, m)
-    # we assume that a procedure that calls something indirectly
-    # has side-effects:
-    if tfNoSideEffect notin t.flags: incl(c.p.owner.flags, sfSideEffect)
   elif t != nil and t.kind == tyTypeDesc:
     if n.len == 1: return semObjConstr(c, n, flags)
     return semConv(c, n)
@@ -1040,9 +1034,6 @@ proc semSym(c: PContext, n: PNode, s: PSym, flags: TExprFlags): PNode =
 
     markUsed(n.info, s)
     styleCheckUse(n.info, s)
-    # if a proc accesses a global variable, it is not side effect free:
-    if sfGlobal in s.flags:
-      incl(c.p.owner.flags, sfSideEffect)
     result = newSymNode(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:

compiler/seminst.nim

@@ -172,9 +172,10 @@ proc fixupInstantiatedSymbols(c: PContext, s: PSym) =
       popInfoContext()
 
 proc sideEffectsCheck(c: PContext, s: PSym) =
-  if {sfNoSideEffect, sfSideEffect} * s.flags ==
-      {sfNoSideEffect, sfSideEffect}:
-    localError(s.info, errXhasSideEffects, s.name.s)
+  when false:
+    if {sfNoSideEffect, sfSideEffect} * s.flags ==
+        {sfNoSideEffect, sfSideEffect}:
+      localError(s.info, errXhasSideEffects, s.name.s)
 
 proc instGenericContainer(c: PContext, info: TLineInfo, header: PType,
                           allowMetaTypes = false): PType =

compiler/sempass2.nim

@@ -59,7 +59,7 @@ type
     init: seq[int] # list of initialized variables
     guards: TModel # nested guards
     locked: seq[PNode] # locked locations
-    gcUnsafe, isRecursive, isToplevel: bool
+    gcUnsafe, isRecursive, isToplevel, hasSideEffect: bool
     maxLockLevel, currLockLevel: TLockLevel
   PEffects = var TEffects
 
@@ -192,6 +192,14 @@ proc markGcUnsafe(a: PEffects; reason: PNode) =
       a.owner.gcUnsafetyReason = newSym(skUnknown, getIdent("<unknown>"),
                                         a.owner, reason.info)
 
+when true:
+  template markSideEffect(a: PEffects; reason: typed) =
+    a.hasSideEffect = true
+else:
+  template markSideEffect(a: PEffects; reason: typed) =
+    a.hasSideEffect = true
+    markGcUnsafe(a, reason)
+
 proc listGcUnsafety(s: PSym; onlyWarning: bool; cycleCheck: var IntSet) =
   let u = s.gcUnsafetyReason
   if u != nil and not cycleCheck.containsOrIncl(u.id):
@@ -226,12 +234,16 @@ proc useVar(a: PEffects, n: PNode) =
         message(n.info, warnUninit, s.name.s)
       # prevent superfluous warnings about the same variable:
       a.init.add s.id
-  if {sfGlobal, sfThread} * s.flags != {} and s.kind in {skVar, skLet}:
+  if {sfGlobal, sfThread} * s.flags != {} and s.kind in {skVar, skLet} and
+      s.magic != mNimVm:
     if s.guard != nil: guardGlobal(a, n, s.guard)
     if {sfGlobal, sfThread} * s.flags == {sfGlobal} and
         (tfHasGCedMem in s.typ.flags or s.typ.isGCedMem):
       #if warnGcUnsafe in gNotes: warnAboutGcUnsafe(n)
       markGcUnsafe(a, s)
+    else:
+      markSideEffect(a, s)
+
 
 type
   TIntersection = seq[tuple[id, count: int]] # a simple count table
@@ -495,6 +507,8 @@ proc propagateEffects(tracked: PEffects, n: PNode, s: PSym) =
   if notGcSafe(s.typ) and sfImportc notin s.flags:
     if warnGcUnsafe in gNotes: warnAboutGcUnsafe(n)
     markGcUnsafe(tracked, s)
+  if tfNoSideEffect notin s.typ.flags:
+    markSideEffect(tracked, s)
   mergeLockLevels(tracked, n, s.getLockLevel)
 
 proc notNilCheck(tracked: PEffects, n: PNode, paramType: PType) =
@@ -550,12 +564,16 @@ proc trackOperand(tracked: PEffects, n: PNode, paramType: PType) =
       if notGcSafe(op) and not isOwnedProcVar(a, tracked.owner):
         if warnGcUnsafe in gNotes: warnAboutGcUnsafe(n)
         markGcUnsafe(tracked, a)
+      elif tfNoSideEffect notin op.flags and not isOwnedProcVar(a, tracked.owner):
+        markSideEffect(tracked, a)
     else:
       mergeEffects(tracked, effectList.sons[exceptionEffects], n)
       mergeTags(tracked, effectList.sons[tagEffects], n)
       if notGcSafe(op):
         if warnGcUnsafe in gNotes: warnAboutGcUnsafe(n)
         markGcUnsafe(tracked, a)
+      elif tfNoSideEffect notin op.flags:
+        markSideEffect(tracked, a)
   notNilCheck(tracked, n, paramType)
 
 proc breaksBlock(n: PNode): bool =
@@ -684,6 +702,7 @@ proc track(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)
       else:
         mergeLockLevels(tracked, n, op.lockLevel)
       var effectList = op.n.sons[0]
@@ -702,6 +721,10 @@ proc track(tracked: PEffects, n: PNode) =
           if not (a.kind == nkSym and a.sym == tracked.owner):
             if warnGcUnsafe in gNotes: warnAboutGcUnsafe(n)
             markGcUnsafe(tracked, a)
+        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)
     for i in 1 .. <len(n): trackOperand(tracked, n.sons[i], paramType(op, i))
     if a.kind == nkSym and a.sym.magic in {mNew, mNewFinalize, mNewSeq}:
       # may not look like an assignment, but it is:
@@ -912,8 +935,15 @@ proc trackProc*(s: PSym, body: PNode) =
     else:
       listGcUnsafety(s, onlyWarning=true)
       #localError(s.info, warnGcUnsafe2, s.name.s)
+  if sfNoSideEffect in s.flags and t.hasSideEffect:
+    when false:
+      listGcUnsafety(s, onlyWarning=false)
+    else:
+      localError(s.info, errXhasSideEffects, s.name.s)
   if not t.gcUnsafe:
     s.typ.flags.incl tfGcSafe
+  if not t.hasSideEffect:
+    s.typ.flags.incl tfNoSideEffect
   if s.typ.lockLevel == UnspecifiedLockLevel:
     s.typ.lockLevel = t.maxLockLevel
   elif t.maxLockLevel > s.typ.lockLevel:

tests/effects/tsidee4.nim

@@ -1,13 +1,13 @@
 discard """
   file: "tsidee4.nim"
-  line: 15
-  errormsg: "type mismatch"
+  line: 12
+  errormsg: "'noSideEffect' can have side effects"
 """
 
 var
   global: int
 
-proc dontcare(x: int): int = return x
+proc dontcare(x: int): int = return global
 
 proc noSideEffect(x, y: int, p: proc (a: int): int {.noSideEffect.}): int {.noSideEffect.} =
   return x + y + dontcare(x)