Clarify when returned values might be null

[smoyer64]

When the ParameterResolver's supports() and resolve() methods were changed to use ParameterContext, the statement that the Parameter was never null was replaced by text that stated ParamterResolver is never null. ParameterResolver itself makes no assertion that getParameter() always returns a non-null value. Thus, I'm now checking for null parameters as follows:

    boolean supported = false;
    Parameter parameter = parameterContext.getParameter();
    if (parameter != null && Performance.class.equals(parameter.getType())) {
      supported = true;
    }
    return supported;

For the ParameterContext, it wouldn't make sense to ever have a non-null parameter (otherwise it would be a SomethingElseContext), so this code is probably completely unnecessary. As a defensive coder, I put it in anyway. Here are my recommendations in order of priority/complexity:

• For every returned value, explicitly state in the Javadoc whether or not the return type may be null.
• Globally use Optional when a return type might be null.
• State in the User Guide that no public APIs ever return null.
• Add the JSR-305 annotation library with a provided scope and add @CheckForNull, @Nonnull and @Nullable to all public APIs.

I know the JUnit team's policy on avoiding dependencies - adding JSR-305 as provided allows those of us who chose to perform static analysis on our Engine, Extension and test code to choose to add the dependency to our classpath.

Related Resources

• There is additional detail in Add nullability annotations to code base for better tooling assistance #741.
• SPR-15540 and the related commit.

[sbrannen]

Thanks for raising the issue.

The omission of "never null" from the Javadoc for ParameterContext was simply an oversight that I have now addressed, and we will look into the rest of your proposals during the M2 time frame.

[smoyer64]

Thanks!

I thought that might be the case ... and for ParameterResolvers, just that little sentence makes a huge difference since (almost) every supports() and resolve() call wants access to the parameter.

(Plus I love it when my code base shrinks)

[marcphilipp]

See #741 for additional motivation to use annotations to support static analysis and Kotlin interoperability.

[marcphilipp]

Inspired by IntelliJ complaining about ExceptionUtils.throwAsUncheckedException() maybe returning null (a false-positive), I did a little spike in 24c8438. It's a bit tedious, but the improved IDE support is quite nice and we might even save a little code for unnecessary checks on internal code.

@junit-team/junit-lambda WDYT, should we do this for the whole codebase?

[jbduncan]

@marcphilipp By my understanding, javax.annotation.Nonnull doesn't work out of the box with JPMS modules because of "split packages" (https://blog.codefx.org/java/jsr-305-java-9/), so I'd personally be inclined to use the Checker Framework's nullability annotations or IntelliJ's (or to create a JUnit 5-internal @NotNull annotation). :)

[smoyer64]

@jbduncan commented on the difference between JSR-305 static analysis and the Checker Framework in #961 (comment). If an internal @NotNull annotation is created, why not make is a plug-in for either error-prone or the Checker Framework?

[marcphilipp]

@smoyer64 Could you please elaborate on that "plug-in" option?

[smoyer64]

Both the Checker Framework and Error-Prone allow you to write custom checks - https://checkerframework.org/manual/#creating-a-checker and https://github.com/google/error-prone/wiki/Writing-a-check respectively. If you want to do something custom to JUnit 5 with your annotation, you could plug it into one of the two systems.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. Thank you for your contribution.

[stale]

This issue has been automatically closed due to inactivity. If you have a good use case for this feature, please feel free to reopen the issue.