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

11 files changed, 124 insertions, 20 deletions

diff --git a/compiler/ast.nim b/compiler/ast.nim
index 3b10a89ea..43ea2b39c 100644
--- a/compiler/ast.nim
+++ b/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)
 
diff --git a/compiler/commands.nim b/compiler/commands.nim
index 0545ee2b0..935d7e2be 100644
--- a/compiler/commands.nim
+++ b/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:
diff --git a/compiler/condsyms.nim b/compiler/condsyms.nim
index d82f4811d..a84b0d878 100644
--- a/compiler/condsyms.nim
+++ b/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")
diff --git a/compiler/dfa.nim b/compiler/dfa.nim
index b46db9aed..0f23d7f1a 100644
--- a/compiler/dfa.nim
+++ b/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)
 
diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim
index bdd624281..520ba46ce 100644
--- a/compiler/injectdestructors.nim
+++ b/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 &
-         "` is already consumed at " & toFileLineCol(c. graph.config, s.info))
+    localError(c.graph.config, c.otherRead.info, "sink parameter `" & $s.sym.name.s &
+        "` 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
diff --git a/compiler/options.nim b/compiler/options.nim
index c93fc0648..42406272c 100644
--- a/compiler/options.nim
+++ b/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}
 
diff --git a/compiler/pragmas.nim b/compiler/pragmas.nim
index 559c1c064..3e89917ed 100644
--- a/compiler/pragmas.nim
+++ b/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:
diff --git a/compiler/sempass2.nim b/compiler/sempass2.nim
index 98ef8cf85..af706bee1 100644
--- a/compiler/sempass2.nim
+++ b/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):
-        createTypeBoundOps(t, param.typ, param.info)
+      let typ = param.typ
+      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
diff --git a/compiler/semstmts.nim b/compiler/semstmts.nim
index deffe563c..4d4f70245 100644
--- a/compiler/semstmts.nim
+++ b/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
diff --git a/compiler/sinkparameter_inference.nim b/compiler/sinkparameter_inference.nim
new file mode 100644
index 000000000..1becc250e
--- /dev/null
+++ b/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"
diff --git a/compiler/wordrecg.nim b/compiler/wordrecg.nim
index d03c22952..ed5d2649f 100644
--- a/compiler/wordrecg.nim
+++ b/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",