8258897: wrong translation of capturing local classes inside nested lambdas #1479

hltj · 2020-11-27T12:32:21Z

Issue:
javac crashes with a NPE when compiling such code:

a local class captured a reference type local variable of a enclosed lambda (which contains multiple local variables with different types)
instance the local class in a nested lambda

Affects all versions from Java 8 to 16-ea, here is a example output with OpenJDK 15.0.1:

An exception has occurred in the compiler (15.0.1). Please file a bug against the Java compiler via the Java bug reporting page (http://bugreport.java.com) after checking the Bug Database (http://bugs.java.com) for duplicates. Include your program, the following diagnostic, and the parameters passed to the Java compiler in your report. Thank you.
java.lang.NullPointerException: Cannot read field "sym" because "this.lvar[1]" is null
    at jdk.compiler/com.sun.tools.javac.jvm.Code.emitop0(Code.java:571)
    at jdk.compiler/com.sun.tools.javac.jvm.Items$LocalItem.load(Items.java:399)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.genArgs(Gen.java:889)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.visitNewClass(Gen.java:1942)
    at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCNewClass.accept(JCTree.java:1800)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.genExpr(Gen.java:864)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.visitExec(Gen.java:1723)
    at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCExpressionStatement.accept(JCTree.java:1532)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.genDef(Gen.java:597)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.genStat(Gen.java:632)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.genStat(Gen.java:618)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.genStats(Gen.java:669)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.visitBlock(Gen.java:1084)
    at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCBlock.accept(JCTree.java:1047)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.genDef(Gen.java:597)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.genStat(Gen.java:632)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.genMethod(Gen.java:954)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.visitMethodDef(Gen.java:917)
    at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCMethodDecl.accept(JCTree.java:893)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.genDef(Gen.java:597)
    at jdk.compiler/com.sun.tools.javac.jvm.Gen.genClass(Gen.java:2395)
    at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.genCode(JavaCompiler.java:756)
    at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.generate(JavaCompiler.java:1644)
    at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.generate(JavaCompiler.java:1612)
    at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:973)
    at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:317)
    at jdk.compiler/com.sun.tools.javac.main.Main.compile(Main.java:176)
    at jdk.compiler/com.sun.tools.javac.Main.compile(Main.java:59)
    at jdk.compiler/com.sun.tools.javac.Main.main(Main.java:45)
printing javac parameters to: /tmp/javac.20201126_141249.args

The minimal reproducing code is:

class Main {
    static Runnable runnable = () -> {
        boolean b0 = false;
        String s0 = "hello";

        class Local {
            int i = s0.length();
        }

        Runnable dummy = () -> new Local();
    };
}

If the captured variable is a primitive type, the code will compile but cause a runtime crash at startup. e.g.:

public class CaptureInt {
    static Runnable runnable = () -> {
        boolean b0 = false;
        int i0 = 5;

        class Local {
            int i = i0 + 2;
        }

        Runnable dummy = () -> new Local();
    };

    public static void main(String args[]) {
    }
}

Reason & Experimental solution:

During compilation, the captured variable was correctly synthetized as a synthetic argument in the synthetic methods for the nested lambda after desugar().
But the synthetic argument was not used when loading free variables, and it always use the original symbol for the original variable as if not in a nested lambda.
My experimental solution is substituting the symbols in free variables that were captured as synthetic arguments in nested lambda.
I'm not sure whether the code style as well as the solution itself are appropriate. Welcome any advice and better solution.

Progress

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

Issue

JDK-8258897: Wrong translation of capturing local classes inside nested lambdas

Reviewers

Jan Lahoda (@lahodaj - Reviewer) ⚠️ Review applies to 86cb0ad

Contributors

Bernard Blaser <bsrbnd@openjdk.org>

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1479/head:pull/1479
$ git checkout pull/1479

bridgekeeper · 2020-11-27T12:32:51Z

Hi @hltj, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user hltj" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2020-11-27T12:33:42Z

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

hltj · 2020-11-27T12:37:24Z

I had signed OCA, but named as jyw, the email and full name are identical.

hltj · 2020-11-27T12:58:09Z

/signed

bridgekeeper · 2020-11-27T12:59:15Z

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

…Code.java:571)

bridgekeeper · 2020-12-28T02:16:43Z

@hltj 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!

mlbridge · 2020-12-28T15:22:07Z

Webrevs

hltj · 2020-12-29T02:43:49Z

Bernard's reply:
http://mail.openjdk.java.net/pipermail/compiler-dev/2020-December/015865.html

The problem seems to be that 'lambdaTranslationMap' links the
untranslated local variable to its capture. So, what do you think of
the following solution to keep the translated local variable's mapping
with the original symbol (both examples and langtools:tier1 are OK on
jdk14u)?

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java                                                      
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java                             
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java                             
@@ -2057,7 +2057,13 @@                                                                                        
                         };
                         break;
                     case LOCAL_VAR:
