[lworld] RFR: 8237072: [lworld] Add support for denoting and deriving the reference projection
Maurizio Cimadamore
mcimadamore at openjdk.java.net
Thu Apr 30 17:10: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) {
> }
> }
Overall this looks very good. Note that changes to dot apply cleanly on latest lworld branch, so you might need some
rebasing.
I'm impressed at how much we could get done with relatively small set of changes in the core classes. As discussed
offline, my mine gripe with the proposed implementation approach is that, I think, it distorts the user model view a
little. In my mental model:
* there is ONE class in the langauge; the class might be `inline` or not. If it's inline, might be ref-default or not.
* Every type defines a reference projection. If a type is a type that comes from an ordinary class, then the reference
projection of that type is the type itself.
* inline classes do not give birth to a single type (like normal classes do - e.g. think of` j.l.String`). But they can
give birth to two distinct type (pretty much like a generic class can give birth to an infinite set of types)
* to help translation, a single inline class will be translated into a ref/val classes pair - which means a single
inline class will give rise to *two* runtime types
In other words, I think that appealing with symmetry with the generic use case, where a single declared class give rise
to many types might be cleaner in the long run than having two declared classes (as in the proposed implementation).
This choice surfaces up in a bunch of places where we have relationship which are seemingly defined on symbols (e.g.
`Symbol::outermostClass`, and friends) in which we have to always ask first whether the class symbol we are dealing
with is a *reference* class symbol, and, if not, go back to that. This choice also backfires when it comes to keep the
members of the two fictional symbols in sync - so there's a number of places where we have to update the scope of one
class in the same way we update the scope of the other class.
Note: all these things are not blockers - but to me they might be signs that some alternate implementation might be
possible.
The current approach will require adjustments in places where symbols are expected, but something like `V.ref` is
found - for instance I put together this fictional example:
package a;
import a.V.ref;
inline class V {
int x = 0;
}
and I got this:
error: import requires canonical name for V$ref
import a.V.ref;
^
which seems a bit confusing (more generally I note that the compiler is often emitting V$ref and V$val which should be
normalized to the syntax used by developers - e.g. dropping the dollar sign).
I'm afraid that when the time will come and we will start looking at the annotation processing machinery, the choice of
having two symbols (hence two `Elements`) for the `val` and `ref` projection might become untenable (since at that
point I believe the annotation processor user would expect to get two different `TypeMirror`s for the projections which
point to the same underlying declared `Element`). Since `Element` is 1-1 with `Symbol`, this might become a problem.
On a different topic, I noted something on name resolution:
inline class V {
int y = 52;
static class ref {
int x;
}
void m(V.ref vref) {
vref.x = 12;
}
}
Which gives:
error: cannot find symbol
vref.x = 12;
^
symbol: variable x
location: variable vref of type V$ref
In other words, the compiler is always resolving `.ref` to a projection - which is fine for now, but there's a deeper
question as to whether we're ok with this behavior and if we want to even allow defining a nested class called `ref` or
`val` inside an inline class.
Another thing I noted when playing with ref projection is that we don't seem to be getting NPE at runtime when passing
nulls back and forth:
static inline class V {
int y = 52;
}
static void m(V.ref vr) {
V v = vr;
}
public static void main(String[] args) {
m(null);
}
This compiles and runs correctly, while I'd expect an NPE to be thrown inside `m`. I've inspected the bytecode:
0: aload_0
1: checkcast #7 // class "QV;"
4: astore_1
5: return
So, this might be an issue with the VM not doing the right thing with the cast (e.g. not ruling nulls out) - and
ultimately related to this discussion:
https://mail.openjdk.java.net/pipermail/valhalla-spec-experts/2020-April/001288.html
We should check as to whether this behavior is expected, and if it should be rectified; or whether we should add an NPE
check on our side.
This is all I have for now.
-------------
PR: https://git.openjdk.java.net/valhalla/pull/32
More information about the valhalla-dev
mailing list