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

[Srikanth Adayapalam]

On Thu, 8 Apr 2021 11:22:50 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> In general, it looks good. I have three main comments:
> 
> * the refProjectionOrSelf trick is a good one, I believe
> * consider using reference projection always when doing asSuper (e.g. adding that projection inside asSuper, directly)

Will experiment with this and report back. This could minimize code changes. 

> * 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.

Possible. The rule of thumb I used for changing calls to asSuper into isSubtype was simply, whether (a) the call site is interested in the return value of asSuper as opoosed to just checking whether it is null or not and (b) whether generic class symbol could be passed in as the second argument to original asSuper call and if so retain it as asSuper itself. 

So it is likely more sites are changed than strictly necessary. The way I see it some of the overeaget changes to isSubtype result in equivalent semantics with no regression. Still it is worth minimizing ripples and for that reason I will attempt to restore these.

> 
> Note that, as we're exploring new territory, it is very possible that I have overlooked some underlying issue here - but thanks for taking the time to guide me through all the problems!

I am grappling with similar challenges. I.e clearly characterizing which calls involve hazards of which kind. This involves reasoning about every site.

> 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)

Good point.

> 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.

Agreed.

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

Indeed, it is stronger than it ought to be. It should say it may not be always what is desired at call site.

> 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?)

Plausible, will experiment.

> 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, ...

OK

> 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).

OK

> 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?

OK

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

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