From 47acc80f4e05053d2653c7218434bc7fad880741 Mon Sep 17 00:00:00 2001 From: Andreas Rumpf Date: Wed, 9 Jun 2021 17:33:19 +0200 Subject: [PATCH] make strict funcs analysis smarter (#18219) * make strict funcs analysis smarter: varParam[i] = v is different from varParam[i][] = v * added a test case * Update compiler/varpartitions.nim Co-authored-by: Clyybber --- compiler/varpartitions.nim | 87 +++++++++++++++---------- tests/effects/tfuncs_cannot_mutate2.nim | 27 ++++++++ tests/effects/tstrict_funcs.nim | 17 +++++ 3 files changed, 95 insertions(+), 36 deletions(-) create mode 100644 tests/effects/tfuncs_cannot_mutate2.nim diff --git a/compiler/varpartitions.nim b/compiler/varpartitions.nim index 810ce8586b..9cf1e38ade 100644 --- a/compiler/varpartitions.nim +++ b/compiler/varpartitions.nim @@ -175,12 +175,18 @@ proc root(v: var Partitions; start: int): int = v.s[it].con = Connection(kind: dependsOn, parent: result) it = next -proc potentialMutation(v: var Partitions; s: PSym; info: TLineInfo) = +proc potentialMutation(v: var Partitions; s: PSym; level: int; info: TLineInfo) = let id = variableId(v, s) if id >= 0: let r = root(v, id) - let flags = if s.kind == skParam and isConstParam(s): - {isMutated, isMutatedDirectly} + let flags = if s.kind == skParam: + if isConstParam(s): + {isMutated, isMutatedDirectly} + elif s.typ.kind == tyVar and level <= 1: + # varParam[i] = v is different from varParam[i][] = v + {} + else: + {isMutated} else: {isMutated} @@ -339,36 +345,44 @@ proc pathExpr(node: PNode; owner: PSym): PNode = if result == nil and borrowFromConstExpr(n): result = n -proc allRoots(n: PNode; result: var seq[PSym]; followDotExpr = true) = +const + RootEscapes = 1000 # in 'p(r)' we don't know what p does to our poor root. + # so we assume a high level of indirections + +proc allRoots(n: PNode; result: var seq[(PSym, int)]; level: int) = case n.kind of nkSym: if n.sym.kind in {skParam, skVar, skTemp, skLet, skResult, skForVar}: - result.add(n.sym) + result.add((n.sym, level)) - of nkDotExpr, nkDerefExpr, nkBracketExpr, nkHiddenDeref, - nkCheckedFieldExpr, nkAddr, nkHiddenAddr: - if followDotExpr: - allRoots(n[0], result, followDotExpr) + of nkDerefExpr, nkHiddenDeref: + allRoots(n[0], result, level+1) + of nkBracketExpr, nkDotExpr, nkCheckedFieldExpr, nkAddr, nkHiddenAddr: + allRoots(n[0], result, level) of nkExprEqExpr, nkExprColonExpr, nkHiddenStdConv, nkHiddenSubConv, nkConv, nkStmtList, nkStmtListExpr, nkBlockStmt, nkBlockExpr, nkCast, nkObjUpConv, nkObjDownConv: if n.len > 0: - allRoots(n.lastSon, result, followDotExpr) + allRoots(n.lastSon, result, level) of nkCaseStmt, nkObjConstr: for i in 1.. 1: - allRoots(n[1], result, followDotExpr) + # XXX We really need the unwritten RFC here and distinguish between + # proc `[]`(x: var Container): var T # resizes the container + # and + # proc `[]`(x: Container): var T # only allows for slot mutation + allRoots(n[1], result, RootEscapes) else: let m = getMagic(n) case m @@ -387,12 +401,12 @@ proc allRoots(n: PNode; result: var seq[PSym]; followDotExpr = true) = let paramType = typ.n[i].typ if not paramType.isCompileTimeOnly and not typ.sons[0].isEmptyType and canAlias(paramType, typ.sons[0]): - allRoots(it, result, followDotExpr) + allRoots(it, result, RootEscapes) else: - allRoots(it, result, followDotExpr) + allRoots(it, result, RootEscapes) of mSlice: - allRoots(n[1], result, followDotExpr) + allRoots(n[1], result, level+1) else: discard "harmless operation" else: @@ -466,10 +480,10 @@ proc destMightOwn(c: var Partitions; dest: var VarIndex; n: PNode) = dest.flags.incl ownsData elif n.typ.kind in {tyLent, tyVar}: # we know the result is derived from the first argument: - var roots: seq[PSym] - allRoots(n[1], roots) + var roots: seq[(PSym, int)] + allRoots(n[1], roots, RootEscapes) for r in roots: - connect(c, dest.sym, r, n[1].info) + connect(c, dest.sym, r[0], n[1].info) else: let magic = if n[0].kind == nkSym: n[0].sym.magic else: mNone @@ -582,19 +596,19 @@ proc deps(c: var Partitions; dest, src: PNode) = if borrowChecking in c.goals: borrowingAsgn(c, dest, src) - var targets, sources: seq[PSym] - allRoots(dest, targets) - allRoots(src, sources) + var targets, sources: seq[(PSym, int)] + allRoots(dest, targets, 0) + allRoots(src, sources, 0) let destIsComplex = containsPointer(dest.typ) for t in targets: if dest.kind != nkSym and c.inNoSideEffectSection == 0: - potentialMutation(c, t, dest.info) + potentialMutation(c, t[0], t[1], dest.info) if destIsComplex: for s in sources: - connect(c, t, s, dest.info) + connect(c, t[0], s[0], dest.info) if cursorInference in c.goals and src.kind != nkEmpty: let d = pathExpr(dest, c.owner) @@ -602,7 +616,8 @@ proc deps(c: var Partitions; dest, src: PNode) = let vid = variableId(c, d.sym) if vid >= 0: destMightOwn(c, c.s[vid], src) - for s in sources: + for source in sources: + let s = source[0] if s == d.sym: discard "assignments like: it = it.next are fine" elif {sfGlobal, sfThread} * s.flags != {} or hasDisabledAsgn(c.g, s.typ): @@ -638,9 +653,9 @@ proc potentialMutationViaArg(c: var Partitions; n: PNode; callee: PType) = if constParameters in c.goals and tfNoSideEffect in callee.flags: discard "we know there are no hidden mutations through an immutable parameter" elif c.inNoSideEffectSection == 0 and containsPointer(n.typ): - var roots: seq[PSym] - allRoots(n, roots) - for r in roots: potentialMutation(c, r, n.info) + var roots: seq[(PSym, int)] + allRoots(n, roots, RootEscapes) + for r in roots: potentialMutation(c, r[0], r[1], n.info) proc traverse(c: var Partitions; n: PNode) = inc c.abstractTime @@ -682,12 +697,12 @@ proc traverse(c: var Partitions; n: PNode) = if i < L: let paramType = parameters[i].skipTypes({tyGenericInst, tyAlias}) if not paramType.isCompileTimeOnly and paramType.kind in {tyVar, tySink, tyOwned}: - var roots: seq[PSym] - allRoots(it, roots) + var roots: seq[(PSym, int)] + allRoots(it, roots, RootEscapes) if paramType.kind == tyVar: if c.inNoSideEffectSection == 0: - for r in roots: potentialMutation(c, r, it.info) - for r in roots: noCursor(c, r) + for r in roots: potentialMutation(c, r[0], r[1], it.info) + for r in roots: noCursor(c, r[0]) if borrowChecking in c.goals: # a call like 'result.add toOpenArray()' can also be a borrow @@ -703,10 +718,10 @@ proc traverse(c: var Partitions; n: PNode) = when false: # XXX investigate if this is required, it doesn't look # like it is! - var roots: seq[PSym] - allRoots(n[0], roots) + var roots: seq[(PSym, int)] + allRoots(n[0], roots, RootEscapes) for r in roots: - potentialMutation(c, r, it.info) + potentialMutation(c, r[0], r[1], it.info) of nkTupleConstr, nkBracket: for child in n: traverse(c, child) diff --git a/tests/effects/tfuncs_cannot_mutate2.nim b/tests/effects/tfuncs_cannot_mutate2.nim new file mode 100644 index 0000000000..d5082e57b9 --- /dev/null +++ b/tests/effects/tfuncs_cannot_mutate2.nim @@ -0,0 +1,27 @@ +discard """ + errormsg: "'copy' can have side effects" + nimout: '''an object reachable from 'y' is potentially mutated +tfuncs_cannot_mutate2.nim(15, 7) the mutation is here +tfuncs_cannot_mutate2.nim(13, 10) is the statement that connected the mutation to the parameter +''' +""" + +{.experimental: "strictFuncs".} + +func copy[T](x: var openArray[T]; y: openArray[T]) = + for i in 0..high(x): + x[i] = y[i] + + x[0].a = nil + +type + R = ref object + a, b: R + data: string + +proc main = + var a, b: array[3, R] + b = [R(data: "a"), R(data: "b"), R(data: "c")] + copy a, b + +main() diff --git a/tests/effects/tstrict_funcs.nim b/tests/effects/tstrict_funcs.nim index 044bc7ee15..9d20f5d7ee 100644 --- a/tests/effects/tstrict_funcs.nim +++ b/tests/effects/tstrict_funcs.nim @@ -27,3 +27,20 @@ block: var x = @[0, 1] let z = x &&& 2 + + +func copy[T](x: var openArray[T]; y: openArray[T]) = + for i in 0..high(x): + x[i] = y[i] + +type + R = ref object + a, b: R + data: string + +proc main = + var a, b: array[3, R] + b = [R(data: "a"), R(data: "b"), R(data: "c")] + copy a, b + +main()