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

[Srikanth Adayapalam]

On Thu, 30 Apr 2020 17:08:30 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> Overall this looks very good. Note that changes to dot apply cleanly on latest lworld branch, so you might need some
> rebasing.

Thanks for the review, Maurizio!

(I will look into rebasing, At the time I raised the PR request it was even with openjdk/valhalla)

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

[...]

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

I agree this suggestion if it is implementable, has promising upside potential to make the code relatively simpler.

I chose the present approach because:

* some of the state that (naturally) today belong in the class symbol abstraction needs to be distinct between the two
  projections. (name, flags, type ...)

* Various parts of javac code compare tsyms and assume the types are same if they are same (on the non-parameterized type
  path)

* there are places where we operates only on symbols and it is not clear we will unambiguously know the symbol ownership
  in those places.

These may all be solvable or worked around satisfactorily.

I agree that upside potential is promising enough to explore the presently unknown downside potential - JDK-8244227 is
raised to explore that, to prototype it so we can make an informed call.


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

I disagree with the vocabulary. It is not backfiring! It is additional work we knowingly sign up for :) But yes, it
would be good if we don't have to.

I must mention here that the LW2 implementation actually used two ClassSymbol shells that shared the members_field. I
had to opt for  a deeper copy to solve some problem I ran into which I am unable to recollect at my advanced geriatric
stage :)
 
> 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 have added this case to JDK-8244229 Thanks!


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

Hmm. As discussed offline, we may have to use some smoke and mirrors anyways to make them appear as one symbol when
dealing with binary classes from classpath.

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

I have added this test case to JDK-8244229 Thanks!

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

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