Treat Void return type as @Nullable Void when checking return statements #595
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 should we open an issue with the reduced code example you found? |
msridhar
commented
May 7, 2022
| @@ -728,6 +728,12 @@ private Description checkReturnExpression( | |||
| // check for unboxing | |||
| return returnType.isPrimitive() ? doUnboxingCheck(state, retExpr) : Description.NO_MATCH; | |||
| } | |||
| if (ASTHelpers.isSameType(returnType, Suppliers.JAVA_LANG_VOID_TYPE.get(state), state)) { | |||
There was a problem hiding this comment.
This is restoring precisely the behavior we had before #586; we only special-cased Void for return types.
Pull Request Test Coverage Report for Build #844
💛 - Coveralls |
lazaroclapp
approved these changes
May 7, 2022
Might need some more careful rephrasing, but: #597 |
msridhar
changed the title
lazaroclapp
added a commit
that referenced
this pull request
Jun 20, 2022
This is a follow up on #595. See that PR for some of the soundness caveats related to method overriding and the nullability of Void in our current implementation. This does, however, avoid some otherwise awkward suppressions, as shown in #608. There are two correct ways of hadling Void: a) Having it default to `@Nullable` as a type, which requires us adding a "default nullability" for some types/classes and explictly contradicts JSpecify (jspecify/jspecify#51) b) Supporting generics, and requiring explictly annotating `@Nullable Void`. This will probably also require third-party libraries which we consider annotated (such as CF), to adopt this convention. I believe (b) is the way forward long-term, which means that, for now, this hack might be the best we can do without generics support. Once NullAway supports generics, both this change and #595 should be reverted.
lazaroclapp
added a commit
that referenced
this pull request
Jun 21, 2022
This is a follow up on #595. See that PR for some of the soundness caveats related to method overriding and the nullability of Void in our current implementation. This does, however, avoid some otherwise awkward suppressions, as shown in #608. There are two correct ways of hadling Void: a) Having it default to `@Nullable` as a type, which requires us adding a "default nullability" for some types/classes and explictly contradicts JSpecify (jspecify/jspecify#51) b) Supporting generics, and requiring explictly annotating `@Nullable Void`. This will probably also require third-party libraries which we consider annotated (such as CF), to adopt this convention. I believe (b) is the way forward long-term, which means that, for now, this hack might be the best we can do without generics support. Once NullAway supports generics, both this change and #595 should be reverted.
lazaroclapp
added a commit
that referenced
this pull request
Jun 21, 2022
This is a follow up on #595. See that PR for some of the soundness caveats related to method overriding and the nullability of Void in our current implementation. This does, however, avoid some otherwise awkward suppressions, as shown in #608. There are two correct ways of handling Void: a) Having it default to `@Nullable` as a type, which requires us adding a "default nullability" for some types/classes and explicitly contradicts JSpecify (jspecify/jspecify#51) b) Supporting generics, and requiring explicitly annotating `@Nullable Void`. This will probably also require third-party libraries which we consider annotated (such as CF), to adopt this convention. I believe (b) is the way forward long-term, which means that, for now, this hack might be the best we can do without generics support. Once NullAway supports generics, both this change and #595 should be reverted.
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 reverts the user-visible change from #586. We have found use cases involving generics and method overriding where requiring writing of
@Nullable Voidcurrently requires adding some ugly error suppressions. We need to think through those cases more carefully before requiring@NullableonVoid.To be clear, this restored handling of
Voidis unsound. We treat aVoidreturn type as@Nullable Voidwhen checkingreturnstatements (so they are allowed to returnnull), but at call sites of methods with aVoidreturn type, we assume the method will return a@NonNullvalue. Also, if an overriding method has return typeVoid, we treat it as@NonNull Void, which leads to unsound checking of method subtyping.