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

[Maurizio Cimadamore]

On Fri, 14 Jun 2024 19:49:16 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

> > I'm worried that the fix might not go deep enough. E.g. what about the method-reference variant of the above:
> 
> A couple of random thoughts...
> 
> For me with all current patches applied, the compiler correctly handles the version of `Outer.java` with `Foo::new`:
> 
> ```
> Outer.java:10: error: invalid constructor reference
>             class Bar { static void r() { Supplier<Foo> sfoo = Foo::new; } };
>                                                                ^
>   cannot access constructor Foo()
>     an enclosing instance of type Outer.Inner is not in scope
> ```

The fix is still not doing its job. But it seems another check kicks in, and saves the day - namely:

https://github.com/openjdk/jdk/blob/master/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java#L3713

Note that the error message you get is not the same error message mentioned in this PR - meaning that the check you tweaked is now effectively disabled in this case.


> 
> But it throws the usual NPE on the `Foo.new()` version:
> 
> ```
> java.lang.NullPointerException: Cannot read field "sym" because "this.lvar[0]" is null
> 	at jdk.compiler.interim/com.sun.tools.javac.jvm.Code.emitop0(Code.java:572)
> 	at jdk.compiler.interim/com.sun.tools.javac.jvm.Items$SelfItem.load(Items.java:369)
> 	at jdk.compiler.interim/com.sun.tools.javac.jvm.Gen.visitIdent(Gen.java:2357)
> ```

In this case the check that rescued the above example is not ran, as that check is specific to method references. So the compiler just crashes.

> 
> So `Outer.java` doesn't seem to be a evidence that the fix in PR #19705 is incorrect. It might be of course, but if so it would be for some other reason.
> 
> I guess the question is: must we invoke `resolveImplicitThis()` when the constructed class has `NOOUTERTHIS`?

I'm actually checking what we should do from a JLS perspective. I see some rules in 15.9.2, but it seems to me that the only relevant case to check, for local and anon class creation is that they don't occur in a static context. In all other cases, I don't think it's possible to refer to a local class w/o the necessary enlcosing instance (if you don't have the enclosing instance you are not in the same scope as the local class, so you can't even name it). We're discussing this with Dan and Gavin.
> 
>     * If so, then how do you explain the counter-example (from amber-dev email) for method references when you do that (showing a bogus error being generated):
> 
> 
> ```
> $ cat EarlyLocalTest3.java
> public class EarlyLocalTest3 {
> 
>     class Test {
>         Test() {
>             class InnerLocal { }
>             Runnable r = InnerLocal::new;
>             r.run();
>             super();
>         }
>     }
> }
> $ javac -enable-preview --release 24 EarlyLocalTest3.java
> ../test/langtools/tools/javac/SuperInit/EarlyLocalTest3.java:6: error: cannot reference this before supertype constructor has been called
>             Runnable r = InnerLocal::new;
>                          ^
> ```
Well, there is logic inside Resolve::resolveSelf which seems to look for "env.info.ctorPrologue" and issue the above error if the flag is set. It might be that this code needs an update.
> 
>     * If not, then there must be some other cause for the bug. Regarding this possibility, note there is another bug relating to "bogus capture in static context" e.g. [JDK-8322882](https://bugs.openjdk.org/browse/JDK-8322882). So there may be a common root cause these share.

I think it is the same issue. Attr::visitNewClass has code that is 1-1 with the one you have in this fix. The only difference between the method reference code and the `new` code is the presence of that additional enclosing instance check (the correctness of which I'm also not sure) when the method reference is being resolved, as I explained earlier. If that check wasn't there the two programs would fail in exactly the same way.

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

PR Comment: https://git.openjdk.org/jdk/pull/19705#issuecomment-2168767676