Add command line option to skip specific library models. #741
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lazaroclapp
force-pushed
the
lazaro_ommit_library_models
branch
from
ad832f2
to
6d70589
Compare
Pull Request Test Coverage Report for Build #1070
💛 - Coveralls |
msridhar
reviewed
Feb 24, 2023
| @@ -292,6 +294,11 @@ public boolean isContractAnnotation(String annotationName) { | |||
| return contractAnnotations.contains(annotationName); | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean isSkippedLibraryModel(String classDotMethod) { | |||
| return skippedLibraryModels.stream().anyMatch(classDotMethod::equals); | |||
There was a problem hiding this comment.
Why not just skippedLibraryModels.contains(classDotMethod)?
| @@ -788,34 +791,61 @@ public CombinedLibraryModels(Iterable<LibraryModels> models) { | |||
| new ImmutableSetMultimap.Builder<>(); | |||
| for (LibraryModels libraryModels : models) { | |||
| for (Map.Entry<MethodRef, Integer> entry : libraryModels.failIfNullParameters().entries()) { | |||
| if (shouldSkipModel(entry.getKey())) { | |||
There was a problem hiding this comment.
👍 Good that we just pay the cost when loading the models and not after
msridhar
approved these changes
Feb 27, 2023
There was a problem hiding this comment.
@lazaroclapp I went ahead and fixed my one minor comment, so I'm approving now. I'll hold off on landing so you can make sure 7821282 looks fine first
msridhar
added a commit
to NikitaAware/Nikita-Aware-NullAway
that referenced
this pull request
Mar 8, 2023
Serialization version 3: update method serialization to exclude type use annotations and type arguments (uber#735) This PR updates serialization to version `3` and burns version `2`. Changes in serialization version 3: - Type arguments and Type use annotations are excluded from the serialized method signatures. --- Additional context: This PR updates method serialization to ensure only relevant information in a method signature is serialized. Previous to this PR, method signatures serialization was relying on calling `.toString()` method, but if a parameter contains an annotation of type `@Target({TYPE_USE})` the annotation will exist in the `.toSting()` output which is not part of a method signature. This PR updates serialization to ensure exact method signature as expected is serialized. Suggested change suggested changes ternary operator as a method argument Docs: `-XepExcludedPaths` was added in 2.1.3, not 2.13 (uber#744) Add command line option to skip specific library models. (uber#741) This PR adds the CLI option `-XepOpt:NullAway:IgnoreLibraryModelsFor` which takes a list of methods, given as fully qualified class name + method simple name (i.e. independent of argument types). Methods matching this list will be skipped when loading library models (from any implementation of the `LibraryModels`'s `@AutoService` as well as our included models). This can be used in a per-compilation target basis to disable NullAway's library models for ease of upgrading to new versions with stricter modeling of common JDK or third-party APIs. We considered a few alternative approaches here, but decided against: - Simply using another instance of `LibraryModels` to "invert" the models given by NullAway's default library models: This would have required no code changes, but doesn't work in all cases. If NullAway's default models make the return of method `foo()` `@Nullable`, for example, then a new model that makes the return `@NonNull` will break `@Nullable` overrides of `foo()`. In general, we want to go back to "optimistic assumptions" rather than just replace the library model. - We could have a list of methods in the `LibraryModels` for which to ignore previous models, and have those override any models on those methods coming from a different `LibraryModels` implementation. But, from the point of view of the user configuring NullAway, this is complex: they need to have an instance of custom library models in their build, and changing java plugin classpath deps on a per-target basis is more complex than changing CLI arguments (e.g. due to JVM re-use by the build system). - We could provide more specific disabling of library models (e.g. a specific method signature or removing only one particular kind of model from a method, such as keeping the model on the return value, but removing it from an argument, or removing a null-implies-false model or similar). We could revisit this in the future, but supporting this would make the syntax of the CLI values a lot more complex. For now, we believe just turning off all models for a given method is a sufficient degree of granularity. - Per-package/per-class/regex based ignore specs: See above. Avoiding complexity until we need it. Note: If and when this lands, it needs a Wiki documentation update! --------- Co-authored-by: Manu Sridharan <msridhar@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds the CLI option
-XepOpt:NullAway:IgnoreLibraryModelsForwhich takes a list of methods, given as fully qualified class name + method simple name (i.e. independent of argument types).Methods matching this list will be skipped when loading library models (from any implementation of the
LibraryModels's@AutoServiceas well as our included models). This can be used in a per-compilation target basis to disable NullAway's library models for ease of upgrading to new versions with stricter modeling of common JDK or third-party APIs.We considered a few alternative approaches here, but decided against:
LibraryModelsto "invert" the models given by NullAway's default library models: This would have required no code changes, but doesn't work in all cases. If NullAway's default models make the return of methodfoo()@Nullable, for example, then a new model that makes the return@NonNullwill break@Nullableoverrides offoo(). In general, we want to go back to "optimistic assumptions" rather than just replace the library model.LibraryModelsfor which to ignore previous models, and have those override any models on those methods coming from a differentLibraryModelsimplementation. But, from the point of view of the user configuring NullAway, this is complex: they need to have an instance of custom library models in their build, and changing java plugin classpath deps on a per-target basis is more complex than changing CLI arguments (e.g. due to JVM re-use by the build system).Note: If and when this lands, it needs a Wiki documentation update!