Restrictive annotated method overriding #249
Conversation
…ck method overriding
…ck method overriding
…:shas19/NullAway into restrictive_annotated_method_overridding
| NullAway analysis, | ||
| VisitorState state, | ||
| Symbol.MethodSymbol methodSymbol, | ||
| ImmutableSet<Integer> explicitlyNullablePositions) { |
There was a problem hiding this comment.
Nice! This is piggybacking on the logic for the LibraryModelsHandler which does handle overriding, right?
There was a problem hiding this comment.
Yes. Also is it possible that in future we might want to handle nullness of return values in LibraryModelsHandler?
There was a problem hiding this comment.
You mean for overriding? The case of handling use of null return values from library models is done by onOverrideMayBeNullExpr.
It might be worth adding onUnannotatedInvocationGetExplicitlyNonNullReturn to that handler as well, though, want to do it as another short PR?
There was a problem hiding this comment.
Yeah, I'll go for another short PR
| // @NonNull return | ||
| // values if needed. | ||
| final boolean isOverridenMethodUnannotated = | ||
| NullabilityUtil.isUnannotated(overriddenMethod, config); | ||
| boolean overriddenMethodReturnsNonNull = |
There was a problem hiding this comment.
Actually, I like your final above, can we make this final too?
| "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated", | ||
| "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true")) | ||
| .addSourceLines( | ||
| "Test.java", |
There was a problem hiding this comment.
Suggestion: Rename this to TestNegativeCases.java and the class accordingly. Would make the test easier to read.
| " @Override public @Nullable Object returnsNullable2() { return new Object();}", | ||
| "}") | ||
| .addSourceLines( | ||
| "Test2.java", |
There was a problem hiding this comment.
TestPositiveCases.java?
Following changes are addressed,
If the restrictive annotations are turned on then
@Nullablereturn and the third-party method is restrictively annotated as@NonNullthen triggers an error.@Nullablebut the same position argument to the overridden third-party method is restrictively annotated as@Nullablethen also triggers an error.Fixes #236
Added unit tests.