Skip to content
This repository has been archived by the owner. It is now read-only.

8254571: Erroneous generic type inference in a lambda expression with a checked exception #91

Closed

Conversation

vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Jun 17, 2021

Hi,

The compiler is issuing an error for code like:

class JDK8254571 {
    public static void test() {
        outer(nested(x -> mightThrow()));
    }
    
    static <A> void outer(Object o) {}

    static <B, C, E extends Throwable> B nested(Fun<C,E> fun) {
        return null;
    }

    interface Fun<X, Y extends Throwable> {
        void m(X t) throws Y;
    }

    static void mightThrow() throws Exception {}
} 

there are several things at play here:

  1. for performance reasons we decided to minimize the number of inference variables popped up when inferring nested generic method invocations, see Infer::roots
  2. when an expression is stuck (like in this case this implicit lambda) we decide which inference variables should be present in the related inference context and thrown variables were not part of the equation

So there are three hypothesis here:

  1. there is a bug in the code that determines which variables are passed up to the nested inference context, meaning that it is passing less variables than the bare minimum needed
  2. there is a bug in determining which inference variables should be extracted from stuck expressions
  3. both are buggy

The root cause of this bug is that some types escape from Attr with non instantiated type variables in them. I modified the test case to double check if the issue was not related to lambdas, but then the test passed so in the example above I added:

     static <X, Y extends Throwable> Fun<X, Y> mightThrow2() throws Y { return null; }

which simulates the lambda and the compiler has no issues compiling this invocation:

    outer(nested(mightThrow2()));

so summing up, after double checking the logic of the code that minimized the inference context I realized that the issue is not there. The code is sound and its output only depend on its input, so the issue should be on determining the input. So IMO the issue is in what inference variables we extract from stuck expressions, which is one of the inputs to Infer::roots. Looking for what variables we extract from stuck expressions, I realized that the code currently ignores type variables in the throws clause and this is why my proposed fix is addressing this issue.

Comments?


Progress

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

Issue

  • JDK-8254571: Erroneous generic type inference in a lambda expression with a checked exception

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 91

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 17, 2021

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

@@ -1238,6 +1239,7 @@ public void visitReference(JCMemberReference tree) {
tree.getOverloadKind() != JCMemberReference.OverloadKind.UNOVERLOADED) {
stuckVars.addAll(freeArgVars);
depVars.addAll(inferenceContext.freeVarsIn(descType.getReturnType()));
depVars.addAll(inferenceContext.freeVarsIn(descType.getThrownTypes()));
Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did the same for method references given the similarities between both but I couldn't find a test case that reproduced the issue with a method reference

Copy link
Contributor

@lahodaj lahodaj Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

outer(nested(ConsiderExceptionTVarsInStuckExprs::mightThrow2));
...
static <C> void mightThrow2(C c) throws Exception {}

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep that one makes the trick, thanks

@openjdk openjdk bot added the rfr label Jun 17, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 17, 2021

@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 openjdk bot added the compiler label Jun 17, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 17, 2021

Webrevs

@vicente-romero-oracle
Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle commented Jun 22, 2021

ping

Copy link
Contributor

@lahodaj lahodaj left a comment

Looks reasonable to me. Not sure if I can foresee the consequences, though.

@@ -1238,6 +1239,7 @@ public void visitReference(JCMemberReference tree) {
tree.getOverloadKind() != JCMemberReference.OverloadKind.UNOVERLOADED) {
stuckVars.addAll(freeArgVars);
depVars.addAll(inferenceContext.freeVarsIn(descType.getReturnType()));
depVars.addAll(inferenceContext.freeVarsIn(descType.getThrownTypes()));
Copy link
Contributor

@lahodaj lahodaj Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

outer(nested(ConsiderExceptionTVarsInStuckExprs::mightThrow2));
...
static <C> void mightThrow2(C c) throws Exception {}

@openjdk
Copy link

@openjdk openjdk bot commented Jun 23, 2021

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

8254571: Erroneous generic type inference in a lambda expression with a checked exception

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

  • 1b2147a: 8269125: Klass enqueue element size calculation wrong when traceid value cross compress limit
  • bf70620: 8268961: Parenthesized pattern with guards does not work
  • 8128ca1: 8269066: assert(ZAddress::is_marked(addr)) failed: Should be marked
  • 1323be5: 8269064: Dropped messages of AsyncLogWriter cause memleak
  • ce917b2: 8269148: Update minor GCC version in GitHub Actions pipeline
  • ab7ff1e: 8266885: [aarch64] Crash with 'Field too big for insn' for some tests under compiler/jvmci/jdk.vm.ci.code.test/src/jdk/vm/ci/code/test/
  • 35e4c27: 8268404: [TESTBUG] tools/jpackage/windows/WinInstallerIconTest.java failed "AssertionError: Failed: Check icon"
  • dc12cb7: 8267652: c2 loop unrolling by 8 results in reading memory past array
  • 578c55b: 8267399: C2: java/text/Normalizer/ConformanceTest.java test failed with assertion
  • 8fa2520: 8268888: Upstream 8268230: Foreign Linker API & Windows user32/kernel32: String conversion seems broken
  • ... and 42 more: https://git.openjdk.java.net/jdk17/compare/7d7bdbe135018f1452fa133b294575014e3e871b...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.

@openjdk openjdk bot added the ready label Jun 23, 2021
Copy link
Contributor

@mcimadamore mcimadamore left a comment

I believe the fix is good. We probably overlooked free type variables in the thrown clause (as they are relatively rare) when doing stuck expression computation. These variables are part of the bound set used by JLS chapter 18, so dropping them on the floor (as current code does) is not a good option. This patch rectifies that.

To test with method references, you would need the method reference to be "non-exact": only in that case will the method reference be considered stuck in the same way the implicit lambda is, I believe.

@vicente-romero-oracle
Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle commented Jun 23, 2021

thanks guys for the review

@vicente-romero-oracle
Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle commented Jun 23, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jun 23, 2021

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

  • 1b2147a: 8269125: Klass enqueue element size calculation wrong when traceid value cross compress limit
  • bf70620: 8268961: Parenthesized pattern with guards does not work
  • 8128ca1: 8269066: assert(ZAddress::is_marked(addr)) failed: Should be marked
  • 1323be5: 8269064: Dropped messages of AsyncLogWriter cause memleak
  • ce917b2: 8269148: Update minor GCC version in GitHub Actions pipeline
  • ab7ff1e: 8266885: [aarch64] Crash with 'Field too big for insn' for some tests under compiler/jvmci/jdk.vm.ci.code.test/src/jdk/vm/ci/code/test/
  • 35e4c27: 8268404: [TESTBUG] tools/jpackage/windows/WinInstallerIconTest.java failed "AssertionError: Failed: Check icon"
  • dc12cb7: 8267652: c2 loop unrolling by 8 results in reading memory past array
  • 578c55b: 8267399: C2: java/text/Normalizer/ConformanceTest.java test failed with assertion
  • 8fa2520: 8268888: Upstream 8268230: Foreign Linker API & Windows user32/kernel32: String conversion seems broken
  • ... and 42 more: https://git.openjdk.java.net/jdk17/compare/7d7bdbe135018f1452fa133b294575014e3e871b...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jun 23, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 23, 2021

@vicente-romero-oracle Pushed as commit 7e96318.

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

@vicente-romero-oracle vicente-romero-oracle deleted the JDK-8254571 branch Jun 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler integrated
3 participants