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

Resource Leak Checker: warn of @MustCall on class declaration #5181

Closed
mernst opened this issue Jun 27, 2022 · 3 comments · Fixed by #5183
Closed

Resource Leak Checker: warn of @MustCall on class declaration #5181

mernst opened this issue Jun 27, 2022 · 3 comments · Fixed by #5183

Comments

@mernst
Copy link
Member

mernst commented Jun 27, 2022

Usually, writing @MustCall on a class declaration is the wrong thing. Instead, one should write @InheritableMustCall. However, it is an easy error to make.

I suggest that writing @MustCall on a class declaration should result in a warning or errror, but one that can be suppressed.

@msridhar
Copy link
Contributor

Do you think it would be better for @MustCall itself to be inheritable? Or does that go against some standard design principles? We could also have an optional boolean parameter inheritable for @MustCall, which defaults to true but could be configured as needed.

@kelloggm
Copy link
Contributor

As I recall, we needed @InheritableMustCall because for an annotation to be inheritable, it must be a declaration annotation - type annotations are not inheritable, and @MustCall itself is a type annotation. So, we needed @InheritableMustCall for technical reasons.

I propose that @MustCall on a class declaration should always be an error, with new error text instructing the user to write @InheritableMustCall instead. This would require a few additional warning suppressions in code that already requires warning suppressions (e.g., when @MustCall({}) is written on a particular sub-class of a super class that generally is @MustCall("close"), such as ByteArrayOutputStream), but otherwise I think that it would be fine. And, those @MustCall({}) annotations could just as easily (and maybe should be?) @InheritableMustCall annotations instead.

@msridhar
Copy link
Contributor

@kelloggm that sounds good to me.

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