Right, but I think Michael is correct that when something comes from
outside of Polaris, we have to assume it’s nullable even if it’s not
annotated as such.
Also as we are using CDI, it may not be clear whether an implementation of
an interface is coming from “Polaris code” and therefore what assumptions
can be made about the nullability of its members.
Given these considerations, I think developers are forced to do their own
due diligence for nullability when working with non-annotated members. If
we would go with option 1, this means developers are encouraged to do the
opposite and forego a nullability check on non-annotated members. That
seems dangerous.
Another argument against option 1 is that for better or for worse most
variables in Polaris are in fact nullable. If we have the choice between
annotating 90% of variables and annotating 10%, I prefer the latter.
On Tue, May 27, 2025 at 2:12 PM Dmitri Bourlatchkov
<dmitri.bourlatch...@dremio.com.invalid> wrote:
> Yes, the idea is to simplify things by using only one of @Nullable
> and @Nonnull in Polaris code.
>
> However, I lean toward using @Nullable when necessary and assume @Nonnull
> by
> default. Again this is only for Polaris code.
>
> Cheers,
> Dmitri.
>
> On Tue, May 27, 2025 at 5:07 PM Eric Maynard <eric.w.mayn...@gmail.com>
> wrote:
>
> > If we've checked something is non-null or received a non-null value from
> a
> > library, we can continue annotating it as @Nonnull within Polaris. We can
> > also use Optional to get similar functionality to typescript's ? and
> handle
> > missing values that way.
> >
> > In all other cases, developers must assume any given reference can be
> null
> > and do their own checks. It seems to me that there is no need for
> @Nullable
> > in that case. That is, if we label everything that cannot be null as
> > @Nonnull and everything that can be null as @Nullable, then every
> reference
> > should be annotated as one or the other. We can simplify things by just
> > removing the @Nullable annotations.
> >
> > --EM
> >
> > On Tue, May 27, 2025 at 1:56 PM Dmitri Bourlatchkov <di...@apache.org>
> > wrote:
> >
> > > Your example is quite valid, Mike. In my proposal (several emails ago)
> > I'd
> > > classify this as "inputs into Polaris code".
> > >
> > > However, I guess the topic of this thread is slightly different. The
> > > question we're trying to reach consensus on, is about what we should
> > > annotate as @Nullable in Polaris code.
> > >
> > > I'd say if a value is received from external code, which is not
> > annotated,
> > > and returned from a Polaris method without validation, and the returned
> > > value can reasonably be null, then the Polaris method should be
> annotated
> > > as @Nullable. However, if Polaris code checks the value to be non-null,
> > the
> > > return value need not be annotated in Polaris.
> > >
> > > Does this sound reasonable?
> > >
> > > Thanks,
> > > Dmitri.
> > >
> > > On Tue, May 27, 2025 at 4:34 PM Michael Collado <
> collado.m...@gmail.com>
> > > wrote:
> > >
> > > > I generally assume the other way around. Many (most) libraries don't
> > > > annotate the return values of their methods, so I assume everything
> is
> > > > nullable unless specifically told otherwise. I would prefer
> everything
> > be
> > > > non-nullable unless specifically stated (the ? is the one thing I
> would
> > > > steal from typescript if I could), but given that library code can't
> be
> > > > modified to be explicit when null return values are possible, I don't
> > > think
> > > > we can make that assumption.
> > > >
> > > > Mike
> > > >
> > > > On Thu, May 22, 2025 at 10:57 AM Dmitri Bourlatchkov <
> di...@apache.org
> > >
> > > > wrote:
> > > >
> > > > > My preference is for option 1 below.
> > > > >
> > > > > On Thu, May 22, 2025 at 1:53 PM Alex Dutra
> > > <alex.du...@dremio.com.invalid
> > > > >
> > > > > wrote:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > My proposal was centered around compile-time checks and targets
> > > mostly
> > > > > > developers and contributors. I am not questioning the usefulness
> of
> > > > > > runtime checks when these make sense.
> > > > > >
> > > > > > Maybe an example is better than a thousand words. Let's imagine a
> > > > > > simple getOrDefault() method. Which version do you prefer?
> > > > > >
> > > > > > 1. Annotate only nullable items:
> > > > > > public String getOrDefault(@Nullable String s, String def) {
> > return s
> > > > > > == null ? def : Objects.requireNonNull(def); }
> > > > > >
> > > > > > 2. Annotate only non-null items:
> > > > > > @Nonnull public String getOrDefault(String s, @Nonnull String
> def)
> > {
> > > > > > return s == null ? def : Objects.requireNonNull(def); }
> > > > > >
> > > > > > 3. Annotate everything:
> > > > > > @Nonnull public String getOrDefault(@Nullable String s, @Nonnull
> > > > > > String def) { return s == null ? def :
> > Objects.requireNonNull(def); }
> > > > > >
> > > > > > Many places in Polaris are using option 3, which is too verbose
> and
> > > > > > leads to visual fatigue. What I was suggesting to the community
> is
> > to
> > > > > > adopt option 1, that reduces the visual clutter and also assumes
> > > > > > non-null by default.
> > > > > >
> > > > > > (You will notice that I added a runtime check to all three
> > versions.)
> > > > > >
> > > > > > Hope that helps to clarify the discussion.
> > > > > >
> > > > > > Alex
> > > > > >
> > > > > > On Wed, May 21, 2025 at 8:54 PM Yufei Gu <flyrain...@gmail.com>
> > > wrote:
> > > > > > >
> > > > > > > >
> > > > > > > > In my opinion, assuming everything is nullable by default
> isn't
> > > the
> > > > > > > > best approach for writing robust code. I believe a bias
> towards
> > > > > > > > non-nullness leads to more reliable systems.
> > > > > > >
> > > > > > > I agreed with the intention, but am concerned that assuming
> > > > everything
> > > > > is
> > > > > > > non-nullness may discourage null-checking, which is problematic
> > as
> > > > > > runtime
> > > > > > > null-checking isn't a thing.
> > > > > > >
> > > > > > > Yufei
> > > > > > >
> > > > > > >
> > > > > > > On Wed, May 21, 2025 at 9:52 AM Alex Dutra
> > > > > <alex.du...@dremio.com.invalid
> > > > > > >
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi Eric,
> > > > > > > >
> > > > > > > > You're right that annotations don't change the bytecode at
> > > runtime.
> > > > > > > > Their real value comes during compilation, as many static
> > > analysis
> > > > > > > > tools use them to flag potential issues. They can even cause
> > > build
> > > > > > > > failures depending on how you configure them. My IDE
> (IntelliJ)
> > > > > > > > frequently warns me when I forget to handle a potential NPE;
> if
> > > > > you're
> > > > > > > > not seeing similar feedback, it might be worth checking your
> > IDE
> > > > > > > > settings.
> > > > > > > >
> > > > > > > > While the annotations are primarily for compile-time checks,
> > that
> > > > > > > > doesn't mean we can't also incorporate runtime checks. We
> > should
> > > > aim
> > > > > > > > to include these whenever this makes sense, for example by
> > using
> > > > > > > > Guava's Preconditions. This is especially useful if we can't
> > > > > guarantee
> > > > > > > > that a method parameter, for instance, will never be null,
> > > because
> > > > > > > > it's being provided by some external system.
> > > > > > > >
> > > > > > > > In my opinion, assuming everything is nullable by default
> isn't
> > > the
> > > > > > > > best approach for writing robust code. I believe a bias
> towards
> > > > > > > > non-nullness leads to more reliable systems.
> > > > > > > >
> > > > > > > > I am also a big fan of Optional and think we should strive to
> > use
> > > > it
> > > > > > > > as much as possible. That said, it's not always possible,
> > > > especially
> > > > > > > > if you are implementing a third-party interface that doesn't
> > use
> > > > it.
> > > > > > > > Using Optional in class fields and method parameters is also
> > > > > > > > controversial: Optional was designed primarily as a signal
> from
> > > the
> > > > > > > > callee to the caller, to signify: "no result". In other
> words,
> > > its
> > > > > > > > main purpose is to clarify method return types. This post on
> > > Stack
> > > > > > > > Overflow by Brian Goetz is worth reading: [1].
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > >
> > > > > > > > Alex
> > > > > > > >
> > > > > > > > [1]:
> > > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> https://stackoverflow.com/questions/26327957/should-java-8-getters-return-optional-type/26328555#26328555
> > > > > > > >
> > > > > > > >
> > > > > > > > On Wed, May 21, 2025 at 4:37 PM Eric Maynard <
> > > > > eric.w.mayn...@gmail.com
> > > > > > >
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > Thanks for bringing this up as I’ve been confused by this a
> > few
> > > > > > times.
> > > > > > > > >
> > > > > > > > > Before Polaris I hadn’t really encountered these
> annotations
> > > and
> > > > I
> > > > > > was
> > > > > > > > > surprised to learn they don’t “do anything” — that is,
> there
> > is
> > > > no
> > > > > > > > > additional safety you get at runtime when a null value is
> > > passed
> > > > > > into a
> > > > > > > > > parameter marked non-null. Similarly nothing enforces that
> > you
> > > > > handle
> > > > > > > > null
> > > > > > > > > values when something is annotated as nullable.
> > > > > > > > >
> > > > > > > > > For that reason, I tend to assume everything is nullable
> > > > regardless
> > > > > > of
> > > > > > > > > annotation and I would be in favor of standardizing around
> > that
> > > > > > > > assumption.
> > > > > > > > > Iff something is annotated as Non-null a developer should
> > feel
> > > > safe
> > > > > > > > > skipping a check for null, but otherwise they should handle
> > > null.
> > > > > > > > >
> > > > > > > > > I am a big fan of Optional and of trying to reduce the
> usage
> > of
> > > > > null
> > > > > > as
> > > > > > > > > much as possible though.
> > > > > > > > >
> > > > > > > > > On Wed, May 21, 2025 at 3:02 PM Alex Dutra
> > > > > > <alex.du...@dremio.com.invalid
> > > > > > > > >
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi all,
> > > > > > > > > >
> > > > > > > > > > A while ago, we had a discussion regarding which nullness
> > > > > > annotations
> > > > > > > > > > to use and whether we should consider favoring non-null
> by
> > > > > > default. I
> > > > > > > > > > would like to revive that discussion.
> > > > > > > > > >
> > > > > > > > > > We are currently using the `jakarta.annotation` package
> > > > > > consistently,
> > > > > > > > > > but the defaults are not clear: should we consider
> > everything
> > > > as
> > > > > > > > > > non-null by default and only annotate the nullable
> things,
> > or
> > > > the
> > > > > > > > > > other way around? Some classes are cluttered with both
> > > > > annotations,
> > > > > > > > > > which seems unnecessary and confusing.
> > > > > > > > > >
> > > > > > > > > > I would personally be in favor of considering everything
> as
> > > > > > non-null by
> > > > > > > > > > default.
> > > > > > > > > >
> > > > > > > > > > Please let me know your thoughts.
> > > > > > > > > >
> > > > > > > > > > Alex
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
