8299183: Invokers.checkExactType passes parameters to create WMTE in opposite order by mlchung · Pull Request #11870 · openjdk/jdk

mlchung · 2023-01-05T20:45:20Z

Trivial fix. Fix Invokers.checkExactType to call newWrongMethodTypeException(actual, expected) with parameters in right order.

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-8299183: Invokers.checkExactType passes parameters to create WMTE in opposite order

Reviewers

Iris Clark (@irisclark - Reviewer) ⚠️ Review applies to 3d074593
Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11870

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

Using diff file

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

…opposite order

bridgekeeper · 2023-01-05T20:46:56Z

👋 Welcome back mchung! 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 · 2023-01-05T20:49:43Z

@mlchung The following label will be automatically applied to this pull request:

core-libs

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 · 2023-01-05T20:53:49Z

Webrevs

irisclark

Parameter order for method invocation now matches method declaration on line 521.

openjdk · 2023-01-05T22:59:35Z

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

8299183: Invokers.checkExactType passes parameters to create WMTE in opposite order

Reviewed-by: iris, jpai

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

b7eb4e2: 8297306: Incorrect brackets in Javadoc for a constructor of IteratorSpliterator
a638663: 8255119: Monitor::wait takes signed integer as timeout
4395578: Merge
d49851a: 8299689: Make use of JLine for Console as "opt-in"
f79b3d4: 6381945: (cal) Japanese calendar unit test system should avoid multiple static imports
f36f135: 8299501: Usage of constructors of primitive wrapper classes should be avoided in java.util API docs
b8852f6: 8299502: Usage of constructors of primitive wrapper classes should be avoided in javax.xml API docs
679e485: 8043251: Bogus javac error: required: no arguments, found: no arguments
cd10c72: 8299500: Usage of constructors of primitive wrapper classes should be avoided in java.text API docs
bfd5971: 8299757: Update JCov version to 3.0.14
... and 45 more: https://git.openjdk.org/jdk/compare/3e2314d08218dc8a4f4fc61bd4e1d5e58a0129c7...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.

jaikiran · 2023-01-06T01:05:40Z

Hello Mandy, this looks good to me. The copyright year on the file will need an update.

There's another call to this newWrongMethodTypeException method on line 515 and I am guessing that one is already passing the params in the correct order. If I go by that same logic, then there appears to be a direct construction of WrongMethodTypeException on line 497 and I suspect the String passed to it is perhaps using the wrong order? It currently says:

if (handle.hasInvokeExactBehavior() && handle.accessModeType(ad.type) != ad.symbolicMethodTypeExact) {
     throw new WrongMethodTypeException("expected " + handle.accessModeType(ad.type) + " but found "
                    + ad.symbolicMethodTypeExact);
}

Should it instead say:

if (handle.hasInvokeExactBehavior() && handle.accessModeType(ad.type) != ad.symbolicMethodTypeExact) {
    throw new WrongMethodTypeException("expected " + ad.symbolicMethodTypeExact + " but found "
                    + handle.accessModeType(ad.type));
}

mlchung · 2023-01-06T21:14:50Z

@jaikiran good observation.

"expected" and "actual" parameters are confusing. "expected" is ambiguous and it could refer to handle's method type or symbolic type descriptor. I decide to rename the parameters and the exception message for clarity.

jaikiran

Thank you for the changes Mandy. Overall the error message looks more clear now.

There's a failing test in GitHub actions job which appears to be an existing test case which was expecting the older error message in the exception. That would need an update:

test VarHandleTestExact.testExactArraySet(class [Ljava.lang.Object;, "abcd", VarHandleTestExact$$Lambda$87/0x000000010015b1c0@2a9fbca4): failure
java.lang.AssertionError: 'handle's method type (Object[],int,Object)void but found (Object[],int,String)void' did not match the pattern '.*\\Qexpected (Object[],int,Object)void \\E.*'.
	at VarHandleTestExact.assertMatches(VarHandleTestExact.java:214)
	at VarHandleTestExact.doTest(VarHandleTestExact.java:199)
	at VarHandleTestExact.testExactArraySet(VarHandleTestExact.java:153)

jaikiran · 2023-01-07T10:46:40Z

test/jdk/java/lang/invoke/WrongMethodTypeTest.java

+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided


From what I understand, the copyright notice on test files don't use the "Classpath" exception and instead use a different copyright notice.

That's correct. Thanks for catching it. This reveals that the copyright header of several test/jdk/java/lang/invoke tests will need clean up (as this new test was copied from an existing test). I will create a JBS issue for that.

jaikiran

Thank you Mandy for the changes. Looks good to me.

mlchung · 2023-01-10T17:03:17Z

/integrate

openjdk · 2023-01-10T17:05:51Z

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

8b0133f: 8299259: C2: Div/Mod nodes without zero check could be split through iv phi of loop resulting in SIGFPE
d68de02: 8299845: Remove obsolete comments in DirtyCardToOopClosure::get_actual_top
debe587: 8298381: Improve handling of session tickets for multiple SSLContexts
eab1e62: 8297487: G1 Remark: no need to keep alive oop constants of nmethods on stack
c8a8388: 8299789: Compilation of gtest causes build to fail if runtime libraries are in different dirs
5f37cbe: 8297572: Remove unused PrecisionStyle::Precise
f95346e: 8299261: Clean up AWT D3D exports
195f313: 8287925: AArch64: intrinsics for compareUnsigned method in Integer and Long
3a66737: 8299525: RISC-V: Add backend support for half float conversion intrinsics
b7eb4e2: 8297306: Incorrect brackets in Javadoc for a constructor of IteratorSpliterator
... and 54 more: https://git.openjdk.org/jdk/compare/3e2314d08218dc8a4f4fc61bd4e1d5e58a0129c7...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-10T17:06:15Z

@mlchung Pushed as commit a86b6f6.

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

8299183: Invokers.checkExactType passes parameters to create WMTE in …

3d07459

…opposite order

openjdk bot added the rfr Pull request is ready for review label Jan 5, 2023

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jan 5, 2023

irisclark approved these changes Jan 5, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 5, 2023

mlchung added 2 commits January 6, 2023 13:09

further cleanup newWrongMethodTypeException for clarity

a1e718f

Add a regression test

35968a3

jaikiran reviewed Jan 7, 2023

View reviewed changes

Update VarHandleTestExact test and new test's copyright header

7c96302

jaikiran approved these changes Jan 10, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 10, 2023

openjdk bot closed this Jan 10, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 10, 2023

mlchung deleted the JDK-8299183 branch January 24, 2023 18:13

Conversation

mlchung commented Jan 5, 2023 • 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 Jan 5, 2023

Uh oh!

openjdk bot commented Jan 5, 2023

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

irisclark left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

jaikiran commented Jan 6, 2023

Uh oh!

mlchung commented Jan 6, 2023

Uh oh!

jaikiran left a comment

Choose a reason for hiding this comment

Uh oh!

jaikiran Jan 7, 2023

Choose a reason for hiding this comment

Uh oh!

mlchung Jan 9, 2023 • 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!

jaikiran left a comment

Choose a reason for hiding this comment

Uh oh!

mlchung commented Jan 10, 2023

Uh oh!

openjdk bot commented Jan 10, 2023

Uh oh!

openjdk bot commented Jan 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

mlchung commented Jan 5, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 5, 2023 •

edited

Loading

openjdk bot commented Jan 5, 2023 •

edited

Loading

mlchung Jan 9, 2023 •

edited

Loading