Thanks everybody for the opinions. Looks like we all agree on option 2, but nevertheless I'll organize a formal vote after the New Year holidays.
On Wed, Dec 22, 2021 at 9:18 AM Ivan Pavlukhin <vololo...@gmail.com> wrote: > 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 > -- With regards, Aleksandr Polovtcev
