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

[Jan Lahoda]

On Thu, 26 Sep 2024 08:48:20 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:
> 
>   Tweak comments

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

> 51: import com.sun.tools.javac.comp.DeferredAttr.AttrMode;
> 52: import com.sun.tools.javac.comp.MatchBindingsComputer.MatchBindings;
> 53: import com.sun.tools.javac.comp.Resolve.InvalidSymbolError;

Nit: these imports appear to be unused.

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

> 3788:                     Env<AttrContext> env,
> 3789:                     TypeSymbol c,
> 3790:                     Name name,

It is probably a matter of style, but for me, it would be easier if `name` would be dropped, and `names._this` would be used in `findFirst`. It took me a while before I realized `name` can only be `this`, so there cannot be more than one, so `findFirst` is "find it or return null", not "if there are multiple, return one of them".

test/langtools/tools/javac/LocalFreeVarStaticInstantiate.java line 19:

> 17:             }
> 18:         }
> 19:     }

I wonder if the test case should be enhanced with something like:

    Runnable r = () -> {
        Object there = "";
        class Local {
            {
                there.hashCode();
            }
            static {
                new Local();    // can't get there from here
            }
        }
    };


It works currently, as lambdas get a fake method owner if needed, so it would be just to make sure it remains working even if the handling of the lambdas changes.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19904#discussion_r1785973831
PR Review Comment: https://git.openjdk.org/jdk/pull/19904#discussion_r1785981909
PR Review Comment: https://git.openjdk.org/jdk/pull/19904#discussion_r1785966618