Fix handling of `java.lang.Void` and enable a couple more Error Prone checks #586

msridhar · 2022-04-05T20:29:08Z

We now require the developer to write @Nullable on usages of java.lang.Void, consistent with JSpecify (see jspecify/jspecify#51).

This issue was discovered while enabling two new Error Prone checks,
TypeToString and SymbolToString.

coveralls · 2022-04-05T20:33:21Z

Pull Request Test Coverage Report for Build #814

0 of 0 changed or added relevant lines in 0 files are covered.
39 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.02%) to 92.075%

Files with Coverage Reduction	New Missed Lines	%
../nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPath.java	1	95.11%
../nullaway/src/main/java/com/uber/nullaway/NullAway.java	38	93.05%

Totals
Change from base Build #812:	0.02%
Covered Lines:	4775
Relevant Lines:	5186

💛 - Coveralls

lazaroclapp · 2022-04-05T20:36:43Z

build.gradle

@@ -72,6 +72,8 @@ subprojects { project ->
            check("StringSplitter", CheckSeverity.OFF)
            check("WildcardImport", CheckSeverity.ERROR)
            check("MissingBraces", CheckSeverity.ERROR)
+            check("TypeToString", CheckSeverity.ERROR)
+            check("SymbolToString", CheckSeverity.ERROR)


Interesting, these are all suggestion-level, so even with -Werror, we don't get them by default...

Yup. I found these just browsing through the available checks one day a while back

Stephan202 · 2022-04-05T20:40:11Z

nullaway/src/main/java/com/uber/nullaway/NullAway.java

@@ -178,6 +179,8 @@
  // Unmatched, used for when we only want full checker suppressions to work
  static final String CORE_CHECK_NAME = "NullAway.<core>";

+  private static final Supplier<Type> JAVA_LANG_VOID = Suppliers.typeFromString("java.lang.Void");


There's also Suppliers.VOID_TYPE, which looking at the implementation may be more efficient. (Uses state.getSymtab().voidType.)

Oh, or is that the non-boxed variant? In that case, nevermind.

There is Suppliers#JAVA_LANG_VOID_TYPE which I think does what we want. Thanks for the tip!

Missed that one 🤦. Nice!

In the end I realized we really meant to be checking for the unboxed type! 🙂 And there is an isPrimitiveOrVoid() method for doing that.

msridhar · 2022-04-05T21:46:34Z

We now report an error for this code in NullAwayNegativeCases:

NullAway/nullaway/src/test/resources/com/uber/nullaway/testdata/NullAwayNegativeCases.java

Lines 440 to 442 in 4a426db

    
           static Void testVoidType() { 
        
             return null; 
        
           }

I think this is just wrong and we messed up before. In fact, since the return type is java.lang.Void, this method can only return null (since there are no other values assignable to Void), so the return should be annotated @Nullable. Do you agree @lazaroclapp?

lazaroclapp · 2022-04-05T22:12:09Z

I think this is just wrong and we messed up before. In fact, since the return type is java.lang.Void, this method can only return null (since there are no other values assignable to Void), so the return should be annotated @Nullable. Do you agree @lazaroclapp?

Arguably Void == @Nullable Void as @NonNull Void is an empty/error/bottom type... but I wonder how much would need to change in NullAway for there to be a "default-nullable" type/class, vs just saying that if you are returning Void, you must explicitly say @Nullable Void...

lazaroclapp · 2022-04-05T22:14:59Z

@msridhar JSpecify currently says "No special treatment" for Void. So that's good enough for me. @Nullable Void it is. (jspecify/jspecify#51)

Is this technically the first NullAway question, unrelated to direct efforts to support JSpecify, that is settled by referring to the standard? 😉

msridhar · 2022-04-05T22:16:50Z

Is this technically the first NullAway question, unrelated to direct efforts to support JSpecify, that is settled by referring to the standard? 😉

First time! So cool to have a standard 😄

msridhar · 2022-04-05T22:25:04Z

Asking for a new review @lazaroclapp since the main purpose of this PR has changed 🙂 We should note this change in the release notes, as after updating to the next release, users will have to add @Nullable to any java.lang.Void return type

lazaroclapp

LGTM

This reverts the user-visible change from #586. We have found use cases involving generics and method overriding where requiring writing of `@Nullable Void` currently requires adding some ugly error suppressions. We need to think through those cases more carefully before requiring `@Nullable` on `Void`. To be clear, this restored handling of `Void` is unsound. We treat a `Void` return type as `@Nullable Void` when checking `return` statements (so they are allowed to return `null`), but at call sites of methods with a `Void` return type, we assume the method will return a `@NonNull` value. Also, if an overriding method has return type `Void`, we treat it as `@NonNull Void`, which leads to unsound checking of method subtyping.

msridhar added 4 commits January 16, 2022 08:59

WIP

85eb290

Merge branch 'master' into more-ep-checks

f738e05

fix an error

85c78fa

Merge branch 'master' into more-ep-checks

2579beb

lazaroclapp approved these changes Apr 5, 2022

View reviewed changes

Stephan202 reviewed Apr 5, 2022

View reviewed changes

msridhar marked this pull request as ready for review April 5, 2022 20:43

fix check

fe3f616

msridhar enabled auto-merge (squash) April 5, 2022 20:51

fix tests for java.lang.Void

dd72980

msridhar changed the title ~~Enable a couple more Error Prone checks~~ Fix handling of java.lang.Void and enable a couple more Error Prone checks Apr 5, 2022

msridhar requested a review from lazaroclapp April 5, 2022 22:23

msridhar disabled auto-merge April 5, 2022 22:25

lazaroclapp approved these changes Apr 6, 2022

View reviewed changes

msridhar merged commit c9d89bf into master Apr 6, 2022

msridhar deleted the more-ep-checks branch April 6, 2022 01:19

msridhar mentioned this pull request May 7, 2022

Treat Void return type as @Nullable Void when checking return statements #595

Merged

lazaroclapp mentioned this pull request May 7, 2022

JSpecify-style treatment of Void (i.e. not special-casing it) requires generics support #597

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of `java.lang.Void` and enable a couple more Error Prone checks #586

Fix handling of `java.lang.Void` and enable a couple more Error Prone checks #586

msridhar commented Apr 5, 2022 •

edited

Loading

coveralls commented Apr 5, 2022 •

edited

Loading

lazaroclapp Apr 5, 2022

msridhar Apr 5, 2022

Stephan202 Apr 5, 2022

Stephan202 Apr 5, 2022

msridhar Apr 5, 2022

Stephan202 Apr 5, 2022

msridhar Apr 5, 2022

msridhar commented Apr 5, 2022

lazaroclapp commented Apr 5, 2022

lazaroclapp commented Apr 5, 2022 •

edited

Loading

msridhar commented Apr 5, 2022

msridhar commented Apr 5, 2022

lazaroclapp left a comment

Fix handling of java.lang.Void and enable a couple more Error Prone checks #586

Fix handling of java.lang.Void and enable a couple more Error Prone checks #586

Conversation

msridhar commented Apr 5, 2022 • edited Loading

coveralls commented Apr 5, 2022 • edited Loading

Pull Request Test Coverage Report for Build #814

💛 - Coveralls

lazaroclapp Apr 5, 2022

Choose a reason for hiding this comment

msridhar Apr 5, 2022

Choose a reason for hiding this comment

Stephan202 Apr 5, 2022

Choose a reason for hiding this comment

Stephan202 Apr 5, 2022

Choose a reason for hiding this comment

msridhar Apr 5, 2022

Choose a reason for hiding this comment

Stephan202 Apr 5, 2022

Choose a reason for hiding this comment

msridhar Apr 5, 2022

Choose a reason for hiding this comment

msridhar commented Apr 5, 2022

lazaroclapp commented Apr 5, 2022

lazaroclapp commented Apr 5, 2022 • edited Loading

msridhar commented Apr 5, 2022

msridhar commented Apr 5, 2022

lazaroclapp left a comment

Choose a reason for hiding this comment

Fix handling of `java.lang.Void` and enable a couple more Error Prone checks #586

Fix handling of `java.lang.Void` and enable a couple more Error Prone checks #586

msridhar commented Apr 5, 2022 •

edited

Loading

coveralls commented Apr 5, 2022 •

edited

Loading

lazaroclapp commented Apr 5, 2022 •

edited

Loading