sink parameter inference for types that have destructors (#13544)

* ensure capitalize doesn't take an inferred sink parameter

* sink parameter inference: first version, for now disabled. Changed that sink parameters can be consumed multiple times in order to adhere to our spec.

* sink inference can now be disabled with .nosinks; sometimes for proc type interop this is required

* fixes yet another critical DFA bug

* better implementation that also understands if expressions etc

* document sink parameter inference and allow for global disabling

changelog.md

@@ -127,7 +127,6 @@ echo f
 - `=sink` type bound operator is now optional. Compiler can now use combination
   of `=destroy` and `copyMem` to move objects efficiently.
 
-
 ## Language changes
 
 - Unsigned integer operators have been fixed to allow promotion of the first operand.
@@ -150,6 +149,9 @@ echo f
 - The Nim compiler now supports a new pragma called ``.localPassc`` to
   pass specific compiler options to the C(++) backend for the C(++) file
   that was produced from the current Nim module.
+- The compiler now inferes "sink parameters". To disable this for a specific routine,
+  annotate it with `.nosinks`. To disable it for a section of code, use
+  `{.push sinkInference: off.}`...`{.pop.}`.
 
 
 ## Bugfixes

compiler/ast.nim

@@ -229,7 +229,7 @@ type
   TNodeKinds* = set[TNodeKind]
 
 type
-  TSymFlag* = enum    # 40 flags!
+  TSymFlag* = enum    # 41 flags!
     sfUsed,           # read access of sym (for warnings) or simply used
     sfExported,       # symbol is exported from module
     sfFromGeneric,    # symbol is instantiation of a generic; this is needed
@@ -238,6 +238,8 @@ type
     sfGlobal,         # symbol is at global scope
 
     sfForward,        # symbol is forward declared
+    sfWasForwarded,   # symbol had a forward declaration
+                      # (implies it's too dangerous to patch its type signature)
     sfImportc,        # symbol is external; imported
     sfExportc,        # symbol is exported (under a specified name)
     sfMangleCpp,      # mangle as cpp (combines with `sfExportc`)
@@ -847,7 +849,7 @@ type
     position*: int            # used for many different things:
                               # for enum fields its position;
                               # for fields its offset
-                              # for parameters its position
+                              # for parameters its position (starting with 0)
                               # for a conditional:
                               # 1 iff the symbol is defined, else 0
                               # (or not in symbol table)
@@ -1865,6 +1867,9 @@ proc isInlineIterator*(typ: PType): bool {.inline.} =
 proc isClosureIterator*(typ: PType): bool {.inline.} =
   typ.kind == tyProc and tfIterator in typ.flags and typ.callConv == ccClosure
 
+proc isClosure*(typ: PType): bool {.inline.} =
+  typ.kind == tyProc and typ.callConv == ccClosure
+
 proc isSinkParam*(s: PSym): bool {.inline.} =
   s.kind == skParam and (s.typ.kind == tySink or tfHasOwned in s.typ.flags)
 

compiler/commands.nim

@@ -877,6 +877,8 @@ proc processSwitch*(switch, arg: string, pass: TCmdLinePass, info: TLineInfo;
       localError(conf, info, "unknown Nim version; currently supported values are: {1.0}")
   of "benchmarkvm":
     processOnOffSwitchG(conf, {optBenchmarkVM}, arg, pass, info)
+  of "sinkinference":
+    processOnOffSwitch(conf, {optSinkInference}, arg, pass, info)
   of "": # comes from "-" in for example: `nim c -r -` (gets stripped from -)
     handleStdinInput(conf)
   else:

compiler/condsyms.nim

@@ -111,3 +111,5 @@ proc initDefines*(symbols: StringTableRef) =
     # will report the right thing regardless of whether user adds
     # `-d:nimHasLibFFI` in his user config.
     defineSymbol("nimHasLibFFIEnabled")
+
+  defineSymbol("nimHasSinkInference")

compiler/dfa.nim

@@ -591,6 +591,8 @@ proc genUse(c: var Con; orig: PNode) =
     of PathKinds1:
       n = n[1]
     else: break
+    if n.kind in nkCallKinds:
+      gen(c, n)
   if n.kind == nkSym and n.sym.kind in InterestingSyms:
     c.code.add Instr(n: orig, kind: use)
 

compiler/injectdestructors.nim

@@ -321,8 +321,8 @@ proc destructiveMoveVar(n: PNode; c: var Con): PNode =
 proc sinkParamIsLastReadCheck(c: var Con, s: PNode) =
   assert s.kind == nkSym and s.sym.kind == skParam
   if not isLastRead(s, c):
-     localError(c.graph.config, c.otherRead.info, "sink parameter `" & $s.sym.name.s &
+    localError(c.graph.config, c.otherRead.info, "sink parameter `" & $s.sym.name.s &
-         "` is already consumed at " & toFileLineCol(c. graph.config, s.info))
+        "` is already consumed at " & toFileLineCol(c. graph.config, s.info))
 
 type
   ProcessMode = enum
@@ -498,10 +498,10 @@ proc p(n: PNode; c: var Con; mode: ProcessMode): PNode =
     elif n.kind in {nkBracket, nkObjConstr, nkTupleConstr, nkClosure, nkNilLit} +
          nkCallKinds + nkLiterals:
       result = p(n, c, consumed)
-    elif n.kind == nkSym and isSinkParam(n.sym):
+    elif n.kind == nkSym and isSinkParam(n.sym) and isLastRead(n, c):
       # Sinked params can be consumed only once. We need to reset the memory
       # to disable the destructor which we have not elided
-      sinkParamIsLastReadCheck(c, n)
+      #sinkParamIsLastReadCheck(c, n)
       result = destructiveMoveVar(n, c)
     elif isAnalysableFieldAccess(n, c.owner) and isLastRead(n, c):
       # it is the last read, can be sinkArg. We need to reset the memory
@@ -687,9 +687,9 @@ proc moveOrCopy(dest, ri: PNode; c: var Con): PNode =
   of nkObjConstr, nkTupleConstr, nkClosure, nkCharLit..nkNilLit:
     result = genSink(c, dest, p(ri, c, consumed))
   of nkSym:
-    if isSinkParam(ri.sym):
+    if isSinkParam(ri.sym) and isLastRead(ri, c):
       # Rule 3: `=sink`(x, z); wasMoved(z)
-      sinkParamIsLastReadCheck(c, ri)
+      #sinkParamIsLastReadCheck(c, ri)
       let snk = genSink(c, dest, ri)
       result = newTree(nkStmtList, snk, genWasMoved(ri, c))
     elif ri.sym.kind != skParam and ri.sym.owner == c.owner and

compiler/options.nim

@@ -39,7 +39,8 @@ type                          # please make sure we have under 32 options
     optMemTracker,
     optLaxStrings,
     optNilSeqs,
-    optOldAst
+    optOldAst,
+    optSinkInference          # 'sink T' inference
 
   TOptions* = set[TOption]
   TGlobalOption* = enum       # **keep binary compatible**
@@ -200,7 +201,7 @@ type
                           ## the incremental compilation mechanisms
                           ## (+) means "part of the dependency"
     target*: Target       # (+)
-    linesCompiled*: int  # all lines that have been compiled
+    linesCompiled*: int   # all lines that have been compiled
     options*: TOptions    # (+)
     globalOptions*: TGlobalOptions # (+)
     macrosToExpand*: StringTableRef
@@ -315,7 +316,7 @@ const
   DefaultOptions* = {optObjCheck, optFieldCheck, optRangeCheck,
     optBoundsCheck, optOverflowCheck, optAssert, optWarns, optRefCheck,
     optHints, optStackTrace, optLineTrace,
-    optTrMacros, optNilCheck, optStyleCheck}
+    optTrMacros, optNilCheck, optStyleCheck, optSinkInference}
   DefaultGlobalOptions* = {optThreadAnalysis,
     optExcessiveStackTrace, optListFullPaths}
 

compiler/pragmas.nim

@@ -23,7 +23,7 @@ const
     wExportNims, wExtern, wDeprecated, wNodecl, wError, wUsed, wAlign}
     ## common pragmas for declarations, to a good approximation
   procPragmas* = declPragmas + {FirstCallConv..LastCallConv,
-    wMagic, wNoSideEffect, wSideEffect, wNoreturn, wDynlib, wHeader,
+    wMagic, wNoSideEffect, wSideEffect, wNoreturn, wNosinks, wDynlib, wHeader,
     wCompilerProc, wNonReloadable, wCore, wProcVar, wVarargs, wCompileTime, wMerge,
     wBorrow, wImportCompilerProc, wThread,
     wAsmNoStackFrame, wDiscardable, wNoInit, wCodegenDecl,
@@ -53,7 +53,7 @@ const
     wLinearScanEnd, wPatterns, wTrMacros, wEffects, wNoForward, wReorder, wComputedGoto,
     wInjectStmt, wExperimental, wThis, wUsed}
   lambdaPragmas* = declPragmas + {FirstCallConv..LastCallConv,
-    wNoSideEffect, wSideEffect, wNoreturn, wDynlib, wHeader,
+    wNoSideEffect, wSideEffect, wNoreturn, wNosinks, wDynlib, wHeader,
     wThread, wAsmNoStackFrame,
     wRaises, wLocks, wTags, wGcSafe, wCodegenDecl} - {wExportNims, wError, wUsed}  # why exclude these?
   typePragmas* = declPragmas + {wMagic, wAcyclic,
@@ -357,6 +357,7 @@ proc pragmaToOptions(w: TSpecialWord): TOptions {.inline.} =
   of wByRef: {optByRef}
   of wImplicitStatic: {optImplicitStatic}
   of wPatterns, wTrMacros: {optTrMacros}
+  of wSinkInference: {optSinkInference}
   else: {}
 
 proc processExperimental(c: PContext; n: PNode) =
@@ -889,6 +890,9 @@ proc singlePragma(c: PContext, sym: PSym, n: PNode, i: var int,
       of wNoDestroy:
         noVal(c, it)
         incl(sym.flags, sfGeneratedOp)
+      of wNosinks:
+        noVal(c, it)
+        incl(sym.flags, sfWasForwarded)
       of wDynlib:
         processDynLib(c, it, sym)
       of wCompilerProc, wCore:

compiler/sempass2.nim

@@ -16,6 +16,7 @@ when defined(useDfa):
   import dfa
 
 import liftdestructors
+include sinkparameter_inference
 
 #[ Second semantic checking pass over the AST. Necessary because the old
    way had some inherent problems. Performs:
@@ -774,6 +775,10 @@ proc track(tracked: PEffects, n: PNode) =
 
     for i in 0..<n.safeLen:
       track(tracked, n[i])
+    if op != nil and op.kind == tyProc:
+      for i in 1..<min(n.safeLen, op.len):
+        if op[i].kind == tySink:
+          checkForSink(tracked.config, tracked.owner, n[i])
   of nkDotExpr:
     guardDotAccess(tracked, n)
     for i in 0..<n.len: track(tracked, n[i])
@@ -793,6 +798,7 @@ proc track(tracked: PEffects, n: PNode) =
     when false: cstringCheck(tracked, n)
     if tracked.owner.kind != skMacro:
       createTypeBoundOps(tracked, n[0].typ, n.info)
+    checkForSink(tracked.config, tracked.owner, n[1])
   of nkVarSection, nkLetSection:
     for child in n:
       let last = lastSon(child)
@@ -871,6 +877,11 @@ proc track(tracked: PEffects, n: PNode) =
         addDiscriminantFact(tracked.guards, x)
       if tracked.owner.kind != skMacro:
         createTypeBoundOps(tracked, x[1].typ, n.info)
+
+      if x.kind == nkExprColonExpr:
+        checkForSink(tracked.config, tracked.owner, x[1])
+      else:
+        checkForSink(tracked.config, tracked.owner, x)
     setLen(tracked.guards.s, oldFacts)
     if tracked.owner.kind != skMacro:
       # XXX n.typ can be nil in runnableExamples, we need to do something about it.
@@ -882,6 +893,7 @@ proc track(tracked: PEffects, n: PNode) =
       track(tracked, n[i])
       if tracked.owner.kind != skMacro:
         createTypeBoundOps(tracked, n[i].typ, n.info)
+      checkForSink(tracked.config, tracked.owner, n[i])
   of nkPragmaBlock:
     let pragmaList = n[0]
     let oldLocked = tracked.locked.len
@@ -927,7 +939,9 @@ proc track(tracked: PEffects, n: PNode) =
         createTypeBoundOps(tracked, n.typ, n.info)
         createTypeBoundOps(tracked, n[0].typ, n[0].info)
   of nkBracket:
-    for i in 0..<n.safeLen: track(tracked, n[i])
+    for i in 0..<n.safeLen:
+      track(tracked, n[i])
+      checkForSink(tracked.config, tracked.owner, n[i])
     if tracked.owner.kind != skMacro:
       createTypeBoundOps(tracked, n.typ, n.info)
   else:
@@ -1035,8 +1049,10 @@ proc trackProc*(c: PContext; s: PSym, body: PNode) =
     let params = s.typ.n
     for i in 1..<params.len:
       let param = params[i].sym
-      if isSinkTypeForParam(param.typ):
+      let typ = param.typ
-        createTypeBoundOps(t, param.typ, param.info)
+      if isSinkTypeForParam(typ) or
+          (t.config.selectedGC in {gcArc, gcOrc} and isClosure(typ.skipTypes(abstractInst))):
+        createTypeBoundOps(t, typ, param.info)
 
   if not isEmptyType(s.typ[0]) and
       ({tfNeedsInit, tfNotNil} * s.typ[0].flags != {} or

compiler/semstmts.nim

@@ -1884,6 +1884,7 @@ proc semProcAux(c: PContext, n: PNode, kind: TSymKind,
     if sfForward notin proto.flags and proto.magic == mNone:
       wrongRedefinition(c, n.info, proto.name.s, proto.info)
     excl(proto.flags, sfForward)
+    incl(proto.flags, sfWasForwarded)
     closeScope(c)         # close scope with wrong parameter symbols
     openScope(c)          # open scope for old (correct) parameter symbols
     if proto.ast[genericParamsPos].kind != nkEmpty:
@@ -1962,6 +1963,7 @@ proc semProcAux(c: PContext, n: PNode, kind: TSymKind,
     if proto != nil: localError(c.config, n.info, errImplOfXexpected % proto.name.s)
     if {sfImportc, sfBorrow, sfError} * s.flags == {} and s.magic == mNone:
       incl(s.flags, sfForward)
+      incl(s.flags, sfWasForwarded)
     elif sfBorrow in s.flags: semBorrow(c, n, s)
   sideEffectsCheck(c, s)
   closeScope(c)           # close scope for parameters

compiler/sinkparameter_inference.nim

@@ -0,0 +1,70 @@
+#
+#
+#           The Nim Compiler
+#        (c) Copyright 2020 Andreas Rumpf
+#
+#    See the file "copying.txt", included in this
+#    distribution, for details about the copyright.
+#
+
+proc checkForSink*(config: ConfigRef; owner: PSym; arg: PNode) =
+  #[ Patterns we seek to detect:
+
+    someLocation = p # ---> p: sink T
+    passToSink(p)    # p: sink
+    ObjConstr(fieldName: p)
+    [p, q] # array construction
+
+    # Open question:
+    var local = p # sink parameter?
+    passToSink(local)
+  ]#
+  if optSinkInference notin config.options: return
+  case arg.kind
+  of nkSym:
+    if arg.sym.kind == skParam and
+        arg.sym.owner == owner and
+        owner.typ != nil and owner.typ.kind == tyProc and
+        arg.sym.typ.hasDestructor and
+        arg.sym.typ.kind notin {tyVar, tySink, tyOwned}:
+      # Watch out: cannot do this inference for procs with forward
+      # declarations.
+      if sfWasForwarded notin owner.flags:
+        let argType = arg.sym.typ
+
+        let sinkType = newType(tySink, owner)
+        sinkType.size = argType.size
+        sinkType.align = argType.align
+        sinkType.paddingAtEnd = argType.paddingAtEnd
+        sinkType.add argType
+
+        arg.sym.typ = sinkType
+        owner.typ[arg.sym.position+1] = sinkType
+
+        #message(config, arg.info, warnUser,
+        #  ("turned '$1' to a sink parameter") % [$arg])
+        #echo config $ arg.info, " turned into a sink parameter ", arg.sym.name.s
+      elif sfWasForwarded notin arg.sym.flags:
+        # we only report every potential 'sink' parameter only once:
+        incl arg.sym.flags, sfWasForwarded
+        message(config, arg.info, hintPerformance,
+          ("could not turn '$1' to a sink parameter " &
+          "because '$2' was forward declared") % [arg.sym.name.s, owner.name.s])
+      #echo config $ arg.info, " candidate for a sink parameter here"
+  of nkStmtList, nkStmtListExpr, nkBlockStmt, nkBlockExpr:
+    if not isEmptyType(arg.typ):
+      checkForSink(config, owner, arg.lastSon)
+  of nkIfStmt, nkIfExpr, nkWhen:
+    for branch in arg:
+      let value = branch.lastSon
+      if not isEmptyType(value.typ):
+        checkForSink(config, owner, value)
+  of nkCaseStmt:
+    for i in 1..<arg.len:
+      let value = arg[i].lastSon
+      if not isEmptyType(value.typ):
+        checkForSink(config, owner, value)
+  of nkTryStmt:
+    checkForSink(config, owner, arg[0])
+  else:
+    discard "nothing to do"

compiler/wordrecg.nim

@@ -42,7 +42,7 @@ type
     wImportCompilerProc,
     wImportc, wImportJs, wExportc, wExportCpp, wExportNims, wIncompleteStruct, wRequiresInit,
     wAlign, wNodecl, wPure, wSideEffect, wHeader,
-    wNoSideEffect, wGcSafe, wNoreturn, wMerge, wLib, wDynlib,
+    wNoSideEffect, wGcSafe, wNoreturn, wNosinks, wMerge, wLib, wDynlib,
     wCompilerProc, wCore, wProcVar, wBase, wUsed,
     wFatal, wError, wWarning, wHint, wLine, wPush, wPop, wDefine, wUndef,
     wLineDir, wStackTrace, wLineTrace, wLink, wCompile,
@@ -52,7 +52,7 @@ type
     wBoundChecks, wOverflowChecks, wNilChecks,
     wFloatChecks, wNanChecks, wInfChecks, wStyleChecks,
     wNonReloadable, wExecuteOnReload,
-    wAssertions, wPatterns, wTrMacros, wWarnings,
+    wAssertions, wPatterns, wTrMacros, wSinkInference, wWarnings,
     wHints, wOptimization, wRaises, wWrites, wReads, wSize, wEffects, wTags,
     wDeadCodeElimUnused,  # deprecated, dead code elim always happens
     wSafecode, wPackage, wNoForward, wReorder, wNoRewrite, wNoDestroy,
@@ -128,7 +128,7 @@ const
     "importcompilerproc", "importc", "importjs", "exportc", "exportcpp", "exportnims",
     "incompletestruct",
     "requiresinit", "align", "nodecl", "pure", "sideeffect",
-    "header", "nosideeffect", "gcsafe", "noreturn", "merge", "lib", "dynlib",
+    "header", "nosideeffect", "gcsafe", "noreturn", "nosinks", "merge", "lib", "dynlib",
     "compilerproc", "core", "procvar", "base", "used",
     "fatal", "error", "warning", "hint", "line",
     "push", "pop", "define", "undef", "linedir", "stacktrace", "linetrace",
@@ -140,7 +140,7 @@ const
     "floatchecks", "nanchecks", "infchecks", "stylechecks",
     "nonreloadable", "executeonreload",
 
-    "assertions", "patterns", "trmacros", "warnings", "hints",
+    "assertions", "patterns", "trmacros", "sinkinference", "warnings", "hints",
     "optimization", "raises", "writes", "reads", "size", "effects", "tags",
     "deadcodeelim",  # deprecated, dead code elim always happens
     "safecode", "package", "noforward", "reorder", "norewrite", "nodestroy",

doc/advopt.txt

@@ -145,3 +145,4 @@ Advanced options:
                             works better with `--stackTrace:on`
                             see also https://nim-lang.github.io/Nim/estp.html
   --benchmarkVM:on|off      enable benchmarking of VM code with cpuTime()
+  --sinkInference:on|off    en-/disable sink parameter inference (default: on)

doc/destructors.rst

@@ -256,6 +256,23 @@ An implementation is allowed, but not required to implement even more move
 optimizations (and the current implementation does not).
 
 
+Sink parameter inference
+========================
+
+The current implementation does a limited form of sink parameter
+inference. The `.nosinks`:idx: pragma can be used to disable this inference
+for a single routine:
+
+.. code-block:: nim
+
+  proc addX(x: T; child: T) {.nosinks.} =
+    x.s.add child
+
+To disable it for a section of code, one can
+use `{.push sinkInference: off.}`...`{.pop.}`.
+
+The details of the inference algorithm are currently undocumented.
+
 
 Rewrite rules
 =============

lib/pure/unicode.nim

@@ -701,7 +701,7 @@ proc capitalize*(s: string): string {.noSideEffect, procvar,
     doAssert capitalize("βeta") == "Βeta"
 
   if len(s) == 0:
-    return s
+    return ""
   var
     rune: Rune
     i = 0

lib/system/assertions.nim

@@ -15,8 +15,10 @@ proc `$`(info: InstantiationInfo): string =
 
 # ---------------------------------------------------------------------------
 
+when not defined(nimHasSinkInference):
+  {.pragma: nosinks.}
 
-proc raiseAssert*(msg: string) {.noinline, noreturn.} =
+proc raiseAssert*(msg: string) {.noinline, noreturn, nosinks.} =
   sysFatal(AssertionError, msg)
 
 proc failedAssertImpl*(msg: string) {.raises: [], tags: [].} =

tests/assert/tassert_c.nim

@@ -6,8 +6,8 @@ discard """
 const expected = """
 tassert_c.nim(35)        tassert_c
 tassert_c.nim(34)        foo
-assertions.nim(27)       failedAssertImpl
+assertions.nim(29)       failedAssertImpl
-assertions.nim(20)       raiseAssert
+assertions.nim(22)       raiseAssert
 fatal.nim(55)            sysFatal"""
 
 proc tmatch(x, p: string): bool =

tests/destructor/tarc2.nim

@@ -13,9 +13,13 @@ proc create(): T = T(s: @[], data: "abc")
 proc addX(x: T; data: string) =
   x.data = data
 
+{.push sinkInference: off.}
+
 proc addX(x: T; child: T) =
   x.s.add child
 
+{.pop.}
+
 proc main(rootName: string) =
   var root = create()
   root.data = rootName

tests/destructor/tconsume_twice.nim

@@ -1,7 +1,7 @@
 discard """
   cmd: "nim c --newruntime $file"
-  errormsg: "sink parameter `a` is already consumed at tconsume_twice.nim(11, 10)"
+  errormsg: "'=' is not available for type <owned Foo>; requires a copy because it's not the last read of 'a'; another read is done here: tconsume_twice.nim(13, 10); routine: consumeTwice"
-  line: 13
+  line: 11
 """
 type
   Foo = ref object