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 >
