-
Notifications
You must be signed in to change notification settings - Fork 292
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
Allow Library Models to override annotations. #624
Allow Library Models to override annotations. #624
Conversation
Hi @lazaroclapp @msridhar. This PR is ready for review. Thank you. |
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.
Some style/naming/organization change requests, and one more test case needed, but the actual behavior of the change seems good to me!
"-d", | ||
temporaryFolder.getRoot().getAbsolutePath(), | ||
"-XepOpt:NullAway:AnnotatedPackages=com.uber", | ||
"-XepOpt:NullAway:AllowLibraryModelsOverrideAnnotations=true")) |
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.
Worth adding the converse test. A test case showing that, when XepOpt:NullAway:AllowLibraryModelsOverrideAnnotations
is not set (or set to false, if you think we might change the default), the library model for com.uber.Foo
is still ignored.
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.
@@ -709,4 +710,40 @@ public void staticCallZeroArgsNullCheck() { | |||
"}") | |||
.doTest(); | |||
} | |||
|
|||
@Test | |||
public void allowLibraryModelsOverrideAnnotations() { |
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.
Maybe worth it to have test cases for this in a new test class, rather than NullAwayCoreTests
? Maybe a NullAwayCustomLibraryModelsTests.java
(extending NullAwayTestsBase
) which could:
- Have a general method to get a test helper with
TestLibraryModels.class
already in the processor path:makeLibraryModelsTestHelperWithArgs
- Include this test case and its converse (see comment below) in that new class.
- Move
libraryModelsOverrideRestrictiveAnnotations
fromNullAwayAcknowledgeRestrictiveAnnotationsTests
to this new class, so we have all the test cases that loadTestLibraryModels.class
into-processorpath
in the same place.
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.
" return bar();", | ||
" }", | ||
" void run() {", | ||
" // just to make sure, flow analysis is also impacted by library models information", |
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.
👍
@@ -349,4 +351,9 @@ public String getErrorURL() { | |||
public boolean acknowledgeAndroidRecent() { | |||
return acknowledgeAndroidRecent; | |||
} | |||
|
|||
@Override | |||
public boolean allowLibraryModelsOverrideAnnotations() { |
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.
Ok, naming here could be improved. We already allow library models to override annotations (namely, they override restrictive annotations in "unannotated" code - ok, fine, that one was bad naming on our part! 😉 But the point remains...)
I would call this flag:
-XepOpt:NullAway:AcknowledgeLibraryModelsOfAnnotatedCode
for the EP flag(I am also fine with "allow" vs "acknowledge", but I am thinking of consistency with some existing flags)acknowledgeLibraryModelsOfAnnotatedCode()
for the method andacknowledgeLibraryModelsOfAnnotatedCode
for the field (I don't see what "activation" means or adds and it isn't necessary, seeacknowledgeAndroidRecent
for an example of how this looks like)- "Allow Library Models of annotated code" as the PR's title (side note, generally, imperative mood is recommended in commit subjects - this is part of Uber's Git commit message best practices, but I've also seen it as standard practice elsewhere in most major OSS projects. So, "Allow" instead of "Allowing" or "Allows").
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.
Did all the renamings 13eb085
Thank you very much for the note, I will definitely follow this practice in future PRs and commits :)
@@ -292,4 +292,12 @@ public interface Config { | |||
* similarly for {@code @RecentlyNonNull} | |||
*/ | |||
boolean acknowledgeAndroidRecent(); | |||
|
|||
/** | |||
* Checks if {@link LibraryModels} can override annotations on annotated source codes. |
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.
"source code". It's an uncountable noun (like "water"), unless you are talking about "return codes" or "opcodes" or some other discrete set of codes.
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.
Fixed it fa145f2
Removed myself from reviewers as I won't have time to look at this one. But the approach looks good! |
Hi @lazaroclapp sorry I forgot to request a review when I resolved the comments, it is now ready for another round of review. |
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.
Some minor nits on this pass. Test refactoring looks good, though!
* Checks if {@link LibraryModels} can override annotations on annotated source code. | ||
* | ||
* @return true if NullAway should use information provided by {@link LibraryModels} on annotated | ||
* source codes. |
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.
"source code"
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.
@@ -84,6 +84,9 @@ final class ErrorProneCLIFlagsConfig extends AbstractConfig { | |||
static final String FL_FIX_SERIALIZATION_CONFIG_PATH = | |||
EP_FL_NAMESPACE + ":FixSerializationConfigPath"; | |||
|
|||
static final String FL_ALLOW_LIBRARY_MODELS_OVERRIDE_ANNOTATIONS = |
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.
Missed this one on my original list, but let's change the name of this constant to also match the new name of the flag.
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.
private CompilationTestHelper makeLibraryModelsTestHelperWithArgs(List<String> args) { | ||
// Adding directly to args will throw an UnsupportedOperationException. Throughout all tests in | ||
// NullAway, list of args are created via Arrays.asList which the returned list does not support | ||
// addAll. To comply with the rest of the tests, we created tests arguments with the same |
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.
This comment is a bit cumbersome to parse, at least to me, I would rewrite it as:
Adding directly to args will throw an UnsupportedOperationException, since that list is created by calling Arrays.asList (for consistency with the rest of NullAway's test cases), which produces a list which doesn't support add/addAll. Because of this, before we add our additional arguments, we must first copy the list into a mutable ArrayList.
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.
…mipour/NullAway into nimak/library-models-controller
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. Landing it once the build passes 🎉
This PR enables `Annotator` to process downstream dependencies while making decisions for public methods with non-primitive return types. This PR is a follow up for [#624](uber/NullAway#624) Before starting the main process, annotator will do the followings: 1. Collects `public` method with **non-primitive-return-type** 2. Collects regions in downstream dependencies that will potentially introduce a new error if a method in target module is annotated. 3. Constructs the conflict graph and computes the effect in the collected regions for each method. 4. Aggregates the result of analysis on each sub-module and makes a uniformed report. 5. Stores the effect on submodule and will provide the information to main process while making decisions. To activate this feature, flags below must be passed to `Annotator`: 1. `-ddbc or --downstream-dependencies-build-command` path to a file containing all downstream dependencies build commands separated by new line. 2. `-nlmlp or --nullaway-library-model-loader-path` path to library model loader for `NullAway`. This PR also adds javadoc / perform refactoring on huge segment of the code.
This is a follow up to changes in #639, #636, and #624. Here we generalize the former Handler callback: - onUnannotatedInvocationGetExplicitlyNonNullReturn which was used to change the nullability of method returns in unannotated code for the purpose of method `@Override` checks (i.e. subtyping). We provide instead: - onOverrideMethodInvocationReturnNullability Which allows handlers to change the nullability of method returns for both annotated and unannotated code, for the same purposes. Additionally, we implement this new handler callback in `LibraryModelsHandler`. Before this change, marking a return as non-null in a library model affected checking of method invocations, but not necessarily overriding/subtyping checks.
This is a follow up to changes in #639, #636, and #624. Here we generalize the former Handler callback: - onUnannotatedInvocationGetExplicitlyNonNullReturn which was used to change the nullability of method returns in unannotated code for the purpose of method `@Override` checks (i.e. subtyping). We provide instead: - onOverrideMethodInvocationReturnNullability Which allows handlers to change the nullability of method returns for both annotated and unannotated code, for the same purposes. Additionally, we implement this new handler callback in `LibraryModelsHandler`. Before this change, marking a return as non-null in a library model affected checking of method invocations, but not necessarily overriding/subtyping checks.
…#641) This is a follow up to changes in #639, #636, and #624. Here we generalize the former Handler callback: - onUnannotatedInvocationGetExplicitlyNonNullReturn which was used to change the nullability of method returns in unannotated code for the purpose of method `@Override` checks (i.e. subtyping). We provide instead: - onOverrideMethodInvocationReturnNullability Which allows handlers to change the nullability of method returns for both annotated and unannotated code, for the same purposes. Additionally, we implement this new handler callback in `LibraryModelsHandler`. Before this change, marking a return as non-null in a library model affected checking of method invocations, but not necessarily overriding/subtyping checks.
Currently
LibraryModels
can only give information regarding unannotated source code. This PR enablesLibraryModels
to override annotations in an annotated source code. This behavior is controlled by a flag.To enable this feature, the following error prone flag must be passed:
The main motivation for this PR is to prepare the foundation for Annotator to perform upstream API analysis and is an alternative way for GH-620.