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

Maurizio Cimadamore mcimadamore at openjdk.java.net
Thu Apr 30 17:36:09 UTC 2020


On Thu, 30 Apr 2020 04:27:10 GMT, Srikanth Adayapalam <sadayapalam at openjdk.org> wrote:

> Hello Maurizio & Jim,
> 
>     May I request you to please review this patch for valhalla inline types support
> under the new scheme ? TIA.
> 
> I.   What does this patch do?
> II.  What can be skipped in the review?
> III. Recommended order for review.
> IV.  Known problems, limitations and deferred tasks.
> V.   Report to language designers on open problems.
> 
> I. What does this patch do:
> 
>     - From every inline class declaration V, derive two classes V and V.ref
>     - These two are subtypes at the VM level, but are related (only) by inline
>       widening conversion and narrowing conversion at the language level.
>     - The two types have the same fields and methods with the same shape and
>       accessibility rules.
>     - Add source level support for V.ref and V.val notation.
>     - Get rid of the experimental support for generics over values. Until we
>       figure out the full story with generics and inlines, carrying along this
>       experimental support is becoming untenable/unsustainable.
>     - Get rid of the experimental support added for noncovariant value arrays
>     - Get rid of LW2's "Nullable Projections" including the V? syntax
> 
> II. These files can be skipped as they simply revert change and align
>     with origin/master: (roll back V and V? nullable projections of LW2)
> 
>     JavacParser.java
>     AttrContext.java
>     JCTree.java
>     Pretty.java
>     Printer.java
>     RichDiagnosticsFormatter.java
>     TaskFactory.java
>     TreeCopier.java
>     TypePrinter.java
> 
> III. Recommended order for review:
> 
>     - Type.java, Symbol.java
> 
>             Look at the new internal APIs to query/compute various projections.
>             Every class type, class symbol, method and field support an API
>             to return the "other" projection i.e its doppleganger in the alternate
>             universe.
> 
>             Most crucially ClassSymbol.referenceProjection()
> 
>     - Attr.java:
> 
>             Source support for .ref/.val
> 
>     - MemberEnter.java:
>     - TypeEnter.java:
>     - TransTypes.java:
> 
>             Co-evolve V.val and V.ref in lock step (also TransValues.java)
> 
>     - TransValues.java:
> 
>             Fold all V.ref.member accesses to ((V) V.ref).member access.
> 
>     - Resolve.java:
> 
>             Changes to make sure method look up works OK in a world where values are islands.
> 
>     - Types.java:
>   
>             Implement inline widening/narrowing relationship.
> 
>     - ProjectionRelationsTest.java:
> 
>             Verify all relationships between V and V.ref and V[] and V.ref[]
> 
>     - ClassWriter.java:
>             
>             Dual class generation scheme with subtyping relationship at the binary/VM level.
> 
>     - ClassReader.java:
> 
>             Merge the dual classes back, sever the subtyping relationship and make
>             them operate as a pair of classes that can convert to each other.
> 
>     - New tests:
>     
>             Most importantly ProjectionRelationsTest.java
> 
>     - Other files.
> 
>  
> IV. There are many known issues that have been deliberately deferred to a later iteration:
> 
>     - With Brian's consent I am using a simpler translation strategy than what is
>       outlined in the SoV documents. These are good enough for now, but when
>       VBC migration is undertaken, will have to enhanced.
>     - No support for ref-default and val-default classes yet.
>     - No support for class hierarchy sealing.
>     - You can't do new V.ref() (V.ref is abstract) although SoV calls for it.
>     - Handling of .ref and .val is quick and dirty; Need revisit.
>     - The nest mate related attributes are not fully properly emitted as of now
>     - Need to verify that attributes from V are not carried over inadvertantly to
>       V$ref.class
>     - Class hierarchy walking in a world where inline types are islands calls for
>       a type switch. I have done this only in places uncovered by existing tests.
>       We need to go through various utility methods (similar to what is done in
>       Symbol.java and Resolve.java) to make sure these changes are consistently
>       applied.
>     - Diamond inference with value classes crashes now (implementation creates factory
>       methods, projections are missing and need to be created)
> 
> 
> V. Problems in the interplay of inlines types with generics and inference:
> 
>     Removing the experimental support for generics over values results in several
> constructs that used to compile earlier (albeit only with -XDallowGenericsOverValues)
> not compiling anymore.
> 
>     These expose some issues that feed into the language design. We need to evolve
> a short term answer (so that the JDK components tests that are impacted don't get
> blocked) and a long term one (the real solution)
> 
> Here is a snippet that captures these problems:
> import java.util.Arrays;
> 
> interface I {}
> 
> public inline class X implements I {
> 
>     int x = 10;
> 
>     void problem_01() {
>         X [] a1 = new X[0];
>         X [] a2 = Arrays.copyOf(a1, a2.length, X[].class);
>       
> /*
> error: incompatible types: inference variable T has incompatible bounds
>         X [] a2 = Arrays.copyOf(a1, a2.length, X[].class);
>                                          ^
>     lower bounds: X,Object
>     lower bounds: X$ref
>   where T,U are type-variables:
>     T extends Object declared in method <T,U>copyOf(U[],int,Class<? extends T[]>)
>     U extends Object declared in method <T,U>copyOf(U[],int,Class<? extends T[]>)
> 1 error
> */
>     }
> 
>     void foo(Class<? extends I> p) {}
> 
>     void problem_02() {
>         foo(X.class);
> /*
> error: incompatible types: Class<X> cannot be converted to Class<? extends I>
>         foo(X.class);
> */
>     }
> 
>     String data() {
>         return null;
>     }
> 
>     // Problem: 3, we infer a stream of X.ref's causing
>     // the method reference to not type check.
>     void unbound_lookup_with_projected_receiver() {
>         X [] a = new X[0];
>         Arrays.stream(a).map(X::data).filter(p -> p != null).forEach(System.out::println);
> /*
> error: incompatible types: invalid method reference
>         Arrays.stream(a).map(X::data).filter(p -> p != null).forEach(System.out::println);
>                              ^
>     method data in class X cannot be applied to given types
>       required: no arguments
>       found:    X$ref
>       reason: actual and formal argument lists differ in length
> */
>     }
> 
>     void problem_04() {
> /*
>     test/hotspot/jtreg/runtime/valhalla/valuetypes/UnsafeTest.java:125: warning: [removal] putObject(Object,long,Object) in
>     Unsafe has been deprecated and marked for removal
>             U.putObject(v, off_o, List.of("Value1", "Value2", "Value3"));
>              ^
> /home/srikanth/valhalla/test/valhalla/test/hotspot/jtreg/runtime/valhalla/valuetypes/UnsafeTest.java:127: error: method
> valueHeaderSize in class Unsafe cannot be applied to given types;
>             U.putInt(v, off_v + off_i - U.valueHeaderSize(Value2.class), 999);
>                                          ^
>   required: Class<V>
>   found:    Class<Value2>
>   reason: inference variable V has incompatible bounds
>     equality constraints: Value2
>     lower bounds: Object
>   where V is a type-variable:
>     V extends Object declared in method <V>valueHeaderSize(Class<V>)
> */
>     }
> 
>     void problem_05() {
> /*
> test/hotspot/jtreg/compiler/valhalla/valuetypes/TestIntrinsics.java:119: error: incomparable types: Class<CAP#1> and
> Class<MyValue1$ref>
>         boolean check2 = MyValue1.class.asIndirectType().getSuperclass() == MyValue1.ref.class;
>                                                                          ^
>   where CAP#1 is a fresh type-variable:
>     CAP#1 extends Object super: MyValue1 from capture of ? super MyValue1
> */
>     }
> 
>     public static void main(String [] args) {
>     }
> }

