Make typeRel behave to spec (#22261)

The goal of this PR is to make `typeRel` accurate to it's definition for generics: ``` # 3) When used with two type classes, it will check whether the types # matching the first type class (aOrig) are a strict subset of the types matching # the other (f). This allows us to compare the signatures of generic procs in # order to give preferrence to the most specific one: ``` I don't want this PR to break any code, and I want to preserve all of Nims current behaviors. I think that making this more accurate will help serve as ground work for the future. It may not be possible to not break anything but this is my attempt. So that it is understood, this code was part of another PR (#22143) but that problem statement only needed this change by extension. It's more organized to split two problems into two PRs and this issue, being non-breaking, should be a more immediate improvement. --------- Co-authored-by: Andreas Rumpf <[email protected]> (cherry picked from commit b2ca6be)
nim-lang · Apr 18, 2024 · 4f78a4d · 4f78a4d
1 parent bb82d46
commit 4f78a4d
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 29 deletions.
diff --git a/compiler/sigmatch.nim b/compiler/sigmatch.nim
@@ -456,6 +456,38 @@ proc handleFloatRange(f, a: PType): TTypeRelation =
       else: result = isIntConv
     else: result = isNone
 
+proc getObjectTypeOrNil(f: PType): PType =
+  #[
+    Returns a type that is f's effective typeclass. This is usually just one level deeper
+    in the hierarchy of generality for a type. `object`, `ref object`, `enum` and user defined
+    tyObjects are common return values.
+  ]#
+  if f == nil: return nil
+  case f.kind:
+  of tyGenericInvocation, tyCompositeTypeClass, tyAlias:
+    if f.len <= 0 or f[0] == nil:
+      result = nil
+    else:
+      result = getObjectTypeOrNil(f[0])
+  of tyGenericBody, tyGenericInst:
+    result = getObjectTypeOrNil(f.lastSon)
+  of tyUserTypeClass:
+    if f.isResolvedUserTypeClass:
+      result = f.base  # ?? idk if this is right
+    else:
+      result = f.lastSon
+  of tyStatic, tyOwned, tyVar, tyLent, tySink:
+    result = getObjectTypeOrNil(f.base)
+  of tyInferred:
+    # This is not true "After a candidate type is selected"
+    result = getObjectTypeOrNil(f.base)
+  of tyTyped, tyUntyped, tyFromExpr:
+    result = nil
+  of tyRange:
+    result = f.lastSon
+  else:
+    result = f
+
 proc genericParamPut(c: var TCandidate; last, fGenericOrigin: PType) =
   if fGenericOrigin != nil and last.kind == tyGenericInst and
      last.len-1 == fGenericOrigin.len:
@@ -470,7 +502,8 @@ proc isObjectSubtype(c: var TCandidate; a, f, fGenericOrigin: PType): int =
   var depth = 0
   var last = a
   while t != nil and not sameObjectTypes(f, t):
-    assert t.kind == tyObject
+    if t.kind != tyObject:  # avoid entering generic params etc
+      return -1
     t = t[0]
     if t == nil: break
     last = t
@@ -994,8 +1027,8 @@ proc typeRel(c: var TCandidate, f, aOrig: PType,
   # of the designated type class.
   #
   # 3) When used with two type classes, it will check whether the types
-  # matching the first type class are a strict subset of the types matching
-  # the other. This allows us to compare the signatures of generic procs in
+  # matching the first type class (aOrig) are a strict subset of the types matching
+  # the other (f). This allows us to compare the signatures of generic procs in
   # order to give preferrence to the most specific one:
   #
   # seq[seq[any]] is a strict subset of seq[any] and hence more specific.
@@ -1151,7 +1184,6 @@ proc typeRel(c: var TCandidate, f, aOrig: PType,
       # being passed as parameters
       return isNone
   else: discard
-
   case f.kind
   of tyEnum:
     if a.kind == f.kind and sameEnumTypes(f, a): result = isEqual
@@ -1242,6 +1274,8 @@ proc typeRel(c: var TCandidate, f, aOrig: PType,
         return inferStaticsInRange(c, fRange, a)
       elif c.c.matchedConcept != nil and aRange.rangeHasUnresolvedStatic:
         return inferStaticsInRange(c, aRange, f)
+      elif result == isGeneric and concreteType(c, aa, ff) == nil:
+        return isNone
       else:
         if lengthOrd(c.c.config, fRange) != lengthOrd(c.c.config, aRange):
           result = isNone
@@ -1329,12 +1363,17 @@ proc typeRel(c: var TCandidate, f, aOrig: PType,
   of tyTuple:
     if a.kind == tyTuple: result = recordRel(c, f, a)
   of tyObject:
-    if a.kind == tyObject:
-      if sameObjectTypes(f, a):
+    let effectiveArgType = if useTypeLoweringRuleInTypeClass:
+        a
+      else:
+        getObjectTypeOrNil(a)
+    if effectiveArgType == nil: return isNone
+    if effectiveArgType.kind == tyObject:
+      if sameObjectTypes(f, effectiveArgType):
         result = isEqual
         # elif tfHasMeta in f.flags: result = recordRel(c, f, a)
       elif trIsOutParam notin flags:
-        var depth = isObjectSubtype(c, a, f, nil)
+        var depth = isObjectSubtype(c, effectiveArgType, f, nil)
         if depth > 0:
           inc(c.inheritancePenalty, depth)
           result = isSubtype
@@ -1530,6 +1569,8 @@ proc typeRel(c: var TCandidate, f, aOrig: PType,
 
   of tyGenericInvocation:
     var x = a.skipGenericAlias
+    if x.kind == tyGenericParam and x.len > 0:
+      x = x.lastSon
     let concpt = f[0].skipTypes({tyGenericBody})
     var preventHack = concpt.kind == tyConcept
     if x.kind == tyOwned and f[0].kind != tyOwned:
@@ -1540,7 +1581,6 @@ proc typeRel(c: var TCandidate, f, aOrig: PType,
       c.calleeSym != nil and
       c.calleeSym.kind in {skProc, skFunc} and c.call != nil and not preventHack:
       let inst = prepareMetatypeForSigmatch(c.c, c.bindings, c.call.info, f)
-      #echo "inferred ", typeToString(inst), " for ", f
       return typeRel(c, inst, a, flags)
 
     if x.kind == tyGenericInvocation:
@@ -1569,9 +1609,6 @@ proc typeRel(c: var TCandidate, f, aOrig: PType,
       var fskip = skippedNone
       let aobj = x.skipToObject(askip)
       let fobj = genericBody.lastSon.skipToObject(fskip)
-      var depth = -1
-      if fobj != nil and aobj != nil and askip == fskip:
-        depth = isObjectSubtype(c, aobj, fobj, f)
       result = typeRel(c, genericBody, x, flags)
       if result != isNone:
         # see tests/generics/tgeneric3.nim for an example that triggers this
@@ -1597,7 +1634,10 @@ proc typeRel(c: var TCandidate, f, aOrig: PType,
               put(c, key, x)
             elif typeRel(c, old, x, flags + {trDontBind}) == isNone:
               return isNone
-
+      var depth = -1
+      if fobj != nil and aobj != nil and askip == fskip:
+        depth = isObjectSubtype(c, aobj, fobj, f)
+
       if result == isNone:
         # Here object inheriting from generic/specialized generic object
         # crossing path with metatypes/aliases, so we need to separate them
@@ -1659,8 +1699,9 @@ proc typeRel(c: var TCandidate, f, aOrig: PType,
     considerPreviousT:
       let target = f[0]
       let targetKind = target.kind
-      let effectiveArgType = a.skipTypes({tyRange, tyGenericInst,
-                                          tyBuiltInTypeClass, tyAlias, tySink, tyOwned})
+      var effectiveArgType = a.getObjectTypeOrNil()
+      if effectiveArgType == nil: return isNone
+      effectiveArgType = effectiveArgType.skipTypes({tyBuiltInTypeClass})
       if targetKind == effectiveArgType.kind:
         if effectiveArgType.isEmptyContainer:
           return isNone
@@ -1782,7 +1823,11 @@ proc typeRel(c: var TCandidate, f, aOrig: PType,
         if tfWildcard in a.flags:
           a.sym.transitionGenericParamToType()
           a.flags.excl tfWildcard
-        else:
+        elif doBind:
+          # The mechanics of `doBind` being a flag that also denotes sig cmp via
+          # negation is potentially problematic. `IsNone` is appropriate for
+          # preventing illegal bindings, but it is not necessarily appropriate
+          # before the bindings have been finalized.
           concrete = concreteType(c, a, f)
           if concrete == nil:
             return isNone

diff --git a/doc/manual_experimental.md b/doc/manual_experimental.md
@@ -838,6 +838,7 @@ The concept matches if:
 
 a) all expressions within the body can be compiled for the tested type
 b) all statically evaluable boolean expressions in the body are true
+c) all type modifiers specified match their respective definitions
 
 The identifiers following the `concept` keyword represent instances of the
 currently matched type. You can apply any of the standard type modifiers such

diff --git a/tests/overload/tor_isnt_better.nim b/tests/overload/tor_isnt_better.nim
@@ -1,18 +1,19 @@
-discard """
-  errormsg: "ambiguous call;"
-  line: 16
-"""
-
-# bug #8568
-
 type
   D[T] = object
   E[T] = object
 
-proc g(a: D|E): string = "foo D|E"
-proc g(a: D): string = "foo D"
+block: # PR #22261
+  proc d(x: D):bool= false
+  proc d(x: int | D[SomeInteger]):bool= true
+  doAssert d(D[5]()) == false
 
-proc test() =
-  let x = g D[int]()
 
-test()
+block: # bug #8568
+#[
+  Since PR #22261 and amendment has been made. Since D is a subset of D | E but
+  not the other way around `checkGeneric` should favor proc g(a: D) instead
+  of asserting ambiguity
+]#
+  proc g(a: D|E): string = "foo D|E"
+  proc g(a: D): string = "foo D"
+  doAssert g(D[int]()) == "foo D"
diff --git a/tests/stdlib/t21406.nim b/tests/stdlib/t21406.nim
@@ -1,5 +1,7 @@
 import std/[times, strformat]
 import std/assertions
 
-doAssert fmt"{getTime()}" == $getTime()
-doAssert fmt"{now()}" == $now()
+let aTime = getTime()
+doAssert fmt"{aTime}" == $aTime
+let aNow = now()
+doAssert fmt"{aNow}" == $aNow