diff options
| -rw-r--r-- | changelog.md | 27 | ||||
| -rw-r--r-- | compiler/sempass2.nim | 5 | ||||
| -rw-r--r-- | compiler/varpartitions.nim | 106 | ||||
| -rw-r--r-- | doc/manual_experimental.rst | 38 | ||||
| -rw-r--r-- | tests/effects/tfuncs_cannot_mutate.nim | 5 |
5 files changed, 134 insertions, 47 deletions
diff --git a/changelog.md b/changelog.md index e30e9038b..d4161373c 100644 --- a/changelog.md +++ b/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 - serialization/deserialization of custom types. +- A new module `std/jsonutils` with hookable `jsonTo,toJson,fromJson` operations for json + serialization/deserialization of custom types was added. -- 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` +- A new proc `heapqueue.find[T](heap: HeapQueue[T], x: T): int` to get index of element ``x`` + 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. -- Add `typetraits.elementType` to get element type of an iterable. +- Added `random.gauss`, that uses the ratio of uniforms method of sampling from a Gaussian distribution. +- 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 diff --git a/compiler/sempass2.nim b/compiler/sempass2.nim index c6d6d9241..f6fce712a 100644 --- a/compiler/sempass2.nim +++ b/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: diff --git a/compiler/varpartitions.nim b/compiler/varpartitions.nim index b89b7d1da..1dee5607d 100644 --- a/compiler/varpartitions.nim +++ b/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 `$`*(config: ConfigRef; g: MutationInfo): string = + result = "" + 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: Partitions): bool = - for g in p.graphs: - if g == {isMutated, connectsConstParam}: return true +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: - var it = start - while v.s[it].kind == dependsOn: - let next = v.s[it].parent - v.s[it] = VarIndex(kind: dependsOn, parent: result) - it = next - -proc potentialMutation(v: var Partitions; s: PSym) = + inc depth + if depth > 0: + # path compression: + var it = start + while v.s[it].kind == dependsOn: + let next = v.s[it].parent + v.s[it] = VarIndex(kind: dependsOn, parent: result) + it = next + +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 = - if v.s[rb].kind == isRootOf: - v.graphs[v.s[rb].graphIndex] - else: - {} + var rbFlags: set[SubgraphFlag] = {} + var mutatedHere = unknownLineInfo + if v.s[rb].kind == isRootOf: + 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): bool = +proc mutatesNonVarParameters*(s: PSym; n: PNode; info: var MutationInfo): 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) diff --git a/doc/manual_experimental.rst b/doc/manual_experimental.rst index b0614885a..43aa84f20 100644 --- a/doc/manual_experimental.rst +++ b/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. diff --git a/tests/effects/tfuncs_cannot_mutate.nim b/tests/effects/tfuncs_cannot_mutate.nim index ec3ad43f7..2990894ed 100644 --- a/tests/effects/tfuncs_cannot_mutate.nim +++ b/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".} |