Types.isSameType() unification

[Maurizio Cimadamore]

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>