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

8244681: Add a warning for possibly lossy conversion in compound assignments #8599

Closed
wants to merge 21 commits into from

Conversation

asotona
Copy link
Member

@asotona asotona commented May 9, 2022

Please review this patch adding new lint option, lossy-conversions, to javac to warn about type casts in compound assignments with possible lossy conversions.

The new lint warning is shown if the type of the right-hand operand of a compound assignment is not assignment compatible with the type of the variable.

The implementation of the warning is based on similar check performed to emit "possible lossy conversion" compilation error for simple assignments.

Proposed patch also include complex matrix-style test with positive and negative test cases of lossy conversions in compound assignments.

Proposed patch also disables this new lint option in all affected JDK modules and libraries to allow smooth JDK build. Individual cases to address possibly lossy conversions warnings in JDK are already addressed in a separate umbrella issue and its sub-tasks.

Thanks for your review,
Adam


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
  • Change requires a CSR request to be approved

Issues

  • JDK-8244681: Add a warning for possibly lossy conversion in compound assignments
  • JDK-8293797: Release Note: Javac warns about type casts in compound assignments with possible lossy conversions
  • JDK-8286377: Add javac lint warning for possibly lossy conversion in compound assignments (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8599

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented May 9, 2022

👋 Welcome back asotona! 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 rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels May 9, 2022
@openjdk
Copy link

openjdk bot commented May 9, 2022

@asotona The following labels will be automatically applied to this pull request:

  • build
  • client
  • compiler
  • core-libs
  • hotspot-jfr
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added serviceability serviceability-dev@openjdk.org build build-dev@openjdk.org client client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org hotspot-jfr hotspot-jfr-dev@openjdk.org labels May 9, 2022
@mlbridge
Copy link

mlbridge bot commented May 9, 2022

@erikj79
Copy link
Member

erikj79 commented May 9, 2022

/reviewers 2 reviewer

Copy link
Member

@erikj79 erikj79 left a comment

Choose a reason for hiding this comment

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

Build changes look good.

@openjdk
Copy link

openjdk bot commented May 9, 2022

@erikj79
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with 2 of role reviewers).

@jddarcy
Copy link
Member

jddarcy commented May 9, 2022

I see there is already a bug filed to address situations found by the new warning in the JDK's libraries (JDK-8286374). As a matter of policy, I recommend the (potential) warnings be addressed in at least the java.base and java.desktop modules before the new warning is enabled. In other words, a priority should be given to keeping java.base and java.desktop compiling successfully with all warnings enabled.

@asotona
Copy link
Member Author

asotona commented May 10, 2022

I agree with the priority to keep java.base and java.desktop clean from possibly lossy conversions, so the related issues should probably raise from P4 priority level.

However this lint warning as a part of the javac is critical to confirm that the situations have been correctly addressed.
If we want to avoid "blind" patching, we only two possible scenarios:

  1. big homogenous patch including hundreds of fixed lines of code across many "moving-target" classes, together with lint warning implemented and enabled
  2. javac lint patch (disabled for affected JDK modules build) goes first, so each case can be resolved, reviewed and validated in individual patch

From complexity and cost perspective I prefer the second scenario.

…gnments

recommended correction of the warning wording
fixed typo in test method name
…gnments

recommended correction of the warning description
Copy link
Member

@magicus magicus left a comment

Choose a reason for hiding this comment

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

Check updates on JDK-8286374 subtasks.

@egahlin
Copy link
Member

egahlin commented May 11, 2022

Lossy conversion issues for jdk.jfr and jdk.management.jfr. have been fixed.

@mlbridge
Copy link

mlbridge bot commented May 11, 2022

Mailing list message from Patrick Chen on build-dev:

I checked you pr look good to me @Roger

Le mer. 11 mai 2022 ? 15:35, Roger Riggs <rriggs at openjdk.java.net> a ?crit :

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 25, 2022

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

@asotona
Copy link
Member Author

asotona commented Jul 26, 2022

ping to keep the PR open

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 23, 2022

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

@asotona
Copy link
Member Author

asotona commented Sep 14, 2022

/issue add JDK-8293797

@openjdk
Copy link

openjdk bot commented Sep 14, 2022

@asotona
Adding additional issue to issue list: 8293797: Release Note: Javac warns about type casts in compound assignments with possible lossy conversions.

@asotona
Copy link
Member Author

asotona commented Sep 14, 2022

Please review also linked Release Note issue.

Thanks,
Adam

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Sep 15, 2022
@openjdk
Copy link

openjdk bot commented Sep 15, 2022

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

8244681: Add a warning for possibly lossy conversion in compound assignments
8293797: Release Note: Javac warns about type casts in compound assignments with possible lossy conversions

Reviewed-by: erikj, prr

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

  • 8f3bbe9: 8293472: Incorrect container resource limit detection if manual cgroup fs mounts present
  • 1caba0f: 8292948: JEditorPane ignores font-size styles in external linked css-file
  • eeb625e: 8290169: adlc: Improve child constraints for vector unary operations
  • 2057070: 8293815: P11PSSSignature.engineUpdate should not print debug messages during normal operation
  • 7376c55: 8293769: RISC-V: Add a second temporary register for BarrierSetAssembler::load_at
  • d191e47: 8293768: Add links to JLS 19 and 20 from SourceVersion enum constants
  • a75ddb8: 8293122: (fs) Use file cloning in macOS version of Files::copy method
  • 95c7c55: 8293402: hs-err file printer should reattempt stack trace printing if it fails
  • 211fab8: 8291669: [REDO] Fix array range check hoisting for some scaled loop iv

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 Pull request is ready to be integrated label Sep 15, 2022
@asotona
Copy link
Member Author

asotona commented Sep 15, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Sep 15, 2022

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

  • 15cb1fb: 8256265: G1: Improve parallelism in regions that failed evacuation
  • b31a03c: 8293695: Implement isInfinite intrinsic for RISC-V
  • 8f3bbe9: 8293472: Incorrect container resource limit detection if manual cgroup fs mounts present
  • 1caba0f: 8292948: JEditorPane ignores font-size styles in external linked css-file
  • eeb625e: 8290169: adlc: Improve child constraints for vector unary operations
  • 2057070: 8293815: P11PSSSignature.engineUpdate should not print debug messages during normal operation
  • 7376c55: 8293769: RISC-V: Add a second temporary register for BarrierSetAssembler::load_at
  • d191e47: 8293768: Add links to JLS 19 and 20 from SourceVersion enum constants
  • a75ddb8: 8293122: (fs) Use file cloning in macOS version of Files::copy method
  • 95c7c55: 8293402: hs-err file printer should reattempt stack trace printing if it fails
  • ... and 1 more: https://git.openjdk.org/jdk/compare/7f3250d71c4866a64eb73f52140c669fe90f122f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 15, 2022
@openjdk openjdk bot closed this Sep 15, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 15, 2022
@openjdk
Copy link

openjdk bot commented Sep 15, 2022

@asotona Pushed as commit aff5ff1.

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

@asotona
Copy link
Member Author

asotona commented Nov 4, 2022

/issue remove JDK-8293797

@openjdk
Copy link

openjdk bot commented Nov 4, 2022

@asotona The command issue can only be used in open pull requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org client client-libs-dev@openjdk.org compiler compiler-dev@openjdk.org core-libs core-libs-dev@openjdk.org hotspot-jfr hotspot-jfr-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
8 participants