Make library models override annotations by default. #636
Conversation
This change removes the short-lived `AcknowledgeLibraryModelsOfAnnotatedCode` configuration flag. It sets the behavior of NullAway to always allow library models to override the nullability of annotated code, with an important caveat regarding #445: Whenever we have a method `D.m` which overrides a method `C.m` for which we have a library model, NullAway's behavior depends on whether `D.m` is an annotated or unannotated method: 1) If `D.m` is within unannotated / third-party code, we consider the library model of `C.m` to extend to `D.m` (and to all such third-party overriding methods of `C.m`). This minimizes redundancy in our library models and makes them more robust to the changes within third-party library internals. 2) However, for an overriding `D.m` within annotated code, we don't propagate the library model, requiring either `D.m` to be annotated in a compatible manner or else a separate library model for it. This makes it so that libraries for which the user doesn't have the source code (i.e. can't add annotations), but which they wish to add to the `AnnotatedPackages` list, require more comprehensive library models than "unannotated" libraries. However, this is fully consistent with the intent behind marking code as annotated, which is stricter checking of its nullability.
|
Didn't recall the entire discussion when I made this change, but this ended up being solution (3) from this comment: #453 (comment) I wonder if it's still worth it to take a step back and implement option (2) instead, but (3) seems to handle most use cases. |
Pull Request Test Coverage Report for Build #905
💛 - Coveralls |
I think (3) is better for now. I think we should avoid the complexity of introducing a first-party distinction if we can avoid it. |
Do we already have tests for this somewhere? I'm particularly interested in case 2 and making sure there are override errors for |
Yes we do! In particular, it was this test that reminded me to not just undo #445 by switching the default in the obvious way: NullAwayFrameworkTests.overridingNativeModelsInAnnotatedCodeDoesNotPropagateTheModel For the other case, the tests introduced with the flag cover it, and we just removed the flag but kept the |
|
Thanks! Here's another case. I added this test to @Test
public void testFunctionMethodOverride() {
defaultCompilationHelper
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.google.common.base.Function;",
"import javax.annotation.Nullable;",
"class Test {",
" public static void testFoo() {",
" Function<Object, Object> f = new Function<Object, Object>() {\n" +
" @Override\n" +
" @Nullable\n" +
" public Object apply(Object input) {\n" +
" return null;\n" +
" }\n" +
" };",
" }",
"}"
).doTest();
}Our built-in library models list I was expecting to get an override error about the return type, not the parameter type. Is this expected behavior? Or is this due to a weird interaction with our |
Basically, we have the following two methods which are used for parameter override checking of unannotated code:
Key word being I think the way to move forward here is to simplify those two unrelated chains of callbacks, plus the code that handles the "annotated" case, to a slightly more general One question is whether that should be this same PR or a new one? |
|
Yes, this logic is all getting a bit complicated 😅 I think maybe we should try to write some documentation again on how all these features (annotated packages, library models, flags) interact. I think it's better to address the latest bug in a separate PR as it is in some sense unrelated to this pull request. I'll review the code in this PR soon. |
| // When looking up library models of annotated code, we match the exact method signature only, | ||
| // overriding implementations that share the same model must be explicitly given their own |
There was a problem hiding this comment.
| // When looking up library models of annotated code, we match the exact method signature only, | |
| // overriding implementations that share the same model must be explicitly given their own | |
| // When looking up library models of annotated code, we match the exact method signature only; | |
| // overriding methods in subclasses must be explicitly given their own |
lazaroclapp
deleted the
lazaro_flip_default_library_models_annotated_code
branch
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.
This change removes the short-lived
AcknowledgeLibraryModelsOfAnnotatedCodeconfiguration flag.
It sets the behavior of NullAway to always allow library models to override
the nullability of annotated code, with an important caveat regarding #445:
Whenever we have a method
D.mwhich overrides a methodC.mfor which wehave a library model, NullAway's behavior depends on whether
D.mis anannotated or unannotated method:
D.mis within unannotated / third-party code, we consider the librarymodel of
C.mto extend toD.m(and to all such third-party overridingmethods of
C.m). This minimizes redundancy in our library models and makesthem more robust to the changes within third-party library internals.
D.mwithin annotated code, we don't propagatethe library model, requiring either
D.mto be annotated in a compatiblemanner or else a separate library model for it.
This makes it so that libraries for which the user doesn't have the source code
(i.e. can't add annotations), but which they wish to add to the
AnnotatedPackageslist, require more comprehensive library models than "unannotated" libraries.
However, this is fully consistent with the intent behind marking code as annotated,
which is stricter checking of its nullability.