8164714: Constructor.newInstance creates instance of inner class with null outer class #23875

liach · 2025-03-03T21:49:55Z

The Java Language Specification anticipates that inner classes always have non-null enclosing instances. It ensures the non-nullness by enforcing null checks at the use sites that provides immediately enclosing instances to inner class constructors, such as for super invocations, or an outer.new Inner() invocation.

However, the translations do not require a null check in the actual constructor, when the immediately enclosing instance is received through a mandated parameter and stored into a synthetic field or discarded. As a result, class file constructs, such as core reflection, method handles, or arbitrary class files can pass in null for the immediately enclosing instance, and later execution may fail with NPE by chance if any enclosing instance is used.

This patch proposes to add a null check against the "outer this" in inner class constructors that call a superclass constructor, including when the "outer this" is discarded immediately thereafter (#4966) for consistency. This null check will be emitted regardless of source or target versions. This change is considered an implementation artifact like the synthetic field that captures the enclosing instance; as a result, there is no JLS change.

The reason for this eager NPE decision is that there is no compatibility of such NPE behaviors - any evolution of the inner classes constructed with null enclosing instances may suddenly start using an enclosing instance and fail with NPE. Therefore, there's no compatibility aspect in such out-of-spec usages of passing null as immediately enclosing instance, and this null check can be considered such an evolution.

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-8351274 to be approved

Issues

JDK-8164714: Constructor.newInstance creates instance of inner class with null outer class (Bug - P4)
JDK-8351274: javac to null check immediately enclosing instance in inner class constructors (CSR)

Reviewers

Vicente Romero (@vicente-romero-oracle - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23875

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

Using diff file

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

Using Webrev

Link to Webrev Comment

… null outer class

bridgekeeper · 2025-03-03T21:50:23Z

👋 Welcome back liach! 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 · 2025-03-03T21:50:35Z

@liach This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8164714: Constructor.newInstance creates instance of inner class with null outer class

Reviewed-by: vromero

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 76 new commits pushed to the master branch:

fa0b18b: 8352509: Update jdk.test.lib.SecurityTools jar method to accept List parameter
3ac9678: 8351224: Deprecate com.sun.tools.attach.AttachPermission for removal
997aa17: 8351310: Deprecate com.sun.jdi.JDIPermission for removal
c953e0e: 8350579: Remove Template Assertion Predicates belonging to a loop once it is folded away
67c4405: 8352866: TestLogJIT.java runs wrong test class
bab9372: 8352618: Remove old deprecated functionality in the build system
512b9b1: 8196896: Use SYSROOT_CFLAGS in dtrace gensrc
721ef76: 8352696: JFR: assert(false): EA: missing memory path
c002b97: 8352676: Opensource JMenu tests - series1
bdcac98: 8347459: C2: missing transformation for chain of shifts/multiplications by constants
... and 66 more: https://git.openjdk.org/jdk/compare/c7f333888be052aa37fe878bfc2785fc47fbeaaa...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-03-03T21:51:09Z

@liach The following label will be automatically applied to this pull request:

compiler

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

mlbridge · 2025-03-05T15:08:31Z

Webrevs

…osing-this-null-check

vicente-romero-oracle · 2025-03-05T15:54:16Z

test/langtools/tools/javap/AnnoTest.java

+ * Copyright (c) 2014, 2025, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it


why changing this test?

CP differences when Objects.requireNonNull is emitted

then we should add the bug id to the test

I noted https://bugs.openjdk.org/browse/JDK-8292275 also changed many tests that this patch has changed but did not add the bug id. Is the bug id only to indicate which bug's regression is a test testing against?

Jtreg supports the -bug: option to run matching tests.

Adding the bugid is an aid in verifying bugs. If it was missed being added earlier it can be added now. But it is unlikely it will be missed by anyone.

I don't think this test is associated to the inner class feature - it is just that the outputs this test is testing against is too closely tied to irrelevant implementation details.

vicente-romero-oracle · 2025-03-05T18:31:50Z

test/langtools/tools/javap/classfile/T6887895.java

                "java/lang/Object",
                "java/lang/String",
                "T6887895",
-                "T6887895$Test"


not sure why this test is changed

Now Objects is used for requireNonNull

we should then add the bug id to the test

vicente-romero-oracle

looks sensible

vicente-romero-oracle · 2025-03-10T16:41:15Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

-    /** Return tree simulating the assignment {@code this.this$n = this$n}.
-     */
-    JCStatement initOuterThis(int pos, VarSymbol rhs) {
+    /** Return tree simulating null checking outer this and assigning. */


probably this comment should state that the assignment is generated or not depending on the value of argument stores

…osing-this-null-check

mcimadamore · 2025-03-21T12:34:01Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java

-                    make.Ident(rhs)).setType(lhs.erasure(types)));
+                    sourceExp).setType(lhs.erasure(types));
+        } else {
+            Assert.check(nullCheckOuterThis);


This code seems repeated twice. Perhaps you could first create the expression (which could be null checked or not) and then use that expression to store in the this$0 field (if needed).

Thanks for the comment. Done.

mcimadamore · 2025-03-21T12:38:10Z

The compiler changes look good - and I think more generally this looks like a good enforcement to do in the generated code (also in perspective, consider what's coming in Valhalla).

liach · 2025-03-25T18:59:25Z

Thanks for the reviews! I will also deliver the release note shortly.

/integrate

openjdk · 2025-03-25T19:01:06Z

Going to push as commit 60544a1.
Since your change was applied there have been 81 commits pushed to the master branch:

c856b34: 8352587: C2 SuperWord: we must avoid Multiversioning for PeelMainPost loops
993eae4: 8346948: Update CLDR to Version 47.0
e98838f: 8352065: [PPC64] C2: Implement PopCountVL, CountLeadingZerosV and CountTrailingZerosV nodes
03105fc: 8351601: [JMH] test UnixSocketChannelReadWrite failed for 2 threads config
fe03e2e: 8351897: Extra closing curly brace typos in Javadoc
fa0b18b: 8352509: Update jdk.test.lib.SecurityTools jar method to accept List parameter
3ac9678: 8351224: Deprecate com.sun.tools.attach.AttachPermission for removal
997aa17: 8351310: Deprecate com.sun.jdi.JDIPermission for removal
c953e0e: 8350579: Remove Template Assertion Predicates belonging to a loop once it is folded away
67c4405: 8352866: TestLogJIT.java runs wrong test class
... and 71 more: https://git.openjdk.org/jdk/compare/c7f333888be052aa37fe878bfc2785fc47fbeaaa...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-25T19:01:16Z

@liach Pushed as commit 60544a1.

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

8164714: Constructor.newInstance creates instance of inner class with…

3ba5e71

… null outer class

openjdk bot added the compiler compiler-dev@openjdk.org label Mar 3, 2025

Fix javap test

3778a3c

openjdk bot added the csr Pull request needs approved CSR before integration label Mar 5, 2025

liach marked this pull request as ready for review March 5, 2025 15:04

openjdk bot added the rfr Pull request is ready for review label Mar 5, 2025

Merge branch 'master' of https://github.com/openjdk/jdk into fix/encl…

9251340

…osing-this-null-check

vicente-romero-oracle reviewed Mar 5, 2025

View reviewed changes

vicente-romero-oracle approved these changes Mar 10, 2025

View reviewed changes

liach added 4 commits March 18, 2025 10:25

Merge branch 'master' of https://github.com/openjdk/jdk into fix/encl…

3954341

…osing-this-null-check

Emit null check by flag/release

682bd6f

Merge branch 'master' of https://github.com/openjdk/jdk into fix/encl…

76ff686

…osing-this-null-check

Better docs

6729faa

vicente-romero-oracle approved these changes Mar 19, 2025

View reviewed changes

mcimadamore reviewed Mar 21, 2025

View reviewed changes

Reduce code repetition

d2e10cd

openjdk bot removed the csr Pull request needs approved CSR before integration label Mar 25, 2025

vicente-romero-oracle approved these changes Mar 25, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 25, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 25, 2025

openjdk bot closed this Mar 25, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 25, 2025

8164714: Constructor.newInstance creates instance of inner class with null outer class #23875

8164714: Constructor.newInstance creates instance of inner class with null outer class #23875

Uh oh!

Conversation

liach commented Mar 3, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 3, 2025

Uh oh!

openjdk bot commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 3, 2025

Uh oh!

mlbridge bot commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcimadamore commented Mar 21, 2025

Uh oh!

liach commented Mar 25, 2025

Uh oh!

openjdk bot commented Mar 25, 2025

Uh oh!

openjdk bot commented Mar 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

liach commented Mar 3, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 3, 2025 •

edited

Loading

mlbridge bot commented Mar 5, 2025 •

edited

Loading