[lworld] RFR: 8237072: [lworld] Add support for denoting and deriving the reference projection
Jim Laskey
github.com+63007666+JimLaskey at openjdk.java.net
Fri May 1 11:49:52 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) {
> }
> }
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:1669
// TODO: TYP?, CLINT? - are you tracking?
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.)
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: 1715
The `other' projection: If `this' - mix of quotes.
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.
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
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.
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java:1627
Tracking unspecified behaviour?
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java:2194
Comment indent :-)
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.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java:901
Tracking tolerate Value.class for now?
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?
-------------
PR: https://git.openjdk.java.net/valhalla/pull/32
More information about the valhalla-dev
mailing list