-                        ret = new VarSymbol(sym.flags() & FINAL, sym.name, sym.type, translatedSym);
+                        ret = new VarSymbol(sym.flags() & FINAL, sym.name, sym.type, translatedSym) {
+                            @Override
+                            public Symbol baseSymbol() {
+                                //keep mapping with original symbol
+                                return sym;
+                            }
+                        };
                         ((VarSymbol) ret).pos = ((VarSymbol) sym).pos;
                         break;
                     case PARAM:
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
@@ -1221,7 +1221,7 @@
                 //sym is a local variable - check the lambda translation map to
                 //see if sym has been translated to something else in the current
                 //scope (by LambdaToMethod)
-                Symbol translatedSym = lambdaTranslationMap.get(sym);
+                Symbol translatedSym = lambdaTranslationMap.get(sym.baseSymbol());
                 if (translatedSym != null) {
                     tree = make.at(tree.pos).Ident(translatedSym);
                 }

…ed lambdas

hltj · 2020-12-29T04:47:55Z

/contributor add bsrbnd

openjdk · 2020-12-29T04:47:57Z

@hltj
Contributor Bernard Blaser <bsrbnd@openjdk.org> successfully added.

lahodaj

Seems sensible to me. Some recommendations on improving the text comment format are inline.

lahodaj · 2021-01-25T12:15:36Z

test/langtools/tools/javac/lambda/8258897/CaptureVariables.java

