Remove redundant requireNonNull when using config objects

[findepi]

Config objects are mutable, therefore they are always unpacked in a
constructor. Thus, they do not require requireNonNull, as since Java's
implicit NullPointerException message will be as good.

[losipiuk]

This is a bit controversial as with this change you can remove explicit requireNotNull because there is a.call(). But if later on a.call() is removed and a is used below, being passed somewhere, it is hard to notice that requireNotNull should be
reintroduced. Sth like in this snippet

{
  requireNotNull(a, "as is null).someCall();
....
  doSomething(a);
}

I am not saying that this is showstopper for this PR. Just sth to think about.

[findepi]

That's a good point & something I was also concerned about.

Now it's a question to Maintainers for how we review new code

• do we still require requireNonNull in constructors that's technically redundant?
  • based on @martint 's approval of Remove redundant requireNonNull when copying Optional content #13925 i think at Martin wouldn't require that

If we don't require technically redundant requireNonNull in new code, we should be OK removing it from existing code. @losipiuk would you agree with this one?

[hashhar]

I'd personally argue to keep it - our goal is to make code more readable and maintainable even if there is some marginal additional cost when writing the code.

Code can change in future and what fails with a nice NPE message today may fail in not so nice ways in future. The current "rule" of always use rnn in constructors is simple and easy to follow for both authors and reviewers IMO.

[findepi]

I don't know whether we have universal broad agreement, but I think we should be OK removing requireNonNull for config objects. That's because config objects are not supposed to be assigned to a field, so a code that takes a config, but doesn't read from it (yet), is usually a wrong code.

[losipiuk]

our goal is to make code more readable

As readability goes I actually think that code without explicit rnn is more readable. After all, this is just a boilerplate. There is a general rule that objects passed as arguments are expected to be not null unless explicitly marked as @Nullable.
I think that for common cases like wrapping collection passed via argument using Immutable*.copyOf or Optional.map we can live without rnn.
For less obvious cases (multiple uses of object where some of those are calls of the method on object) I would keep explicit rnn.

[findepi]

(quality of error message depends on validation withing someMethod, but I think we should not assume this is good).

The calling code and doSomething are equally matured code bases. It's not parent-child relationship, we shouldn't feel overly responsible for other methods.

[losipiuk]

(quality of error message depends on validation withing someMethod, but I think we should not assume this is good).

The calling code and doSomething are equally matured code bases. It's not parent-child relationship, we shouldn't feel overly responsible for other methods.

I tend to disagree. Often we have little control over the doSomething. It may be an external library we just have to use. An extra check before the call may make debugging significantly easier if all we end up with is logged exception stacktrace (which may be useful or not).

[findepi]

We don't pass config objects to external libraries.

i agree that in some cases we may get a somewhat worse message
i think that overall the increased readability is worth it.
these rnn's were kind of necessity, but they don't help read the code

[hashhar]

I think as long as we only avoid rnn for config objects it is safer. But while reviewing let's make sure others don't copy the missing rnn to other types of objects (since there's no way for the code to indicate to future writer why rnn was ok to skip). Feel free to merge this - I don't feel very strongly about this but will keep eye open for future PRs.

[findepi]

I think as long as we only avoid rnn for config objects it is safer. But while reviewing let's make sure others don't copy the missing rnn to other types of objects

i admit i removed more rrn than just config objects (hence two separate commits, something i forgot about).
but those other removals are safe too.

I am quite convinced that we are not going to require rnn around resourceProviderConstructorArg in new code like

this.resouce = resourceProviderConstructorArg.newResource()

and therefore i am convinced this is a fine change.

let me merge as is and see how it goes.

i am absolutely fine restoring some rnn whenever we find that some exc messages aren't informative enough.

[findepi]

(just rebased to resolve a conflict)