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

Add library model for Guava's Closer.register #632

Merged
merged 1 commit into from
Aug 5, 2022

Conversation

lazaroclapp
Copy link
Collaborator

This is a follow up to #628 and our support for @ParametricNullness in NullAway 0.9.9.
It turns out that com.google.common.io.Closer annotates it's register method as:

  @CanIgnoreReturnValue
  @ParametricNullness
  public <C extends @Nullable Closeable> C register(@ParametricNullness C closeable) {...}

Where the return is only really @Nullable if the argument is @Nullable (it is the same reference).

This PR adds a Library Model for this method, which is equivalent to adding the @Contract("null -> null")
annotation to it.

Note that, in order for this model to be helpful, AcknowledgeLibraryModelsOfAnnotatedCode
must be set, which would seem to argue for making that the default.

@lazaroclapp
Copy link
Collaborator Author

Ran into this case while upgrading internally (used in tooling, so my usual sanity tests pre-release missed it 😅 )

@coveralls
Copy link

Pull Request Test Coverage Report for Build #901

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 92.49%

Totals Coverage Status
Change from base Build #900: 0.001%
Covered Lines: 4803
Relevant Lines: 5193

💛 - Coveralls

@msridhar
Copy link
Collaborator

msridhar commented Aug 5, 2022

Note that, in order for this model to be helpful, AcknowledgeLibraryModelsOfAnnotatedCode
must be set, which would seem to argue for making that the default.

Maybe we should just get rid of the flag entirely? I'm not sure how likely it is that a user will have a library model for annotated code and will prefer the annotations in the code. I'm also fine with turning the flag on by default if we want to preserve the option.

@lazaroclapp
Copy link
Collaborator Author

lazaroclapp commented Aug 5, 2022

Note that, in order for this model to be helpful, AcknowledgeLibraryModelsOfAnnotatedCode
must be set, which would seem to argue for making that the default.

Maybe we should just get rid of the flag entirely? I'm not sure how likely it is that a user will have a library model for annotated code and will prefer the annotations in the code. I'm also fine with turning the flag on by default if we want to preserve the option.

I think for next release we should keep the flag and just flip the default just in case. I do worry that we will forget and never remove the flag, though... in either case, will make that as a separate PR if only for better Changelog tracking 🙂

@lazaroclapp lazaroclapp merged commit a8051cd into master Aug 5, 2022
@lazaroclapp lazaroclapp deleted the lazaro_guava_31_model_closer branch August 5, 2022 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants