Types.isSameType() unification
Vicente Romero
vicente.romero at oracle.com
Mon Jan 22 15:26:07 UTC 2018
On 01/22/2018 08:01 AM, Maurizio Cimadamore 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?
the patch looks OK modulo some documentation, inline comments, that
applied to the old settings and that could be updated,
>
> Cheers
> Maurizio
Thanks,
Vicente
>
>
> 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>
>>>
>>
>
More information about the compiler-dev
mailing list