I don't mind fixing my code or suppressing nullness errors, but the cost of the null check itself is hurting my productivity. In the best case, null checks add about ten minutes to the build time for large modules like :sdks:java:core. In the worst case, they cause my build to fail altogether, because the framework logs a warning that "Memory constraints are impeding performance," which is interpreted by -Wall as an error. It might not be a problem on big machines with a lot of memory, but on my Macbook, and on the Github Actions executors it is a real problem. https://issues.apache.org/jira/browse/BEAM-11837
Since nullness checks seem to work fine for now on Jenkins, I propose making them opt-in rather than opt-out, and only run them on Jenkins by default. On Tue, Mar 2, 2021 at 12:13 PM Kyle Weaver <kcwea...@google.com> wrote: > Can you try adding the generated classes to generatedClassPatterns in the > JavaNatureConfiguration? > > > https://github.com/apache/beam/blob/03b883b415d27244ddabb17a0fb5bab147b86f89/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy#L92 > > > On Tue, Mar 2, 2021 at 12:05 AM Reuven Lax <re...@google.com> wrote: > >> I'm running into a problem with this check. I added a protocol-buffer >> file to a module (google-cloud-platform) that previously did have any >> protobuf files in it. The generated files contain lines that violate this >> null checker, so they won't compile. I can't annotate the files, because >> they are codegen files. I tried adding the package to spotbugs-filter.xml, >> but that didn't help. >> >> Any suggestions? >> >> Reuven >> >> On Fri, Jan 22, 2021 at 10:38 AM Brian Hulette <bhule...@google.com> >> wrote: >> >>> >>> >>> On Fri, Jan 22, 2021 at 1:18 AM Jan Lukavský <je...@seznam.cz> wrote: >>> >>>> Hi, >>>> >>>> I'll give my two cents here. >>>> >>>> I'm not 100% sure that the 1-5% of bugs are as severe as other types of >>>> bugs. Yes, throwing NPEs at user is not very polite. On the other hand, >>>> many of these actually boil down to user errors. Then we might ask what a >>>> correct solution would be. If we manage to figure out what the actual >>>> problem is and tell user what specifically is missing or going wrong, that >>>> would be just awesome. On the other hand, if a tool used for avoiding >>>> "unexpected" NPEs forces us to code >>>> >>>> Object value = Objects.requireNonNull(myNullableObject); // or >>>> similar using Preconditions >>>> value.doStuff(); >>>> >>>> instead of just >>>> >>>> myNullableObject.doStuff() >>>> >>>> what we actually did, is a) made a framework happy, and b) changed a >>>> line at which NPE is thrown by 1 (and yes, internally prevented JVM from >>>> thrown SIGSEGV at itself, but that is deeply technical thing). Nothing >>>> changed semantically, from user perspective. >>>> >>> I'd argue there's value in asking Beam developers to make that change. >>> It makes us acknowledge that there's a possibility myNullableObject is >>> null. If myNullableObject being null is something relevant to the user we >>> would likely want to wrap it in some other exception or provide a more >>> useful message than just NPE(!!). >>> >>>> >>>> Now, given that the framework significantly rises compile time (due to >>>> all the checks), causes many "bugs" being reported by static code analysis >>>> tools (because the framework adds @Nonnull default annotations everywhere, >>>> even when Beam's code actually counts with nullability of a field), and >>>> given how much we currently suppress these checks ($ git grep BEAM-10402 | >>>> wc -l -> 1981), I'd say this deserves a deeper discussion. >>>> >>> The reason there are so many suppressions is that fixing all the errors >>> is a monumental task. Rather than addressing them all, Kenn >>> programmatically added suppressions for classes that failed the checks ( >>> https://github.com/apache/beam/pull/13261). This allowed us to start >>> running the checker on the code that passes it while the errors are fixed. >>> >>>> Jan >>>> >>>> >>>> On 1/20/21 10:48 PM, Kenneth Knowles wrote: >>>> >>>> Yes, completely sound nullability checking has been added to the >>>> project via checkerframework, based on a large number of NPE bugs (1-5% >>>> depending on how you search, but many other bugs likely attributable to >>>> nullness-based design errors) which are extra embarrassing because NPEs >>>> have were essentially solved, even in practice for Java, well before Beam >>>> existed. >>>> >>>> Checker framework is a pluggable type system analysis with some amount >>>> of control flow sensitivity. Every value has a type that is either nullable >>>> or not, and certain control structures (like checking for null) can alter >>>> the type inside a scope. The best way to think about it is to consider >>>> every value in the program as either nullable or not, much like you think >>>> of every value as either a string or not, and to view method calls as >>>> inherently stateful/nondetermistic. This can be challenging in esoteric >>>> cases, but usually makes the overall code health better anyhow. >>>> >>>> Your example illustrates how problematic the design of the Java >>>> language is: the analysis cannot assume that `getDescription` is a pure >>>> function, and neither should you. Even if it is aware of >>>> boolean-short-circuit it would not be sound to accept this code. There is >>>> an annotation for this in the cases where it is true (like proto-generate >>>> getters): >>>> https://checkerframework.org/api/org/checkerframework/dataflow/qual/Pure.html >>>> >>>> The refactor for cases like this is trivial so there isn't a lot of >>>> value to thinking too hard about it. >>>> >>>> if (statusCode.equals(Code.INVALID_ARGUMENT) >>>> @Nullable String desc = statusCode.toStatus().getDescription(); >>>> if (desc != null && desc.contains("finalized")) { >>>> return false; >>>> } >>>> } >>>> >>>> To a casual eye, this may look like a noop change. To the analysis it >>>> makes all the difference. And IMO this difference is real. Humans may >>>> assume it is a noop and humans would be wrong. So many times when you >>>> think/expect/hope that `getXYZ()` is a trivial getter method you later >>>> learn that it was tweaked for some odd reason. I believe this code change >>>> makes the code better. Suppressing these errors should be exceptionally >>>> rare, and never in normal code. It is far better to improve your code than >>>> to suppress the issue. >>>> >>>> It would be very cool for the proto compiler to annotate enough that >>>> new-and-improved type checkers could make things more convenient. >>>> >>>> Kenn >>>> >>>> On Mon, Jan 11, 2021 at 8:53 PM Reuven Lax <re...@google.com> wrote: >>>> >>>>> I can use that trick. However I'm surprised that the check appears to >>>>> be so simplistic. >>>>> >>>>> For example, the following code triggers the check on >>>>> getDescription().contains(), since getDescription returns a Nullable >>>>> string. However even a simplistic analysis should realize that >>>>> getDescription() was checked for null first! I have a dozen or so cases >>>>> like this, and I question how useful such a simplistic check it will be. >>>>> >>>>> if (statusCode.equals(Code.INVALID_ARGUMENT) && >>>>> statusCode.toStatus().getDescription() != null && >>>>> statusCode.toStatus().getDescription().contains("finalized")) { return >>>>> false; >>>>> } >>>>> >>>>> >>>>> On Mon, Jan 11, 2021 at 8:32 PM Boyuan Zhang <boyu...@google.com> >>>>> wrote: >>>>> >>>>>> Yeah it seems like the checker is enabled: >>>>>> https://issues.apache.org/jira/browse/BEAM-10402. I used >>>>>> @SuppressWarnings({"nullness" )}) to suppress the error when I think it's >>>>>> not really a concern. >>>>>> >>>>>> On Mon, Jan 11, 2021 at 8:28 PM Reuven Lax <re...@google.com> wrote: >>>>>> >>>>>>> Has extra Nullable checking been enabled in the Beam project? I have >>>>>>> a PR that was on hold for several months, and I'm struggling now with >>>>>>> compile failing to complaints about assigning something that is >>>>>>> nullable to >>>>>>> something that is not nullable. Even when the immediate control flow >>>>>>> makes >>>>>>> it absolutely impossible for the variable to be null. >>>>>>> >>>>>>> Has something changed here? >>>>>>> >>>>>>> Reuven >>>>>>> >>>>>>
