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

[Maurizio Cimadamore]

On Wed, 26 Jun 2024 11:17:50 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`.

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

> 3066: 
> 3067:         void checkNewInnerClass(DiagnosticPosition pos, Env<AttrContext> env, Type type) {
> 3068:             boolean isLocal = type.tsym.owner.kind == MTH;

This is a bit subtle - we need to run the new checks for _all_ inner classes. E.g. if a local class `C` has no enclosing instance (because is declared in a static context), we might still need to check that `new C()` is valid, because `C` might capture variables from the enclosing (static) method.

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

> 4869:      * but pointing to a class for which an enclosing instance is not available.
> 4870:      */
> 4871:     class BadConstructorReferenceError extends InvalidSymbolError {

This was no longer needed. We had a duplication between a check issued when resolving method references (see `ConstructorReferenceLookupHelper`) and a post-resolution check in `Attr::visitReference`. I think keeping the latter makes much more sense, as that brings more consistency between the code paths for instance creation expression and constructor reference expression.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19904#discussion_r1654653135
PR Review Comment: https://git.openjdk.org/jdk/pull/19904#discussion_r1654650467