Types.isSameType() unification
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Mon Jan 22 12:07:19 UTC 2018
Quick update,
I was unable to build the JDK with the patch you sent; the issue was
that the patch alters the loose behaviro of type equality used in
subtyping tests, so tests like the one below:
class Foo<X extends I { }
Comparator<Foo<? extends I>> <: Comparator<Foo<?>>
Started to fail. While this is ok spec-wise, the compiler currently
tries to detect cases where two type arguments are effectively the same
thing by using info on declared bounds. This is done in
Types::containsTypeEquivalent, which is not used with a strict check. We
might or might not want to get rid of that one too, but that's a battle
for another day :-)
The solution was to reinstate the use of containsTypeEquivalent in the
type argument check in the sametype visitor. That gets the build going.
I'm now running the tests and see where we're at.
Maurizio
On 22/01/18 09:34, Maurizio Cimadamore wrote:
> Hi B.
>
> interesting approach, thanks for the patch; digging a bit into the
> history the loose vs. strict dichotomy started here:
>
> http://hg.openjdk.java.net/lambda/lambda/langtools/rev/1df20330f6bd#l1.63
>
> As you can see, javac started out with a loose behavior, but then a
> stricter semantics (closer to JLS) was required during incorporation:
> e.g. a new bound should be added to an inference var only if this
> bound is different from the existing ones; the loose same-ness check
> would consider cloned inference-vars equivalent and, as such, was
> missing out on some extra constraints - hence the strict version.
>
> At the time it did not occur to me that, maybe, the loose version was
> just wrong and the compiler could be better off with the stricter
> version; also, since we were in the middle of replacing an inference
> engine with a new one, we were trying to limit the impact of new
> changes as much as possible.
>
> Since your patch makes strict behavior the only choice, the problem
> with inference variable bounds is naturally addressed - but of course
> all code calling isSameTypes normally would now see a change in
> behavior, so we need to make sure that the new behavior is acceptable
> in all cases. But your analysis seems to suggest that most of the
> stuff just works and that was just me being overly conservative at the
> time - in which case we can happily change the code, and eliminate
> this source of complexity.
>
> I'll do some more tests on your patch and see how it goes.
>
> Maurizio
>
>
> On 20/01/18 22:17, B. Blaser wrote:
>> Hi,
>>
>> As noted previously in [1], 'isSameTypeLoose' doesn't work properly
>> with '==' instances of a same type variable with recursive bounds
>> which is indeed wrong.
>>
>> Moreover, 'isSameType' should be the unique 'isSameTypeStrict'
>> relation as explained in [2].
>>
>> So, I simply tried to remove 'isSameTypeLoose' but I got, of course,
>> some failing tests like [3] & [4].
>>
>> As it seems that 'isSameTypeStrict' is too strict for wildcards I kept
>> the default behavior in such cases, as here under.
>>
>> I did this experiment on a quite old JDK10 revision (sorry) with
>> encouraging test results (nothing seems to be broken).
>>
>> What do you think?
>>
>> Thanks,
>> Bernard
>>
>> [1]
>> http://mail.openjdk.java.net/pipermail/compiler-dev/2017-November/011291.html
>> [2]
>> http://mail.openjdk.java.net/pipermail/compiler-dev/2017-November/011296.html
>> [3] tools/javac/generics/wildcards/neg/CastTest.java
>> [4] tools/javac/generics/wildcards/pos/CastTest.java
>>
>> diff --git
>> a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
>> b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
>> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
>> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
>> @@ -1031,9 +1031,7 @@
>> return isSameType(t, s, false);
>> }
>> public boolean isSameType(Type t, Type s, boolean strict) {
>> - return strict ?
>> - isSameTypeStrict.visit(t, s) :
>> - isSameTypeLoose.visit(t, s);
>> + return isSameTypeStrict.visit(t, s);
>> }
>> // where
>> abstract class SameTypeVisitor extends TypeRelation {
>> @@ -1177,44 +1175,6 @@
>> }
>>
>> /**
>> - * Standard type-equality relation - type variables are
>> considered
>> - * equals if they share the same type symbol.
>> - */
>> - TypeRelation isSameTypeLoose = new LooseSameTypeVisitor();
>> -
>> - private class LooseSameTypeVisitor extends SameTypeVisitor {
>> -
>> - /** cache of the type-variable pairs being (recursively)
>> tested. */
>> - private Set<TypePair> cache = new HashSet<>();
>> -
>> - @Override
>> - boolean sameTypeVars(TypeVar tv1, TypeVar tv2) {
>> - return tv1.tsym == tv2.tsym && checkSameBounds(tv1,
>> tv2);
>> - }
>> - @Override
>> - protected boolean containsTypes(List<Type> ts1,
>> List<Type> ts2) {
>> - return containsTypeEquivalent(ts1, ts2);
>> - }
>> -
>> - /**
>> - * Since type-variable bounds can be recursive, we need
>> to protect against
>> - * infinite loops - where the same bounds are checked
>> over and over recursively.
>> - */
>> - private boolean checkSameBounds(TypeVar tv1, TypeVar tv2) {
>> - TypePair p = new TypePair(tv1, tv2, true);
>> - if (cache.add(p)) {
>> - try {
>> - return visit(tv1.getUpperBound(),
>> tv2.getUpperBound());
>> - } finally {
>> - cache.remove(p);
>> - }
>> - } else {
>> - return false;
>> - }
>> - }
>> - };
>> -
>> - /**
>> * Strict type-equality relation - type variables are
>> considered
>> * equals if they share the same object identity.
>> */
>> @@ -1227,17 +1187,6 @@
>> protected boolean containsTypes(List<Type> ts1,
>> List<Type> ts2) {
>> return isSameTypes(ts1, ts2, true);
>> }
>> -
>> - @Override
>> - public Boolean visitWildcardType(WildcardType t, Type s) {
>> - if (!s.hasTag(WILDCARD)) {
>> - return false;
>> - } else {
>> - WildcardType t2 = (WildcardType)s;
>> - return t.kind == t2.kind &&
>> - isSameType(t.type, t2.type, true);
>> - }
>> - }
>> };
>>
>> // </editor-fold>
>
More information about the compiler-dev
mailing list