-
Notifications
You must be signed in to change notification settings - Fork 134
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
Add JooqResultStreamLeak ErrorProne rule #1055
Conversation
Generate changelog in
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks great, thanks @ilyanep.
} | ||
|
||
private static boolean isStreamOrCursor(MethodInvocationTree tree, VisitorState state) { | ||
boolean isStream = ASTHelpers.isSubtype(ASTHelpers.getReturnType(tree), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could factor out ASTHelpers.getReturnType(tree)
and call it once.
Actually, could we replace the checks for either Stream
or Cursor
with a single check for AutoCloseable
which covers both cases (and any potential future types)?
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", | ||
linkType = BugPattern.LinkType.CUSTOM, | ||
providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, | ||
severity = BugPattern.SeverityLevel.ERROR, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind if we bump this down to a warning for the initial rollout? We can bump it up after we're confident it does exactly what we want.
CompilationTestHelper.newInstance(JooqResultStreamLeak.class, getClass()); | ||
|
||
private final BugCheckerRefactoringTestHelper refactoringTestHelper = | ||
BugCheckerRefactoringTestHelper.newInstance(new JooqResultStreamLeak(), getClass()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more nit: mind using our RefactoringValidator
instead of BugCheckerRefactoringTestHelper
? Ours validates that the refactored result also passes validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks @ilyanep
Released 2.34.0 |
After this PR
==COMMIT_MSG==
Adds an ErrorProne rule,
JooqResultStreamLeak
, which ensures that result streams and cursors returned from jOOQ results are closed in a try-with-resources block.==COMMIT_MSG==