8225559: assertion error at TransTypes.visitApply

[vicente-romero-oracle]

Please review this PR that is fixing a bug in javac. Depending on variations of the input the compiler fails with an assertion error or with the infamous NPE. So for code like:

src/pkg/Bar.java:

package pkg;
public abstract class Bar<T> {
    protected Bar() {}
    protected Bar(Class<?> c) {}
}

src/Test.java:

import pkg.Bar;
import java.util.function.Supplier;

class Test {
    public void foo() {
        supply(getSupplier(new Bar<>(){}));
    }

    static <U> Supplier<U> getSupplier(Bar<U> t) {
        return null;
    }

    static <U> void supply(Supplier<U> supplier) {}
}

see that the new class expression is defining an anonymous class which is invoking a protected constructor in the super class, all of this should just work, and it works if the user doesn't use diamond and instead the type parameters are provided explicitly. So what happens with the diamond?

The reason is deep into the speculative attribution machinery, oh boy.... So when we are dealing with diamond expression that define anonymous classes, the speculative attribution will at some point clone that expression but it will nuke its class definition basically in the expression: new Bar<>(){} the {} will be removed.

so we have:
new Bar<>(){} (Original)
new Bar<>() (Copy1)

but in order for the rest of the code to know that the original expression was actually defining an anonymous class, the compiler does a trick. Which can be seen at: com.sun.tools.javac.tree.TreeMaker::SpeculativeNewClass basically an anonymous class which overrides method JCTree.JCNewClass::classDeclRemoved is created. But the bug presented itself for cases when the speculative attribution needs to clone Copy1 and now it doesn't have the {}, the class definition, for the cloning code to know that that expression was copied from Original which was defining an anonymous class. So we need to double check by making use of method JCTree.JCNewClass::classDeclRemoved which is what this fix is doing.

Also it is important to know if the new class expression was defining an anonymous inner class or not because that makes the difference in case the new class expression is accessing a protected constructor as in this case.

TIA

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8225559: assertion error at TransTypes.visitApply

Reviewers

• Srikanth Adayapalam (@sadayapalam - Reviewer) ⚠️ Review applies to abe51ca
• Jan Lahoda (@lahodaj - Reviewer) ⚠️ Review applies to abe51ca

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4647/head:pull/4647
$ git checkout pull/4647

Update a local copy of the PR:
$ git checkout pull/4647
$ git pull https://git.openjdk.java.net/jdk pull/4647/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4647

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4647.diff

[bridgekeeper]

👋 Welcome back vromero! 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]

@vicente-romero-oracle 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]

Webrevs

• 01: Full - Incremental (1f859463)
• 00: Full (abe51ca3)

[sadayapalam]

Looks reasonable,

+1 assuming sanity check done against original test case (and not just the simplified stripped down test case)

[openjdk]

@vicente-romero-oracle 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:

8225559: assertion error at TransTypes.visitApply

Reviewed-by: sadayapalam, jlahoda

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 47 new commits pushed to the master branch:

• 82bfc5d: 8268960: com/sun/net/httpserver/Headers.java: Ensure mutators normalize keys and disallow null for keys and values
• 18f356a: 8267307: Introduce new client property for XAWT: xawt.mwm_decor_title
• 54a9c3e: 8133873: Simplify {Register,Unregister}NMethodOopClosure
• 06d2620: 8268298: jdk/jfr/api/consumer/log/TestVerbosity.java fails: unexpected log message
• d89e630: 8266746: C1: Replace UnsafeGetRaw with UnsafeGet when setting up OSR entry block
• 4660f72: 8268870: Remove dead code in metaspaceShared
• 9def3b0: Merge
• 9ac63a6: 8262841: Clarify the behavior of PhantomReference::refersTo
• aba6c55: 8269703: ProblemList vmTestbase/nsk/jvmti/scenarios/sampling/SP07/sp07t002/TestDescription.java on Windows-X64 with -Xcomp
• 3e02224: 8269513: Clarify the spec wrt useOldISOCodes system property
• ... and 37 more: https://git.openjdk.java.net/jdk/compare/e238cbd596d29d10e45e4bad7a10708c4022097f...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.

[lahodaj]

lgtm

[vicente-romero-oracle]

Looks reasonable,

+1 assuming sanity check done against original test case (and not just the simplified stripped down test case)

yes that one pass too, but I will add it to the regression test

[vicente-romero-oracle]

/integrate

[openjdk]

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

• 82bfc5d: 8268960: com/sun/net/httpserver/Headers.java: Ensure mutators normalize keys and disallow null for keys and values
• 18f356a: 8267307: Introduce new client property for XAWT: xawt.mwm_decor_title
• 54a9c3e: 8133873: Simplify {Register,Unregister}NMethodOopClosure
• 06d2620: 8268298: jdk/jfr/api/consumer/log/TestVerbosity.java fails: unexpected log message
• d89e630: 8266746: C1: Replace UnsafeGetRaw with UnsafeGet when setting up OSR entry block
• 4660f72: 8268870: Remove dead code in metaspaceShared
• 9def3b0: Merge
• 9ac63a6: 8262841: Clarify the behavior of PhantomReference::refersTo
• aba6c55: 8269703: ProblemList vmTestbase/nsk/jvmti/scenarios/sampling/SP07/sp07t002/TestDescription.java on Windows-X64 with -Xcomp
• 3e02224: 8269513: Clarify the spec wrt useOldISOCodes system property
• ... and 37 more: https://git.openjdk.java.net/jdk/compare/e238cbd596d29d10e45e4bad7a10708c4022097f...master

Your commit was automatically rebased without conflicts.

[openjdk]

@vicente-romero-oracle Pushed as commit de61328.

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