8194743: Compiler implementation for Statements before super()

[archiecobbs]

This is a first draft of a patch for JEP 447.

Summary of changes:

1. Track when we're within a constructor "prologue" via new flag AttrContext.ctorPrologue
2. Add checks for illegal early access to this in constructor prologues, and update existing checks to distinguish between static context vs. constructor prologue context
3. Verify allowed placement of super()/this() calls via new method Check.checkSuperInitCalls()
4. Remove/refactor assumptions in several places that super()/this() was always the first statement

The changes in Flow.java are an example of #4. Flow.FlowAnalyzer checks for uncaught checked exceptions. For initializer blocks, this was previously done by requiring that any checked exceptions thrown be declared as thrown by all constructors containing super(). This list of checked exceptions was being pre-calculated before recursing into the initial constructors. This worked because initializer blocks were executed at the beginning of each initial constructor right after super() is called.

Now initializer blocks are traversed as each super() invocation is encountered, reflecting what actually happens at runtime. Similarly, final fields are marked as DA after encountering this(), not automatically at the beginning of those constructors. These changes produce equivalent checks, but are compatible with the new flexibility of placement of super()/this() as well as possible future changes that could occur along these same lines.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires CSR request JDK-8302041 to be approved

Issues

• JDK-8194743: Compiler implementation for Statements before super() (Sub-task - P4)
• JDK-8302041: Permit additional statements before this/super in constructors (CSR)

Reviewers