@@ -0,0 +1,65 @@
+import java.util.function.Supplier;
+
+/**


It would be nice to tweak the test to so that:
-the comment with @test is first, and imports follow
-@test is augmented with /nodynamiccopyright/
-@bug is added
-@summary is added

(applies to both tests)

And the action tag @run main CaptureVariables should be added.

@hltj Some documents are helpful to you:
Regression Test Harness for the JDK: jtreg
The JDK Test Framework: Tag Language Specification

Thanks to @lahodaj and @lgxbslgx ! I will add the tags and move the comment first.

@lahodaj I think the tests in this case can use the copyright header instead of /nodynamiccopyright/. What's your opinion?

@lgxbslgx - you are right, I am sorry. /nodymiccopyright/ is used when line numbers are significant, and I somehow thought they are here, but you are right they are not. So there should be a normal header here.

Thanks. The copyright comments added.

openjdk · 2021-01-25T12:16:45Z

⚠️ @hltj the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout javac-npe-fix
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

openjdk · 2021-01-25T12:16:46Z

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

8258897: wrong translation of capturing local classes inside nested lambdas

Co-authored-by: Bernard Blaser <bsrbnd@openjdk.org>
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 456 new commits pushed to the master branch:

80760a3: 8260669: Missing quotes in fixpath.sh
c0cde7d: 8259814: test/jdk/tools/jlink/plugins/CompressorPluginTest.java has compilation issues
aec0377: 8257498: Remove useless skeleton predicates
ab727f0: 8260591: Shenandoah: improve parallelism for concurrent thread root scans
cf94208: 8259395: Patching automatic module with additional packages re-creates module without "requires java.base"
039affc: 8260577: Unused code in AbstractCompiler after Shark compiler removal
8a9004d: 8260574: Remove parallel constructs in GenCollectedHeap::process_roots
0da9cad: 8260501: [Vector API] Improve register usage for shift operations on x86
a61ff87: 8260685: ProblemList 2 compiler/jvmci/compilerToVM tests in Xcomp configs
fcfe647: 8260462: Missing in Modality.html
... and 446 more: https://git.openjdk.java.net/jdk/compare/97c99b5d7d4fc057a7ebc378d1e7dd915eaf0bb3...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@lahodaj) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

hltj · 2021-02-01T14:07:26Z

/integrate

openjdk · 2021-02-01T14:08:37Z

@hltj
Your change (at version a4c6aec) is now ready to be sponsored by a Committer.

lahodaj · 2021-02-26T13:09:04Z

/sponsor

openjdk · 2021-02-26T13:10:04Z

@lahodaj @hltj Since your change was applied there have been 807 commits pushed to the master branch:

d7efb4c: 8262199: issue in jli args.c
7603278: 8260198: TypeInstPtr::dump2() emits multiple lines if Verbose is set
0a4e710: 8261954: Dependencies: Improve iteration over class hierarchy under context class
722142e: 8261520: JDK-8261302 breaks runtime/NMT/CheckForProperDetailStackTrace.java
bcca100: 4710675: JTextArea.setComponentOrientation does not work with correct timing
fce5765: 8262433: doclint: reference error in module jdk.incubator.foreign
059ede0: 8262428: doclint warnings in java.xml module
8256517: 8262421: doclint warnings in jdk.compiler module
29c603f: 8262227: Change SystemDictionary::find() to return an InstanceKlass*.
35c0a69: 8262416: ProblemList TestHeapDumpForLargeArray.java due to JDK-8262386
... and 797 more: https://git.openjdk.java.net/jdk/compare/97c99b5d7d4fc057a7ebc378d1e7dd915eaf0bb3...master

Your commit was automatically rebased without conflicts.

Pushed as commit de3f519.

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

bridgekeeper bot added the oca Needs verification of OCA signatory status label Nov 27, 2020

openjdk bot added the compiler compiler-dev@openjdk.org label Nov 27, 2020

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Nov 27, 2020

hltj mentioned this pull request Nov 27, 2020

8229862: NPE in jdk.compiler/com.sun.tools.javac.jvm.Code.emitop0(Code.java:570) #1221

Closed

3 tasks

hltj changed the title ~~[WIP] Fix another NPE jdk.compiler/com.sun.tools.javac.jvm.Code.emitop0(Code.java:571)~~ [WIP] Fix another NPE in jdk.compiler/com.sun.tools.javac.jvm.Code.emitop0(Code.java:571) Nov 27, 2020

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Nov 30, 2020

hltj force-pushed the javac-npe-fix branch from 26d966f to 5801c5c Compare December 14, 2020 12:38

hltj changed the title ~~[WIP] Fix another NPE in jdk.compiler/com.sun.tools.javac.jvm.Code.emitop0(Code.java:571)~~ Fix another NPE in jdk.compiler/com.sun.tools.javac.jvm.Code.emitop0(Code.java:571) Dec 14, 2020

hltj added 5 commits December 15, 2020 21:20

fix another NPE in jdk.compiler/com.sun.tools.javac.jvm.Code.emitop0(…

9dec0d2

…Code.java:571)

remove redundancy test cases & rename the rest test cases

8704929

merge test cases & introduce more cases with anonymous cases enabled

9208263

merge test cases & introduce more cases with anonymous cases enabled

a051ce8

fix code style for test cases

b8602f6

hltj force-pushed the javac-npe-fix branch from 2a0d592 to b8602f6 Compare December 15, 2020 13:23

hltj added 2 commits December 17, 2020 21:02

update variables order in test case & remove redundancy test cases

1b12e23

Merge branch 'master' into javac-npe-fix

5146062

hltj changed the title ~~Fix another NPE in jdk.compiler/com.sun.tools.javac.jvm.Code.emitop0(Code.java:571)~~ 8258897: wrong translation of capturing local classes inside nested lambdas Dec 28, 2020

openjdk bot added the rfr Pull request is ready for review label Dec 28, 2020

hltj added 3 commits December 29, 2020 12:20

Merge branch 'master' into javac-npe-fix

74330aa

revert the experimental fix in 9dec0d2

46ff490

rename test case directory name

841959d

fix 8258897: wrong translation of capturing local classes inside nest…

86cb0ad

…ed lambdas

lahodaj approved these changes Jan 25, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 25, 2021

hltj added 3 commits January 26, 2021 12:37

tweak the test: add informational tags

810b909

tweak the test comments

f142d14

add copyright comments

a4c6aec

openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 1, 2021

openjdk bot closed this Feb 26, 2021

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 26, 2021

8258897: wrong translation of capturing local classes inside nested lambdas #1479

8258897: wrong translation of capturing local classes inside nested lambdas #1479

Uh oh!

Conversation

hltj commented Nov 27, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Download

Uh oh!

bridgekeeper bot commented Nov 27, 2020

Uh oh!

openjdk bot commented Nov 27, 2020

Uh oh!

hltj commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hltj commented Nov 27, 2020

Uh oh!

bridgekeeper bot commented Nov 27, 2020

Uh oh!

bridgekeeper bot commented Dec 28, 2020

Uh oh!

mlbridge bot commented Dec 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

hltj commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hltj commented Dec 29, 2020

Uh oh!

openjdk bot commented Dec 29, 2020

Uh oh!

lahodaj left a comment

Choose a reason for hiding this comment

Uh oh!

lahodaj Jan 25, 2021

Choose a reason for hiding this comment

Uh oh!

lgxbslgx Jan 25, 2021

Choose a reason for hiding this comment

Uh oh!

hltj Jan 26, 2021 • 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!

lgxbslgx Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

lahodaj Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

hltj Jan 27, 2021 • 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!

openjdk bot commented Jan 25, 2021

Uh oh!

openjdk bot commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hltj commented Feb 1, 2021

Uh oh!

openjdk bot commented Feb 1, 2021

Uh oh!

lahodaj commented Feb 26, 2021

Uh oh!

openjdk bot commented Feb 26, 2021

Uh oh!

Uh oh!

hltj commented Nov 27, 2020 •

edited by openjdk bot

Loading

hltj commented Nov 27, 2020 •

edited

Loading

mlbridge bot commented Dec 28, 2020 •

edited

Loading

hltj commented Dec 29, 2020 •

edited

Loading

hltj Jan 26, 2021 •

edited

Loading

hltj Jan 27, 2021 •

edited

Loading

openjdk bot commented Jan 25, 2021 •

edited

Loading