RFR: 8268312: Compilation error with nested generic functional interface [v4]

[Maurizio Cimadamore]

On Wed, 6 Jul 2022 02:53:36 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> 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
>
> Vicente Romero has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
> 
>  - Merge master
>  - addressing another review iteration
>  - addressing review comments
>  - 8268312: Compilation error with nested generic functional interface

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.

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

> 4179:             return diags.create(dkind, log.currentSource(), pos,
> 4180:                       "cant.apply.symbol",
> 4181:                       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.

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

> 218:      * Set to true if a compressed diagnostic is reported
> 219:      */
> 220:     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).

-------------

PR: https://git.openjdk.org/jdk/pull/5586