-
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
New error-prone rule: AutoCloseableMustBeClosed #1673
Conversation
If a constructor or method returns an AutoCloseable, it should be annotated @MustBeClosed to ensure callers appropriately close resources in concert with the MustBeClosedChecker. See https://errorprone.info/bugpattern/MustBeClosedChecker
Generate changelog in
|
...ne-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java
Outdated
Show resolved
Hide resolved
...ne-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java
Show resolved
Hide resolved
I generally dislike Or do we expect there are repos with |
I think we want to roll this out to a few repos before hitting everything, we can either:
I'm curious how it works with one large internal repo in particular, that normally catches the edge cases. |
Hey @pkoenig10 as @carterkozak proposed I was mainly thinking of starting it out as a |
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
Released 3.72.0 |
Before this PR
Methods and constructors of
AutoCloseable
instances are often not annotated with@MustBeClosed
, so MustBeClosedChecker does not always do complete analysis to identify potential resource leaks.After this PR
==COMMIT_MSG==
A new suggested error-prone rule
AutoCloseableMustBeClosed
annotates methods and constructors that return anAutoCloseable
type as@MustBeClosed
to allow forMustBeClosedChecker
to perform analysis that resources are appropriately closed.See https://errorprone.info/bugpattern/MustBeClosedChecker
==COMMIT_MSG==
Possible downsides?
For now this is just a
SUGGESTION
level to test out on several projects.This may be noisy both in terms of additional annotations as well as requiring manual work to resolve findings flagged by
MustBeClosedChecker
(both valid and false positives where anAutoCloseable
is passed through layers of abstractions).One example that we may wish to exclude is the case of
java.util.stream.Stream
which implementsAutoCloseable
; however, in most cases it is undesirable to clutter stream usages and methods returningStream
with try-with-resources, and a limitation ofStream
terminal . This is limitation is partially covered by the existing StreamResourceLeak checks.