8029633: Raw inner class constructor ref should not perform diamond inference #9784

vicente-romero-oracle · 2022-08-05T20:20:14Z

This PR is synchronizing the compiler with the spec, in particular with this portion of section 15.13.1 Compile-Time Declaration of a Method Reference:

– If the method reference expression has the form ClassType ::
  [TypeArguments] new , then the potentially applicable methods are a set of
  notional methods corresponding to the constructors of ClassType.
  If ClassType is a raw type, but is not a non- static member type of a raw type,
  the candidate notional member methods are those specified in §15.9.3 for a
  class instance creation expression that uses <> to elide the type arguments to a
  class. Otherwise, the candidate notional member methods are the constructors
  of ClassType, treated as if they were methods with return type ClassType.

so javac should treat the code below as a raw constructor invocation:

class Outer<T> {
    class Inner<U> {}

    Supplier<Outer<T>.Inner<String>> s = Outer.Inner::new;
}

currently javac is rejecting this code

TIA

PS. Please review the related CSR too

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 a CSR request to be approved

Issues

JDK-8029633: Raw inner class constructor ref should not perform diamond inference
JDK-8292052: Raw inner class constructor ref should not perform diamond inference (CSR)

Reviewers

Jan Lahoda (@lahodaj - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9784

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

Using diff file

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

…nference

bridgekeeper · 2022-08-05T20:21:25Z

👋 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 · 2022-08-05T20:24:23Z

@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.

vicente-romero-oracle · 2022-08-05T20:25:46Z

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

@@ -3686,7 +3686,8 @@ class ConstructorReferenceLookupHelper extends ReferenceLookupHelper {
                List<Type> typeargtypes, MethodResolutionPhase maxPhase) {
            super(referenceTree, names.init, site, argtypes, typeargtypes, maxPhase);
            if (site.isRaw()) {
-                this.site = new ClassType(site.getEnclosingType(), site.tsym.type.getTypeArguments(), site.tsym, site.getMetadata());
+                this.site = new ClassType(site.getEnclosingType(),
+                        !site.tsym.isInner() ? site.tsym.type.getTypeArguments() : List.nil(), site.tsym, site.getMetadata());
                needsInference = true;


Note: so here it seems like we are still doing diamond inference but check that we are creating a new class type with no arguments so no inference will be done in this case (site is an inner class), but we are still setting needsInference to true in order to reuse the code in Resolve::findDiamond that creates a new MethodSymbol for the constructor with the right result type instead of void

mlbridge · 2022-08-05T20:31:34Z

Webrevs

bridgekeeper · 2022-09-02T22:52:07Z

@vicente-romero-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2022-10-01T01:34:36Z

@vicente-romero-oracle This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

vicente-romero-oracle · 2022-10-17T17:47:03Z

/open

openjdk · 2022-10-17T17:47:20Z

@vicente-romero-oracle This pull request is now open

lahodaj · 2022-10-18T07:42:59Z

Just to verify I understand it correctly, having something along the lines:
... = Outer.Inner::new
(where Inner is a parameterized type) based on some conditions, we either use diamond inference, and will produce a parameterized type as the type of the Outer.Inner::new expression; or we will produce a raw type as the type of the expression. Is that correct?

I think this patch is in the right direction, but I am not sure about this case:

class Outer2 {
    class Inner1 {}
    class Inner2<U> {}

    Supplier<Outer2.Inner2<String>> s4 = Outer2.Inner2::new;
}

Note that here: Outer2.Inner2 is a raw type, is a non-static member type, but the enclosing type is not raw, and hence, based on the conditions:

If ClassType is a raw type, but is not a non- static member type of a raw type,

it seems it should get the diamond inference here? Should the new condition be !site.tsym.isInner() || !site.getEnclosingType().isRaw() instead of !site.tsym.isInner(), or possibly (to mimic the spec) something like !(site.tsym.isInner() && site.getEnclosingType().isRaw())?

(The difference is observable as an unchecked warning.)

Thanks.

vicente-romero-oracle · 2022-10-18T19:12:33Z

@lahodaj I think you are right. Good catch! I have updated the PR, thanks!

lahodaj

Looks good to me, thanks!

vicente-romero-oracle · 2022-10-18T21:49:56Z

Looks good to me, thanks!

thanks for the review

openjdk · 2022-10-19T17:17:47Z

@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:

8029633: Raw inner class constructor ref should not perform diamond inference

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

7b1c676: 8295662: jdk/incubator/vector tests fail "assert(VM_Version::supports_avx512vlbw()) failed"
5eaf568: 8295668: validate-source failure after JDK-8290011
e238920: 8295372: CompactNumberFormat handling of number one with decimal part
a5f6e31: 8295456: (ch) sun.nio.ch.Util::checkBufferPositionAligned gives misleading/incorrect error
e27bea0: 8290011: IGV: Remove dead code and cleanup
d37ce4c: 8290368: Introduce LDAP and RMI protocol-specific object factory filters to JNDI implementation
21aeb9e: 8295429: Update harfbuzz md file
1d883c5: 8295417: Pass $AR to binutils cross-build
fc88957: 8286707: JFR: Don't commit JFR internal jdk.JavaMonitorWait events
857b0f9: 8293409: [vectorapi] Intrinsify VectorSupport.indexVector
... and 17 more: https://git.openjdk.org/jdk/compare/0233ba763d84e6da8ec03df5d021a13c5fbbc871...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.

vicente-romero-oracle · 2022-10-20T15:41:28Z

/integrate

openjdk · 2022-10-20T15:43:25Z

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

7bc9692: 8294670: Enhanced switch statements have an implicit default which does not complete normally
95dd376: 8291914: generated constructors are considered compact when they shouldn't
9b97162: 7039014: Confusing error message for method conflict
78dc497: 8294550: Sealed check for casts isn't applied to array components
c08ff2c: 8294705: Disable an assertion in test/jdk/java/util/DoubleStreamSums/CompensatedSums.java
d5a1521: 8295470: Update openjdk.java.net => openjdk.org URLs in test code
9d0cfd1: 8294948: Document IllegalArgumentException and NullPointerException thrown by URLStreamHandler::parseURL and URLStreamHandler::setURL
dcd4650: 8294916: Cancelling a request must eventually cause its response body subscriber to be unregistered
4f994c0: 8295709: Linux AArch64 builds broken after JDK-8294438
545021b: 8294438: Fix misleading-indentation warnings in hotspot
... and 33 more: https://git.openjdk.org/jdk/compare/0233ba763d84e6da8ec03df5d021a13c5fbbc871...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-10-20T15:43:44Z

@vicente-romero-oracle Pushed as commit 6707bfb.

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

8029633: Raw inner class constructor ref should not perform diamond i…

6c87afe

…nference

openjdk bot added the rfr Pull request is ready for review label Aug 5, 2022

openjdk bot added the compiler compiler-dev@openjdk.org label Aug 5, 2022

vicente-romero-oracle commented Aug 5, 2022

View reviewed changes

openjdk bot added the csr Pull request needs approved CSR before integration label Aug 8, 2022

bridgekeeper bot closed this Oct 1, 2022

openjdk bot reopened this Oct 17, 2022

vicente-romero-oracle added 2 commits October 18, 2022 14:20

Merge branch 'master' into JDK-8029633

d886743

addressing review comments

80b90ac

lahodaj approved these changes Oct 18, 2022

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Oct 19, 2022

openjdk bot added the ready Pull request is ready to be integrated label Oct 19, 2022

openjdk bot added the integrated Pull request has been integrated label Oct 20, 2022

openjdk bot closed this Oct 20, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 20, 2022

8029633: Raw inner class constructor ref should not perform diamond inference #9784

8029633: Raw inner class constructor ref should not perform diamond inference #9784

Uh oh!

Conversation

vicente-romero-oracle commented Aug 5, 2022 • 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 Aug 5, 2022

Uh oh!

openjdk bot commented Aug 5, 2022

Uh oh!

vicente-romero-oracle Aug 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

bridgekeeper bot commented Sep 2, 2022

Uh oh!

bridgekeeper bot commented Oct 1, 2022

Uh oh!

vicente-romero-oracle commented Oct 17, 2022

Uh oh!

openjdk bot commented Oct 17, 2022

Uh oh!

lahodaj commented Oct 18, 2022

Uh oh!

vicente-romero-oracle commented Oct 18, 2022

Uh oh!

lahodaj left a comment

Choose a reason for hiding this comment

Uh oh!

vicente-romero-oracle commented Oct 18, 2022

Uh oh!

openjdk bot commented Oct 19, 2022

Uh oh!

vicente-romero-oracle commented Oct 20, 2022

Uh oh!

openjdk bot commented Oct 20, 2022

Uh oh!

openjdk bot commented Oct 20, 2022

Uh oh!

Uh oh!

vicente-romero-oracle commented Aug 5, 2022 •

edited by openjdk bot

Loading

vicente-romero-oracle Aug 5, 2022 •

edited

Loading

mlbridge bot commented Aug 5, 2022 •

edited

Loading