On Fri, Mar 12, 2021 at 3:05 PM Kyle Weaver <[email protected]> wrote:
> > On the other hand, modifying core is a less common developer use case so > passing a couple flags to skip it seems manageable for those people who are > touching the core. > > Every time I check out a new git branch that has a modified core compared > to the previous branch (i.e. almost always) it needs to be rebuilt, and > runs the null checker again. I usually use Intellij to run tests, and I > haven't figured out how to make it pass skipCheckerFramework by default, so > I've resorted to just setting "skipCheckerFramework=true" in my > gradle.properties. > Makes sense. We have had good experiences with spotless and spotbugs so I think it can work as a separate CI job. You might also benefit from having a cache (not just up-to-date checking). Actually the project as a whole would benefit from a shared distributed cache I'm guessing. Kenn > > On Fri, Mar 12, 2021 at 2:53 PM Kenneth Knowles <[email protected]> wrote: > >> I'm OK with this as long as they are treated as strictly merge blocking. >> >> On the other hand, modifying core is a less common developer use case so >> passing a couple flags to skip it seems manageable for those people who are >> touching the core. >> >> Kenn >> >> On Fri, Mar 12, 2021 at 1:19 PM Pablo Estrada <[email protected]> wrote: >> >>> Does it make sense to add a Jenkins precommit suite that runs null >>> checking exclusively? >>> >>> On Fri, Mar 12, 2021 at 11:57 AM Kyle Weaver <[email protected]> >>> wrote: >>> >>>> 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 <[email protected]> >>>> 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 <[email protected]> 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 <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On Fri, Jan 22, 2021 at 1:18 AM Jan Lukavský <[email protected]> >>>>>>> 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 <[email protected]> >>>>>>>> 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 <[email protected]> >>>>>>>>> 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 <[email protected]> >>>>>>>>>> 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 >>>>>>>>>>> >>>>>>>>>>
