RFR(m): 8140281 deprecate Optional.get()

[Brian Goetz]

As the person who chose the original (terrible) name, let me weigh in...

I think calling this method "get()" was our biggest API mistake in Java 
8.  Now, one could argue that, if this is the biggest mistake we made, 
then we did pretty darn good.  Which may be true, but ... make no 
mistake, it was the wrong name (mea culpa), and experience has borne out 
that it is widely misused.  Subjectively, about half the uses of .get() 
I see are wrong -- and wrong in the obvious, avoidable way -- they don't 
consider the optional might be empty. So they've just traded an 
unexpected NPE for an unexpected NSEE.

Its problem is, essentially: it looks harmless, but isn't, but it sure 
seems like the method you obviously want when you're browsing the 
autocomplete list / javadoc.  It's a hazard both to code writers (pick 
the wrong method because the name is bad) and to code readers 
(deceptively harmless looking.)

Methods like orElse or ifPresent look harmless, and are; methods like 
orElseThrow have potentially harmful side-effects but have names that 
are transparent about what harm they could do.  But Optional.get() is 
neither safe nor transparent -- and yet awfully tempting-looking -- and 
this leads to frequent misuse.  Naming matters, and this one was wrong, 
and the harm is observable.  I wish I'd caught it before 8 shipped.

Stepping back a bit ... one of the most frequent complaints we get is 
"mistakes never get fixed".  So, we've decided to be more serious about 
deprecation -- Dr. Deprecator is suiting up!  But that means, for any 
given item, some people are going to disagree about whether deprecation 
is suitable, and some will be inconvenienced by the deprecation -- this 
is unavoidable.

Why prioritize this one?  In part, because it's a *new* mistake, and has 
had less time to become entrenched -- and this is the very first 
opportunity we have to fix it.  If we can't fix an API mistake at the 
very first opportunity, the logical consequence is we can't ever fix 
anything.  And that doesn't seem to be the world we want to retreat to.

Similarly, is this the worst mistake in all of Java?  Surely not. But 
its also not reasonable to say "you can't fix X until you've fixed 
everything worse than X" -- again, that's a recipe for never fixing 
anything.  So, in the context of a deprecation effort, this seems an 
entirely reasonable candidate.

I'd like to see it fixed, and the sooner the better.


On 4/26/2016 6:43 AM, Stephen Colebourne wrote:
> In OpenGamma Strata we have 546 uses of Optional.get(). Renaming this
> would be painful and add no value.
>
> While I understand the pain from some developers not understanding the
> feature, this is hardly unique in the world of Java. Developers learn
> the right way of doing something soon enough.
>
> And while
>
> if (opt.isPresent()) {
>    opt.get()
> }
>
> is sometimes not ideal, in other cases it is the only practical choice
> (eg. where the method needs to have a return statement inside the if
> statement).
>
> Changing this to
>
> if (opt.isPresent()) {
>    opt.getWhenPresent()
> }
>
> is just noise - I can see the "present" part twice.
>
> I just don't think I can support the rename (although many of the
> webrev tidy-ups are probably good).
>
> Stephen
>
>
>
> On 26 April 2016 at 00:05, Stuart Marks <stuart.marks at oracle.com> wrote:
>> Hi all,
>>
>> Please review these webrevs that deprecate Optional.get() and to replace it
>> with Optional.getWhenPresent(). The corresponding changes are also applied
>> to OptionalDouble.getAsDouble(), OptionalInt.getAsInt(), and
>> OptionalLong.getAsLong().
>>
>> Unlike most deprecations, this isn't about the function or the utility of
>> some API, it's about the name. The solution is basically to rename the API.
>> The problem is that "get" shows up as the "obvious" choice in things like
>> IDE code completion, leading to code that mishandles empty Optionals.
>> Typical Stack Overflow discourse runs something like this:
>>
>>      Q: what do I do with this Optional thing
>>
>>      A: just call get()
>>
>>      Q: thanks, it works!
>>
>> Of course, it works until it doesn't.
>>
>> Examining the JDK's use of Optional.get(), I didn't see very many cases that
>> called get() without first checking for the presence of a value. But I did
>> see quite a number of cases like this:
>>
>>      if (opt.isPresent()) {
>>          doSomething(opt.get());
>>      } else {
>>          doSomethingElse();
>>      }
>>
>> In many of these cases, the code could be refactored to use other Optional
>> methods such as filter(), map(), or ifPresent().
>>
>> In any case this reinforces the contention that use of get() leads to poor
>> code.
>>
>> For this changeset, in just about all cases I've simply replaced the call to
>> get() with a call to getWhenPresent(). In a couple cases I replaced the
>> stream calls
>>
>>      .filter(Optional::isPresent).map(Optional::get)
>>
>> with
>>
>>      .flatMap(Optional::stream)
>>
>> which I hope will become the new idiom for unwrapping a stream of Optionals.
>>
>> While many cases could be cleaned up further, I didn't change them. The
>> reasons are that I didn't want to spend too much time putting code cleanup
>> into the critical path of this changeset (I'd be happy to help later); doing
>> so would create potential conflicts with code coming in from the Jigsaw
>> forest; and there are non-obvious places where converting from a conditional
>> to one of the lambda-based methods could cause performance problems at
>> startup.
>>
>> There are also a few cases where simplification is prevented because it
>> would end up causing the resulting lambda expressions to throw checked
>> exceptions. :-(
>>
>> Webrevs here:
>>
>>    http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.langtools/
>>
>>    http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.jdk/
>>
>> Thanks,
>>
>> s'marks