Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8200145: Conditional expression mistakenly treated as standalone #2324

Closed
wants to merge 3 commits into from

Conversation

lgxbslgx
Copy link
Member

@lgxbslgx lgxbslgx commented Jan 30, 2021

Hi all,

If the argument Type t of the method Types.unboxedType is an ErrorType, the Types.unboxedType may return the wrong result. And in this case, Attr.isBooleanOrNumeric and Attr.isBooleanOrNumeric return the wrong result, too.

This patch fixes it and adds a test case.
Thank you for taking the time to review.

Best Regards.
-- xiong


Progress

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

Issue

  • JDK-8200145: Conditional expression mistakenly treated as standalone

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2324

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 30, 2021

👋 Welcome back lgxbslgx! 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 openjdk bot added the rfr Pull request is ready for review label Jan 30, 2021
@openjdk
Copy link

openjdk bot commented Jan 30, 2021

@lgxbslgx 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 compiler-dev@openjdk.org label Jan 30, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 30, 2021

Webrevs

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 27, 2021

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

@shipilev
Copy link
Member

shipilev commented Apr 16, 2021

I think we want to ping @lahodaj to see if this makes sense.

Copy link
Contributor

@mcimadamore mcimadamore left a comment

The fix looks good - it's a good "quality of life" fix, which improves the conditional classification by making it more resilient to erroneous types. I wonder if the fix can be made more minimal (see code comment).

@@ -4323,6 +4323,8 @@ public Type boxedTypeOrType(Type t) {
* Return the primitive type corresponding to a boxed type.
*/
public Type unboxedType(Type t) {
if (t.hasTag(ERROR))
Copy link
Contributor

@mcimadamore mcimadamore Apr 16, 2021

Choose a reason for hiding this comment

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

This is not necessary, right? After all, you check for errors upfront, in the conditional code. Reason I'm asking is that it's not clear to me as to whether we should just return the error type, or no type. Also, this might affect code which is unrelated to the one you are fixing.

Copy link
Contributor

@lahodaj lahodaj Apr 16, 2021

Choose a reason for hiding this comment

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

(I am not an expert on types, so I may be wrong.)

This method seems to return either the primitive type for which t is a box, or noType. So returning a noType for erroneous type seems reasonable to me. Basically, it would mean that an erroneous type is not a box for any type, which seems sensible. (Currently, every erroneous type is seen as the box for the first primitive type in the sequence, which I think is byte.)

I agree only one of these two changes is needed, although I'd personally probably try to go with the one here, in unboxedType, as it seems like a generally desirable behavior to me. I agree it can have effects on other parts of the code, though, so it has more potential to break something.

Copy link
Contributor

@mcimadamore mcimadamore Apr 16, 2021

Choose a reason for hiding this comment

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

I'm ok with going for the proposed fix, as long as tests are ok. Approving.

Copy link
Member Author

@lgxbslgx lgxbslgx Apr 17, 2021

Choose a reason for hiding this comment

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

Yes, this is not necessary if we only solve this bug. But it is useful logically. And all the related tests passed locally and passed at the Pre-submit tests. So I think it is good to remain it.

@@ -1956,7 +1956,7 @@ private boolean isBooleanOrNumeric(Env<AttrContext> env, JCExpression tree) {
}
//where
boolean primitiveOrBoxed(Type t) {
return (!t.hasTag(TYPEVAR) && types.unboxedTypeOrType(t).isPrimitive());
return (!t.hasTag(TYPEVAR) && !t.hasTag(ERROR) && types.unboxedTypeOrType(t).isPrimitive());
Copy link
Contributor

@mcimadamore mcimadamore Apr 16, 2021

Choose a reason for hiding this comment

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

Good catch - you can also use !t.isErroneous() here.

Copy link
Member Author

@lgxbslgx lgxbslgx Apr 17, 2021

Choose a reason for hiding this comment

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

Fixed.

@openjdk
Copy link

openjdk bot commented Apr 16, 2021

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

8200145: Conditional expression mistakenly treated as standalone

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 (@mcimadamore) 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).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Apr 16, 2021
@lgxbslgx
Copy link
Member Author

lgxbslgx commented Apr 17, 2021

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 17, 2021
@openjdk
Copy link

openjdk bot commented Apr 17, 2021

@lgxbslgx
Your change (at version 2fe7665) is now ready to be sponsored by a Committer.

@openjdk openjdk bot removed the sponsor Pull request is ready to be sponsored label Apr 17, 2021
@lgxbslgx
Copy link
Member Author

lgxbslgx commented Apr 17, 2021

I updated the code according to the comment. Thank you for reviewing again and sponsoring.

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 17, 2021
@openjdk
Copy link

openjdk bot commented Apr 17, 2021

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

@lgxbslgx
Copy link
Member Author

lgxbslgx commented Apr 21, 2021

@mcimadamore Could I get your help to re-review and sponsor? Thanks a lot.

@mcimadamore
Copy link
Contributor

mcimadamore commented Apr 21, 2021

/sponsor

@openjdk openjdk bot closed this Apr 21, 2021
@openjdk 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 Apr 21, 2021
@openjdk
Copy link

openjdk bot commented Apr 21, 2021

@mcimadamore @lgxbslgx Since your change was applied there have been 63 commits pushed to the master branch:

  • 41fc7dd: 8199079: Test javax/swing/UIDefaults/6302464/bug6302464.java is unstable
  • 45c474a: 8168408: Test java/awt/Focus/ActualFocusedWindowTest/ActualFocusedWindowBlockingTest.java fails intermittentently on windows
  • 18ee419: 8198422: Test java/awt/font/StyledMetrics/BoldSpace.java is unstable
  • da86029: 8265326: Strange Characters in G1GC GC Log
  • 7879adb: 8265343: Update Debian-based cross-compilation recipes
  • 98cb81b: 8265237: String.join and StringJoiner can be improved further
  • ed477da: 8264945: Optimize the code-gen for Math.pow(x, 0.5)
  • 7146104: 8265421: java/lang/String/StringRepeat.java test is missing a memory requirement
  • b5c92ca: 8265106: IGV: Enforce en-US locale while parsing ideal graph
  • 3de0dcb: 8265483: All-caps “JAVA” in the top navigation bar
  • ... and 53 more: https://git.openjdk.java.net/jdk/compare/cb8394a841023b2cf4a9103f47f50e3fd28c5b04...master

Your commit was automatically rebased without conflicts.

Pushed as commit 07a7510.

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

@lgxbslgx lgxbslgx deleted the JDK-8200145 branch Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler compiler-dev@openjdk.org integrated Pull request has been integrated
4 participants