From b2ca6bedae6420396682cbf06b3c628cfcf9baf1 Mon Sep 17 00:00:00 2001 From: Ryan McConnell Date: Sat, 30 Sep 2023 04:34:14 +0000 Subject: [PATCH] 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 --- compiler/sigmatch.nim | 75 ++++++++++++++++++++++++------ doc/manual_experimental.md | 1 + tests/overload/tor_isnt_better.nim | 25 +++++----- tests/stdlib/t21406.nim | 6 ++- 4 files changed, 78 insertions(+), 29 deletions(-) diff --git a/compiler/sigmatch.nim b/compiler/sigmatch.nim index ee93321c8b..3867a67b70 100644 --- a/compiler/sigmatch.nim +++ b/compiler/sigmatch.nim @@ -453,6 +453,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: @@ -467,7 +499,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 @@ -997,8 +1030,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. @@ -1154,7 +1187,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 @@ -1245,6 +1277,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 @@ -1332,12 +1366,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 @@ -1533,6 +1572,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: @@ -1543,7 +1584,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: @@ -1572,9 +1612,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 @@ -1600,7 +1637,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 @@ -1662,8 +1702,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 @@ -1785,7 +1826,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 index d05a693bb9..41d463ff8b 100644 --- a/doc/manual_experimental.md +++ b/doc/manual_experimental.md @@ -919,6 +919,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 index 5ef8bc7c4c..ce92009d03 100644 --- 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 index 5b96227ce6..86bf7b0c7c 100644 --- 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