8268312: Compilation error with nested generic functional interface #5586

vicente-romero-oracle · 2021-09-20T19:00:29Z

Please review this PR, which is my proposal to fix an existing regression. This code:

import java.util.Optional;

class App {
    public static void main(String[] args) {
        Optional.of("").map(outer -> {
            Optional.of("")
                .map(inner -> returnGeneric(outer))
                .ifPresent(String::toString);
            return "";
        });
    }

    private static <RG> RG returnGeneric(RG generic) {
        return generic;
    }
}

is not accepted by javac but if the user passes the -Xdiags:verbose option then the code compiles. I tracked down the reason for this puzzling difference and I found that it is due to our diagnostic rewriters which can generate more detailed positions for error messages but in cases like the one above can trick the compiler to generate an error message too early. The code deciding if an error message should be deferred or not, depending on the position, is at DeferredDiagnosticHandler::report. We decide to do the rewriting if we are in diagnostics compact mode, this is why the error doesn't occur with the -Xdiags:verbose option. This fix will made some diagnostics to appear at a slightly different position, but won't make the compiler reject correct code. Comments?

TIA

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

Issue

JDK-8268312: Compilation error with nested generic functional interface

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer) ⚠️ Review applies to a00c8dab

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5586

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

Using diff file

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

bridgekeeper · 2021-09-20T19:01:08Z

👋 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 · 2021-09-20T19:06:05Z

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

openjdk · 2021-09-20T19:06:07Z

@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 · 2021-09-20T19:12:10Z

Webrevs

vicente-romero-oracle · 2021-09-28T00:52:04Z

ping

mcimadamore

Very tricky issue. Basically, the deferred diagnostic handler checks whether the diagnostic is associated with the tree being speculatively attributed. This is required, because sometimes, when attributing a piece of code, we could have completion events, causing the compiler to jump from one file to another - it is important that completion related issues are logged, even if we're inside deferred attribution, which is why the logic is there.

The resolution diagnostic rewrite throws a spanner in the work though - because it can sometime update the diagnostic position to make it appear like the error was on an outermost node, which then will fall outside the tree covered by the deferred diagnostic handler, and result in spurious errors being generated.

I think a more robust solution would be to leave the diagnostic as is (since the diagnostic handler in DeferredAttr depends on it), but at the same time, maybe record something on the JCDiagnostic object - e.g. like a Supplier which allows the diagnostic to be simplified later, by Log, rather than eagerly, by Resolve. It is possible that, if we do this, we might no longer require a COMPRESSED flag on the diagnostic at all - after all, if Log is in charge of the rewriting, it knows which diagnostics have been touched.

mcimadamore · 2021-09-29T14:00:23Z

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

