[lworld] [Rev 01] RFR: 8237072: [lworld] Add support for denoting and deriving the reference projection

Srikanth Adayapalam sadayapalam at openjdk.java.net
Mon May 4 00:26:40 UTC 2020


On Fri, 1 May 2020 11:41:30 GMT, Srikanth Adayapalam <sadayapalam at openjdk.org> wrote:

>> Another odd test:
>> 
>> static inline class V  {
>>         int y = 52;
>> 
>>         class Bar { }
>>         static class Baz { }
>>     }
>> 
>>     public static void main(String[] args) {
>>         new V.ref().new Bar();
>>         new V().new Bar();
>>         V.Baz baz1;
>>         V.ref.Baz baz2;
>>     }
>> 
>> This gives:
>> 
>> error: cannot find symbol
>>         new V.ref().new Bar();
>>                         ^
>>   symbol:   class Bar
>>   location: class V$ref
>> error: cannot find symbol
>>         V.ref.Baz baz2;
>>              ^
>>   symbol:   class Baz
>>   location: class V$ref
>> 2 errors
>> Now, I don't know what's the status of inner classes and values - but this seems to suggest that member types are not
>> copied from one scope to another (since the nested names are only resolved from the `val` projection.
>
>> Another odd test:
>> 
>> ```
>> static inline class V  {
>>         int y = 52;
>> 
>>         class Bar { }
>>         static class Baz { }
>>     }
>> 
>>     public static void main(String[] args) {
>>         new V.ref().new Bar();
>>         new V().new Bar();
>>         V.Baz baz1;
>>         V.ref.Baz baz2;
>>     }
>> ```
>> 
>> This gives:
>> 
>> ```
>> error: cannot find symbol
>>         new V.ref().new Bar();
>>                         ^
>>   symbol:   class Bar
>>   location: class V$ref
>> error: cannot find symbol
>>         V.ref.Baz baz2;
>>              ^
>>   symbol:   class Baz
>>   location: class V$ref
>> 2 errors
>> ```
> 
> This is acknowledged as a known TODO in
> com.sun.tools.javac.code.Symbol.ClassSymbol#referenceProjection
> 
> I have raised JDK-8244233 to cover it,

> ```
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java:546
>     `outermost' - mix of quotes.
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java:1319
>     `other' projection: If `this' - mix of quotes.
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java: 1715
>     The `other' projection: If `this' - mix of quotes.
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java:1967
>     The `other' projection: If `this' - mix of quotes (hate when cut and paste propagates the problem.)
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java:1027
>     `other' projection: If `this' - mix of quotes

I have cleaned up these, but this is a common coding/commenting convention in Javac. See that in the same files
Symbol.java & Type.java, there are about 20 other unrelated places where quotes in this `style' exist :)

> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java:1669
>      // TODO: TYP?, CLINT? - are you tracking?

Yes: JDK-8244233

> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java:1672
>     I haven't seen enough Valhalla code to fully comment but 'r' and 'v' prefixes for 'ref' and 'val' might be too subtle.
>     One lower case letter in an overloaded variable name is visually harder to discern. (Brevity is not the next readers
>     friend.)

Agreed it is poor choice. Fixed by expanding to ref* and val*

> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java:1688
>     I think this.flatname.append('$', this.name.table.names.ref); should be broken out into two? support methods.
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Symbol.java:1805
>    Just wonder if this common projection pattern (p ? p.x : null) could be broken out into support methods.

I have left these as is for now, as it is not clear what exactly is being suggested. If the original code is not clear
enough, do outline what you have in mind. TIA

> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java:1048
>    What if isValue(es) and !isValue(et)? Doesn't really follow the pattern of the old code.
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java:1234
>    Same question as above.

Basically, we are checking here if T[] <: S[]. This is true if T.ref <: S. Example of such a check being is Point [] <:
Object []

If the polarity is reversed and we are checking if Object[] <: Point [], we would come up with
    es = Point
    et =  Object

and return isSubtype(object, Point)

which will return false which is indeed the right answer for if Object[] <: Point []

> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java:1627
>    Tracking unspecified behaviour?

Two clarifications: (1) there is no "behavior" here. it is only a comment I have left in for our reference as we are
going to hit it the moment we are going to add support for inline type arguments. (2) And because we are going to hit
it most definitely as soon as we start working on generics support that there is no ticket for that per se.
 
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java:2194
>     Comment indent :-)

Fixed.

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java:4074
>     Naively: What happens if rs.resolveIdent does raise an exception? sym undefined? exception propagated?
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java:4197
>     Similarly to above.

The top level exception handling really happens in com.sun.tools.javac.main.Main#compile(java.lang.String[],
com.sun.tools.javac.util.Context) in the various catch blocks between lines 330-356.


> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java:901
>     Tracking tolerate Value.class for now?

I have fixed the comment to just say: "tolerate Value.class" dropping the "for now" - The comment can be confusing. We
should *always* tolerate and not report error on Value.class - thanks for catching this. (the "for now" originally
meant "may be there are other such cases we will discover we may have need to support also" but we have't found
anything else that must be tolerated)

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java:420
>     Is there a plan to handle projection conversation without modifying the tree?
> ```

No, this may not be doable - it is the intrinsic/organic cost of saying inline types are islands (i.e they have no
supertypes of any kind in the lang model including jlO and any declared super types). The best that can be done is to
ensure (a) the swicheroo happens for the narrowest window needed (b) the tree is restored in a finally block

Many many thanks for the review Jim!

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

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


More information about the valhalla-dev mailing list