Let's clarify what we talk about when we say "presumed" about non-annotated
arguments - my opinion below.
>From my POV, for human developers presuming @Nonnull would be natural. I
certainly do that (unless javadoc is explicit). The "natural" aspect, IMHO,
comes from the fact that returning a value normally means returning some
substantial information (i.e. not null). The null case is not natural,
therefore it needs to be annotated explicitly.
IDEs, AFAIK, also presume @Nonnull for unannotated cases... At least IJ
does not flag potential NPE issues for these values.
Re: "trust" - I do not think we can trust annotations either way. They help
to reduce the likelihood of NPE mistakes, but given current Polaris CI,
they do not guarantee anything regardless of how and where we put them.
Performing reliable nullness code flow analysis is not a trivial task (e.g.
it requires analyzing compiled library code) and will probably require
efforts that outweigh the benefits.
Cheers,
Dmitri.
On Mon, Jun 2, 2025 at 11:49 AM Eric Maynard <eric.w.mayn...@gmail.com>
wrote:
> I think frequency is a hugely important goal, behind perhaps only accuracy
> (if I see an annotation or a lack of one, can I trust it?).
>
> The largest group of parameters we currently have is certainly
> non-annotated arguments, no? And those arguments are currently presumed
> nullable?
>
> --EM
>
> On Mon, Jun 2, 2025 at 8:42 AM Alex Dutra <alex.du...@dremio.com.invalid>
> wrote:
>
> > Hi all,
> >
> > > So now, the onus is on me, as the developer to write [...]
> >
> > But this remark is valid both ways, right? If the default is non-null
> > and my method can return null, the onus is on me to annotate it with
> > @Nullable; but if the default is nullable and my method cannot return
> > null, then the onus is again on me to annotate it with @Nonnull.
> >
> > What really matters for me is: how **frequently** would I need to
> > annotate things with @Nullable if we assume non-null by default,
> > versus how frequently would I need to annotate things with @Nonnull if
> > we assume nullable by default?
> >
> > From a quick glance, it seems to me we have more non-null arguments
> > than nullable ones.
> >
> > Thanks,
> > Alex
> >
> > On Wed, May 28, 2025 at 2:10 AM Dmitri Bourlatchkov <di...@apache.org>
> > wrote:
> > >
> > > The thing is that IntelliJ, for example, does not infer @Nullable very
> > well.
> > >
> > > For example:
> > >
> > > Map<String, String> m = new HashMap<>();
> > > private @Nullable String value() {
> > > return m.get("key");
> > > }
> > >
> > > private void test() {
> > > String value = value();
> > > if (value.length() > 3) {
> > > m.put("key", value);
> > > };
> > > }
> > >
> > > This code gives a warning on `value.length()`. However, if you remove
> > > @Nullable the warning disappears.
> > >
> > > So the onus is on the developer to put @Nullable where relevant. Then
> > IDEs
> > > will help.
> > >
> > > If we accept that, assuming @Nonnull by default is the next logical
> step.
> > >
> > > Cheers,
> > > Dmitri.
> > >
> > > On Tue, May 27, 2025 at 7:50 PM Michael Collado <
> collado.m...@gmail.com>
> > > wrote:
> > >
> > > > The concern, for me, is whether the onus is on the developer or on
> the
> > > > tools. For example, if I write a method:
> > > >
> > > > public String getTheValue() { return map.get("value");}
> > > >
> > > > with no annotation, the assumption is the return value of getTheValue
> > is
> > > > always non-null even though java.util.Map does not make any such
> > guarantee.
> > > > So now, the onus is on me, as the developer to write
> > > >
> > > > public @Nullable String getTheValue() { return map.get("value");}
> > > >
> > > > I can do this, but the reality is that I'm lazy and I'm probably
> going
> > to
> > > > forget and there's a high likelihood that nobody is going to call me
> > out on
> > > > it during code review because it's one small function in a 5,000 line
> > PR :)
> > > > For me, I'd much rather have the static analysis tool assume that
> > anything
> > > > not annotated might possibly be null so that when I write the
> > following, I
> > > > get a bug during the build:
> > > >
> > > > String foo = getTheValue();
> > > > return foo.length();
> > > >
> > > > Now the tool is actually doing its job of reminding me that I'm
> using a
> > > > value without having checked for nullability and if I want to avoid
> > that
> > > > warning (maybe because I'm using a special java.util.Map), I need to
> > update
> > > > my annotations. Given that the Iceberg codebase doesn't use these
> > > > annotations, I think we'll be dealing with nullable values far more
> > often
> > > > than non-nullable ones.
> > > >
> > > > Mike
> > > >
> > > > On Tue, May 27, 2025 at 2:25 PM Eric Maynard <
> eric.w.mayn...@gmail.com
> > >
> > > > wrote:
> > > >
> > > > > 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
> > > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> >
>
