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.

Sign up for GitHub

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

Jump to bottom

Suppress new Error Prone warning #6120

Merged
merged 7 commits into from
Aug 3, 2023
Merged

Suppress new Error Prone warning #6120

merged 7 commits into from
Aug 3, 2023

Conversation

smillst
Copy link
Member

@smillst smillst commented Aug 3, 2023

No description provided.

@smillst smillst mentioned this pull request Aug 3, 2023
Closed
1 task
mernst
mernst previously approved these changes Aug 3, 2023
View reviewed changes
@mernst mernst enabled auto-merge (squash) August 3, 2023 16:27
msridhar
msridhar reviewed Aug 3, 2023
View reviewed changes
framework/src/main/java/org/checkerframework/common/reflection/DefaultReflectionResolver.java Outdated
Comment on lines 69 to 72
// Error Prone is warning on calls to classSym.getEnclosedElements()
// which can cause a crash when code is complied using JDK 17 -source 11 -target 11 and then
// run using JDK 11. The Checker Framework is compiled using -source 8 -target 8, so this
// is not currently a problem. See https://github.com/google/error-prone/issues/3895.
Copy link
Contributor

Choose a reason for hiding this comment

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

NullAway is compiled using -source 8 -target 8 and this is still a problem. I tried to explain what is going on here: uber/NullAway#795 (comment)

@msridhar
Copy link
Contributor

msridhar commented Aug 3, 2023

@mernst @smillst if you build releases of the Checker Framework on JDK 17, you may want to fix this issue rather than suppressing; see my comment above. I'm going to turn off the auto-merge just to let you confirm.

@msridhar msridhar disabled auto-merge August 3, 2023 17:28
@smillst
Copy link
Member Author

smillst commented Aug 3, 2023

@mernst @smillst if you build releases of the Checker Framework on JDK 17, you may want to fix this issue rather than suppressing; see my comment above. I'm going to turn off the auto-merge just to let you confirm.

Thanks for pointing this out and the further explanation. We do build the release with JDK 17, so I fixed it so that all the calls to getEnclosedElement are on variables with static type of Symbol.

msridhar
msridhar requested changes Aug 3, 2023
View reviewed changes
framework/src/main/java/org/checkerframework/common/reflection/DefaultReflectionResolver.java Outdated
@@ -517,7 +521,7 @@ private List<Symbol> getMethodSymbolsfor(
List<Symbol> result = new ArrayList<>();
ClassSymbol classSym = (ClassSymbol) sym;
while (classSym != null) {
for (Symbol s : classSym.getEnclosedElements()) {
for (Symbol s : sym.getEnclosedElements()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like classSym gets updated on each loop iteration? Not sure this will behave the same as the previous code.

msridhar
msridhar approved these changes Aug 3, 2023
View reviewed changes
Copy link
Contributor

@msridhar msridhar left a comment

Choose a reason for hiding this comment

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

Now LGTM. If I get time, I may try to improve the Error Prone check so that you don't need to suppress in cases like this one (as calls to Symbol.getEnclosedElements() are ok).

@msridhar msridhar enabled auto-merge (squash) August 3, 2023 19:51
@msridhar msridhar merged commit 4d93a7b into typetools:master Aug 3, 2023
29 checks passed
@mernst
Copy link
Member

mernst commented Aug 3, 2023

@msridhar Thanks for your insight on this issue.
For future reference, when enabling auto-squash-and-merge, please remove the crufty per-commit-message list from the squashed commit message. Thanks!

@mernst mernst deleted the errorprone branch August 3, 2023 20:28
@msridhar
Copy link
Contributor

msridhar commented Aug 3, 2023

@msridhar Thanks for your insight on this issue.
For future reference, when enabling auto-squash-and-merge, please remove the crufty per-commit-message list from the squashed commit message. Thanks!

@mernst sure, sorry about that. If you like, there are new defaults that we can set for the project when it comes to squashed commit messages. See below, which is under the Settings for the repo:

Screenshot 2023-08-03 at 14 29 47

My preference is "Default to pull request title and description." Then, the PR description (if any) will be used as the body of the commit message.

@smillst
Copy link
Member Author

smillst commented Aug 4, 2023

@msridhar I changed the default to "Default to pull request title and description." Thanks for pointing out this option!

wmdietl pushed a commit to eisop/checker-framework that referenced this pull request Oct 16, 2023
@smillst @renovate @wmdietl
Suppress new Error Prone warning (typetools#6120)
44bfb60 
* Update versions.errorprone to v2.21.0

* Update versions.errorprone to v2.21.0

* Suppress warning.

* Add comment.

* Tweak comment.

* Fix bug.

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msridhar msridhar msridhar approved these changes

@mernst mernst mernst left review comments

Assignees

@mernst mernst

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@smillst @msridhar @mernst