[lworld] RFR: 8244227: [lworld] Explore an implementation where the reference projection and value projection types are backed by a single class symbol [v5]

[Maurizio Cimadamore]

On Thu, 8 Apr 2021 08:09:14 GMT, Srikanth Adayapalam <sadayapalam at openjdk.org> wrote:

>> This PR overhauls the existing implementation/representation of reference projection types  and the inline types so that both share the same underlying symbol, much like how an infinite family of parameterized types that originate from the same generic type are modelled using the same underlying symbol.
>
> Srikanth Adayapalam has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Where possible prefer Types.isSubtype over Types.asSuper for loss less subtyping determination; Merge Two flavors of Types.asSuper into one; Make sure to switch to projection types for sucessful hierarchy lookup via asSuper
>  - Reverse all changes made to Types.asSuper both at definition site and call sites.

In general, it looks good. I have two main comments:
* consider using reference projection always when doing asSuper (e.g. adding that projection inside asSuper, directly)
* I believe the patch is replacing too many use sites with subtyping. There are several checks where we want to assert that T is an instance of C, where C can be AutoCloseable, Iterable, Serializable. It is not clear to me as to why we'd want these replaced with subtyping - in the sense that here we seem to be asking about a propoerty of the primitive class _declaration_ rather than the particular projection we happen to have on hand.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java line 2045:

> 2043:             // check for a direct implementation
> 2044:             if (other.isOverridableIn((TypeSymbol)owner) &&
> 2045:                 types.isSubtype(owner.type, other.owner.type) &&

This seems inconsistent with what has been done below (see other comment)

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java line 2114:

> 2112:             // check for a direct implementation
> 2113:             if (other.isOverridableIn((TypeSymbol)owner) &&
> 2114:                 types.asSuper(owner.type.referenceProjectionOrSelf(), other.owner) != null) {

So, for source ovrride we use `asSuper`, but for binary override we use `isSubtype`. Shouldn't we use asSuper in both cases here? Overriding is typically a business between class declarations, so symbols should be good? Or can you envision a case where you want to check if a method on Foo.val overrides something on a Foo.ref and you want to say "no" there? In other words, I think the check here is meant to say: make sure that the owner of the method symbols are related by subclassing - which seems to call for asSuper.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 2209:

> 2207:      *
> 2208:      * 1. Since Foo.ref and Foo.val share the same symbol, that of Foo.class, a call to
> 2209:      *    asSuper(Foo.ref.type, Foo.val.type.tsym) would return non-null. This is NOT correct

I don't think I agree with the spirit of this comment :-)

There is only one symbol, so Foo.val.type.tsym == Foo. So:

asSuper(Foo.ref.type, Foo.val.type.tsym) ==
asSuper(Foo.ref.type, Foo) ==
true

This is not different from when you have:

asSuper(ListOfString.type, ListOfInteger.type.tsym) != null

I think the real issue here, is that we can't rely on "asSuper" for code generation, because, really, this routine isn't sharp enough. In the case of Gen::visitTypeCast, the issue is that we have to decide whether a checkcast should be emitted or not; we typically do that by comparing symbols. That strategy here is simply insufficient - as the symbols are the same - just the types will be different. This will be, again, the case when we will face a cast from List to List<int> - these two types will have same symbol, but different representation in the classfile, so a checkcast will be needed there.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 2223:

> 2221:      *    isSubType(ArrayList<String>.type, List<T>.type) is false;) So care needs to be exercised.
> 2222:      *
> 2223:      * 2. Given a primitive class Foo, a call to asSuper(Foo.type, SuperclassOfFoo.tsym) and/or

Question here: is Foo.type refusing to play because the "default" semantics attached to Foo.type is the one of the "value projection" ? If so, then I agree with the fix (e.g. using reference projection or self). Also, given where the model is, I don't think this is an "hack" or a "workaround" - it's basically a requirement: inheritance info lives on the .ref side of the fence.

In fact, so much so that I'll raise: shouldn't this call to "refProjectionOrSelf" be part of asSuper? If asSuper is always use to check for inheritance  relationship, isn't it always the case that you want the reference projection to be used? (and then, if a callsite ends up with too lose a semantics, maybe that's a sign that using asSuper there was the wrong choice?)

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1919:

> 1917:     void checkAutoCloseable(DiagnosticPosition pos, Env<AttrContext> env, Type resource) {
> 1918:         if (!resource.isErroneous() &&
> 1919:             types.isSubtype(resource.referenceProjectionOrSelf(), syms.autoCloseableType) &&

I think this wants asSuper? You are projecting anyway.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 2698:

> 2696:         checkCompatibleConcretes(pos, c);
> 2697: 
> 2698:         boolean implementsIdentityObject = types.isSubtype(c.referenceProjectionOrSelf(), syms.identityObjectType);

Not sure you need full blown subtyping here. You are just asking: is this thing derived from "identityObject" - in all these cases asSuper is what you want. Same for AutoCloseable checks. Iterable, ...

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java line 1862:

> 1860:                     return true;
> 1861:                 }
> 1862:                 return types.isSubtype(tree.target, syms.serializableType);

That's another "is this a subclass of XYZ" check - asSuper is fine here (with ref projection).

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java line 1725:

> 1723:     private JCStatement makeResourceCloseInvocation(JCExpression resource) {
> 1724:         // convert to AutoCloseable if needed
> 1725:         if (!types.isSubtype(resource.type.referenceProjectionOrSelf(), syms.autoCloseableType)) {

asSuper?

src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java line 2279:

> 2277:            !types.isSameType(tree.expr.type, tree.clazz.type) &&
> 2278:             (!tree.clazz.type.isReferenceProjection() || !types.isSameType(tree.clazz.type.valueProjection(), tree.expr.type)) &&
> 2279:            !types.isSubtype(tree.expr.type, tree.clazz.type)) {

This is, I think, the main place, where isSubtype is truly necessary.

-------------

PR: https://git.openjdk.java.net/valhalla/pull/375