• Vicente Romero (@vicente-romero-oracle - Reviewer) ⚠️ Review applies to e2f88137
• Julian Waters (@TheShermanTanker - Committer) ⚠️ Review applies to e2f88137
• Maurizio Cimadamore (@mcimadamore - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13656/head:pull/13656
$ git checkout pull/13656

Update a local copy of the PR:
$ git checkout pull/13656
$ git pull https://git.openjdk.org/jdk.git pull/13656/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13656

View PR using the GUI difftool:
$ git pr show -t 13656

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13656.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back acobbs! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@archiecobbs The following labels will be automatically applied to this pull request:

• compiler
• kulla

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 19: Full (bbdb814f)
• 18: Full (99a277e5)
• 17: Full - Incremental (738c10a5)
• 16: Full - Incremental (2356ac6f)
• 15: Full - Incremental (3e75e19e)
• 14: Full - Incremental (150d794c)
• 13: Full - Incremental (599cf0e5)
• 12: Full - Incremental (a5510b79)
• 11: Full (4ff4c5c5)
• 10: Full (e2f88137)
• 09: Full - Incremental (599d761b)
• 08: Full - Incremental (b0b13b2a)
• 07: Full - Incremental (c6cf80fc)
• 06: Full (a5f8cc5e)
• 05: Full (276e449d)
• 04: Full (543a596a)
• 03: Full - Incremental (73bb3270)
• 02: Full - Incremental (3c9322b6)
• 01: Full - Incremental (0e638da2)
• 00: Full (0eac8b68)

[mlbridge]

Mailing list message from Jim Laskey on compiler-dev:

Don?t forget to update the tests to use the @enablePreview flag.

?

[liach]

Quick question, since I don't know much about javac, I wonder what happens if you declare a local class or an anonymous class in the ctor prologue? Will that local/anonymous class have a mandated outer instance parameter in its constructor, and thus having access to a newly allocated but non-initialized object via ClassName.this?

[archiecobbs]

what happens if you declare a local class or an anonymous class in the ctor prologue?

You can declare such a class in the prologue, but you can't instantiate it there:

    public static class LocalTest {
        public LocalTest() {
            class Foo {
                Foo() {
                    LocalTest.this.hashCode();
                }
            }
            new Foo();    // ILLEGAL
            super();
            new Foo();    // ALLOWED 
        }
    }       

Note that:

• The prologue includes the statements prior to the superclass constructor and the parameter expressions within the superclass constructor invocation
• The rules that apply to the prologue are the same rules that previously applied only to parameter expressions within the superclass constructor invocation

So if we "backport" your question to JDK 20, you get the current behavior:

import java.util.concurrent.atomic.*;
public class LocalTest extends AtomicReference<Object> {
    public class Inner {
    }
    public LocalTest(int x) {
        super(new Inner());     // ILLEGAL
    }
    public LocalTest(char x) {
        super(null);
        new Inner();            // ALLOWED
    }
}

[mcimadamore]

Overall, this is a very good patch. Not only it adds a new feature (which I found myself reaching for several times), but it also does so by consolidating a bunch of logic in the compiler code. Well done!

[mlbridge]

Mailing list message from Alex Buckley on compiler-dev:

Hi Archie,

Anything concerning the science of when nested classes have enclosing
instances is pretty scary, so I'd like to make sure we're all heading in
the same direction.

On 9/23/2023 8:19 AM, Archie Cobbs wrote:

public class Test {
public Test(int x) {
}

 public Test\(\) \{
     this\(switch \(0\) \{
       default \-> \{
         class Local \{
         \}
         yield 0\;
       \}
     \}\)\;
 \}

}

According to JLS 21 ?15.9.2, class `Local` does not have an enclosing instance.

Yet here is what's actually emitted by the compiler:

$ javap -classpath classes -c Test$1Local
Compiled from "Test.java"
class Test$1Local {
final Test this$0;

Test$1Local(Test);
Code:
0: aload_0
1: aload_1
2: putfield #1 // Field this$0:LTest;
5: aload_0
6: invokespecial #7 // Method java/lang/Object."<init>":()V
9: return
}

JLS 15.9.2 says what happens when an inner local class is instantiated,
but does not determine whether Local is declared as inner in the first
place. JLS 8.1.3 is where we find that Local is specified to be inner
(because it is not implicitly static). So, javac's emission of
Test$1Local is in line with the JLS -- great!

An explicit goal of JEP 447 is **any existing program must compile to
the same bytecode**. So if we adhere to that goal, local classes
declared in pre-construction contexts must have outer instances.

I don't see this explicit goal stated in JEP 447. There's a sentence
about compilation in Testing -- "We will compile all JDK classes using
the previous and new versions of the compiler and verify that the
resulting bytecode is identical." -- but that sounds merely like a
nice-to-have property, and one which holds only for JDK classes.

If 100% class file compatibility for all existing source code is a goal,
then please say that in the Goals and give an example in the
Description. ("compiles to same bytecode" is actually a higher bar than
binary compatibility, which is why I didn't say "100% binary
compatibility".)

I think this is the most natural behavior anyway. Anonymous classes,
with their "immediate enclosing instance with respect to superclass S",
and their declare-and-instantiate-all-at-once property, are the
oddballs. It's appropriate for them to have a special rule where their
implicit outer instance "disappears" in a pre-construction context
because it would never be possible to provide one.

I'm not, per se, disagreeing with specifying that local classes declared
in a pre-construction context are inner. It's certainly attractive from
a compatibility POV to be able to move a local class declaration around
within a constructor body (maybe before super(..)/this(..), maybe after)
and observe no class file change because the local class is always inner
(unless it's a local enum class or local record class of course). On the
other hand, we have to guarantee that no instantiation of the local
class is possible before super(..)/this(..) has completed normally.
Here's a program that tries to instantiate a Local which observes x with
a value other than 5. What does the JLS say about the program? What does
javac do?

public class Test {
public int x = 5;
public Test(int i) {}

public Test() {
class Local {
{ System.out.println(x); }
}
try {
this(switch (0) { default -> throw new Exception(); });
} finally {
new Local(); // Legal or illegal?
}
}
}

Don't worry about JEP 401 until JEP 447 has thoroughly clarified the
matter of whether such local classes are inner or not.

Alex

[mlbridge]

Mailing list message from Archie Cobbs on compiler-dev:

Hi Alex,

An explicit goal of JEP 447 is **any existing program must compile to
the same bytecode**. So if we adhere to that goal, local classes
declared in pre-construction contexts must have outer instances.
I don't see this explicit goal stated in JEP 447. There's a sentence
about compilation in Testing -- "We will compile all JDK classes using
the previous and new versions of the compiler and verify that the
resulting bytecode is identical." -- but that sounds merely like a
nice-to-have property, and one which holds only for JDK classes.

You're right.. the stated goal is actually "Do not change the behavior of
any existing program."

Making sure the emitted bytecode matches is just one way to accomplish that
goal (and possibly overkill).

I think this is the most natural behavior anyway. Anonymous classes,

with their "immediate enclosing instance with respect to superclass S",
and their declare-and-instantiate-all-at-once property, are the
oddballs. It's appropriate for them to have a special rule where their
implicit outer instance "disappears" in a pre-construction context
because it would never be possible to provide one.

I'm not, per se, disagreeing with specifying that local classes declared
in a pre-construction context are inner. It's certainly attractive from
a compatibility POV to be able to move a local class declaration around
within a constructor body (maybe before super(..)/this(..), maybe after)
and observe no class file change because the local class is always inner
(unless it's a local enum class or local record class of course).

Agreed.

On the other hand, we have to guarantee that no instantiation of the local
class is possible before super(..)/this(..) has completed normally.

Yes.. and same thing for non-static member classes, so this is not special
to local classes.

Don't worry about JEP 401 until JEP 447 has thoroughly clarified the

matter of whether such local classes are inner or not.

Thanks - so yes that central question remains... how should we treat local
classes in pre-construction contexts?

The current implementation answers this question "they are inner classes
with outer instances just like they would be in any other non-static
location".

Admittedly that answer was chosen in part because it is the most
conservative change (in particular, it produces the same bytecode).

But OK let's be a little bolder and ask what would be the BEST change?

My latest thinking is we should just allow local classes to be declared
"static".

Then "class Local" gives the current behavior (always having an outer
instance in a non-static context), while "static class Local" gives the
same behavior as a static member class.

You could then declare AND instantiate a static local class in a
pre-construction context - might be handy for doing pre-construction
"housekeeping".

Plus, this simplifies a developer's mental model for the various types of
classes, because now local classes and member classes are the same, except
of course for their lexical location and scope.

This would be the best of both worlds... ?

Here's a program that tries to instantiate a Local which observes x with

a value other than 5. What does the JLS say about the program?

JLS says this()/super() have to be "top level", so they can't be inside a try
{ } block.

What does javac do?

Test.java:10: error: switch expression does not have any result expressions
this(switch (0) { default -> throw new Exception(); });
^
Test.java:12: error: cannot reference this before supertype constructor has
been called
new Local(); // Legal or illegal?
^
Test.java:10: error: calls to this() not allowed here
this(switch (0) { default -> throw new Exception(); });
^
3 errors

(The first error can be worked around by inserting case 1 -> 2; of course).

-Archie

--
Archie L. Cobbs
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/compiler-dev/attachments/20230925/4f24a1d2/attachment-0001.htm>

[mlbridge]

Mailing list message from Alex Buckley on compiler-dev:

On 9/25/2023 10:13 AM, Archie Cobbs wrote:

 > An explicit goal of JEP 447 is \*\*any existing program must compile to
 > the same bytecode\*\*\. So if we adhere to that goal\, local classes
 > declared in pre\-construction contexts must have outer instances\.
I don\'t see this explicit goal stated in JEP 447\. There\'s a sentence
about compilation in Testing \-\- \"We will compile all JDK classes using
the previous and new versions of the compiler and verify that the
resulting bytecode is identical\.\" \-\- but that sounds merely like a
nice\-to\-have property\, and one which holds only for JDK classes\.

You're right.. the stated goal is actually "Do not change the behavior
of any existing program."

Making sure the emitted bytecode matches is just one way to accomplish
that goal (and possibly overkill).

Goals: (always the hardest single section of a JEP to write, BTW)

- Re: "In constructors, allow statements that do not access ..." -- this
Goal basically repeats the Summary in stating the solution. That's not
quite what a Goal is meant to do. The term "initialization logic" is
also a bit wonky because it can be read as dealing with initialization
of the current instance, which is the one thing you can't do before
super/this. The Goal should be: "Give developers greater freedom in
expressing the behavior of a constructor, allowing more natural
placement of logic that currently must be factored into static methods
or into arguments to super(..)." I am not sure that
statements-before-super allow logic which previously was "simply
impossible to express."

- Drop "Correct an error in the specification which defines constructor
invocations as a static context." Spec issues don't drive features.

- Re: "Preserve existing safety and initialization guarantees for
constructors." -- This sounds good but I bet 99% of readers couldn't
enumerate an "existing safety guarantee" if you demanded one from them.
Please spell out if this is about, say, guaranteeing that constructors
still run in top-down order. Also, please also avoid "initialization",
because you are not talking about the initialization of a class (see JLS
12.4) but rather the instantiation of a class.

- Re: "Do not change the behavior of any existing program." -- stated
more fully, this goal is "Perfect source compatibility: Any existing
source code, if recompiled, is legal and has unchanged behavior."
However, that could be a goal of almost any new language feature, so we
don't usually state it. And if we cast things in terms of bytecode --
"Do not change how existing source code is compiled to a class file" --
then it sounds pretty esoteric. I'd drop this goal.

Thanks - so yes that central question remains... how should we treat
local classes in pre-construction contexts?

The current implementation answers this question "they are inner classes
with outer instances just like they would be in any other non-static
location".

Admittedly that answer was chosen in part because it is the most
conservative change (in particular, it produces the same bytecode).

But OK let's be a little bolder and ask what would be the BEST change?

My latest thinking is we should just allow local classes to be declared
"static".

Then "class Local" gives the current behavior (always having an outer
instance in a non-static context), while "static class Local" gives the
same behavior as a static member class.

This is a big language change that I'm not going to comment on. You
should review the discussion around JEP 384 about how local record
classes were made implicitly static, never explicitly static.

Here\'s a program that tries to instantiate a Local which observes x
with
a value other than 5\. What does the JLS say about the program\?

JLS says this()/super() have to be "top level", so they can't be inside
a try { } block.

What does javac do\?

Test.java:10: error: switch expression does not have any result expressions
? ? ? ? ? ? ?this(switch (0) { default -> throw new Exception(); });
? ? ? ? ? ? ? ? ? ^
Test.java:12: error: cannot reference this before supertype constructor
has been called
? ? ? ? ? ? ?new Local(); ?// Legal or illegal?
? ? ? ? ? ? ?^
Test.java:10: error: calls to this() not allowed here
? ? ? ? ? ? ?this(switch (0) { default -> throw new Exception(); });
? ? ? ? ? ? ? ? ?^
3 errors

Oops, yes. So, can you guarantee, in the JEP, that a non-static local
class declared in the pre-construction context can never access
enclosing state?

Alex

[mlbridge]

Mailing list message from Archie Cobbs on compiler-dev:

Hi Alex,

Thanks for the JEP comments, I will get with Gavin (who is managing the
drafts) on these.

On Mon, Sep 25, 2023 at 1:25?PM Alex Buckley <alex.buckley at oracle.com>
wrote:

Test.java:12: error: cannot reference this before supertype constructor
has been called
new Local(); // Legal or illegal?
^
Test.java:10: error: calls to this() not allowed here
this(switch (0) { default -> throw new Exception(); });
^
3 errors

Oops, yes. So, can you guarantee, in the JEP, that a non-static local
class declared in the pre-construction context can never access
enclosing state?

Reading that literally, no that's not guaranteed at all. You are allowed to
declare a (non-static) local class in a pre-construction context and then
instantiate it later, after super(), and access enclosing state.

What you can't do is instantiate it before super(). That's enforced by this
new line in ?15.9.2:

If the class instance creation expression occurs in the pre-construction
context of class *K* (8.8.7.1
<https://cr.openjdk.org/~gbierman/jep447/jep447-20230905/specs/statements-before-super-jls.html#jls-8.8.7.1>)
and the immediately enclosing instance is also an instance of *K* then a
compile-time error occurs.

Note this language applies to both local classes and member classes.

-Archie

--
Archie L. Cobbs
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/compiler-dev/attachments/20230925/91e37e0b/attachment.htm>

[archiecobbs]

/integrate

[openjdk]

@archiecobbs
Your change (at version bbdb814) is now ready to be sponsored by a Committer.

[vicente-romero-oracle]

/sponsor

[openjdk]

Going to push as commit 12e983a.
Since your change was applied there have been 249 commits pushed to the master branch:

• 5e24aaf: 8320001: javac crashes while adding type annotations to the return type of a constructor
• f9e9131: 8319703: Serial: Remove generationSpec
• a006d7e: 8294549: configure script should detect unsupported path
• 4977922: 8320330: Improve implementation of RShift Value
• a40d8d9: 8314220: Configurable InlineCacheBuffer size
• 1272368: 8318113: CSS.BackgroundImage doesn't implement equals
• 28d3762: 8320618: NPE: Cannot invoke "java.lang.constant.ClassDesc.isArray()" because "this.sym" is null
• f6e5559: 8320601: ProblemList java/lang/invoke/lambda/LambdaFileEncodingSerialization.java on linux-all
• bddcd08: 8304701: Request with timeout aborts later in-flight request on HTTP/1.1 cxn
• 91279fc: 8319778: Remove unreachable code in ObjectSynchronizer::exit
• ... and 239 more: https://git.openjdk.org/jdk/compare/e4803e0cbf00da89b98c8703769edc403bb5055b...master

Your commit was automatically rebased without conflicts.

[openjdk]

@vicente-romero-oracle @archiecobbs Pushed as commit 12e983a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.