Added some specific file comments

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

> 440:     public Symbol referenceProjection() {
> 441:         return null;
> 442:     }

How much would it help to have this return `this` ? It seems to me that there a lot of `if <value> ...
referenceProjection()` going around.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java line 1197:

> 1196:         public Type valueProjection() {
> 1197:             if (!isReferenceProjection() || projection !=  null)
> 1198:                 return projection;

Question here - since you have two symbols, getting a reference projection and value projection shouldn't be as simple
as doing `tsym.XYZprojection().type` ? Why do we need to create new types here?

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

> 1759:                     if (isValue(t)) {
> 1760:                         // (s) Value ? == (s) Value.ref
> 1761:                         t = t.referenceProjection();

I didn't get these comments

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

> 2193:
> 2194:                // No man may be an island, but the bell tolls for a value.
> 2195:                 if (isValue(t))

:-)

src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/ClassReader.java line 2605:

> 2604:         readClassFileInternal(c);
> 2605:         if (c.isValue()) {
> 2606:             /* http://cr.openjdk.java.net/~briangoetz/valhalla/sov/04-translation.html

This approach leads to very compact code - but in javac-land it's a bit odd that we have to prune away members at this
late stage. I wonder if some of the pruning could happen earlier (e.g. in TransValues?)

test/langtools/tools/javac/valhalla/lworld-values/InferredValueParameterizationTest.java line 2:

> 1: /*
> 2:  * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
> 3:  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

The golden file seems to still be there  - even though this is now a positive test

src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties line 3704:

> 3703: compiler.err.generic.parameterization.with.value.type=\
> 3704:     Inferred type {0} involves generic parameterization by an inline type
> 3705:

Do you have examples when this can happen? Or is this a leftover (in which case I think Check.java could be cleaned up
a bit)

test/langtools/tools/javac/valhalla/lworld-values/ProjectionRelationsTest.java line 1:

> 1: /*
> 2:  * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.

You might want to look at this test framework:

test/langtools/tools/lib/types/TypeHarness.java

An example of such a test is:

test/langtools/tools/javac/types/PrimitiveConversionTest.java

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

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


More information about the valhalla-dev mailing list