strict func: much better error messages (#15068)

* strict func: much better error messages
* documented the 'strict funcs' mode

changelog.md

@@ -101,7 +101,7 @@
 - `osproc.execCmdEx` now takes an optional `input` for stdin, `workingDir` and `env`
   parameters.
 
-- Add `ssl_config` module containing lists of secure ciphers as recommended by
+- Added a `ssl_config` module containing lists of secure ciphers as recommended by
   [Mozilla OpSec](https://wiki.mozilla.org/Security/Server_Side_TLS)
 
 - `net.newContext` now defaults to the list of ciphers targeting
@@ -110,27 +110,28 @@
   users from the use of weak and insecure ciphers while still provides
   adequate compatibility with the majority of the Internet.
 
-- new module `std/jsonutils` with hookable `jsonTo,toJson,fromJson` for json
+- A new module `std/jsonutils` with hookable `jsonTo,toJson,fromJson` operations for json
-  serialization/deserialization of custom types.
+  serialization/deserialization of custom types was added.
 
-- new proc `heapqueue.find[T](heap: HeapQueue[T], x: T): int` to get index of element ``x``.
+- A new proc `heapqueue.find[T](heap: HeapQueue[T], x: T): int` to get index of element ``x``
-- Add `rstgen.rstToLatex` convenience proc for `renderRstToOut` and `initRstGenerator`
+  was added.
+- Added `rstgen.rstToLatex` convenience proc for `renderRstToOut` and `initRstGenerator`
   with `outLatex` output.
-- Add `os.normalizeExe`, e.g.: `koch` => `./koch`.
+- Added `os.normalizeExe`, e.g.: `koch` => `./koch`.
 - `macros.newLit` now preserves named vs unnamed tuples; use `-d:nimHasWorkaround14720`
   to keep old behavior.
-- Add `random.gauss`, that uses the ratio of uniforms method of sampling from a Gaussian distribution.
+- Added `random.gauss`, that uses the ratio of uniforms method of sampling from a Gaussian distribution.
-- Add `typetraits.elementType` to get element type of an iterable.
+- Added `typetraits.elementType` to get element type of an iterable.
 - `typetraits.$` changes: `$(int,)` is now `"(int,)"` instead of `"(int)"`;
   `$tuple[]` is now `"tuple[]"` instead of `"tuple"`;
   `$((int, float), int)` is now `"((int, float), int)"` instead of `"(tuple of (int, float), int)"`
-- add `macros.extractDocCommentsAndRunnables` helper
+- Added `macros.extractDocCommentsAndRunnables` helper
 
 - `strformat.fmt` and `strformat.&` support `= specifier`. `fmt"{expr=}"` now
   expands to `fmt"expr={expr}"`.
 - deprecations: `os.existsDir` => `dirExists`, `os.existsFile` => `fileExists`
 
-- Add `jsre` module, [Regular Expressions for the JavaScript target.](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions)
+- Added `jsre` module, [Regular Expressions for the JavaScript target.](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions)
 - Made `maxLines` argument `Positive` in `logging.newRollingFileLogger`,
   because negative values will result in a new file being created for each logged
   line which doesn't make sense.
@@ -144,7 +145,7 @@
 
 ## Language changes
 - The `=destroy` hook no longer has to reset its target, as the compiler now automatically inserts
-  wasMoved calls where needed.
+  `wasMoved` calls where needed.
 - In the newruntime it is now allowed to assign to the discriminator field
   without restrictions as long as case object doesn't have custom destructor.
   The discriminator value doesn't have to be a constant either. If you have a
@@ -217,6 +218,10 @@ proc mydiv(a, b): int {.raises: [].} =
 
 - Remove `sharedlists.initSharedList`, was deprecated and produces undefined behaviour.
 
+- There is a new experimental feature called "strictFuncs" which makes the definition of
+  `.noSideEffect` stricter. [See](manual_experimental.html#stricts-funcs)
+  for more information.
+
 
 ## Compiler changes
 

compiler/sempass2.nim

@@ -1237,8 +1237,9 @@ proc trackProc*(c: PContext; s: PSym, body: PNode) =
     patchResult(t, ensuresSpec)
     effects[ensuresEffects] = ensuresSpec
 
+  var mutationInfo = MutationInfo()
   if strictFuncs in c.features and not t.hasSideEffect and t.hasDangerousAssign:
-    t.hasSideEffect = mutatesNonVarParameters(s, body)
+    t.hasSideEffect = mutatesNonVarParameters(s, body, mutationInfo)
 
   if sfThread in s.flags and t.gcUnsafe:
     if optThreads in g.config.globalOptions and optThreadAnalysis in g.config.globalOptions:
@@ -1251,7 +1252,7 @@ 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)
+      localError(g.config, s.info, ("'$1' can have side effects" % s.name.s) & (g.config $ mutationInfo))
   if not t.gcUnsafe:
     s.typ.flags.incl tfGcSafe
   if not t.hasSideEffect and sfSideEffect notin s.flags:

compiler/varpartitions.nim

@@ -11,7 +11,7 @@
 ## Nim's write tracking. The used algorithm is "union find"
 ## with path compression.
 
-import ast, types
+import ast, types, lineinfos, options, msgs
 from trees import getMagic
 from isolation_check import canAlias
 
@@ -31,14 +31,36 @@ type
     of dependsOn: parent: int
     of isRootOf: graphIndex: int
 
+  MutationInfo* = object
+    param: PSym
+    mutatedHere, connectedVia: TLineInfo
+    flags: set[SubgraphFlag]
+
   Partitions = object
     symToId: seq[PSym]
     s: seq[VarIndex]
-    graphs: seq[set[SubgraphFlag]]
+    graphs: seq[MutationInfo]
 
-proc hasSideEffect(p: Partitions): bool =
+proc `$`*(config: ConfigRef; g: MutationInfo): string =
-  for g in p.graphs:
+  result = ""
-    if g == {isMutated, connectsConstParam}: return true
+  if g.flags == {isMutated, connectsConstParam}:
+    result.add "\nan object reachable from '"
+    result.add g.param.name.s
+    result.add "' is potentially mutated"
+    if g.mutatedHere != unknownLineInfo:
+      result.add "\n"
+      result.add config $ g.mutatedHere
+      result.add " the mutation is here"
+    if g.connectedVia != unknownLineInfo:
+      result.add "\n"
+      result.add config $ g.connectedVia
+      result.add " is the statement that connected the mutation to the parameter"
+
+proc hasSideEffect(p: var Partitions; info: var MutationInfo): bool =
+  for g in mitems p.graphs:
+    if g.flags == {isMutated, connectsConstParam}:
+      info = g
+      return true
   return false
 
 template isConstParam(a): bool = a.kind == skParam and a.typ.kind != tyVar
@@ -48,7 +70,8 @@ proc registerVariable(p: var Partitions; n: PNode) =
     p.symToId.add n.sym
     if isConstParam(n.sym):
       p.s.add VarIndex(kind: isRootOf, graphIndex: p.graphs.len)
-      p.graphs.add({connectsConstParam})
+      p.graphs.add MutationInfo(param: n.sym, mutatedHere: unknownLineInfo,
+                            connectedVia: unknownLineInfo, flags: {connectsConstParam})
     else:
       p.s.add VarIndex(kind: isEmptyRoot)
 
@@ -56,31 +79,40 @@ proc variableId(p: Partitions; x: PSym): int {.inline.} = system.find(p.symToId,
 
 proc root(v: var Partitions; start: int): int =
   result = start
+  var depth = 0
   while v.s[result].kind == dependsOn:
     result = v.s[result].parent
-  # path compression:
+    inc depth
-  var it = start
+  if depth > 0:
-  while v.s[it].kind == dependsOn:
+    # path compression:
-    let next = v.s[it].parent
+    var it = start
-    v.s[it] = VarIndex(kind: dependsOn, parent: result)
+    while v.s[it].kind == dependsOn:
-    it = next
+      let next = v.s[it].parent
+      v.s[it] = VarIndex(kind: dependsOn, parent: result)
+      it = next
 
-proc potentialMutation(v: var Partitions; s: PSym) =
+proc potentialMutation(v: var Partitions; s: PSym; info: TLineInfo) =
   let id = variableId(v, s)
   if id >= 0:
     let r = root(v, id)
     case v.s[r].kind
     of isEmptyRoot:
       v.s[r] = VarIndex(kind: isRootOf, graphIndex: v.graphs.len)
-      v.graphs.add({isMutated})
+      v.graphs.add MutationInfo(param: if isConstParam(s): s else: nil, mutatedHere: info,
+                            connectedVia: unknownLineInfo, flags: {isMutated})
     of isRootOf:
-      v.graphs[v.s[r].graphIndex].incl isMutated
+      let g = addr v.graphs[v.s[r].graphIndex]
+      if g.param == nil and isConstParam(s):
+        g.param = s
+      if g.mutatedHere == unknownLineInfo:
+        g.mutatedHere = info
+      g.flags.incl isMutated
     else:
       assert false, "cannot happen"
   else:
     discard "we are not interested in the mutation"
 
-proc connect(v: var Partitions; a, b: PSym) =
+proc connect(v: var Partitions; a, b: PSym; info: TLineInfo) =
   let aid = variableId(v, a)
   if aid < 0:
     return
@@ -91,26 +123,37 @@ proc connect(v: var Partitions; a, b: PSym) =
   let ra = root(v, aid)
   let rb = root(v, bid)
   if ra != rb:
+    var param = PSym(nil)
+    if isConstParam(a): param = a
+    elif isConstParam(b): param = b
+
     let paramFlags =
-      if isConstParam(a) or isConstParam(b):
+      if param != nil:
         {connectsConstParam}
       else:
         {}
 
     # for now we always make 'rb' the slave and 'ra' the master:
-    let rbFlags =
+    var rbFlags: set[SubgraphFlag] = {}
-      if v.s[rb].kind == isRootOf:
+    var mutatedHere = unknownLineInfo
-        v.graphs[v.s[rb].graphIndex]
+    if v.s[rb].kind == isRootOf:
-      else:
+      var gb = addr v.graphs[v.s[rb].graphIndex]
-        {}
+      if param == nil: param = gb.param
+      mutatedHere = gb.mutatedHere
+      rbFlags = gb.flags
 
     v.s[rb] = VarIndex(kind: dependsOn, parent: ra)
     case v.s[ra].kind
     of isEmptyRoot:
       v.s[ra] = VarIndex(kind: isRootOf, graphIndex: v.graphs.len)
-      v.graphs.add(paramFlags + rbFlags)
+      v.graphs.add MutationInfo(param: param, mutatedHere: mutatedHere,
+                            connectedVia: info, flags: paramFlags + rbFlags)
     of isRootOf:
-      v.graphs[v.s[ra].graphIndex].incl paramFlags + rbFlags
+      var g = addr v.graphs[v.s[ra].graphIndex]
+      if g.param == nil: g.param = param
+      if g.mutatedHere == unknownLineInfo: g.mutatedHere = mutatedHere
+      g.connectedVia = info
+      g.flags.incl paramFlags + rbFlags
     else:
       assert false, "cannot happen"
 
@@ -144,8 +187,6 @@ proc allRoots(n: PNode; result: var seq[PSym]) =
       let m = getMagic(n)
       case m
       of mNone:
-        # we do significantly better here by using the available escape
-        # information:
         if n[0].typ.isNil: return
         var typ = n[0].typ
         if typ != nil:
@@ -177,12 +218,12 @@ proc deps(p: var Partitions; dest, src: PNode) =
   allRoots(src, sources)
   for t in targets:
     if dest.kind != nkSym:
-      potentialMutation(p, t)
+      potentialMutation(p, t, dest.info)
 
     proc wrap(t: PType): bool {.nimcall.} = t.kind in {tyRef, tyPtr}
     if types.searchTypeFor(t.typ, wrap):
       for s in sources:
-        connect(p, t, s)
+        connect(p, t, s, dest.info)
 
 proc traverse(p: var Partitions; n: PNode) =
   case n.kind
@@ -224,13 +265,12 @@ proc traverse(p: var Partitions; n: PNode) =
         if paramType.typ.kind == tyVar:
           var roots: seq[PSym]
           allRoots(n, roots)
-          for r in roots: potentialMutation(p, r)
+          for r in roots: potentialMutation(p, r, n.info)
 
   else:
     for child in n: traverse(p, child)
 
-
+proc mutatesNonVarParameters*(s: PSym; n: PNode; info: var MutationInfo): bool =
-proc mutatesNonVarParameters*(s: PSym; n: PNode): bool =
   var par = Partitions()
   if s.kind != skMacro:
     let params = s.typ.n
@@ -240,4 +280,4 @@ proc mutatesNonVarParameters*(s: PSym; n: PNode): bool =
       registerVariable(par, s.ast[resultPos])
 
   traverse(par, n)
-  result = hasSideEffect(par)
+  result = hasSideEffect(par, info)

doc/manual_experimental.rst

@@ -1844,3 +1844,41 @@ via ``.noSideEffect``. The rules 3 and 4 can also be approximated by a different
 
 5. A global or thread local variable (or a location derived from such a location)
    can only passed to a parameter of a ``.noSideEffect`` proc.
+
+
+Strict funcs
+============
+
+Since version 1.4 a stricter definition of "side effect" is available. In addition
+to the existing rule that a side effect is calling a function with side effects
+the following rule is also enforced:
+
+Any mutation to an object does count as a side effect if that object is reachable
+via a parameter that is not declared as a ``var`` parameter.
+
+For example:
+
+.. code-block:: nim
+
+  {.experimental: "strictFuncs".}
+
+  type
+    Node = ref object
+      le, ri: Node
+      data: string
+
+  func len(n: Node): int =
+    # valid: len does not have side effects
+    var it = n
+    while it != nil:
+      inc result
+      it = it.ri
+
+  func mut(n: Node) =
+    let m = n # is the statement that connected the mutation to the parameter
+    m.data = "yeah" # the mutation is here
+    # Error: 'mut' can have side effects
+    # an object reachable from 'n' is potentially mutated
+
+
+The algorithm behind this analysis is currently not documented.

tests/effects/tfuncs_cannot_mutate.nim

@@ -1,6 +1,9 @@
 discard """
   errormsg: "'mutate' can have side effects"
-  line: 25
+  nimout: '''an object reachable from 'n' is potentially mutated
+tfuncs_cannot_mutate.nim(34, 15) the mutation is here
+tfuncs_cannot_mutate.nim(32, 7) is the statement that connected the mutation to the parameter'''
+  line: 28
 """
 
 {.experimental: "strictFuncs".}