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

[Stuart Marks]

On 4/25/16 5:46 PM, Vitaly Davidovich wrote:
>     We've introduced Optional to avoid this. The problem is that the obvious
>     thing to do is to get() the value out of the Optional, but this buys us
>     right back into the what-if-the-value-is-missing case that we had with nulls.
>
> Why is this the "obvious" thing? Because a few stackoverflow threads went like
> that?

Well, the Stack Overflow exchange was a caricature. But without looking too 
hard, I found this code: [2]

     return stringList.stream().filter(...).findFirst().get() != null;

[2] http://stackoverflow.com/q/36346409/1441122

The person, presumably new to Java 8, was able to determine that findFirst() 
returned an Optional instead of the desired value, and was able to figure out 
enough to call get(), and even thought enough about the possibly-absent case to 
check it against null. But the person had to ask on Stack Overflow about how to 
solve the problem.

Now I obviously don't know the exact thought process that went on, but get() has 
among the shortest of name of the Optional methods, and has no arguments, so it 
seems like it ought to be called more more frequently compared to more 
specialized methods like ifPresentOrElse() or orElseThrow().

There are other items on Stack Overflow such as these: [3] [4]. But these 
probably aren't what you're looking for. :-)

[3] http://stackoverflow.com/a/26328555/1441122

[4] http://stackoverflow.com/a/23464794/1441122


> It's not clear it returns an Optional - it could be returning some other type
> that happens to have a getWhenPresent.  What would be clear it's an Optional is
> to have a local of that type and assign find(pk) to it.

Of course there's always a possibility that there's some other API that has 
getWhenPresent() on it. There isn't one in the JDK, and I've been unable to find 
one elsewhere. So "getWhenPresent" should be different enough to trigger a 
different thought pattern, as opposed to a plain "get" which fades into the 
background.

The Optional API was designed to be chained, so you don't want to pull it into a 
local variable.

> Descriptive names are good, but like Jon, I'm not buying this change.  Is it
> really wrong to tell people to read the javadoc? And is that worth the
> deprecation churn? It's going to be needlessly verbose (esp in existing code
> that already checked isPresent) for no strong benefit.  If someone is mindlessly
> having their IDE write code for them, this isn't going to prevent poor code.
> For code review purposes, if it's important to call out that something is
> Optional then use the type somewhere explicitly.

Personally I'm generally skeptical of simple name changes, but after having gone 
through a bunch of examples of the use of get(), my skepticism melted away. It 
really does seem to be misused a significant fraction of the time, possibly even 
a majority of the time.

Beginners and Java 8 learners will probably fall into the trap of forgetting to 
check. But JDK programmers, who are decidedly not beginners, are writing code 
that uses get() unnecessarily:

========================================

http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.langtools/src/jdk.jdeps/share/classes/com/sun/tools/jdeps/DependencyFinder.java.sdiff.html

  206                         if (source.isPresent()) {
  207                             executor.runTask(source.get(), deque);
  208                         }

could be rewritten as

         source.ifPresent(archive -> executor.runTask(archive, deque));

========================================

http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.langtools/src/jdk.jdeps/share/classes/com/sun/tools/jdeps/JdepsTask.java.sdiff.html

  476         Optional<String> req = options.requires.stream()
  477                 .filter(mn -> !modules.containsKey(mn))
  478                 .findFirst();
  479         if (req.isPresent()) {
  480             throw new BadArgs("err.module.not.found", req.get());
  481         }

could be rewritten as

         options.requires.stream()
                .filter(mn -> !modules.containsKey(mn))
                .findFirst()
                .ifPresent(s -> throw new BadArgs("err.module.not.found", s));

========================================

http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/tool/JShellTool.java.sdiff.html

1203             String hist = replayableHistory
1204                     .subList(first + 1, replayableHistory.size())
1205                     .stream()
1206                     .reduce( (a, b) -> a + RECORD_SEPARATOR + b)
1207                     .get();

could be rewritten as

         String hist = String.join(RECORD_SEPARATOR,
             replayableHistory.subList(first + 1, replayableHistory.size()));

(It's also not clear to me that the sublist is guaranteed to be nonempty, so the 
first snippet might fail in get().)

========================================

http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.jdk/src/java.base/share/classes/java/lang/module/Resolver.java.sdiff.html

  100                 if (mref.location().isPresent())
  101                     trace("  (%s)", mref.location().get());

could be rewritten as

         mref.location().ifPresent(loc -> trace("  (%s)", loc);

========================================

http://cr.openjdk.java.net/~smarks/reviews/8140281/webrev.0.jdk/src/java.base/share/classes/java/lang/reflect/Layer.java.sdiff.html

  364         Optional<Configuration> oparent = cf.parent();
  365         if (!oparent.isPresent() || oparent.get() != this.configuration()) {
  366             throw new IllegalArgumentException(
  367                     "Parent of configuration != configuration of this Layer");

could be rewritten as

         cf.parent()
           .filter(cfg -> cfg == this.configuration())
           .orElseThrow(() -> throw new IllegalArgumentException(
               "Parent of configuration != configuration of this Layer"));

========================================

If you go to grepcode.com, find the source for Optional.get() -- I used 8u40-b25 
-- and then click the little arrow next to get(), you can find a bunch of uses 
of Optional.get(). A startlingly large fraction of them can be simplified in a 
similar manner to the JDK examples I showed above, bypassing the need to call 
get(). (I did a quick check of the first ten examples, and I think about seven 
could be improved, depending on how you count.)

It really does look to me like get() is a bad API. It's not people "mindlessly" 
having their IDE write code. The problem is that get() is deceptively 
attractive. Even people remember to check it, they put the checking around the 
get(), instead of looking at Optional's other methods. This is what results in 
the needlessly verbose code.

A minority of the cases are justified in using get(). If this is renamed to 
getWhenPresent(), it becomes slightly more verbose, but it also specifically 
states that the programmer is making an assertion about the presence of a value 
-- a worthwhile improvement.

Is it worth the churn? Well, the problem is only going to get worse. I believe 
that if we don't deprecate get() now, a few years down the road, the problem 
will be so bad, we'll say "I wish we had deprecated it back in JDK 9 when we 
were talking about it then." Just like right now we're saying, "I wish we had 
fixed it before JDK 8 shipped."

s'marks