RFR(m): 8140281 deprecate Optional.get()
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
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@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
Hello! While I'm not a reviewer, I agree with Stephen. While I understand the rationale, such renaming would cause even more confusion and pain. Also I think this is not the worst part of Java API, so if we start to rename things, where should we stop then? With best regards, Tagir Valeev. SC> In OpenGamma Strata we have 546 uses of Optional.get(). Renaming this SC> would be painful and add no value. SC> While I understand the pain from some developers not understanding the SC> feature, this is hardly unique in the world of Java. Developers learn SC> the right way of doing something soon enough. SC> And while SC> if (opt.isPresent()) { SC> opt.get() SC> } SC> is sometimes not ideal, in other cases it is the only practical choice SC> (eg. where the method needs to have a return statement inside the if SC> statement). SC> Changing this to SC> if (opt.isPresent()) { SC> opt.getWhenPresent() SC> } SC> is just noise - I can see the "present" part twice. SC> I just don't think I can support the rename (although many of the SC> webrev tidy-ups are probably good). SC> Stephen SC> On 26 April 2016 at 00:05, Stuart Marks <stuart.marks@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
Hi, If Optional.get() is the method that gets most attention from beginners and learners, then perhaps its javadoc is the place that could be improved by making it a honey pot for advice about Optional use. The assumption that the average programmer starts reading documentation of some new API from the class-level javadocs is perhaps wrong. Quick help (Ctrl-Q in IDEs like IDEA, etc.) is context-sensitive and brings up the method-level javadoc. That's where the story should begin, I think... Regards, Peter On 04/26/2016 01:10 PM, Tagir F. Valeev wrote:
Hello!
While I'm not a reviewer, I agree with Stephen. While I understand the rationale, such renaming would cause even more confusion and pain. Also I think this is not the worst part of Java API, so if we start to rename things, where should we stop then?
With best regards, Tagir Valeev.
SC> In OpenGamma Strata we have 546 uses of Optional.get(). Renaming this SC> would be painful and add no value.
SC> While I understand the pain from some developers not understanding the SC> feature, this is hardly unique in the world of Java. Developers learn SC> the right way of doing something soon enough.
SC> And while
SC> if (opt.isPresent()) { SC> opt.get() SC> }
SC> is sometimes not ideal, in other cases it is the only practical choice SC> (eg. where the method needs to have a return statement inside the if SC> statement).
SC> Changing this to
SC> if (opt.isPresent()) { SC> opt.getWhenPresent() SC> }
SC> is just noise - I can see the "present" part twice.
SC> I just don't think I can support the rename (although many of the SC> webrev tidy-ups are probably good).
SC> Stephen
SC> On 26 April 2016 at 00:05, Stuart Marks <stuart.marks@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
From my experience, devs in general only read the doc when an exception is thrown so i agree with you.
And in the wake of deprecating Optional.get(), I propose to rename RuntimeException to RTFMException :) Remi Le 26 avril 2016 20:05:22 CEST, Peter Levart <peter.levart@gmail.com> a écrit :
Hi,
If Optional.get() is the method that gets most attention from beginners
and learners, then perhaps its javadoc is the place that could be improved by making it a honey pot for advice about Optional use. The assumption that the average programmer starts reading documentation of some new API from the class-level javadocs is perhaps wrong. Quick help
(Ctrl-Q in IDEs like IDEA, etc.) is context-sensitive and brings up the
method-level javadoc. That's where the story should begin, I think...
Regards, Peter
On 04/26/2016 01:10 PM, Tagir F. Valeev wrote:
Hello!
While I'm not a reviewer, I agree with Stephen. While I understand the rationale, such renaming would cause even more confusion and pain. Also I think this is not the worst part of Java API, so if we start to rename things, where should we stop then?
With best regards, Tagir Valeev.
SC> In OpenGamma Strata we have 546 uses of Optional.get(). Renaming this SC> would be painful and add no value.
SC> While I understand the pain from some developers not understanding the SC> feature, this is hardly unique in the world of Java. Developers learn SC> the right way of doing something soon enough.
SC> And while
SC> if (opt.isPresent()) { SC> opt.get() SC> }
SC> is sometimes not ideal, in other cases it is the only practical choice SC> (eg. where the method needs to have a return statement inside the if SC> statement).
SC> Changing this to
SC> if (opt.isPresent()) { SC> opt.getWhenPresent() SC> }
SC> is just noise - I can see the "present" part twice.
SC> I just don't think I can support the rename (although many of the SC> webrev tidy-ups are probably good).
SC> Stephen
SC> On 26 April 2016 at 00:05, Stuart Marks <stuart.marks@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
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@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
On 4/26/16 2:55 PM, Brian Goetz wrote:
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. ... I'd like to see it fixed, and the sooner the better.
Thanks Brian. I should probably offer a mea culpa of my own, which is that I just started pumping out webrevs without providing any context. For some of the previous ones this hasn't been a big deal, but this most recent one has turned into an unpleasant surprise for several of you. JEP 277 (Enhanced Deprecation) is not only about refining the @Deprecated annotation itself, but employing it to deprecate things that have needed to be deprecated for quite a long time. I've been working a list of such things for some time. So what I'll do is set aside the Optional.get() webrev for the moment and start a new thread for discussion of the deprecation list. This should give people a chance to see what we're trying to do, and to assess the impact of deprecating things and how to assess the tradeoffs in doing so, independently of any particular webrev. I'll try to get this out in the next day or so. I'll send it here (core-libs-dev) since all the things on the list so far fall within the core libs area. s'marks
I've yet to hear one supporter on this thread besides yourself and Stuart. Is there some usability survey or something that actually indicates a significant sample of people don't like the name? Guava Optional behaves and is named the same way, and I've not heard anyone complain about that (I and many people I know used it for years). I think the conversation would be *maybe* different if picking a name for a new thing, but the deprecation churn here adds no value, IMO, and is going to be viewed as an annoyance by a large swath of people. On Tuesday, April 26, 2016, Brian Goetz <brian.goetz@oracle.com> wrote:
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@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
-- Sent from my phone
Guava owner here, and yes, I've never heard such a complaint about our Optional.get() method. This class has about a quarter of a million usages inside Google alone (I'm not kidding), and people seem quite happy with it. We have been planning to migrate these usages from our Optional class to yours (sounds crazy, but this is how we roll). Threads like this one -- and the threat of changing Optional in-place incompatibly to become a value type as part of Valhalla -- are making us, for the first time, seriously question whether that is such a good idea. Please don't rename this method. On Tue, Apr 26, 2016 at 4:38 PM, Vitaly Davidovich <vitalyd@gmail.com> wrote:
I've yet to hear one supporter on this thread besides yourself and Stuart. Is there some usability survey or something that actually indicates a significant sample of people don't like the name? Guava Optional behaves and is named the same way, and I've not heard anyone complain about that (I and many people I know used it for years).
I think the conversation would be *maybe* different if picking a name for a new thing, but the deprecation churn here adds no value, IMO, and is going to be viewed as an annoyance by a large swath of people.
On Tuesday, April 26, 2016, Brian Goetz <brian.goetz@oracle.com> wrote:
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@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
-- Sent from my phone
-- Kevin Bourrillion | Java Librarian | Google, Inc. | kevinb@google.com
On 4/26/16 9:20 PM, Kevin Bourrillion wrote:
Guava owner here, and yes, I've never heard such a complaint about our Optional.get() method. This class has about a quarter of a million usages inside Google alone (I'm not kidding), and people seem quite happy with it.
Hi Kevin, thanks for your comments. Interesting to hear yours (and Google's) experience with Guava's Optional.get(). How does Google avoid misusing it? Better programmers, better reviews, ability to refactor globally, or something else? Based on some of the code I've seen in the JDK and on grepcode.com (posted upthread), perhaps 50% or more of the uses of the JDK's Optional.get() are bad or suspect. Usually issues like this can be mitigated with Stack Overflow answers, blog articles, conference talks, better documentation, etc. But when the rate of misuse is this high, maybe it means that there's something wrong with the API itself. Now it may be the case that a particular proposal to fix the problem creates more pain than is worthwhile. I'm sympathetic to this, and I'm willing to discuss variations on or alternatives to proposals that reduce the pain. And I'm also prepared to accept, after exploring alternatives, that there might not be any that are sufficiently pain-free to proceed. But I'm also hearing that "Optional.get() isn't a problem." I'm really surprised by this. After looking at a bunch of code that uses Optional.get() it really looks like a problem to me. But maybe somebody can explain to me why it isn't.
We have been planning to migrate these usages from our Optional class to yours (sounds crazy, but this is how we roll). Threads like this one -- and the threat of changing Optional in-place incompatibly to become a value type as part of Valhalla -- are making us, for the first time, seriously question whether that is such a good idea.
Please don't rename this method.
I'll set aside the issue of value types for this discussion. We're not proposing to rename the method, strictly speaking. (I did use the word "rename" in my initial message on this thread. This was imprecise. If this misled anyone, I apologize.) A rename is strictly the addition of method and the removal of the old one. This is source and binary incompatible. We're not proposing that. We're also not proposing some temporary overlap of the old and the new for a few releases, after which the old would be removed. If we were to do that, we'd annotate the old method with @Deprecated(forRemoval=true). But the proposal is to deprecate with forRemoval=false (which is the default value, so it's elided). Instead, the proposal is to deprecate the old method, adjust the documentation to refer to the new method (and other Optional methods) and to leave the old method there indefinitely. This changes the behavior of IDEs and the way compilers issue warning messages, but otherwise it's source and binary compatible. Does this still make you reluctant to migrate to the JDK's Optional? If so, I'd like to understand this better, in order to improve the proposal. s'marks
Hello, I agree, using the isPresent()/get() is fine, especially for code which interfaces between Optional-using APIs and null/default using APIs. Especially if you do not want/should use streams or lambda. And as I recall the "do not use Optional everywhere" motto is repeatet from Oracle as well, so those Interfaces are to be expected. Optional<> res = calculateOptionalResult(); If (res.isPresent()) doSomething(res.get()); // at this location 'orThrow' would be highly disturbing the reading flow // else // doSomething(); So the major problem with that is, it makes it obvious it has not much advantage over null (and the language needs a ?. Operator ,) Gruss Bernd -- http://bernd.eckenfels.net -----Original Message----- From: Kevin Bourrillion <kevinb@google.com> To: Vitaly Davidovich <vitalyd@gmail.com> Cc: core-libs-dev <core-libs-dev@openjdk.java.net> Sent: Mi., 27 Apr. 2016 7:51 Subject: Re: RFR(m): 8140281 deprecate Optional.get() Guava owner here, and yes, I've never heard such a complaint about our Optional.get() method. This class has about a quarter of a million usages inside Google alone (I'm not kidding), and people seem quite happy with it. We have been planning to migrate these usages from our Optional class to yours (sounds crazy, but this is how we roll). Threads like this one -- and the threat of changing Optional in-place incompatibly to become a value type as part of Valhalla -- are making us, for the first time, seriously question whether that is such a good idea. Please don't rename this method. On Tue, Apr 26, 2016 at 4:38 PM, Vitaly Davidovich <vitalyd@gmail.com> wrote:
I've yet to hear one supporter on this thread besides yourself and Stuart. Is there some usability survey or something that actually indicates a significant sample of people don't like the name? Guava Optional behaves and is named the same way, and I've not heard anyone complain about that (I and many people I know used it for years).
I think the conversation would be *maybe* different if picking a name for a new thing, but the deprecation churn here adds no value, IMO, and is going to be viewed as an annoyance by a large swath of people.
On Tuesday, April 26, 2016, Brian Goetz <brian.goetz@oracle.com> wrote:
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@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
-- Sent from my phone
-- Kevin Bourrillion | Java Librarian | Google, Inc. | kevinb@google.com
On 27/04/16 00:38, Vitaly Davidovich wrote:
I've yet to hear one supporter on this thread besides yourself and Stuart.
Do you really want to turn this discussion into even more of a bikeshed discussion? Count me in as in favour of deprecation, but let's not get into +1s and -1s. I'm perfectly happy to let Stuart and Brian explain themselves — what they say makes sense to me. Andrew.
On Wednesday, April 27, 2016, Andrew Haley <aph@redhat.com> wrote:
On 27/04/16 00:38, Vitaly Davidovich wrote:
I've yet to hear one supporter on this thread besides yourself and Stuart.
Do you really want to turn this discussion into even more of a bikeshed discussion?
Not at all. Simply saying I find this proposal odd, and I didn't get the feeling I'm alone by reading the other responses here.
Count me in as in favour of deprecation, but let's not get into +1s and -1s. I'm perfectly happy to let Stuart and Brian explain themselves — what they say makes sense to me.
Andrew.
-- Sent from my phone
On 27/04/16 11:51, Vitaly Davidovich wrote:
On Wednesday, April 27, 2016, Andrew Haley <aph@redhat.com> wrote:
On 27/04/16 00:38, Vitaly Davidovich wrote:
I've yet to hear one supporter on this thread besides yourself and Stuart.
Do you really want to turn this discussion into even more of a bikeshed discussion?
Not at all. Simply saying I find this proposal odd, and I didn't get the feeling I'm alone by reading the other responses here.
You're not alone, but not everyone should chime in. Now that I'm here I have to say that deprecating bad names is an excellent thing to do, and what enhanced deprecation should be doing. Andrew.
On Wednesday, April 27, 2016, Andrew Haley <aph@redhat.com> wrote:
On 27/04/16 11:51, Vitaly Davidovich wrote:
On Wednesday, April 27, 2016, Andrew Haley <aph@redhat.com <javascript:;>> wrote:
On 27/04/16 00:38, Vitaly Davidovich wrote:
I've yet to hear one supporter on this thread besides yourself and Stuart.
Do you really want to turn this discussion into even more of a bikeshed discussion?
Not at all. Simply saying I find this proposal odd, and I didn't get the feeling I'm alone by reading the other responses here.
You're not alone, but not everyone should chime in. Now that I'm here I have to say that deprecating bad names is an excellent thing to do, and what enhanced deprecation should be doing.
Sure, but there's no agreement it's a bad name to begin with. It's a fine name, with precedent, and avoids visual noise when used as intended. Optional has something like a dozen methods with very simple javadoc - if a developer misused it, they'll learn and move on. There's really no issue here at all, as far as I'm concerned. I understand Brian and Stuart's thinking, but it's not addressing any real issue, IMO.
Andrew.
-- Sent from my phone
On 27/04/16 09:31, Andrew Haley wrote:
what they say makes sense to me It makes sense to me to. Having an innocently-named get() method throwing an exception is not something you see everyday. And in this case it's doubly confusing because one could imagine also a different behavior (i.e. return null if no object is there). So I'm in favor for making things clearer by choosing a more explicit name (whether the proposed one or a better one).
Cheers Maurizio
2016-04-27 19:43 GMT+02:00 Maurizio Cimadamore <maurizio.cimadamore@oracle.com>:
On 27/04/16 09:31, Andrew Haley wrote:
what they say makes sense to me
It makes sense to me to. Having an innocently-named get() method throwing an exception is not something you see everyday. And in this case it's doubly confusing because one could imagine also a different behavior (i.e. return null if no object is there). So I'm in favor for making things clearer by choosing a more explicit name (whether the proposed one or a better one).
This thread looks funny, so I chime in too. +1 for the change overall, I really do like when methods are self explanatory and I don't need to read the manual ;) But please consider the getWhenPresent sounds to me like it's trying to suggest that the method would block and returns *when* the value is present, not sure if it's just me and the fact that I'm not native english speaker though. Cheers, Mario -- pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF Fingerprint: BA39 9666 94EC 8B73 27FA FC7C 4086 63E3 80F2 40CF Java Champion - Blog: http://neugens.wordpress.com - Twitter: @neugens Proud GNU Classpath developer: http://www.classpath.org/ OpenJDK: http://openjdk.java.net/projects/caciocavallo/ Please, support open standards: http://endsoftpatents.org/
On 26 April 2016 at 22:55, Brian Goetz <brian.goetz@oracle.com> wrote:
As the person who chose the original (terrible) name, let me weigh in...
We start from a different premise - I do not think that get() is a terrible name. Nor was it the biggest API mistake in Java 8 (I've come to believe parallelStream() was that). FWIW, if I agreed that it was a terrible name, I would be in favour of the rename at the earliest opportunity. How about Duration.getNano( which should have been getNanos() ? Kevin has already outlined the key reason for the name get() - that Guava uses it and has done for many years. It is therefore the accepted name for the method. More broadly, it is used in association with isPresent() and as such seeing "getWhenPresent" repeats "present" and is far too much in your face. Ultimately, Optional is a class that requires a degree of learning. That learning is not hard and will happen with time. Where things went wrong IMO was adding Optional without tackling null-management in the language, but thats a separate debate. My experience is certainly not that 50% of users use it wrong. I do accept that many will not use orElse() or map() when they should, but that is fine - so long as they use isPresent() before get(), the rest can come later. To be clear, a lot of the claimed justification for this from Stuart's earlier message is that people are using get() instead of orElse(), orElseThrow(), filter(), map() etc. _but I don't care about that at all_. Overuse of isPresent() is rife but fine. The only case that matters is use of get() without any check. On 27 April 2016 at 06:42, Stuart Marks <stuart.marks@oracle.com> wrote:
Briefly, it strictly isn't a rename. The old method would be deprecated not-for-removal, and would be left in place indefinitely. Does this still create pain? If so, is there some way the proposal can be modified to reduce it?
We keep our code free of all deprecations. As such, this will require change when we adopt Java 9. So, this is a rename in practical terms. The replacement being significantly longer will also cause line-length issues and reformatting in places. TLDR, we view Optoinal as a new core API added in Java 8 that was have relied on and used widely. It is fine as is, and does not need any alteration. Stephen
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/16 3:43 AM, Stephen Colebourne wrote:
In OpenGamma Strata we have 546 uses of Optional.get(). Renaming this would be painful and add no value.
Hi Stephen, I just sent a reply [1] to Kevin B, wherein I clarified "rename." Briefly, it strictly isn't a rename. The old method would be deprecated not-for-removal, and would be left in place indefinitely. Does this still create pain? If so, is there some way the proposal can be modified to reduce it? s'marks
So far, this thread has been mostly "I DON'T LIKE THIS CHANGE." But let's talk about a real underlying issue instead, mkay? While no one has actually said this (I guess everyone was too busy slinging rhetoric and raising strawman objections) there is at least one real issue here, which is: - I have a library - I want it to be free of deprecation warnings - I want it to compile cleanly on platform versions N...N+k, where something is deprecated with an alternative in M>N, and the alternative does not exist in N. That leaves the hypothetical library provider above (which I assume is Stephen's situation, though it was left unsaid) with some bad choices: - @SuppressWarnings the use everywhere - Compile without deprecation warnings - Have multiple sourcebases I think the real issue is that @SuppressWarnings is too blunt a tool to be useful in this situation, so deprecation causes collateral pain for library developers. If we can make the deprecation more painless for this class of developers (the ones disproportionately affeted), then I think much of the noise goes away. On 4/27/2016 1:42 AM, Stuart Marks wrote:
On 4/26/16 3:43 AM, Stephen Colebourne wrote:
In OpenGamma Strata we have 546 uses of Optional.get(). Renaming this would be painful and add no value.
Hi Stephen,
I just sent a reply [1] to Kevin B, wherein I clarified "rename."
Briefly, it strictly isn't a rename. The old method would be deprecated not-for-removal, and would be left in place indefinitely.
Does this still create pain? If so, is there some way the proposal can be modified to reduce it?
s'marks
Better suppress warnings would be nice, and the problem statement below is reasonable, what concerns me more is the impact on clients of the Strata API. Strata uses Optional heavily - we have "done the right thing" and do not return null from any public methods. As such, a *lot* of methods return Optional. If this change happens, then everyone who adopts Strata today and codes against our API (legitimately using isPresent() followed by get() ) will suffer when Java 9 is released. I don't see how a better SuppressWarnings helps those clients. FWIW, I will have to consider migrating to Guava Optional if I can't rely on a core API like JDK Optional remaining stable. <sarcasm> I propose that we deprecate List.get(int). To compensate, we should add a new method getButPleaseCheckListSizeFirst(int) The rationale is that lots of developers call get(int) without correctly checking the list size. I estimate that 69.3724% of all developers misuse the method. </sarcasm> If I thought the method needed renaming, I'd go along with this. I'm am accepting the demise of Class.newInstance() for example, although I think even that is a very marginal deprecation. If it seems to Kevin that get() is the right name, and others like me agree, perhaps its worth considering whether the change is actually appropriate? FWIW, the best example in JSR-310 of a design mistake is TemporalAmount.get(TemporalUnit): http://stackoverflow.com/questions/24491243/why-cant-i-get-a-duration-in-min... Renaming that method to unitValue(TemporalUnit) or similar would actually be a useful change (as it is a framework method and almost all uses of that method in application code are wrong). Stephen On 27 April 2016 at 15:48, Brian Goetz <brian.goetz@oracle.com> wrote:
So far, this thread has been mostly "I DON'T LIKE THIS CHANGE." But let's talk about a real underlying issue instead, mkay?
While no one has actually said this (I guess everyone was too busy slinging rhetoric and raising strawman objections) there is at least one real issue here, which is:
- I have a library - I want it to be free of deprecation warnings - I want it to compile cleanly on platform versions N...N+k, where something is deprecated with an alternative in M>N, and the alternative does not exist in N.
That leaves the hypothetical library provider above (which I assume is Stephen's situation, though it was left unsaid) with some bad choices: - @SuppressWarnings the use everywhere - Compile without deprecation warnings - Have multiple sourcebases
I think the real issue is that @SuppressWarnings is too blunt a tool to be useful in this situation, so deprecation causes collateral pain for library developers. If we can make the deprecation more painless for this class of developers (the ones disproportionately affeted), then I think much of the noise goes away.
On 4/27/2016 1:42 AM, Stuart Marks wrote:
On 4/26/16 3:43 AM, Stephen Colebourne wrote:
In OpenGamma Strata we have 546 uses of Optional.get(). Renaming this would be painful and add no value.
Hi Stephen,
I just sent a reply [1] to Kevin B, wherein I clarified "rename."
Briefly, it strictly isn't a rename. The old method would be deprecated not-for-removal, and would be left in place indefinitely.
Does this still create pain? If so, is there some way the proposal can be modified to reduce it?
s'marks
On 04/27/2016 04:44 PM, Stephen Colebourne wrote:
Better suppress warnings would be nice, and the problem statement below is reasonable, what concerns me more is the impact on clients of the Strata API. Strata uses Optional heavily - we have "done the right thing" and do not return null from any public methods. As such, a *lot* of methods return Optional. If this change happens, then everyone who adopts Strata today and codes against our API (legitimately using isPresent() followed by get() ) will suffer when Java 9 is released. I don't see how a better SuppressWarnings helps those clients.
Sure, but almost all of the uses of Optional.get() will be in the future. This rather reminds me of the early UNIX bug report about make syntax. The bug was that make(1) treated tab characters and spaces differently. This bug could have been fixed but there were about 20 users of make already, and they would have had to change their makefiles. And probably everyone reading this mail has suffered because of that bug. Andrew.
Hi, Looking at real problems is good, and in this case there’s quite a few layers of issues underlying issues indeed. First the problem of the “get” method underlain by the deprecation issue. As mentioned, the name “get” is a good, proper name for a method. There are no problems with the name itself. The problem is with the implementation backing this name not matching what it should obviously-from-just-reading-the-name be really doing. A misbehaving implementation is nothing new and is a relatively easy problem to fix. First, not throwing a previously thrown exception should not break too many consumers. Likewise returning a previously not returned value won’t deprive consumers of any values they are relying on getting. Still, some consumers might be broken. On balance, if the evidence presented earlier the thread holds true then many (perhaps quite a few more!) consumers -- currently silently broken -- would get silently mended. As a bonus, returning null and not throwing a NSEE doesn’t disqualify Optional from becoming a value type in future nor prevent it from containing a future value type. The solution to how this is to be done is left as an exercise for the reader. 😊 But yeah, there is a solution. Against this backdrop the suggestion to deprecate the get-method without changing its implementation in essence changes the thrown NoSuchElementException into a quasi-checked exception via creative (ab)use of the deprecation mechanism. That leaves the class with just as many methods with problems as there were in the beginning. If the get-method is then recreated under a different name with all the problems in the implementation left unfixed, Optional ends up with 1 more method with problems than what it started with. This way of “fixing” the problem seems to me like a workaround to deeper issues with the language. -- Have a nice day, Timo Sent from Mail for Windows 10 From: Brian Goetz Sent: Wednesday, April 27, 2016 16:49 To: Stuart Marks; Stephen Colebourne Cc: core-libs-dev Subject: Re: RFR(m): 8140281 deprecate Optional.get() So far, this thread has been mostly "I DON'T LIKE THIS CHANGE." But let's talk about a real underlying issue instead, mkay? While no one has actually said this (I guess everyone was too busy slinging rhetoric and raising strawman objections) there is at least one real issue here, which is: - I have a library - I want it to be free of deprecation warnings - I want it to compile cleanly on platform versions N...N+k, where something is deprecated with an alternative in M>N, and the alternative does not exist in N. That leaves the hypothetical library provider above (which I assume is Stephen's situation, though it was left unsaid) with some bad choices: - @SuppressWarnings the use everywhere - Compile without deprecation warnings - Have multiple sourcebases I think the real issue is that @SuppressWarnings is too blunt a tool to be useful in this situation, so deprecation causes collateral pain for library developers. If we can make the deprecation more painless for this class of developers (the ones disproportionately affeted), then I think much of the noise goes away. On 4/27/2016 1:42 AM, Stuart Marks wrote:
On 4/26/16 3:43 AM, Stephen Colebourne wrote:
In OpenGamma Strata we have 546 uses of Optional.get(). Renaming this would be painful and add no value.
Hi Stephen,
I just sent a reply [1] to Kevin B, wherein I clarified "rename."
Briefly, it strictly isn't a rename. The old method would be deprecated not-for-removal, and would be left in place indefinitely.
Does this still create pain? If so, is there some way the proposal can be modified to reduce it?
s'marks
participants (13)
-
Andrew Haley
-
Brian Goetz
-
ecki＠zusammenkunft.net
-
Kevin Bourrillion
-
Mario Torre
-
Maurizio Cimadamore
-
Peter Levart
-
Rémi Forax
-
Stephen Colebourne
-
Stuart Marks
-
Tagir F. Valeev
-
Timo Kinnunen
-
Vitaly Davidovich