Types.isSameType() unification

[B. Blaser]

Looks good to me (thanks for all relevant updates).

Cheers,
Bernard

On 22 January 2018 at 14:01, Maurizio Cimadamore
<maurizio.cimadamore at oracle.com> wrote:
> The attached patch looks good test-wise; apart from the
> containsTypeEquivalent change (discussed in earlier email) I've also done
> some cosmetic changes to get rid of redundant code.
>
> Vicente, could you please take a second look?
>
> Cheers
> Maurizio
>
>
>
> On 22/01/18 12:07, Maurizio Cimadamore wrote:
>>
>> 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>
>>>
>>>
>>
>