RFR: 8334248: Invalid error for early construction local class constructor method reference [v4]

[Vicente Romero]

On Fri, 28 Jun 2024 17:45:48 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This PR fixes some of the checks that are applied when a new inner class (whether member, local or anonymous) is constructed, either via `new`, or, indirectly, via `super`. These checks currently reside in `Resolve::resolveImplicitThis`, but this routine fails to take into account the differences between the various cases (e.g. the checks for member inner classes are different than those for local/anon classes), and this results in spurious compilation errors. Below is an attempt to describe what *should* happen, in the various cases.
>> 
>> #### Member inner classes
>> 
>> Whenever we see `new M()` where `M` is a member inner class, we need to infer an expression for `M`‘s enclosing instance, given none is provided. This inference problem is also present when checking a `super(...)` constructor call: if the superclass is a member inner class `M`, then validating the constructor call implies inferring a suitable enclosing instance for `M`, as if we were checking new `M()`.
>> 
>> This inference process should look at *all* the enclosing instances available to us, and pick the innermost enclosing instance of type `C` such that:
>> * `C.this` is accessible (e.g. not in `C`‘s early construction context) and;
>> * `C` is a subclass of `M`‘s enclosing class.
>> 
>> The crucial observation here is that there can be **multiple** valid enclosing instances, and the innermost one might not be available due to early construction context, so we need to be able to skip that, and jump to the next. See the test `EarlyIndirectOuterCapture`, which fails to compile today, but is accepted with the fixes in this PR.
>> 
>> This check is defined in `Reslve::findSelfContaining`.
>> 
>> #### Local and anonymous inner classes
>> 
>> When creating local and anonymous inner classes, we should **not** check for the availability of an enclosing instance, as JLS 15.9.2 is silent about this. What matters, for local and anon classes, is that the class creation expression occurs in a context where we can access local variables defined in the method in which the local class is defined. This means that code like the one in the `LocalFreeVarStaticInstantiate` test is now correctly rejected.
>> 
>> This check is defined in `Reslve::findLocalClassOwner`.
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Address review comment

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 3078:

> 3076:                     rs.findSelfContaining(pos, env, type.getEnclosingType().tsym, names._this);
> 3077:             if (res.exists()) {
> 3078:                 rs.accessBase(res, pos, env.enclClass.sym.type, names._this, true);

I wonder if the call to accesBase should be part of Resolve::findLocalClassOwner and Resolve::findSelfContaining? in other case clients needs to know a bit more about Resolve's internals. Although there are other places in Attr where we invoke Resolve:accessBase, so probably in the fence with this one

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line 3768:

> 3766:                     Env<AttrContext> env,
> 3767:                     TypeSymbol c,
> 3768:                     Name name) {

won't the name always be `this`, so I guess we can remove the parameter?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19904#discussion_r1659192151
PR Review Comment: https://git.openjdk.org/jdk/pull/19904#discussion_r1659214404