diff --git a/compiler/ast/report_enums.nim b/compiler/ast/report_enums.nim index aeb4fdba0d..8c0df549e6 100644 --- a/compiler/ast/report_enums.nim +++ b/compiler/ast/report_enums.nim @@ -627,6 +627,8 @@ type rsemCannotBorrowImmutable rsemCannotBorrow2 rsemMustBeConstructor + rsemOverlappingParamBorrows + rsemPotentialAliasViolation rsemCyclicTree rsemCyclicDependency diff --git a/compiler/ast/reports_sem.nim b/compiler/ast/reports_sem.nim index 8e41a097b5..1809d10e6e 100644 --- a/compiler/ast/reports_sem.nim +++ b/compiler/ast/reports_sem.nim @@ -202,7 +202,7 @@ type of rsemDiagnostics: diag*: SemDiagnostics - of rsemIllegalBorrow: + of rsemIllegalBorrow, rsemOverlappingParamBorrows: borrow*: PNode isProblemMutation*: bool problem*, usage*: TLineInfo diff --git a/compiler/front/cli_reporter.nim b/compiler/front/cli_reporter.nim index 44c0060522..ff0f91efb3 100644 --- a/compiler/front/cli_reporter.nim +++ b/compiler/front/cli_reporter.nim @@ -576,6 +576,15 @@ proc reportBody*(conf: ConfigRef, r: SemReport): string = result.add "cannot borrow from expression: " & $r.ast of rsemMustBeConstructor: result.add "RHS of compound view assignment must be constructor expression" + of rsemOverlappingParamBorrows: + if r.isProblemMutation: + result.add "cannot create another mutable borrow\n" + result.add "$1 another, potentially overlapping, mutable borrow was created here" % [conf.toStr(r.usage)] + else: + result.add "cannot create mutable borrow\n" + result.add "$1 a potentially overlapping immutable borrow was created here" % [conf.toStr(r.usage)] + of rsemPotentialAliasViolation: + result.add "cannot create borrow because the location could be modified through '$1'" % [$r.ast] of rsemPragmaRecursiveDependency: result.add "recursive dependency: " diff --git a/compiler/sem/borrowchecks.nim b/compiler/sem/borrowchecks.nim index 05da8aedb4..23fdc5f52d 100644 --- a/compiler/sem/borrowchecks.nim +++ b/compiler/sem/borrowchecks.nim @@ -5,10 +5,14 @@ import compiler/ast/[ ast, renderer, - types + types, + lineinfos ], compiler/front/[ options, msgs + ], + compiler/sem/[ + aliases ] from compiler/ast/reports_sem import SemReport @@ -21,14 +25,14 @@ import std/private/asciitables type InstrKind* = enum goto, loop, fork - def, use, mut, kill, borrow + def, use, mut, kill, borrow, mborrow call Instr* = object n*: PNode ## contains the def/use/mut case kind: InstrKind of goto, loop, fork: dest*: int - of borrow: + of borrow, mborrow: borrower: PNode else: discard @@ -71,7 +75,7 @@ proc codeListing(c: ControlFlowGraph, start = 0; last = -1): string = result.add ($i & " " & $c[i].kind) result.add "\t" case c[i].kind - of def, use, mut, kill, borrow, call: + of def, use, mut, kill, borrow, mborrow, call: result.add renderTree(c[i].n) of goto, fork, loop: result.add "L" @@ -262,8 +266,12 @@ proc genCase(c: var Con; n: PNode) = endings.add c.gotoI(it.lastSon) proc genBlock(c: var Con; n: PNode) = - withBlock(n[0].sym): - c.gen(n[1]) + if n[0].kind != nkEmpty: + withBlock(n[0].sym): + c.gen(n[1]) + else: + withBlock nil: + c.gen(n[1]) proc genBreakOrRaiseAux(c: var Con, i: int, n: PNode) = for v in c.blocks[i].vars ..< c.vars.len: @@ -365,10 +373,60 @@ proc skipConvDfa*(n: PNode): PNode = result = result[1] else: break -type AliasKind* = enum +type AliasKind = enum yes, no, maybe -proc aliases*(obj, field: PNode): AliasKind = +proc root(n: PNode): PNode + +template isRef(t: PType): bool = + t.skipTypes({tyGenericInst, tyAlias, tySink}).kind == tyRef + +proc aliases(path: PNode, typ: PType): bool = + ## Computes whether the location named by `path` can be mutated through a + ## pointer of type `typ`. + assert typ.kind in {tyPtr, tyRef} + # things to consider: + # * a ref always points to the heap + # * a ref always points to a complete location (unless not final :( ) + # * a ptr can point anywhere + let root = root(path) + # XXX: root skips view derefs, which could result in unwanted behaviour + # XXX: this won't work... at least not in the way ``aliases`` is used right + # now + case root.kind + of nkSym: + # some non-heap location + if typ.kind == tyRef: + result = false # a guaranteed no + elif root.sym.kind in {skLet, skForVar}: + # immutable aliasing is irrelevant + result = false + else: + let p = if root.typ.kind in {tyVar, tyLent}: root.typ.base + else: root.typ + + result = typ.base.isPartOf(p) != arNo or + p.isPartOf(typ.base) != arNo + # XXX: the above doesn't take ``a.b.c``, where `typ` is a pointer to + # `a.b`, into account + of nkDerefExpr: + if isRef(root[0].typ): + if sameType(root[0].typ, typ): + # XXX: why sameType? What about distinct types? + result = true + elif typ.kind == tyPtr: + result = sameType(root[0].typ.base, typ.base) + elif typ.kind == tyPtr: + # given ``p[].x`` and ``T`` + result = path.typ.isPartOf(typ.base) != arNo or + root.typ.isPartOf(typ.base) != arNo + else: + # a ``ref A`` can only alias some locations in a ``ptr B`` if A == B + result = sameType(root[0].typ.base, typ.base) + else: + doAssert false + +proc aliases(obj, field: PNode): AliasKind = ##[ ============ =========== ==== @@ -392,7 +450,7 @@ obj field alias kind ]## - template collectImportantNodes(result, n) = + template collectImportantNodes(result, n, root) = var result: seq[PNode] var n = n while true: @@ -404,12 +462,38 @@ obj field alias kind of nkDotExpr, nkBracketExpr: result.add n n = n[0] - of nkSym: - result.add n; break - else: return no + of nkSym, nkDerefExpr: + root = n; break + else: + doAssert false - collectImportantNodes(objImportantNodes, obj) - collectImportantNodes(fieldImportantNodes, field) + var rootA, rootB: PNode + collectImportantNodes(objImportantNodes, obj, rootA) + collectImportantNodes(fieldImportantNodes, field, rootB) + + if rootA.kind == rootB.kind: + case rootA.kind + of nkSym: + if rootA.sym.id == rootB.sym.id: + result = yes + else: + # the paths cannot alias + return no + of nkDerefExpr: + discard "XXX: missing" + else: + discard "XXX: missing" + elif rootA.kind == nkDerefExpr: + # XXX: incomplete + if rootA[0].typ.skipTypes({tyGenericInst, tyAlias, tySink}).kind == tyPtr: + if isPartOf(rootA[0].typ.skipTypes(abstractInst), field.typ) != TAnalysisResult.arNo: + return maybe # could alias + else: + # refs cannot point to locals + return no + + # FIXME: a.x and a[0] aren't considered to alias, due to the nkBracketExpr + # vs. nkDotExpr usage... # If field is less nested than obj, then it cannot be part of/aliased by obj if fieldImportantNodes.len < objImportantNodes.len: return no @@ -430,8 +514,6 @@ obj field alias kind return no case currFieldPath.kind - of nkSym: - if currFieldPath.sym != currObjPath.sym: return no of nkDotExpr: if currFieldPath[1].sym != currObjPath[1].sym: return no of nkBracketExpr: @@ -455,18 +537,20 @@ proc skipTrivials(n: PNode): PNode = proc isInteresting(c: Con, s: PSym): bool = s.kind in InterestingSyms +proc isInteresting(n: PNode): bool = + (n.kind == nkSym and n.sym.kind in InterestingSyms) or + n.kind in {nkDerefExpr, nkHiddenDeref} + proc genUse(c: var Con; orig: PNode) = let n = skipTrivials(orig) - if (n.kind == nkSym and c.isInteresting(n.sym)) or - n.kind in {nkDerefExpr, nkHiddenDeref}: + if isInteresting(n): c.code.add Instr(n: orig, kind: use) proc genMut(c: var Con; orig: PNode) = let n = skipTrivials(orig) - if (n.kind == nkSym and c.isInteresting(n.sym)) or - n.kind in {nkDerefExpr, nkHiddenDeref}: + if isInteresting(n): c.code.add Instr(n: orig, kind: mut) proc genDef(c: var Con; orig: PNode) = @@ -477,7 +561,7 @@ proc genDef(c: var Con; orig: PNode) = return n = skipTrivials(n) - if n.kind in {nkSym, nkDerefExpr, nkHiddenDeref}: + if isInteresting(n): # an assignment to a sub-location, e.g., ``a.b = c`` c.code.add Instr(n: orig, kind: mut) @@ -507,28 +591,31 @@ proc genCall(c: var Con; n: PNode) = for i in 1.. 0 and canRaiseConservative(n[0]): @@ -552,7 +639,10 @@ proc genMagic(c: var Con; n: PNode; m: TMagic) = genUse(c, n[2]) gen(c, n[3]) genUse(c, n[3]) - c.code.add Instr(n: n[1], kind: borrow, borrower: c.borrowCtx) + if directViewType(n.typ) == immutableView: + c.code.add Instr(n: n[1], kind: borrow, borrower: c.borrowCtx) + else: + c.code.add Instr(n: n[1], kind: mborrow, borrower: c.borrowCtx) else: genCall(c, n) @@ -628,7 +718,11 @@ proc gen(c: var Con; n: PNode) = n[0] gen(c, x) - c.code.add Instr(n: x, kind: borrow, borrower: c.borrowCtx) + if c.borrowCtx.kind in nkCallKinds or n.typ.kind == tyVar: + c.code.add Instr(n: x, kind: mborrow, borrower: c.borrowCtx) + else: + # XXX: this is wrong! lent doesn't imply immutable borrow + c.code.add Instr(n: x, kind: borrow, borrower: c.borrowCtx) of nkBracketExpr: gen(c, n[0]) genUse(c, n[1]) # the index operand is used @@ -646,8 +740,15 @@ proc gen(c: var Con; n: PNode) = of nkForStmt: genFor(c, n) of nkStmtList, nkStmtListExpr, nkChckRangeF, nkChckRange64, nkChckRange, - nkBracket, nkCurly, nkPar, nkTupleConstr, nkClosure, nkObjConstr, nkYieldStmt: + nkBracket, nkCurly, nkPar, nkTupleConstr, nkClosure, nkRange: for x in n: gen(c, x) + of nkYieldStmt: + withContext n: + gen(c, n[0]) + # TODO: yield probably needs its own DFA instruction + of nkObjConstr: + for i in 1.. 0: + body + return + + case t.kind + of tyDistinct, tyAlias, tyGenericInst, tyInferred, tyVar, tyLent: + result = maybeIndirect(path, t[^1], marker) + of tyArray, tySequence, tyOpenArray: + recurse(elemType(t)): + result.add newTreeI(nkBracketExpr, unknownLineInfo, nil, newIntNode(nkIntLit, 0)) + of tyTuple: + for i in 0.. 0: + return + + case n.kind + of nkSym: + recurse(n.sym.typ): + result.add newTreeI(nkDotExpr, unknownLineInfo, nil, n) + of nkRecList: + for it in n.items: + recurse(it) + of nkRecCase: + recurse(n[0]) + for it in n.items: + recurse(it.lastSon) + else: + doAssert false + + result = aux(path, t.n, marker) + of tyProc, tyPointer, IntegralTypes: + discard "not relevant" + else: + discard + proc report(config: ConfigRef, borrow, use: PNode, problem: Instr) = # TODO: report a better error for kills (i.e., the borrower outliving the borrowee) let rep = SemReport(kind: rsemIllegalBorrow, ast: borrow, @@ -851,10 +1026,14 @@ proc verifyLocalBorrow(config: ConfigRef, c: ControlFlowGraph, start: int) = # mutation of the borrower report(config, path, instr.n, c[problem]) break - of borrow: - if overlaps(path, instr.n): + of borrow, mborrow: + if instr.borrower.kind in nkCallKinds: + # the borrow itself is not relevant; there's a mut/use instruction + # following + discard + elif overlaps(path, instr.n): if instr.borrower.kind == nkSym: - if isMutable or instr.borrower.sym.kind == skVar: + if isMutable or instr.kind == mborrow: problem = i else: # a mutable borrow for a var parameter @@ -874,8 +1053,15 @@ proc verifyLocalBorrow(config: ConfigRef, c: ControlFlowGraph, start: int) = proc verifyParamBorrow(config: ConfigRef, c: ControlFlowGraph, start: int) = let path = c[start].n let target = c[start].borrower - - if isGlobal(path) and tfNoSideEffect notin target[0].typ.skipTypes(abstractInst).flags: + let fntyp = target[0].typ.skipTypes(abstractInst) + + if isGlobal(path) and tfNoSideEffect notin fntyp.flags: + # IDEA: instead of disallowing the borrowing at the callsite, we could + # turn the problem on its head and prevent potentially unsafe + # mutations through globals in the *callee*: + # var x = 0 + # proc a(p: var int) = + # x = 1 # disallowed config.localReport(path.info, SemReport(kind: rsemIllegalParamterBorrow)) return @@ -885,31 +1071,62 @@ proc verifyParamBorrow(config: ConfigRef, c: ControlFlowGraph, start: int) = case instr.kind of def, kill, mut: if overlaps(path, instr.n): - problem = i # mutation/kill of the borrowee + report(config, path, target, instr) + break of use: - if overlaps(path, instr.n): - problem = i # use of the borrowee - of borrow: - if overlaps(path, instr.n): - # TODO: must be a proper error - echo "error: cannot reborrow at ", config.toFileLineCol(instr.n.info) + # consider ``f(borrow x, (use x; ...))``. There's no safety issue here + # XXX: really? what about automatic moves? Maybe possible sinks need to + # be marked as such... + discard + of borrow, mborrow: + if instr.borrower.kind in nkCallKinds and instr.borrower != target: + discard "ignore other call's borrows; the mut/use instruction are what's relevant" + elif overlaps(path, instr.n): + config.localReport(instr.n): + SemReport(kind: rsemOverlappingParamBorrows, usage: path.info, + isProblemMutation: c[start].kind == mborrow) break of call: if instr.n == target: # the call consumes the view break + elif isGlobal(path) and tfNoSideEffect notin instr.n[0].typ.skipTypes(abstractInst).flags: + report(config, path, target, instr) else: doAssert false + # look for potential ref/ptr aliasing: + # XXX: this is a very costly analysis... A type flag marking types as + # contaings refs or pointers would reduce the cost significantly + var marker: IntSet + for i in 1.. 0: + problem[^1][0] = fntyp.n[i] + for j in countdown(problem.len - 2, 0): + problem[j][0] = move problem[j + 1] + + config.localReport(path): + SemReport(kind: rsemPotentialAliasViolation, ast: move problem[0]) + # no need to continue searching + break + proc check*(config: ConfigRef, c: ControlFlowGraph) = for pos, it in c.pairs: - if it.kind == borrow: + if it.kind in {borrow, mborrow}: case it.borrower.kind of nkSym: # borrow to local verifyLocalBorrow(config, c, pos) of nkCallKinds: - # borrow to procedure parameter + # borrow to procedure parameter (or iterator result) verifyParamBorrow(config, c, pos) + of nkYieldStmt: + # XXX: currently ignored. It needs to be ensured that mutable borrows + # don't conflict with immutable ones + discard else: config.internalError(it.n.info, $it.borrower.kind) diff --git a/compiler/sem/semexprs.nim b/compiler/sem/semexprs.nim index 788b68eac2..016c821a7c 100644 --- a/compiler/sem/semexprs.nim +++ b/compiler/sem/semexprs.nim @@ -2437,6 +2437,9 @@ proc semYieldVarResult(c: PContext, n: PNode, restype: PType): PNode = addInNimDebugUtils(c.config, "semYieldVarResult", n, result) result = n # n is a production already + # TODO: remove all this special handling. The analysis should be no + # different than a ``x: view = y`` assignment + let t = skipTypes(restype, {tyGenericInst, tyAlias, tySink}) var hasError = false diff --git a/tests/lang_experimental/views/tborrow_checking_3.nim b/tests/lang_experimental/views/tborrow_checking_3.nim new file mode 100644 index 0000000000..5c6bd36c66 --- /dev/null +++ b/tests/lang_experimental/views/tborrow_checking_3.nim @@ -0,0 +1,113 @@ + +{.experimental: "views".} + +proc error() = + proc a(a: var seq[int], b: var seq[int]) = discard + + var s = @[1] + a(s, s) # error; mutable borrows of the same location + +proc error_2() = + type Container = object + children: seq[Container] + + proc p(a, b: var Container) = discard + + var c = Container(children: @[Container()]) + p(c, c.children[0]) # error + +proc error_3() = + type Cont = object + children: seq[ref Cont] + + proc inner(a: var Cont, b: ref Cont) = discard + + var x = Cont() + x.children.add (ref Cont)() + inner(x, x.children[0]) # not safe, because `b` is a 'lent' parameter + # if parameter 'a' were a ref, the same issue exists, but - for ergonomic + # reasons - no error is reported there + +proc error_4() = + type Cont = object + children: seq[ref Cont] + + proc inner(a: var Cont, b: var Cont) = + discard + + let p = (ref Cont)() + var x = Cont(children: @[p]) + inner(x, p[]) # p[] could be modified by `x` (and through itself) + +proc indirect_error() = + type SomeObj = object + x: ptr seq[int] + + proc p(x: var seq[int], y: SomeObj) = + discard + + var x: ref seq[int] + p(x[], SomeObj()) # error; there's a pointer to `seq[int]` in SomeObj + # the actual arguments are ignored by the analysis, only the parameter types + # count + +proc indirect_error_2() = + type SomeRef = ref object + x: ptr seq[int] + + proc p(x: var seq[int], y: SomeRef) = discard + + var x: ref seq[int] + p(x[], nil) # error; same reason as above + +proc self_alias_error() = + type Obj = object + self: ref Obj + + proc inner(x: var Obj) = discard + + var x: ref Obj + inner(x[]) # error; x could hold a ref to itself + +var global = 0 + +proc side_effect_error() = + proc p(x: var seq[int]) = + global = 1 + + # if arc|orc are enabled, seq[int] is a 'lent' parameter + proc p2(x: seq[int]) = + global = 1 + + var r: ref seq[int] + + # p and p2 could read/write through a ref/ptr, for all we know + p2(r[]) + p(r[]) + + var pt: ptr seq[int] + p2(pt[]) + p(pt[]) + +proc address_to_local() = + proc p(x: seq[int]) = + global = 1 + + let x = @[1] + p(x) # not an error; a let cannot (legally) be modified through a pointer + var y = @[1] + p(y) # works, but should it? some global could hold a pointer to `y` + +proc local_borrow() = + proc p1(x: seq[int], y: ptr seq[int]) = + discard + + let a = @[1] + p1(a, nil) # always safe; a let cannot legally be modified through a pointer + var b = @[1] + p1(b, nil) # error; the var could be modified + + proc p2(x: seq[int], y: ref seq[int]) = discard + # always safe; a ref can only point to a heap location + p2(a, nil) + p2(b, nil)