AssertJ's fail* methods do not qualify as throw statements

[jonatan-ivanov]

AssertJ allows users to write their own *Assert classes by extending AbstractAssert which is very convenient if you want to have a TCK with AssertJ's fluent API from your own domain objects.

Unfortunately, NullAway ignores methods in AbstractAssert that meant to stop the control flow and throw an exception: fail*, failure, etc. methods.

For example:

import org.assertj.core.api.AbstractAssert;
import org.jspecify.annotations.Nullable;

class DemoAssert<SELF extends DemoAssert<SELF>> extends AbstractAssert<SELF, DemoAssert.DemoConcept> {

    protected DemoAssert(DemoConcept demoConcept) {
        super(demoConcept, DemoAssert.class);
    }

    static DemoAssert<?> assertThat(DemoConcept actual) {
        return new DemoAssert<>(actual);
    }

    SELF hasLongerValue() {
        if (actual.getValue() == null) {
            failWithMessage("Value is null");
        }
        if (actual.getValue().length() < 5) {
            failWithMessage("Value is shorter than 5 characters: <%s>", actual.getValue());
        }

        return (SELF) this;
    }

    static class DemoConcept {
        @Nullable String getValue() {
            return null;
        }
    }

    public static void main(String[] args) {
        DemoAssert.assertThat(new DemoConcept()).hasLongerValue();
    }
}

produces the following error:

java: [NullAway] dereferenced expression actual.getValue() is @Nullable
    (see http://t.uber.com/nullaway )

But in this setup the .length() call cannot cause an NPE since there is a null-check before and failWithMessage throws an exception so the execution cannot reach the .length() call on a null reference.

Workaround: else if (actual.getValue().length() < 5) { ... } which is cleaner if I ask me but the reproducer above should be ok too and there are cases where there is no workaround:

String demo() { // return type is implicit @NonNull
    if (actual.getValue() == null) {
        failWithMessage("Value is null!");
    }
    return actual.getValue();
}

[msridhar]

This shouldn't be too hard to fix. Basically we want to model library methods that unconditionally throw an exception, like failWithMessage. Right now our LibraryModels interface does not allow for this. Probably the right approach here is to add such support to LibraryModels and the LibraryModelsHandler and then bake in a library model for failWithMessage. The support would require writing code in LibraryModelsHandler that overrides onCFGBuildPhase1AfterVisitMethodInvocation to modify the CFG appropriately, like this code in the ContractsHandler (but much simpler, we just need to call phase.insertUnconditionalThrow for appropriate method calls). It'd be nice to just re-use the ContractsHandler code but I'm not sure how easy that would be.

[scordio]

Could this be addressed directly in AssertJ with a custom contract annotation?

ANSWER: yes, AssertJ can do it, like JUnit did it in AssertionUtils. I'll track this at assertj/assertj#3727.

[msridhar]

@jonatan-ivanov since this can be accomplished with a @Contract annotation as @scordio observed, can we close this issue?

[jonatan-ivanov]

I guess that's not something I can answer but I can definitely add my two cents:

It seems AssertJ's custom @Contract annotation is planned for the next major release (4.x) which means the current users of AssertJ (3.x) will not see this fixed for a while: 4.x needs to be released and then they need to migrate to 4.x which might not be obvious given that it's a major version (and I guess in reality, many users might stay on 3.x for a very long time). From this perspective, I would not close it since it would be nice to have support for AssertJ 3.x (in Nullaway?). I don't know if it is a feasible goal though, what do you think?

Also, in order to make NullAway aware of AssertJ's future @Contract annotation, does NullAway need to be changed or once AssertJ has one, will that work out of the box? (Assuming NullAway:CheckContracts=true but NullAway:CustomContractAnnotations is not set so that users do not need to specify every single @Contract annotation on their classpath.)

[scordio]

From this perspective, I would not close it since it would be nice to have support for AssertJ 3.x (in Nullaway?).

If I understand the custom @Contract correctly, it's unrelated to JSpecify, and the effort to have it for fail methods wouldn't be so big.

Due to other reasons, I'm already considering releasing another 3.x version of AssertJ, so I might just incorporate such a change as well.

Would that help your use case, @jonatan-ivanov?

Whether NullAway needs to be configured or supports it out of the box is still a question for @msridhar.

[jonatan-ivanov]

Yes, @Contract is unrelated to JSpecify, though I hope it will be added to JSpecify in the future. :)

Fixing this in a minor release of AssertJ 3.x would be awesome!

I think either NullAway should be aware AssertJ's @Contract annotation or it should be able to somehow auto-discover it.

[msridhar]

It is a good point that we need a way to auto-discover @Contract annotations. Options:

1. Treat any annotation with simple name Contract as a contract annotation by default. Here my concern is mis-interpretation of annotations not intended to be the same as JetBrain's @Contract annotation, or weird errors due to such annotations; but maybe that's not a significant issue in practice?
2. For custom contract annotations, recognize them as being the same as the JetBrain's @Contract if they have some suitably-named meta-annotation. Here I think we'd want to keep things very simple; JSpecify at first tried to include a general @Implies annotation but backed off of it (for now) due to the complexities involved. So maybe we'd want just something like @SameAsJetbrainsContract (but with a better name). The only issue here is this will only work if the user code has the custom @Contract annotation in their classpath (so we can check for the meta-annotation).

Thoughts?

[scordio]

I tend to vote for option 1, similar to how custom @CheckReturnValue annotations are supported by IDEs and static analysis tools (see a related conversation on X).

The meta-annotation direction also sounds appealing, or perhaps something coming directly from JSpecify, given that frameworks and libraries are now defining their custom versions based on JetBrains semantic (e.g., Spring, JUnit, AssertJ to follow...). I see that some discussion happened at jspecify/jspecify#454, but that has been closed as not planned.

[msridhar]

There have been renewed discussions within JSpecify around adding @Contract (/cc @cpovirk @sdeleuze). But there is work to specify the language of supported contracts and to get broader buy-in. So it will likely take some time, and we'll need a temporary solution to the issue of multiple projects defining their own contract annotations.

[jonatan-ivanov]

I like the idea of a meta-annotation so people can annotate @Contract, but I think this is only a feasible solution if users don't need to depend on NullAway (so I guess JSpecify should have the meta annotation if having @Contract does not work out in time).

The issue with mis-discovering @Contract annotations by name could be eliminated by introducing some kind of a "schema" specifier:

@Contract("nullaway(null -> true)")
public static boolean isEmpty(@Nullable String string) {
    return string == null || string.isEmpty();
}

(Existing users not using the meta annotation or the "schema" should be warned in a log message that the annotation was ignored.)

For me, the preference order is:

1. JSpecify having @Contract
2. JSpecify having a meta-annotation so that users can annotate their own @Contract
3. @Contract is discovered by name (with or without using a "schema" specifier) and maybe also two options for NullAway where users can include/exclude custom @Contract annotations

[scordio]

AssertJ 3.27.4 is now out and fail methods are annotated with org.assertj.core.internal.annotation.@Contract.

[msridhar]

Hi all, haven't forgotten about this, just thinking a bit more. I'm leaning toward recognizing any @Contract annotation but only reporting errors regarding the formatting if it's a JetBrains annotation or listed in the -XepOpt:NullAway:CustomContractAnnotations setting. Hopefully that would be a reasonable interim behavior until a JSpecify @Contract annotation emerges.