Standard astub file for unsound treatment of reflection APIs?

[cpovirk]

Idle thought:

Just as some users want lenient/permissive treatment of the Object parameters of collection types, which is now available through collection-object-parameters-may-be-null.astub, some users want such treatment of the parameters of java.lang.reflect APIs like Method.invoke, Field.get, Field.set, Constructor.newInstance, and arguably more exotic APIs like those on AtomicReferenceFieldUpdater. (I've seen requests for some of these methods both inside Google and in this issue tracker.)

Background for anyone reading this who doesn't already know it: There are 2 kinds of parameters that are relevant here:

• Some methods have a "receiver" parameter, which can safely be null if and only if the method/field is static.
• All methods have "method parameter" or "field value" parameters, which can safely be null if and only if the corresponding underlying method parameter or field is nullable.

Anyway: I don't know how far you want to go down the road of providing such astub files. I can imagine at least two other astub files that users might request offhand (for which I could file separate issues if you're interested):

• Some users want to allow @Nullable Optional.
• Some users want for Object.requireNonNull to tolerate null inputs.

I'm not sure where to draw the line. If I were to make a case for an astub file for the reflection methods specifically, I would argue that the reflection methods can always be used to violate null safety no matter how we annotate them, just as they can be used to violate Java's own type system. For example, we can't stop users from calling method.invoke(receiver, Arrays.asList(null)) (that is, from passing a List<@Nullable Object>) when the method requires a List<@NonNull Object>. I'm not sure there's a way to avoid that, short of checking all parameters to the reflection methods to ensure that none have parameterized supertypes.

(Aside: I should see how well -AresolveReflection works for the cases in which I've had to suppress warnings....)

[wmdietl]

I think we recently discussed having such a stub file as part of #3226, but I can't find the concrete comment right now.

For collections we added the stub file, because the feeling was that the problem occurs more frequently and the number of false positives is higher. For reflection code, there are fewer uses compared to collections and the likelihood of false positives seemed lower (more methods are instance methods than static methods, e.g.).

The point about generic types is a good one and I don't see a simple way to enforce this.

I'm fine with adding such a stub file, with clear warnings about drawbacks of using it.

[mernst]

I'm OK with adding these stub files. Providing them in the Checker Framework seems preferable to having each user devise them individually.

[cpovirk]

Our kmb@ points out that there's never a need to call methods like field.get and method.invoke with null for the receiver: When accessing a static member, you can always pass a dummy instance.

At least in my specific area of work, this probably eliminates much of the need for stubs like I've proposed. However, users who are passing parameters to methods (or setting values of fields) will still have issues (and that includes us, just less often).

[mernst]

You are right.

While I agree you can do that, I consider it a code smell. When I see a non-null value, I assume that is a signal that the field is non-static. I prefer not to use worse code style just to satisfy the checker.

[cpovirk]

That is true. It is especially smelly if the object can't be of the "right" type. For example, if I want to access System.out reflectively, I can't pass an instance of System to my call to field.get, since the type is not publicly instantiable. I could even imagine that another static-analysis tool might look for mismatches between the type of object passed to get and the type the Field was created for.

[vlsi]

Just my 2c: I'm adding nullability annotations to Apache Calcite, and I'm inclined to add Objects.requireNonNull stub that accepts nullable values.

The reasoning is as follows: the existing code uses Objects.requireNonNull a lot, and sometimes there are dependencies between different parameters or fields. For instance: "if scope is not null, then alias must be non-null as well". The code is like if (scope != null) { callMethod(scope, Objects.requireNonNull(alias)); }.

[mernst]

(The discussion of a stub file for requireNonNull is a bit off topic for this issue about reflection.)

If you use a stub file for requireNonNull, then you will not receive notification of all possible exceptions that are due to null pointers. Essentially, requireNonNull becomes a warning suppression rather than code that the Nullness Checker warns about.

That said, you may find it more convenient to use requireNonNull as a warning suppression rather than other mechanisms. I can add such a stub file to the Checker Framework distribution if you wish to contribute it.