On Wed, Mar 17, 2021 at 2:49 AM Jan Lukavský <je...@seznam.cz> wrote:
> If there is no way to configure which annotations should be generated, > then I'd be +1 for removing the checker to separated CI and adding an > opt-in flag for the check when run locally. > Yes. If this answer is correct, then we are out of luck: https://stackoverflow.com/questions/57929137/disable-nonnull-annotation-on-implement-methods A good followup to moving it to a separate CI job would be to attach a full type checking run to the `:check` gradle task. This should be the best place to attach all of our extended checks like spotbugs, spotless, checkstyle, checkerframework. I rarely run `:check` myself (I think it is currently broken in some ways) but it may be good to start. BTW the slowdown we need to solve is local builds, not CI runs. When it was added the slowdown was almost completely prevented by caching. And now that we disable it for test classes (where for some reason it was extra slow) I wonder if it will speed up the main CI runs at all. So I would say the first thing to do is to just disable it by default but enable it in the Jenkins job builders. Kenn We need to solve the issue for dev@ as well, as the undesirable annotations > are already digging their way to the code base: > > git/apache/beam$ git grep UnknownKeyFor > > Another strange thing is that it seems, that we are pulling the > checkerframework as a runtime dependency (at least for some submodules). > When I run `mvn dependency:tree` on one of my projects that uses maven I see > > [INFO] +- com.google.guava:guava:jar:30.1-jre:provided > [INFO] | +- com.google.guava:failureaccess:jar:1.0.1:provided > [INFO] | +- > com.google.guava:listenablefuture:jar:9999.0-empty-to-avoid-conflict-with-guava:provided > [INFO] | +- org.checkerframework:checker-qual:jar:3.5.0:provided > > which is fine, but when I add beam-sdks-java-extensions-euphoria it > changes to > > [INFO] +- > org.apache.beam:beam-sdks-java-extensions-euphoria:jar:2.28.0:compile > [INFO] | \- org.checkerframework:checker-qual:jar:3.7.0:compile > > I'm not a gradle guru, so I cannot tell how this happens, there seems to > be nothing special about euphoria in the gradle. > > Jan > On 3/16/21 7:12 PM, Kenneth Knowles wrote: > > I've asked on checkerframework users mailing list if they or any users > have recommendations for the IntelliJ integration issue. > > It is worth noting that the default annotations inserted into the bytecode > do add value: the @NonNull type annotations are default for > checkerframework but not default for spotbugs. So having the default > inserted enables downstream users to have betters spotbugs heuristic > analysis. Further, the defaults can be adjusted by users so freezing them > at the values we use them at is valuable in general across all tools. > > I think it makes sense to sacrifice these minor value-adds to keep things > simple-looking for IntelliJ users. > > Kenn > > On Tue, Mar 16, 2021 at 10:05 AM Kenneth Knowles <k...@apache.org> wrote: > >> Seems it is an FAQ with no solution: >> https://checkerframework.org/manual/#faq-classfile-annotations >> >> On Tue, Mar 16, 2021 at 10:01 AM Kenneth Knowles <k...@apache.org> wrote: >> >>> Adding -PskipCheckerframework when releasing will solve it for users, >>> but not for dev@. >>> >>> Making it off by default and a separate CI check that turns it on would >>> solve it overall but has the downsides mentioned before. >>> >>> It would be very nice to simply have a flag to not insert default >>> annotations. >>> >>> Kenn >>> >>> On Tue, Mar 16, 2021 at 9:37 AM Jan Lukavský <je...@seznam.cz> wrote: >>> >>>> I believe it is not a problem of Idea. At least I didn't notice >>>> behavior like that with Guava, although Guava uses the framework as well. >>>> Maybe there is a way to tune which annotations should be generated? Or >>>> Guava handles that somehow differently? >>>> On 3/16/21 5:22 PM, Reuven Lax wrote: >>>> >>>> I've also been annoyed at IntelliJ autogenenerating all these >>>> annotations. I believe Kenn said that this was not the intention - maybe >>>> there's an IntelliJ setting that would stop this from happening? >>>> >>>> On Tue, Mar 16, 2021 at 2:14 AM Jan Lukavský <je...@seznam.cz> wrote: >>>> >>>>> I don't know the details of the checkerframework, but there seems a >>>>> contradiction between what code is (currently) generated and what we >>>>> therefore release and what actually the checkerframework states [1]: >>>>> >>>>> @UnknownKeyFor: >>>>> >>>>> Used internally by the type system; should never be written by a >>>>> programmer. >>>>> >>>>> If this annotation is generated for overwritten methods, then I'd say, >>>>> that it means we place a great burden to our users - either not using >>>>> autogenerated methods, or erase all the generated annotations afterwards. >>>>> Either way, that is not "polite" from Beam. >>>>> >>>>> What we should judge is not only a formal purity of code, but what >>>>> stands on the other side is how usable APIs we provide. We should not head >>>>> for 100% pure code and sacrificing use comfort. Every check that leaks to >>>>> user code is at a price and we should not ignore that. No free lunch. >>>>> >>>>> From my point of view: >>>>> >>>>> a) if a check does not modify the bytecode, it is fine and we can use >>>>> it - we are absolutely free to use any tooling we agree on, if our users >>>>> cannot be affected anyhow >>>>> >>>>> b) if a tool needs to be leaked to user, it should be as small >>>>> leakage as possible >>>>> >>>>> c) if a check significantly affects compile performance, it should be >>>>> possible to opt-out >>>>> >>>>> I think that our current setup violates all these three points. >>>>> >>>>> Moving the check to different CI is a possibility (a)), it would then >>>>> require opt-in flag to be able to run the check locally. It would also >>>>> stop >>>>> the leakage (if we would release code without this check). >>>>> >>>>> If we want to keep some annotations for user's benefit (which might be >>>>> fine), it should be really limited to the bare minimum (e.g. only >>>>> @Nullable >>>>> for method arguments and return values, possibly more, I don't know if and >>>>> how we can configure that). Definitely not @UnknownKeyFor, that is simply >>>>> internal to the checker. We should then have opt-out flag for local >>>>> development before committing changes. >>>>> >>>>> Jan >>>>> >>>>> [1] >>>>> https://checkerframework.org/api/org/checkerframework/checker/nullness/qual/UnknownKeyFor.html >>>>> On 3/16/21 8:33 AM, Reuven Lax wrote: >>>>> >>>>> >>>>> >>>>> On Mon, Mar 15, 2021 at 11:42 PM Reuven Lax <re...@google.com> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Mon, Mar 15, 2021 at 9:12 PM Kenneth Knowles <k...@apache.org> >>>>>> wrote: >>>>>> >>>>>>> I will be blunt about my opinions about the general issue: >>>>>>> >>>>>>> - NullPointerExceptions (and similar) are a solved problem. >>>>>>> * They have been since 2003 at the latest [1] (this is when the >>>>>>> types were hacked into Java - the foundation dates back to the 70s or >>>>>>> earlier) >>>>>>> >>>>>> >>>>>> Huh - Fahndrich tried to hire me once to work on a project called >>>>>> Singularity. Small world. Also note that Sanjay Ghemawat is listed in the >>>>>> citations! >>>>>> >>>>> >>>>> Umm, I was confusing names. Fahndrich is actually a former coworker at >>>>> Google :) >>>>> >>>>> >>>>>> >>>>>> >>>>>>> * Checkerframework is a _pluggable_ system where that nullness >>>>>>> type system is a "hello, world" level demo, since 2008 at the latest >>>>>>> [2]. >>>>>>> * Our users should know this and judge us accordingly. >>>>>>> >>>>>>> - Checkerframework should be thought of and described as type >>>>>>> checking, because it is. It is not heuristic nor approximate. >>>>>>> - If your code was unclear about whether something could be null, it >>>>>>> was probably unclear to a person reading it also, and very likely to >>>>>>> have >>>>>>> actual bugs. >>>>>>> - APIs that accept a lot of nullable parameters are, generally >>>>>>> speaking, bad APIs. They are hard to use correctly, less readable, and >>>>>>> very >>>>>>> likely to cause actual bugs. You are forcing your users to deal with >>>>>>> accidental complexity you left behind. >>>>>>> * Corollary to the above two points: Almost all the time, the >>>>>>> changes to clearify nullness make your code better, more readable, >>>>>>> easier >>>>>>> for users or editors. >>>>>>> - It is true that there is a learning curve to programming in this >>>>>>> way. >>>>>>> >>>>>> >>>>>> I agree with the above in a closed system. However as mentioned, some >>>>>> of the APIs we use suffer from this. >>>>>> >>>>>> In a previous life, I saw up close an effort to add such analysis to >>>>>> a large codebase. Two separate tools called Prefix and Prefast were used >>>>>> (the difference between the two is actually quite interesting, but not >>>>>> relevant here). However in order to make this analysis useful, there was >>>>>> a >>>>>> massive effort to properly annotate the entire codebase, including all >>>>>> libraries used. This isn't a perfect example - this was a C++ codebase >>>>>> which is much harder to analyze, and these tools identified far more than >>>>>> simply nullness errors (resource leaks, array indices, proper string null >>>>>> termination, exception behavior, etc.). However the closer we can get to >>>>>> properly annotating the transitive closure of our dependencies, the >>>>>> better >>>>>> this framework will work. >>>>>> >>>>>> >>>>>> >>>>>>> - There are certainly common patterns in Java that do not work very >>>>>>> well, and suppression is sometimes the best option. >>>>>>> * Example: JUnit's @Setup and @Test conventions do not work very >>>>>>> well and it is not worth the effort to make them work. This is actually >>>>>>> because if it were "normal code" it would be bad code. There are complex >>>>>>> inter-method dependencies enforced only by convention. This matters: >>>>>>> sometimes a JUnit test class is called from another class, used as >>>>>>> "normal >>>>>>> code". This does go wrong in practice. Plain old JUnit test cases >>>>>>> frequently go wrong as well. >>>>>>> >>>>>>> And here is my opinion when it comes to Beam: >>>>>>> >>>>>>> - "Community over code" is not an excuse for negligent practices >>>>>>> that cause easily avoidable risk to our users. I will be very >>>>>>> disappointed >>>>>>> if we choose that. >>>>>>> - I think having tooling that helps newcomers write better code by >>>>>>> default is better for the community too. Just like having automatic >>>>>>> formatting is better. Less to haggle about in review, etc. >>>>>>> - A simple search reveals about 170 bugs that we know of [4]. >>>>>>> - So far in almost every module I have fixed I discovered actual new >>>>>>> null errors. Many examples at [5]. >>>>>>> - It is extremely easy to suppress the type checking. Almost all of >>>>>>> our classes have it suppressed already (I did this work, to allow >>>>>>> existing >>>>>>> errors while protecting new code). >>>>>>> - Including the annotations in the shipped jars is an important >>>>>>> feature. Without this, users cannot write null-safe code themselves. >>>>>>> * Reuven highlighted this: when methods are not annotated, we >>>>>>> have to use/implement workarounds. Actually Guava does use >>>>>>> checkerframework >>>>>>> annotations [6] and the problem is that it requires its *input* to >>>>>>> already >>>>>>> be non-null so actually you cannot even use it to convert nullable >>>>>>> values >>>>>>> to non-nullable values. >>>>>>> * Beam has its own [7] that is annotated, actually for yet >>>>>>> another reason: when Guava's checkNotNull fails, it throws NPE when it >>>>>>> should throw IllegalArgumentException. Guava's checkNotNull should not >>>>>>> be >>>>>>> used for input validation! >>>>>>> - It is unfortunate that IntelliJ inserts a bunch of annotations in >>>>>>> user code. I wonder if there is something we can do about that. At the >>>>>>> Java >>>>>>> level, if they are not on the classpath they should be ignored and not >>>>>>> affect users. Coincidentally, the JDK has had NullPointerExceptions in >>>>>>> this >>>>>>> area :-) [8]. >>>>>>> >>>>>>> I understand the pain of longer compile times slowing people down. >>>>>>> That is actually a problem to be solved which does not require lowering >>>>>>> our >>>>>>> standards of quality. How about we try moving it to a separate CI job >>>>>>> and >>>>>>> see how it goes? >>>>>>> >>>>>>> >>>>>> >>>>>>> In my experience stories like Reuven's are much more frustrating in >>>>>>> a separate CI job because you find out quite late that your code has >>>>>>> flaws. >>>>>>> Like when spotless fails, but much more work to fix, and would have been >>>>>>> prevented long ago if it were integrated into the compile. >>>>>>> >>>>>> >>>>>> I agree with this. I prefer to be able to detect on my computer that >>>>>> there are failures, and not have to wait for submission. The complaint >>>>>> was >>>>>> that some people are experiencing trouble on their local machine however, >>>>>> so it seems reasonable to add an opt-out flag (though I would prefer opt >>>>>> out to opt in). >>>>>> >>>>>> >>>>>>> >>>>>>> Kenn >>>>>>> >>>>>>> [1] >>>>>>> https://web.eecs.umich.edu/~bchandra/courses/papers/Fahndrich_NonNull.pdf >>>>>>> [2] >>>>>>> https://homes.cs.washington.edu/~mernst/pubs/pluggable-checkers-issta2008.pdf >>>>>>> [3] >>>>>>> https://github.com/google/guava/blob/fe3fda0ca54076a2268d060725e9a6e26f867a5e/pom.xml#L275 >>>>>>> [4] >>>>>>> https://issues.apache.org/jira/issues/?jql=project%20%3D%20BEAM%20AND%20(summary%20~%20%22NPE%22%20OR%20summary%20~%20%22NullPointerException%22%20OR%20description%20~%20%22NPE%22%20OR%20description%20~%20%22NullPointerException%22%20OR%20comment%20~%20%22NPE%22%20OR%20comment%20~%20%22NullPointerException%22) >>>>>>> [5] https://github.com/apache/beam/pull/12284 and >>>>>>> https://github.com/apache/beam/pull/12162 and >>>>>>> [6] >>>>>>> https://github.com/google/guava/blob/fe3fda0ca54076a2268d060725e9a6e26f867a5e/guava/src/com/google/common/base/Preconditions.java#L878 >>>>>>> [7] >>>>>>> https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/util/Preconditions.java >>>>>>> [8] https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8152174 >>>>>>> >>>>>>> >>>>>>> On Mon, Mar 15, 2021 at 2:12 PM Reuven Lax <re...@google.com> wrote: >>>>>>> >>>>>>>> I have some deeper concerns with the null checks. The fact that >>>>>>>> many libraries we use (including guava) don't always annotate their >>>>>>>> methods >>>>>>>> forces a lot of workarounds. As a very simple example, the return value >>>>>>>> from Preconditions.checkNotNull clearly can never be null, yet the >>>>>>>> nullability checks don't know this. This and other similar cases >>>>>>>> require >>>>>>>> constantly adding extra unnecessary null checks in the code just to >>>>>>>> make >>>>>>>> the checker happy. There have been other cases where I haven't been >>>>>>>> able to >>>>>>>> figure out a way to make the checker happy (often these seem to involve >>>>>>>> using lambdas), and after 10-15 minutes of investigation have given up >>>>>>>> and >>>>>>>> disabled the check. >>>>>>>> >>>>>>>> Now you might say that it's worth the extra pain and ugliness of >>>>>>>> writing "useless" code to ensure that we have null-safe code. However I >>>>>>>> think this ignores a sociological aspect of software development. I >>>>>>>> have a >>>>>>>> higher tolerance than many for this sort of pain, and I'm willing to >>>>>>>> spend >>>>>>>> some time figuring out how to rewrite my code such that it makes the >>>>>>>> checker happy (even though often it forced me to write much more >>>>>>>> awkward >>>>>>>> code). However even I have often found myself giving up and just >>>>>>>> disabling >>>>>>>> the check. Many others will have less tolerance than me, and will >>>>>>>> simply >>>>>>>> disable the checks. At that point we'll have a codebase littered with >>>>>>>> @SuppressWarnings("nullness"), which doesn't really get us where we >>>>>>>> want to >>>>>>>> be. I've seen similar struggles in other codebases, and generally >>>>>>>> having a >>>>>>>> static checker with too many false positives often ends up being worse >>>>>>>> than >>>>>>>> having no checker. >>>>>>>> >>>>>>>> On Mon, Mar 15, 2021 at 10:37 AM Ismaël Mejía <ieme...@gmail.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> +1 >>>>>>>>> >>>>>>>>> Even if I like the strictness for Null checking, I also think that >>>>>>>>> this is adding too much extra time for builds (that I noticed >>>>>>>>> locally >>>>>>>>> when enabled) and also I agree with Jan that the annotations are >>>>>>>>> really an undesired side effect. For reference when you try to auto >>>>>>>>> complete some method signatures on IntelliJ on downstream projects >>>>>>>>> with C-A-v it generates some extra Checkers annotations like >>>>>>>>> @NonNull >>>>>>>>> and others even if the user isn't using them which is not >>>>>>>>> desirable. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Mon, Mar 15, 2021 at 6:04 PM Kyle Weaver <kcwea...@google.com> >>>>>>>>> wrote: >>>>>>>>> >> >>>>>>>>> >> Big +1 for moving this to separate CI job. I really don't like >>>>>>>>> what annotations are currently added to the code we ship. Tools like >>>>>>>>> Idea >>>>>>>>> add these annotations to code they generate when overriding classes >>>>>>>>> and >>>>>>>>> that's very annoying. Users should not be exposed to internal tools >>>>>>>>> like >>>>>>>>> nullability checking. >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > I was only planning on moving this to a separate CI job. The job >>>>>>>>> would still be release blocking, so the same annotations would still >>>>>>>>> be >>>>>>>>> required. >>>>>>>>> > >>>>>>>>> > I'm not sure which annotations you are concerned about. There >>>>>>>>> are two annotations involved with nullness checking, >>>>>>>>> @SuppressWarnings and >>>>>>>>> @Nullable. @SuppressWarnings has retention policy SOURCE, so it >>>>>>>>> shouldn't >>>>>>>>> be exposed to users at all. @Nullable is not just for internal >>>>>>>>> tooling, it >>>>>>>>> also provides useful information about our APIs to users. The user >>>>>>>>> should >>>>>>>>> not have to guess whether a method argument etc. can be null or not, >>>>>>>>> and >>>>>>>>> for better or worse, these annotations are the standard way of >>>>>>>>> expressing >>>>>>>>> that in Java. >>>>>>>>> >>>>>>>>
