Nullness of Throwable.getMessage() cannot be overridden in subclasses

[ConradScott]

NullAway models Throwable.getMessage() as returning @Nullable. I expected to be able to narrow this so that in a subclass the method would return @Nonnull but NullAway still considers the override to return @Nullable, which doesn't seem correct to me.

For example, the following example generates the error Example.java:[14,40] [NullAway] passing @Nullable parameter 'e.getMessage()' where @NonNull is required.

package com.example;

import javax.annotation.Nonnull;
import java.util.Objects;

public final class Example
{
    private Example() {}

    public static void main(final String[] args)
    {
        final Subclass e = new Subclass("Not null");

        throw new Subclass(e.getMessage());
    }

    private static final class Subclass extends RuntimeException
    {
        private static final long serialVersionUID = -8142164305649493896L;

        Subclass(final String message)
        {
            super(message);
        }

        @Nonnull
        @Override
        public String getMessage()
        {
            return Objects.requireNonNull(super.getMessage());
        }
    }
}

I thought I must have misunderstood something and that this behaviour is deliberate, but if I use my own superclass / subclass pair with a similar @Nullable method return in the parent class and @Nonnull in the subclass, it works as I'd expect with no warning from NullAway (and exactly this sort of example is used in the NullAway whitepaper on covariant method overrides).

I'm using NullAway 0.8.0 with ErrorProne 2.4.0 in Java 11. The only NullAway option I'm using is to set the annotated packages.

[lazaroclapp]

I can repro this issue and it's not immediately obvious to me that this should be expected behavior at all. Investigating a bit further.

As a small note, @Nonnull is not needed for annotated packages in NullAway: No annotation means non-null already :)

[lazaroclapp]

Ok, figured this one out:

When handling nullness set via a library model, NullAway will scan upwards to see if a method overrides one of the methods defined in the library model. That is, for the specific case here, when it sees .getMessage() it will traverse the class hierarchy until it eventually hits Throwable.getMessage() and sees it is defined as returning nullable in a model.

This is actually unambiguously what we want for third-party (unannotated) subclasses of third-party classes, otherwise our library models need to be exhaustive over every single exception class.

It's a bit less clear whether this is what we want for first-party / annotated code, or if we should just rely on NullAway's enforcement of the type system on overrides. I'd argue the later might make more sense. And switching to that mode is easy (just add an early return here if the symbol is defined on annotated code).

However, my one worry here is that users might be relying on the current behavior. At the very least this would imply a minor version bump and testing it internally at Uber before changing.

Also, I am not 100% sure now which behavior is more inconsistent (library models work the same for third-party and first-party overrides vs library models should not apply to methods defined in first party code).

@msridhar, thoughts?

p.s. A final alternative is to keep the default behavior but allow @NonNull to override. The problem is that violates our general principle of "non-null in annotated code, unless marked otherwise" creating a sort of "no, really, I mean this to be non-null!", which I am not a fan of.

[ConradScott]

Thanks for the response @lazaroclapp.

To respond to your uncertainty on which behaviour is more (in)consistent: The current state as I understand it, is that if I override a method inherited from one of my own classes and I don't annotate that override with @CheckForNull (or some equivalent), NullAway will assume that it is non-null. And the same is true if I override almost any method from a third-party library. It's only if the method being overridden is one of the special ones defined as returning nullable in a library model that NullAway assumes my override is nullable. If so, it is rather magical, which is a bit unfortunate. But it's not clear to me what consistency the current behaviour has over the alternative, so I'm presumably missing something.

I assume the worry about changing this behaviour is that all Throwable::getMessage overrides (for example) in existing code will change from being nullable to non-null, which could trigger an arbitrary amount of NullAway failures, which would be painful.

Thanks for picking up on the extraneous `@Nonnull'. It was there to keep IntelliJ happy and I hadn't realised NullAway doesn't require it.

[msridhar]

This is a tricky one. My biggest question is, how often does this come up in real-world code, and what are developers doing? IMO the best solution would be that if a Throwable.getMessage() override in first-party code is unannotated, its return type should be @NonNull, even if the library model gives a @Nullable return type for Throwable.getMessage(). This could cause new errors in first-party code if devs were overriding these methods and relying on the behavior of inheriting @Nullable types only when overriding models. My feeling is that won't happen very often, and even if it does, it is bad, since it violates the expectations of someone reading the code who knows NullAway. E.g., right now you could write this and it would typecheck:

    private static final class Subclass extends RuntimeException
    {
       [...]
        @Override
        public String getMessage()
        {
            return null;
        }
    }

This feels wrong to me, against NullAway's design, and probably not what the developer expects. But, if fixing this issue would add many new errors to existing code, we'd have to think a bit about whether it's worth it. Maybe we can test it out at Uber?

[lazaroclapp]

Yeah. That matches my thoughts as well. I think the right way to proceed here is for me to make a PR with the fix, then test it internally before landing. Best case scenario, we'll find no code at Uber breaking with that change, and that will give us some confidence about what we expect to happen in other large codebases using NullAway. If we do see many breakages, then we might need to re-evaluate.

Still, technically a breaking change, so this should probably result in 0.9.0 (which means there weren't any point releases for 0.8.0)

[lazaroclapp]

@ConradScott PR with a fix at #446. Will need to do some extra testing before landing that change.

Like you surmised, people could be relying on the incorrect behavior. Depending on how common that is, we might need to provide some deprecation strategy such as a flag to toggle between the two behaviors. Our hope is that it's uncommon and we can get away with just fixing as a new point release.

Also, if you are able to easily build a local version of NullAway with the patch in that PR, it would be good if you can confirm that it does the right thing for your code. It should. The unit test is based on your example. But it's always helpful to double check if that's not a complicated ask from you :)

[ConradScott]

Not too complicated, and I can confirm that the patch appears to work for my use case (as discussed in this issue): for code where the master branch raises the [NullAway] passing @Nullable parameter 'e.getMessage()' where @NonNull is required error, the version with your PR raises no errors (and otherwise appears to work exactly as the 0.8.0 release).

Many thanks for the swift fix @lazaroclapp, which gives me a way forward. I hope it doesn't cause too much pain to get this released (and that you don't end up needing to add a flag to choose behaviours).

[lazaroclapp]

We've tested that change in isolation on one of our two big repos and found no instance where it introduces any issues. I'll do one last big internal test of all the recent PRs together, and hopefully cut a new release of NullAway during this week! (tomorrow, if all goes well)

[lazaroclapp]

NullAway 0.9.0 is out now, containing the fix for this issue and others. Thanks again for the report, @ConradScott !