JSpecify generics checks for conditional expressions #739
Conversation
msridhar
added
the
jspecify
msridhar
changed the title
msridhar
changed the title
|
@lazaroclapp I've already done some review of this one. Whenever you get time, it'd be great if you could take a look. |
nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java
Outdated
Show resolved
Hide resolved
nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyGenericsTests.java
Show resolved
Hide resolved
| if (!compareNullabilityAnnotations( | ||
| (Type.ClassType) condExprType, (Type.ClassType) truePartType)) { | ||
| reportMismatchedTypeForTernaryOperator( | ||
| truePartTree, condExprType, truePartType, state, analysis); |
There was a problem hiding this comment.
Couldn't we avoid the repeated error message of test ternaryMismatchedAssignmentContext simply by adding an early return here? What am I missing?
There was a problem hiding this comment.
You're right that we could early return and only report one error there. However, I think what we would really want for that test case is an error message saying the conditional expression as a whole has type A<String>, but the context (i.e., the left-hand side of the assignment) requires type A<@Nullable String> which is mismatched. If we early return, we will report the error for the true part of the conditional expression, but not for the expression as a whole, which I think is still not ideal. If you think just reporting for the true part is better than what we do now, we can add the early return.
There was a problem hiding this comment.
Makes sense to keep this as is for now and working on a better reporting location as a follow up, then, I think...
|
Just finished a pass above. Overall this looks good to me, with some nits/comments. Sorry once again for the delay, but I was sick with COVID all of last week (still recovering now). |
|
|
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 <[email protected]>
msridhar
force-pushed
the
generics-ternary-operator-assignment-changes
branch
from
c414cb3 to
dc232c9
Compare
|
@NikitaAware your recent commits were pushed with a wrong email address, so GitHub got confused about the CLA. I tried a force-push to fix this; we'll see if it works. I recommend deleting your local branch and pulling from the remote before making any further changes. |
| if (!compareNullabilityAnnotations( | ||
| (Type.ClassType) condExprType, (Type.ClassType) truePartType)) { | ||
| reportMismatchedTypeForTernaryOperator( | ||
| truePartTree, condExprType, truePartType, state, analysis); |
There was a problem hiding this comment.
Makes sense to keep this as is for now and working on a better reporting location as a follow up, then, I think...
This reverts commit d83b7d0.
This reverts commit d83b7d0.
This reverts commit d83b7d0.
This reverts commit d83b7d0.
This reverts commit d83b7d0.
For a conditional expression e, the type parameter nullability annotations of the true-subpart and false-subpart should match the annotations required by e's context. E.g., if e is the right-hand side of an assignment, the annotations should match those for the type parameters for the assignment's left-hand side:
Other contexts include returns, parameter passing, and being nested inside another conditional expression. In our experience, it seems that
javaccaptures the type parameter nullability required by e's context in the type of e itself. So, our handling of conditional expressions simply checks that the types of both the true and false sub-expressions of e are assignable to (i.e., a subtype of) the type of e, using the same machinery for checking assignments from #715.