proof of concept: prefer unambiguous captured symbols in symchoices

changelog.md

@@ -14,6 +14,14 @@
   ```
   will no longer compile.
 
+- In template and generic bodies, for symbols with only 1 overload in scope,
+  previously the compiler would not allow other overloads in the instantiation
+  context to be used, which was not the case for 0 or more than 1 overloads.
+  Now other overloads are allowed with a preference for the original overload
+  in cases of ambiguity. Some code might need to be changed to use `bind` to
+  prevent outside overloads from replacing a weaker captured overload. See
+  [issue #11184](https://github.com/nim-lang/Nim/issues/11184) for more details.
+
 ## Standard library additions and changes
 
 [//]: # "Changes:"

compiler/ast.nim

@@ -521,6 +521,7 @@ type
     nfHasComment # node has a comment
     nfSkipFieldChecking # node skips field visable checking
     nfOpenSym # node is a captured sym but can be overriden by local symbols
+    nfPreferredSym # node is a preferred sym in a symchoice
 
   TNodeFlags* = set[TNodeFlag]
   TTypeFlag* = enum   # keep below 32 for efficiency reasons (now: 47)
@@ -1097,7 +1098,7 @@ const
                                       nfFromTemplate, nfDefaultRefsParam,
                                       nfExecuteOnReload, nfLastRead,
                                       nfFirstWrite, nfSkipFieldChecking,
-                                      nfOpenSym}
+                                      nfOpenSym, nfPreferredSym}
   namePos* = 0
   patternPos* = 1    # empty except for term rewriting macros
   genericParamsPos* = 2

compiler/lookups.nim

@@ -268,41 +268,6 @@ proc cmpScopes*(ctx: PContext, s: PSym): int =
   else:
     result = 1
 
-proc isAmbiguous*(c: PContext, s: PIdent, filter: TSymKinds, sym: var PSym): bool =
-  result = false
-  block outer:
-    for scope in allScopes(c.currentScope):
-      var ti: TIdentIter
-      var candidate = initIdentIter(ti, scope.symbols, s)
-      var scopeHasCandidate = false
-      while candidate != nil:
-        if candidate.kind in filter:
-          if scopeHasCandidate:
-            # 2 candidates in same scope, ambiguous
-            return true
-          else:
-            scopeHasCandidate = true
-            sym = candidate
-        candidate = nextIdentIter(ti, scope.symbols)
-      if scopeHasCandidate:
-        # scope had a candidate but wasn't ambiguous
-        return false
-
-  var importsHaveCandidate = false
-  var marked = initIntSet()
-  for im in c.imports.mitems:
-    for s in symbols(im, marked, s, c.graph):
-      if s.kind in filter:
-        if importsHaveCandidate:
-          # 2 candidates among imports, ambiguous
-          return true
-        else:
-          importsHaveCandidate = true
-          sym = s
-  if importsHaveCandidate:
-    # imports had a candidate but wasn't ambiguous
-    return false
-
 proc errorSym*(c: PContext, ident: PIdent, info: TLineInfo): PSym =
   ## creates an error symbol to avoid cascading errors (for IDE support)
   result = newSym(skError, ident, c.idgen, getCurrOwner(c), info, {})
@@ -332,6 +297,7 @@ type
     m*: PSym
     mode*: TOverloadIterMode
     symChoiceIndex*: int
+    symChoiceLastPreferred: bool
     currentScope: PScope
     importIdx: int
     marked: IntSet
@@ -611,6 +577,11 @@ proc lookUp*(c: PContext, n: PNode): PSym =
     var ident = considerQuotedIdent(c, n)
     result = searchInScopes(c, ident, amb)
     if result == nil: result = errorUndeclaredIdentifierHint(c, ident, n.info)
+  of nkOpenSymChoice, nkClosedSymChoice:
+    if nfPreferredSym in n[0].flags:
+      result = n[0].sym
+    else:
+      result = nil
   else:
     internalError(c.config, n.info, "lookUp")
     return nil
@@ -691,6 +662,11 @@ proc qualifiedLookUp*(c: PContext, n: PNode, flags: set[TLookupFlag]): PSym =
         localError(c.config, n[1].info, "identifier expected, but got: " &
                    renderTree(n[1]))
         result = errorSym(c, n[1])
+  of nkOpenSymChoice, nkClosedSymChoice:
+    if nfPreferredSym in n[0].flags:
+      result = n[0].sym
+    else:
+      result = nil
   else:
     result = nil
   when false:
@@ -723,7 +699,18 @@ proc initOverloadIter*(o: var TOverloadIter, c: PContext, n: PNode): PSym =
 
   of nkSym:
     result = n.sym
-    o.mode = oimDone
+    if nfPreferredSym in n.flags:
+      # standalone sym node with nfPreferredSym acts like an open symchoice,
+      # see semtempl.symChoice for reasoning
+      o.mode = oimSymChoiceLocalLookup
+      o.symChoiceLastPreferred = true
+      o.currentScope = c.currentScope
+      o.it.h = result.name.h
+      o.it.name = result.name
+      o.marked = initIntSet()
+      incl(o.marked, result.id)
+    else:
+      o.mode = oimDone
   of nkDotExpr:
     result = nil
     o.mode = oimOtherModule
@@ -749,6 +736,7 @@ proc initOverloadIter*(o: var TOverloadIter, c: PContext, n: PNode): PSym =
     o.mode = oimSymChoice
     if n[0].kind == nkSym:
       result = n[0].sym
+      o.symChoiceLastPreferred = nfPreferredSym in n[0].flags
     else:
       o.mode = oimDone
       return nil
@@ -767,6 +755,11 @@ proc lastOverloadScope*(o: TOverloadIter): int =
              else: o.currentScope.depthLevel
   of oimSelfModule:  result = 1
   of oimOtherModule: result = 0
+  of oimSymChoice, oimSymChoiceLocalLookup:
+    if o.symChoiceLastPreferred:
+      result = 999
+    else:
+      result = -1
   else: result = -1
 
 proc nextOverloadIterImports(o: var TOverloadIter, c: PContext, n: PNode): PSym =
@@ -825,6 +818,7 @@ proc nextOverloadIter*(o: var TOverloadIter, c: PContext, n: PNode): PSym =
   of oimOtherModule:
     result = nextModuleIter(o.mit, c.graph)
   of oimSymChoice:
+    o.symChoiceLastPreferred = false
     if o.symChoiceIndex < n.len:
       result = n[o.symChoiceIndex].sym
       incl(o.marked, result.id)
@@ -849,13 +843,15 @@ proc nextOverloadIter*(o: var TOverloadIter, c: PContext, n: PNode): PSym =
     else:
       result = nil
   of oimSymChoiceLocalLookup:
+    o.symChoiceLastPreferred = false
     if o.currentScope != nil:
       result = nextIdentExcluding(o.it, o.currentScope.symbols, o.marked)
       while result == nil:
         o.currentScope = o.currentScope.parent
         if o.currentScope != nil:
+          let name = if n.kind == nkSym: n.sym.name else: n[0].sym.name
           result = firstIdentExcluding(o.it, o.currentScope.symbols,
-                                      n[0].sym.name, o.marked)
+                                       name, o.marked)
         else:
           o.importIdx = 0
           result = symChoiceExtension(o, c, n)

compiler/semexprs.nim

@@ -101,7 +101,8 @@ proc semExprWithType(c: PContext, n: PNode, flags: TExprFlags = {}, expectedType
   elif result.typ.kind == tyError:
     # associates the type error to the current owner
     result.typ = errorType(c)
-  elif efTypeAllowed in flags and result.typ.kind == tyProc and
+  elif {efTypeAllowed, efOperand} * flags != {} and
+      result.typ.kind == tyProc and
       hasUnresolvedParams(result, {}):
     # mirrored with semOperand but only on efTypeAllowed
     let owner = result.typ.owner
@@ -140,14 +141,9 @@ proc resolveSymChoice(c: PContext, n: var PNode, flags: TExprFlags = {}, expecte
   if isSymChoice(n) and efAllowSymChoice notin flags:
     # some contexts might want sym choices preserved for later disambiguation
     # in general though they are ambiguous
-    let first = n[0].sym
-    var foundSym: PSym = nil
-    if first.kind == skEnumField and
-        not isAmbiguous(c, first.name, {skEnumField}, foundSym) and
-        foundSym == first:
-      # choose the first resolved enum field, i.e. the latest in scope
-      # to mirror behavior before overloadable enums
-      n = n[0]
+    let first = n[0]
+    if nfPreferredSym in first.flags:
+      n = first
 
 proc semSymChoice(c: PContext, n: PNode, flags: TExprFlags = {}, expectedType: PType = nil): PNode =
   result = n
@@ -1554,8 +1550,7 @@ proc builtinFieldAccess(c: PContext; n: PNode; flags: var TExprFlags): PNode =
 
 proc dotTransformation(c: PContext, n: PNode): PNode =
   if isSymChoice(n[1]) or
-      # generics usually leave field names as symchoices, but not types
-      (n[1].kind == nkSym and n[1].sym.kind == skType):
+      (n[1].kind == nkSym and n[1].sym.kind in routineKinds + {skType}):
     result = newNodeI(nkDotCall, n.info)
     result.add n[1]
     result.add copyTree(n[0])
@@ -2965,13 +2960,17 @@ proc getNilType(c: PContext): PType =
     c.nilTypeCache = result
 
 proc enumFieldSymChoice(c: PContext, n: PNode, s: PSym): PNode =
-  var o: TOverloadIter
+  var o: TOverloadIter = default(TOverloadIter)
+  var firstPreferred = true
   var i = 0
   var a = initOverloadIter(o, c, n)
+  let firstScope = lastOverloadScope(o)
   while a != nil:
     if a.kind == skEnumField:
       inc(i)
-      if i > 1: break
+      if i > 1:
+        firstPreferred = firstScope > lastOverloadScope(o)
+        break
     a = nextOverloadIter(o, c, n)
   let info = getCallLineInfo(n)
   if i <= 1:
@@ -2991,6 +2990,8 @@ proc enumFieldSymChoice(c: PContext, n: PNode, s: PSym): PNode =
         result.add newSymNode(a, info)
         onUse(info, a)
       a = nextOverloadIter(o, c, n)
+    if firstPreferred:
+      result[0].flags.incl nfPreferredSym
 
 proc semPragmaStmt(c: PContext; n: PNode) =
   if c.p.owner.kind == skModule:

compiler/sempass2.nim

@@ -1145,7 +1145,7 @@ proc track(tracked: PEffects, n: PNode) =
     if n.sym.typ != nil and tfHasAsgn in n.sym.typ.flags:
       tracked.owner.flags.incl sfInjectDestructors
       # bug #15038: ensure consistency
-      if not hasDestructor(n.typ) and sameType(n.typ, n.sym.typ): n.typ = n.sym.typ
+      if n.typ != nil and not hasDestructor(n.typ) and sameType(n.typ, n.sym.typ): n.typ = n.sym.typ
   of nkHiddenAddr, nkAddr:
     if n[0].kind == nkSym and isLocalSym(tracked, n[0].sym):
       useVarNoInitCheck(tracked, n[0], n[0].sym)

compiler/semtempl.nim

@@ -52,23 +52,38 @@ proc symChoice(c: PContext, n: PNode, s: PSym, r: TSymChoiceRule;
                isField = false): PNode =
   var
     a: PSym
-    o: TOverloadIter
+    o: TOverloadIter = default(TOverloadIter)
+    firstPreferred = true
   var i = 0
   a = initOverloadIter(o, c, n)
+  let firstScope = lastOverloadScope(o)
   while a != nil:
     if a.kind != skModule:
       inc(i)
-      if i > 1: break
+      if i > 1:
+        firstPreferred = firstScope > lastOverloadScope(o)
+        break
     a = nextOverloadIter(o, c, n)
   let info = getCallLineInfo(n)
   if i <= 1 and r != scForceOpen:
-    # XXX this makes more sense but breaks bootstrapping for now:
-    # (s.kind notin routineKinds or s.magic != mNone):
-    # for instance 'nextTry' is both in tables.nim and astalgo.nim ...
     if not isField or sfGenSym notin s.flags:
       result = newSymNode(s, info)
-      markUsed(c, info, s)
-      onUse(info, s)
+      if r == scClosed or n.kind == nkDotExpr or
+          # also bind magic procs:
+          (s.magic != mNone and s.kind in routineKinds):
+        markUsed(c, info, s)
+        onUse(info, s)
+      else:
+        # we need a node with a type here so things like default parameters,
+        # which use semGenericStmt, can infer the parameter type
+        # from expressions like `false`
+        # but symchoices having types can mislead the compiler
+        # instead we allow standalone sym nodes to have nfPreferredSym
+        # which acts like an open symchoice in initOverloadIter
+        result.flags.incl nfPreferredSym
+        #result = newTreeIT(nkOpenSymChoice, info, result.typ, result)
+        incl(s.flags, sfUsed)
+        markOwnerModuleAsUsed(c, s)
     else:
       result = n
   elif i == 0:
@@ -88,6 +103,8 @@ proc symChoice(c: PContext, n: PNode, s: PSym, r: TSymChoiceRule;
         result.add newSymNode(a, info)
         onUse(info, a)
       a = nextOverloadIter(o, c, n)
+    if r != scForceOpen and firstPreferred:
+      result[0].flags.incl nfPreferredSym
 
 proc semBindStmt(c: PContext, n: PNode, toBind: var IntSet): PNode =
   result = copyNode(n)

compiler/sigmatch.nim

@@ -246,7 +246,8 @@ proc sumGeneric(t: PType): int =
           result += sumGeneric(a)
       break
     of tyProc:
-      result += sumGeneric(t.returnType)
+      if t.returnType != nil:
+        result += sumGeneric(t.returnType)
       for _, a in t.paramTypes:
         result += sumGeneric(a)
       break
@@ -2416,7 +2417,8 @@ proc paramTypesMatch*(m: var TCandidate, f, a: PType,
         if x.state != csMatch:
           internalError(m.c.graph.config, arg.info, "x.state is not csMatch")
         result = nil
-    if best > -1 and result != nil:
+    if (best > -1 and result != nil) or
+        (best = 0; nfPreferredSym in arg[best].flags):
       # only one valid interpretation found:
       markUsed(m.c, arg.info, arg[best].sym)
       onUse(arg.info, arg[best].sym)

lib/pure/collections/hashcommon.nim

@@ -38,10 +38,10 @@ proc slotsNeeded(count: Natural): int {.inline.} =
   # Make sure to synchronize with `mustRehash` above
   result = nextPowerOfTwo(count * 3 div 2 + 4)
 
-template rawGetKnownHCImpl() {.dirty.} =
+template rawGetKnownHCImpl() =
   if t.dataLen == 0:
     return -1
-  var h: Hash = hc and maxHash(t) # start with real hash value
+  var h {.inject.}: Hash = hc and maxHash(t) # start with real hash value
   while isFilled(t.data[h].hcode):
     # Compare hc THEN key with boolean short circuit. This makes the common case
     # zero ==key's for missing (e.g.inserts) and exactly one ==key for present.

tests/enum/tcrossmodule.nim

@@ -9,7 +9,10 @@ template t =
 
 t()
 
-block: # legacy support for behavior before overloadableEnums
-  # warning: ambiguous enum field 'Success' assumed to be of type MyEnum
+block: # account for scope
   let x = {Success}
   doAssert x is set[MyEnum]
+  proc foo[T](a: T): string = $a
+  doAssert foo(Success) == "Success"
+  proc bar[T](): string = $Success
+  doAssert bar[int]() == "Success"

tests/lookups/mpreferredsym.nim

@@ -0,0 +1,28 @@
+# foo0 has 0 overloads
+
+template myTemplate0*(): string =
+  foo0(bar)
+
+# foo1 has 1 overload
+
+proc foo1(arg: int): string =
+  "foo1 bad"
+
+template myTemplate1*(): string =
+  foo1(bar)
+
+# foo2 has 2 overloads
+
+proc foo2(arg: int): string =
+  "foo2 bad 1"
+
+proc foo2(arg: string): string =
+  "foo2 bad 2"
+
+template myTemplate2*(): string =
+  foo2(bar)
+
+proc overloadToPrefer(x: int): int = x + 1
+
+template singleOverload*: untyped =
+  (overloadToPrefer(123), overloadToPrefer("abc"))

tests/lookups/tpreferredsym.nim

@@ -0,0 +1,21 @@
+import mpreferredsym
+
+block: # issue #11184
+  type MyType = object
+
+  proc foo0(arg: MyType): string = "foo0"
+  proc foo1(arg: MyType): string = "foo1"
+  proc foo2(arg: MyType): string = "foo2"
+
+  proc test() =
+    var bar: MyType
+
+    doAssert myTemplate0() == "foo0"
+    doAssert myTemplate1() == "foo1"
+    doAssert myTemplate2() == "foo2"
+
+  test()
+
+block: 
+  proc overloadToPrefer(x: string): string = x & "def"
+  doAssert singleOverload() == (124, "abcdef")