Types.isSameType() unification
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Mon Jan 22 13:01:44 UTC 2018
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>
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sameTypes-v2.patch
Type: text/x-patch
Size: 10141 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20180122/1d775732/sameTypes-v2-0001.patch>
More information about the compiler-dev
mailing list