+                final DiagnosticPosition pos = d.getDiagnosticPosition();
+                UnaryOperator<JCDiagnostic> rewriter = pos != null ?
+                        diag -> diags.create(preferredKind, preferredSource, pos, "prob.found.req", cause) :
+                        null;
                return diags.create(preferredKind, preferredSource, preferredPos,


I guess here we should return the diagnostic unmodified? E.g. you are still returning a rewritten diagnostic here, the only thing that changes is the position (which is what avoids the bug). What I'm hoping is that we can obtain a new diagnostic (with a rewriter) from an existing diagnostic. See my other comment.

mcimadamore · 2021-09-29T14:04:12Z

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

@@ -4135,8 +4136,7 @@ JCDiagnostic getDiagnostic(JCDiagnostic.DiagnosticType dkind,

            Pair<Symbol, JCDiagnostic> c = errCandidate();
            if (compactMethodDiags) {


I think what 'm really after here is to drop this branch - e.g. always create the "non compressed" diagnostic (e.g. cannot.apply) with the right position.

But, before returning that, attach a rewriter to it e.g.

Symbol ws = c.fst.asMemberOf(site, types); return diags.create(dkind, log.currentSource(), pos, "cant.apply.symbol", kindName(ws), ws.name == names.init ? ws.owner.name : ws.name, methodArguments(ws.type.getParameterTypes()), methodArguments(argtypes), kindName(ws.owner), ws.owner.type, c.snd).withRewriter(() -> MethodResolutionDiagHelper.rewrite(diags, pos, log.currentSource(), dkind, c.snd));

mcimadamore · 2021-09-29T14:05:35Z

src/jdk.compiler/share/classes/com/sun/tools/javac/main/JavaCompiler.java

@@ -1713,9 +1713,6 @@ public void reportDeferredDiagnostics() {
        }
        chk.reportDeferredDiagnostics();
        preview.reportDeferredDiagnostics();
-        if (log.compressedOutput) {


This can be kept, no?

mcimadamore · 2021-09-29T21:18:27Z

src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java

@@ -707,9 +705,6 @@ public void report(JCDiagnostic diagnostic) {
                }
                break;
            }
-            if (diagnostic.isFlagSet(JCDiagnostic.DiagnosticFlag.COMPRESSED)) {


Who sets this now?

bridgekeeper · 2021-10-28T04:06:20Z

@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 · 2021-11-25T10:29:19Z

@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-01-05T18:56:09Z

/open

openjdk · 2022-01-05T18:56:48Z

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

bridgekeeper · 2022-02-02T20:44:45Z

@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-03-03T01:38:50Z

@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-03-23T16:22:00Z

/open

openjdk · 2022-03-23T16:22:23Z

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

vicente-romero-oracle · 2022-03-23T16:24:15Z

/open

openjdk · 2022-03-23T16:25:05Z

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

vicente-romero-oracle · 2022-03-23T16:25:31Z

ping

bridgekeeper · 2022-04-20T20:52:16Z

@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-05-19T01:45:42Z

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

openjdk · 2022-07-06T02:11:18Z

@vicente-romero-oracle this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8268312
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

vicente-romero-oracle · 2022-07-06T02:50:28Z

another stab at this one

mcimadamore

A good step forward. I think the removal of the flag to optionally leave diagnostics uncompressed caused more changes than required - there's several changes to the raw diagnostic output that I was not expecting; ideally this patch should not change the behavior of compiler diagnostic, but should just make the current rewriting machinery work better with the speculative attribution machinery.

mcimadamore · 2022-07-07T09:56:58Z

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

            Symbol ws = c.fst.asMemberOf(site, types);
            return diags.create(dkind, log.currentSource(), pos,
                      "cant.apply.symbol",
+                      d -> MethodResolutionDiagHelper.rewrite(diags, pos, log.currentSource(), dkind, c.snd),


This is a good step in the right direction - e.g. attaching a rewriting function to the diagnostic object itself. I wonder if a simpler solution is possible, as I note here that the call to rewrite is basically just passing info that are attached to the diagnostic being created plus c.snd which is the real new bit of info.

E.g. in principle, we could just create the new diagnostic with a "cause" (e.g. c.snd) and then, when we're about to report the diagnostic, we check if there's a cause set and, if so, we try to late-rewrite the diagnostic. If we do this, perhaps we might be able to avoid the creation of a new instance in a very hot path.

I would also check for other usages of the rewriter framework and make sure they follow the same pattern - right now we have some diagnostics using the late-bound reporting mechanism (such as this), and others (e.g. diamond, but possibly other inference ones as well) which rewrite the diagnostic eagerly. We should try to have only one way to do things. My feeling is that diagnostic rewriting should be a general mechanism, not something that belongs in Resolve (but perhaps closer to Log).

mcimadamore · 2022-07-07T10:02:23Z

src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java

-    /**
-     * Set to true if a compressed diagnostic is reported
-     */
-    public boolean compressedOutput;


I see that this patch drops the option for compressing diagnostic - this seems to go a step too far, and I'm not sure this is what we want, at least in this patch. I believe there's an orthogonal discussion to be had there. I think the flag should stay as is for now (and control whether diagnostics should be rewritten at all in Log).

mcimadamore

Looks good - there seems to be some change in functionality - in the sense that the raw diagnostics now show the note re. compressed diagnostic, while before that did not happen. It would be good to understand why that happens.

mcimadamore · 2022-07-11T10:41:12Z

test/langtools/tools/javac/7132880/T7132880.out

@@ -1,4 +1,5 @@
 T7132880.java:23:15: compiler.err.prob.found.req: (compiler.misc.inconvertible.types: java.lang.String, java.lang.Integer)
 T7132880.java:33:12: compiler.err.cant.apply.symbols: kindname.method, m1, java.lang.String,{(compiler.misc.inapplicable.method: kindname.method, Outer.Inner2, m1(java.lang.Integer), (compiler.misc.no.conforming.assignment.exists: (compiler.misc.inconvertible.types: java.lang.String, java.lang.Integer))),(compiler.misc.inapplicable.method: kindname.method, Outer.Inner2, m1(java.lang.Double), (compiler.misc.no.conforming.assignment.exists: (compiler.misc.inconvertible.types: java.lang.String, java.lang.Double)))}
 T7132880.java:43:12: compiler.err.ref.ambiguous: m2, kindname.method, m2(java.lang.Object,int), Outer.Inner3, kindname.method, m2(int,java.lang.Object), Outer.Inner3
+- compiler.note.compressed.diags


Why are these showing up now?

mcimadamore · 2022-07-11T10:49:37Z

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

@@ -4174,16 +4175,10 @@ JCDiagnostic getDiagnostic(JCDiagnostic.DiagnosticType dkind,
                return null;

            Pair<Symbol, JCDiagnostic> c = errCandidate();
-            if (compactMethodDiags) {


I believe the issue is here - the old code made the rewriting dependent on the value of compactMethodDiags, which in turns looked at which diagnostic formatter was installed. I think we should either move this check in Log (when we decide if we need to rewrite), or don't attach a rewriter if the compactMethodDiags is not set. I think it would be preferable to keep existing diagnostics as they are (and maybe cleanup the behavior of compactMethodDiags later, as an orthogonal fix).

yep I think this is the reason, will upload an updated iteration

mcimadamore · 2022-07-11T21:20:51Z

test/langtools/tools/javac/diags/examples/ProbFoundReqFragment.java

@@ -22,7 +22,9 @@
 */

 // key: compiler.err.prob.found.req
-// key: compiler.misc.prob.found.req
+// key: compiler.misc.kindname.class


Uhm - this sample is meant to test compiler.misc.prob.found.req (e.g. the fragment version). Perhaps we might need to look at the test framework that runs these diagnostic test to understand what we have to change - or what has changed - for this to work as before?

mcimadamore · 2022-07-11T21:21:28Z

test/langtools/tools/javac/Diagnostics/compressed/T8012003b.out

@@ -1,7 +1,7 @@
 T8012003b.java:30:12: compiler.err.prob.found.req: (compiler.misc.invalid.mref: kindname.method, (compiler.misc.cant.apply.symbol: kindname.method, g, java.lang.String, compiler.misc.no.args, kindname.class, T8012003b, (compiler.misc.arg.length.mismatch)))
 T8012003b.java:31:16: compiler.err.prob.found.req: (compiler.misc.stat.expr.expected)
 T8012003b.java:32:22: compiler.err.prob.found.req: (compiler.misc.incompatible.ret.type.in.lambda: (compiler.misc.conditional.target.cant.be.void))
-T8012003b.java:33:12: compiler.err.prob.found.req: (compiler.misc.invalid.mref: kindname.method, (compiler.misc.prob.found.req: (compiler.misc.inconvertible.types: java.lang.Integer, java.lang.String)))
+T8012003b.java:33:12: compiler.err.prob.found.req: (compiler.misc.invalid.mref: kindname.method, (compiler.misc.cant.apply.symbol: kindname.method, g, java.lang.String, java.lang.Integer, kindname.class, T8012003b, (compiler.misc.no.conforming.assignment.exists: (compiler.misc.inconvertible.types: java.lang.Integer, java.lang.String))))


Seems like this diagnostic is not getting compressed?

yep good catch, checking what's going on

uploaded another iteration as discussed offline, thanks

mcimadamore

Looks good!

src/jdk.compiler/share/classes/com/sun/tools/javac/util/AbstractDiagnosticFormatter.java

test/langtools/tools/javac/diags/DiagnosticRewriterTest3.java

openjdk · 2022-07-14T21:07:56Z

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

8268312: Compilation error with nested generic functional interface

Reviewed-by: mcimadamore

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

757a742: 8290177: Improve documentation in G1MMUTracker
890bced: 8290264: java/util/concurrent/locks/Lock/OOMEInAQS.java fails with "exit code: 0"
3ad3950: Merge
fd89ab8: 8288112: C2: Error: ShouldNotReachHere() in Type::typerr()
2bf6285: 8290209: jcup.md missing additional text
73b83e0: 8290207: Missing notice in dom.md
3bb2dc8: 8290290: Remove addition of TimeInstants
c7c2066: 8290221: G1: Merge multiple calls of get_next_marked_addr in HeapRegion::oops_on_memregion_iterate_in_unparsable
be58cbc: 8290269: gc/shenandoah/TestVerifyJCStress.java fails due to invalid tag: required after JDK-8290023
109e21a: 8290080: G1: Remove unnecessary is-obj-dead check in HeapRegion::do_oops_on_memregion_in_humongous
... and 142 more: https://git.openjdk.org/jdk/compare/351560414d7ddc0694126ab184bdb78be604e51f...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.

Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>

…ctDiagnosticFormatter.java Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>

vicente-romero-oracle · 2022-07-15T13:04:55Z

/integrate

openjdk · 2022-07-15T13:06:20Z

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

92deab5: 8028265: Add legacy tz tests to OpenJDK
70fce07: 8290246: test fails "assert(init != __null) failed: initialization not found"
757a742: 8290177: Improve documentation in G1MMUTracker
890bced: 8290264: java/util/concurrent/locks/Lock/OOMEInAQS.java fails with "exit code: 0"
3ad3950: Merge
fd89ab8: 8288112: C2: Error: ShouldNotReachHere() in Type::typerr()
2bf6285: 8290209: jcup.md missing additional text
73b83e0: 8290207: Missing notice in dom.md
3bb2dc8: 8290290: Remove addition of TimeInstants
c7c2066: 8290221: G1: Merge multiple calls of get_next_marked_addr in HeapRegion::oops_on_memregion_iterate_in_unparsable
... and 144 more: https://git.openjdk.org/jdk/compare/351560414d7ddc0694126ab184bdb78be604e51f...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-07-15T13:06:37Z

@vicente-romero-oracle Pushed as commit f3abb82.

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

8268312: Compilation error with nested generic functional interface

6085743

openjdk bot added the rfr Pull request is ready for review label Sep 20, 2021

openjdk bot added the compiler compiler-dev@openjdk.org label Sep 20, 2021

mcimadamore reviewed Sep 28, 2021

View reviewed changes

addressing review comments

95a0839

mcimadamore reviewed Sep 29, 2021

View reviewed changes

bridgekeeper bot closed this Nov 25, 2021

openjdk bot reopened this Jan 5, 2022

bridgekeeper bot closed this Mar 3, 2022

openjdk bot reopened this Mar 23, 2022

bridgekeeper bot closed this May 19, 2022

addressing another review iteration

0b38264

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed rfr Pull request is ready for review labels Jul 6, 2022

Merge master

bf71517

openjdk bot added rfr Pull request is ready for review and removed merge-conflict Pull request has merge conflict with target branch labels Jul 6, 2022

mcimadamore reviewed Jul 7, 2022

View reviewed changes

restoring the option for compressing diagnostics

5380d4c

mcimadamore reviewed Jul 11, 2022

View reviewed changes

vicente-romero-oracle added 2 commits July 11, 2022 13:59

another iteration, addressing review comments

6ccba3d

some missed changes

14f6e83

mcimadamore reviewed Jul 11, 2022

View reviewed changes

vicente-romero-oracle added 2 commits July 14, 2022 01:00

another iteration: addressing review comments

6c3fd35

removing copyright year, no change to the file

a00c8da

mcimadamore approved these changes Jul 14, 2022

View reviewed changes

src/jdk.compiler/share/classes/com/sun/tools/javac/util/AbstractDiagnosticFormatter.java Outdated Show resolved Hide resolved

test/langtools/tools/javac/diags/DiagnosticRewriterTest3.java Outdated Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Jul 14, 2022

vicente-romero-oracle and others added 2 commits July 14, 2022 18:43

Update test/langtools/tools/javac/diags/DiagnosticRewriterTest3.java

e7fa8dc

Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>

Update src/jdk.compiler/share/classes/com/sun/tools/javac/util/Abstra…

a913b30

…ctDiagnosticFormatter.java Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>

openjdk bot added the integrated Pull request has been integrated label Jul 15, 2022

openjdk bot closed this Jul 15, 2022

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

vicente-romero-oracle deleted the JDK-8268312 branch July 15, 2022 13:06

		@@ -4135,8 +4136,7 @@ JCDiagnostic getDiagnostic(JCDiagnostic.DiagnosticType dkind,

		Pair<Symbol, JCDiagnostic> c = errCandidate();
		if (compactMethodDiags) {

8268312: Compilation error with nested generic functional interface #5586

8268312: Compilation error with nested generic functional interface #5586

Uh oh!

Conversation

vicente-romero-oracle commented Sep 20, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Sep 20, 2021

Uh oh!

openjdk bot commented Sep 20, 2021

Uh oh!

openjdk bot commented Sep 20, 2021

Uh oh!

mlbridge bot commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

vicente-romero-oracle commented Sep 28, 2021

Uh oh!

mcimadamore 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!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Oct 28, 2021

Uh oh!

bridgekeeper bot commented Nov 25, 2021

Uh oh!

vicente-romero-oracle commented Jan 5, 2022

Uh oh!

openjdk bot commented Jan 5, 2022

Uh oh!

bridgekeeper bot commented Feb 2, 2022

Uh oh!

bridgekeeper bot commented Mar 3, 2022

Uh oh!

vicente-romero-oracle commented Mar 23, 2022

Uh oh!

openjdk bot commented Mar 23, 2022

Uh oh!

vicente-romero-oracle commented Mar 23, 2022

Uh oh!

openjdk bot commented Mar 23, 2022

Uh oh!

vicente-romero-oracle commented Mar 23, 2022

Uh oh!

bridgekeeper bot commented Apr 20, 2022

Uh oh!

bridgekeeper bot commented May 19, 2022

Uh oh!

openjdk bot commented Jul 6, 2022

Uh oh!

vicente-romero-oracle commented Jul 6, 2022

Uh oh!

mcimadamore 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 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!

vicente-romero-oracle commented Sep 20, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 20, 2021 •

edited

Loading

openjdk bot commented Jul 14, 2022 •

edited

Loading