ParameterMissingNullable should not warn about test code passing null

[PhilippWendler]

Sometimes we want to test that certain APIs throw NullPointerException when a parameter is null without the possibility to use something like Guava's NullPointerTester. Since version 2.14.0, Error Prone's ParameterMissingNullable complains about this.

Test code:

import static org.junit.Assert.assertThrows;

import org.junit.Test;

public class MyTest {

  interface TestInterface {
    void m(Object o);
  }

  @Test
  public void test(TestInterface i) {
    assertThrows(NullPointerException.class, () -> i.m(null));
  }
}

Output:

[javac] src/MyTest.java:13: warning: [ParameterMissingNullable] Parameter has handling for null but is not annotated @Nullable
[javac]     assertThrows(NullPointerException.class, () -> i.m(null));
[javac]                                                        ^
[javac]     (see https://errorprone.info/bugpattern/ParameterMissingNullable)
[javac]   Did you mean 'void m(@Nullable Object o);'?

I would guess that in test code, the risk of passing null somewhere is relatively low compared to other code (chances are high that it either works as intended or fails the test). Thus my suggestion would be to disable this check in test code completely, because a more fine granular exclusion of patterns like the above assertThrows() will likely not be enough in large code bases (where helper methods are used in tests etc.).

Note that the warning message is also confusing: "Parameter has handling for null" is wrong, it needs to be something like "Parameter is set to null".

[Stephan202]

In theory this behavior could also be achieved by omitting ParameterMissingNullable from the test compilation Error Prone arguments. But that can be annoying to configure in e.g. Maven (it'd likely require some configuration duplication or an awkward extra property).

If the logic suggested in this issue were implemented, it'd be nice to have it behind a flag; in our code base we try (to a first approximation) to be as strict with test code as we are with main code.

[cpovirk]

Thanks.

• We could at least make the check ignore test code unless you set -XepOpt:Nullness:Conservative=false. This would make it similar to ReturnMissingNullable already behaves.
• The approach of a single "Should all nullness checks be conservative in every way?" flag seemed unlikely to hold up forever, and the discussion here is more evidence of that. We should figure "something" out.
• We should definitely fix the error message. (It's possible that we should outright split the check in two. This is another question that we'll need to answer for other nullness checks, including ReturnMissingNullable....)