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

Improvement: Upgrade error-prone to 2.4.0 #1370

Merged
merged 8 commits into from
Jun 1, 2020
Merged

Conversation

robert3005
Copy link
Contributor

@robert3005 robert3005 commented May 29, 2020

Before this PR

We are using error-prone 2.3.4

After this PR

https://github.com/google/error-prone/releases/tag/v2.4.0
==COMMIT_MSG==
Upgarde error-prone to 2.4.0 to support latest jdks
==COMMIT_MSG==

Possible downsides?

There's 300 commits in the diff, surely there are bunch of checks that need autofixes applied

@policy-bot policy-bot bot requested a review from iamdanfox May 29, 2020 19:55
@ferozco
Copy link
Contributor

ferozco commented May 29, 2020

Lets try this out on a few repos before letting it rip more widely

@robert3005
Copy link
Contributor Author

We also should remove our jdk 12, 13 overrides as they shouldn't be necessary

@carterkozak
Copy link
Contributor

Many MoreASTHelpers/MoreSuggestedFixes methods are no longer necessary since our fixes were merged upstream.

@robert3005
Copy link
Contributor Author

Cool, will fix those as well. Also found that your JavaTime check bug was fixed so we can reenable that

Robert Kruszewski added 2 commits May 29, 2020 21:09
@robert3005
Copy link
Contributor Author

robert3005 commented May 29, 2020

@carterkozak couldn't find things in MoreAstHelpers that could be removed - have specific methods/bugs in mind? Found some

@robert3005
Copy link
Contributor Author

robert3005 commented May 29, 2020

I found some cases of JdkObsolete check complaining about types from libraries, i.e. right now it doesn't like SortedMap that dropwizard-metrics uses.

@ferozco
Copy link
Contributor

ferozco commented Jun 1, 2020

We should also clean up some of the work arounds in the gradle plugin itself. ex:

if (jdkVersion.compareTo(JavaVersion.toVersion("12.0.1")) >= 0) {

@robert3005
Copy link
Contributor Author

@ferozco I have removed it already

@ferozco
Copy link
Contributor

ferozco commented Jun 1, 2020

👍

@bulldozer-bot bulldozer-bot bot merged commit b012683 into develop Jun 1, 2020
@bulldozer-bot bulldozer-bot bot deleted the rk/errorprone-2.4 branch June 1, 2020 14:08
@pkoenig10
Copy link
Member

FYI these commits in error-prone 2.4.0 are going to cause a lot of failures for common nested imports.
https://github.com/google/error-prone/blame/7037696f4b00c8137394b10d143e37072b6048f6/core/src/main/java/com/google/errorprone/bugpatterns/BadImport.java#L63-L71

@carterkozak
Copy link
Contributor

Would you mind trying the automated fixes and reporting back? If they seem safe and sane we can apply them automatically.

@pkoenig10
Copy link
Member

The suggested fixes seemed to work pretty well, let's apply them automatically.
#1387

@pkoenig10
Copy link
Member

pkoenig10 commented Jun 3, 2020

I also ran into a bunch of issues with JdkObsolete because it matches any use of methods on obsolete classes, even if you're just using an obsolete type returned from a library or passing an obsolete type to a library.

This was due to this change:
google/error-prone@6671c25#diff-065165cb9efc7cfdb26debe40d8ea8ccR230-R245

@ferozco
Copy link
Contributor

ferozco commented Jun 3, 2020

I would be in favour of disabling the rule. I think its pretty lame to get warnings for code you don't control

@pkoenig10
Copy link
Member

Looks like this is being tracked in google/error-prone#1646

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants