Val, > Basically, what I'm suggesting is to accept non-nullable as default
Sounds good for me. Then it should be clearly stated in code guidelines and regular static analysis for this should be organized from the beginning. 2021-12-21 20:35 GMT+03:00, Valentin Kulichenko <valentin.kuliche...@gmail.com>: > Ivan, > > I agree with you. Basically, what I'm suggesting is to accept non-nullable > as default, but if a parameter must be nullable for whatever reason, mark > it with the @Nullable annotation. > > Regarding "a user can put anything there without much thinking": I > understand what you mean, but I believe the annotation is not the problem. > It's nullability itself that causes the ambiguity - null value is always a > special case that means different things for different values. @Nullable at > least sends a signal that nulls need to be considered. If a user doesn't > want to think anyway, there is not much we can do :) (Except trying to make > the variable non-nullable, of course) > > -Val > > On Mon, Dec 20, 2021 at 11:40 PM Ivan Pavlukhin <vololo...@gmail.com> > wrote: > >> Val, >> >> > Therefore, it's crucial to bring the attention of the API's user to >> > such >> parameters. (@Nullable) >> >> This sounds wrong for me. If a method parameter is marked as >> @Nullable, then a user can put anything there without much thinking. >> Opposite happens with @NotNull parameters, with them a user should >> think whether his variable can be null or not. >> >> As for me using nullable variables should be avoided as much as >> possible, but not all developers share this point of view. And >> especially in Ignite codebase it was quite common to use nullable >> variables, for the first time it was very unusual for me. >> >> 2021-12-20 22:06 GMT+03:00, Valentin Kulichenko < >> valentin.kuliche...@gmail.com>: >> > Neither solution is completely bulletproof, and I don't think that's >> > what >> > we are looking for. We need something that provides a reasonable value, >> but >> > also does not clutter the code with too many annotations. >> > >> > I would also keep in mind that annotations are used not only for static >> > analysis, but by developers as well. And from this standpoint, I feel >> > that @Nullable is much more important, because 'null' is a special >> > value >> > which is treated differently in different circumstances. Therefore, >> > it's >> > crucial to bring the attention of the API's user to such parameters. On >> the >> > other hand, non-nullable parameters are inherently safer, so there is >> less >> > need to annotate them. >> > >> > -Val >> > >> > On Mon, Dec 20, 2021 at 7:42 AM Alexander Polovtcev >> > <alexpolovt...@gmail.com> >> > wrote: >> > >> >> Hi, Ivan. >> >> >> >> > it seems not bulletproof >> >> >> >> I completely agree with you. As I wrote in the original message, this >> >> becomes even worse in case of 3-rd party dependencies, because they >> >> may >> >> not >> >> be annotated, which can lead to confusions. But looks like this is not >> >> a >> >> big deal, because these annotations are not intended to cover 100% of >> >> cases >> >> and should not be treated as a guarantee against NPEs. Maybe covering >> >> *some* cases is enough. >> >> >> >> > Perhaps we can insist on not using nulls as much as possible. >> >> >> >> Same here, I agree with you and it correlates with clear API design in >> >> general. However, sometimes nullable parameters and return values are >> >> justified, so how can we formalize that? >> >> >> >> On Sat, Dec 18, 2021 at 10:52 AM Ivan Pavlukhin <vololo...@gmail.com> >> >> wrote: >> >> >> >> > Hi, >> >> > >> >> > While option #2 looks very appealing it seems not bulletproof >> >> > reliable, someone can occasionally miss @Nullable annotation. Option >> >> > #3 seems more practical too me, as missed @NotNull annotations >> >> > cannot >> >> > do much harm. >> >> > >> >> > Also I am thinking about using nullable parameters in general. >> >> > Perhaps >> >> > we can insist on not using nulls as much as possible. With such >> >> > requirement in guidelines option #2 becomes practical. >> >> > >> >> > 2021-12-17 14:47 GMT+03:00, Alexander Polovtcev >> >> > <alexpolovt...@gmail.com >> >> >: >> >> > > Maksim, thank you for the suggestion. >> >> > > >> >> > > I've never used NullAway, but after having a quick look I think it >> >> might >> >> > be >> >> > > an overkill, since it is a plugin for the ErrorProne, which is a >> >> separate >> >> > > tool. I recall some efforts of introducing ErrorProne to Ignite 3 >> and >> >> > they >> >> > > were not successful. But again, I don't have much experience in >> >> > > that >> >> > > regard. I wonder if IDEA inspections would be enough, since they >> >> > > are >> >> easy >> >> > > to use during development and AFAIK are already run during the TC >> >> builds. >> >> > > >> >> > > Regarding Ignite 2, I don't know if it would be viable to add >> >> > > these >> >> > > annotations to the existing code (in order to have meaningful >> >> > > checks), >> >> > > since the codebase is so large. But nevertheless we can try to >> >> > > adopt >> >> the >> >> > > same approach there as well. >> >> > > >> >> > > On Fri, Dec 17, 2021 at 12:10 PM Maksim Timonin < >> >> timoninma...@apache.org >> >> > > >> >> > > wrote: >> >> > > >> >> > >> Hi! >> >> > >> >> >> > >> There is a pretty popular project NullAway [1] that checks code >> >> > >> of >> a >> >> > >> project in compile-time for possible NPE. AFAIK, it works only >> >> > >> with >> >> the >> >> > >> "@Nullable" annotation. I think we can try to add this check to >> >> Ignite2 >> >> > >> and >> >> > >> 3. >> >> > >> >> >> > >> But I wonder, whether smbd already tried to introduce this check? >> If >> >> > not, >> >> > >> I >> >> > >> can try, WDYT? >> >> > >> >> >> > >> [1] https://github.com/uber/NullAway >> >> > >> >> >> > >> On Fri, Dec 17, 2021 at 9:04 AM ткаленко кирилл >> >> > >> <tkalkir...@yandex.ru >> >> > >> >> > >> wrote: >> >> > >> >> >> > >> > I'm for the second option. >> >> > >> > >> >> > >> >> >> > > >> >> > > >> >> > > -- >> >> > > With regards, >> >> > > Aleksandr Polovtcev >> >> > > >> >> > >> >> > >> >> > -- >> >> > >> >> > Best regards, >> >> > Ivan Pavlukhin >> >> > >> >> >> >> >> >> -- >> >> With regards, >> >> Aleksandr Polovtcev >> >> >> > >> >> >> -- >> >> Best regards, >> Ivan Pavlukhin >> > -- Best regards, Ivan Pavlukhin
