Code review: 7012540 (java.util.Objects.nonNull() incorrectly named)

[David Holmes]

Rémi Forax said the following on 01/27/11 18:43:
> On 01/27/2011 09:38 AM, Stephen Colebourne wrote:
>> As I said before, removing this method is the best option now. Get it
>> right in v8.
>> Stephen
> 
> I think we can't.
> This method is already used at many place in the JDK.

I think we can. Those usages will have to be modified anyway. If it is 
really the wrong decision to add this now, and if we really can't get 
some consensus on the best name for the exception throwing method, then 
we should remove it.

I'm not saying we're in that position, just that if we were there's no 
reason we could not undo this.

I think the utility and significance of this method is vastly 
over-estimated, and certainly not worth the level of activity it has caused.

David Holmes

> About the name, I propose: 
> iUsedToUseGetClassHereButNodobyWasAbleToUnderstand()
> 
> Rémi
> 
>>
>> On 27 January 2011 05:05,<mark.reinhold at oracle.com>  wrote:
>>> Executive summary: requireNonNull is the preferred name.
>>>
>>>> Date: Tue, 25 Jan 2011 18:33:47 -0500
>>>> From: brian.goetz at oracle.com
>>>> mark.reinhold at oracle.com wrote:
>>>>> I think requireNonNull(x) is confusing.
>>>> Remember there's two versions of someModifierNonNull being 
>>>> discussed; the one
>>>> currently in Objects is the precondition-enforcing variety, not the
>>>> postcondition-ensuring variety.  Are we talking about the same thing?
>>> Yes, i.e., the former.
>>>
>>>>> For those familiar with the "requires/ensures/modifies" triad of 
>>>>> verbs as
>>>>> a compact way of identifying the preconditions, postconditions, and 
>>>>> side
>>>>> effects of a method or procedure in a comment, a specification, or 
>>>>> a more
>>>>> formal design-by-contract framework, "requires" is just wrong.
>>>>>
>>>>> When analyzing the invocation of foo in your example, the 
>>>>> non-nullity of
>>>>> s and t are preconditions of foo and therefore postconditions of the
>>>>> check method.  Naming the check method "requireNonNull" suggests that
>>>>> the check method itself has a precondition that its argument be 
>>>>> non-null,
>>>>> when in fact it's the check method's postcondition which ensures that
>>>>> property.
>>>>>
>>>>> Since postconditions are labeled "ensures" in the "r/e/m" triad, this
>>>>> method should be named "ensureNonNull".
>>>> Right, there's precedent for "ensureXxx" methods to actually change 
>>>> the state
>>>> of things to ensure the postcondition, such as ensureCapacity() 
>>>> methods in the
>>>> collection implementations.  Given that a part of the motivation for 
>>>> this
>>>> change was to leave room in the namespace for both the 
>>>> precondition-enforcing
>>>> variety (barf on null) and the postcondition-ensuring variety, aka the
>>>> carpet-sweeping variety ("if it is null, make it non-null") 
>>>> ensureNonNull
>>>> sounds a lot more like the the carpet-sweeping version than the 
>>>> version being
>>>> discussed (barf on null).
>>> I was actually arguing that the barfing version should be named
>>> ensureNonNull.
>>>
>>> (Wow, am I actually writing about barfing methods?)
>>>
>>>> The r/e/m framework seems to support require for the throwing 
>>>> version (as
>>>> implemented by this patch): for the throwing version, non-nullity is a
>>>> precondition of the check method (if the condition is not met, 
>>>> error), whereas
>>>> for the carpet-sweeping version, it is a postcondition of the check 
>>>> method (if
>>>> the check method can come up with a reasonable default value).  (It 
>>>> happens to
>>>> be a postcondition of both, but the significant behavior and use of the
>>>> throwing version currently in Objects is to enforce an error when the
>>>> precondition is not met.)  Therefore:
>>>>
>>>>    requireNonNull(x) ->  throw if x == null
>>>>    ensureNonNull(x)  ->  convert x to a non-null value if null
>>>>
>>>> seems like the right taxonomy.
>>> It depends on your perspective.
>>>
>>> In a nested method invocation, the postconditions of the inner method
>>> must relate to the preconditions of the outer method.  That is, for the
>>> expression
>>>
>>>     foo(bar(x))
>>>
>>> to be correct then the postconditions of the bar method must imply the
>>> preconditions of foo method.
>>>
>>> Now suppose that bar is intended to be the barf-on-null method.
>>>
>>> If we consider how to name bar on its own, outside of its intended usage
>>> as in this example, then it's natural (for me, anyway) to conclude that
>>> "ensuresNonNull" is the right name.  What's important about bar is its
>>> postcondition, namely that its return value (x) is not null.  Naming it
>>> "requireNonNull" seems backwards on this view, because "requires" is
>>> always about preconditions, and it's definitely not a precondition of
>>> this method that its argument not be null.
>>>
>>> If, by contrast, we consider how to name bar in the context of the above
>>> example then it's easy to see that "requireNonNull" is the better name,
>>> because what's important in the overall expression is satisfying foo's
>>> precondition that its argument not be null, and so
>>>
>>>     foo(requireNonNull(x))
>>>
>>> reads very fluently.
>>>
>>> A little utility method like this will never (well, hardly ever) be used
>>> in isolation, so I agree now that it makes sense to prefer the second
>>> form, i.e., requireNonNull.
>>>
>>> I'm not sure I agree that the corresponding carpet-sweeping method is
>>> best named ensureNonNull; treating that as a conversion, i.e., 
>>> asNonNull,
>>> may be a better choice.  That, however, is a discussion for another
>>> release.
>>>
>>> - Mark
>>>
>