phobos
phobos copied to clipboard
dlang
WIP: Add non-throwing variants of parse() and to()
Putting it out in the open just to get early feedback.
See also: https://forum.dlang.org/post/[email protected]
Thanks for your pull request and interest in making D better, @nordlow! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:
- My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
- My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
- I have provided a detailed rationale explaining my changes
- New or modified functions have Ddoc comments (with
Params:andReturns:)
Please see CONTRIBUTING.md for more information.
If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.
Bugzilla references
Your PR doesn't reference any Bugzilla issue.
If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.
Testing this PR locally
If you don't have a local development environment setup, you can use Digger to test this PR:
dub fetch digger
dub run digger -- build "master + phobos#6665"
I like the idea (and I'm generally in favor of using more Nullable / Optional in Phobos).
However a few concerns for now:
- should we really promote a different type and not use
Nullable. I'm aware of the problem that Nullable doesn't allow one to store a custom error message. I guess at the very least the API should be similar (maybe we can generalize the Nullable concept like we did with ranges). Also there are lots of nice goodies inNullablelikeapply(the analog of Folly'sthen) -
expected.result(.getin Nullable) should automaticallythrowwhen accessed and I think we might be better off with sth. likeget!ConvException. OnlyNullable.get(<fallback>)isnothrow
Sounds good to me.
Two key questions:
- Is
Nullable.applynothrow? - Can we feed
Nullable.applya lambda and still benothrow?
I'll have a go at your proposal anyway... :)
Update: Changed to using Nullable.
Is Nullable.apply nothrow? Can we feed Nullable.apply a lambda and still be nothrow?
Yes and even @nogc e.g.
https://github.com/dlang/phobos/blob/bb769cfaf51f32a9f20567f885361fe828e9f8a3/std/typecons.d#L3807-L3823
See also: https://dlang.org/changelog/2.080.0.html#std-typecons-nullable-apply
That's because Nullable.get doesn't use exceptions, but only assert
I don’t think we should use Nullable, instead implement a proper Optional type. I have an implementation here [1] that implements the range interface and safe access.
[1] https://github.com/jacob-carlborg/dlp/blob/master/source/dlp/core/optional.d
Can you please describe why we need an Optional instead of a Nullable?
Are there any other reasons than the above mentioned?
Can you please describe why we need an Optional instead of a Nullable? Are there any other reasons than the above mentioned?
No, it's mostly because it has a range interface and safe access interface. I also think it's a better, more descriptive name.
How the is the interface @safer?
AFAICT, Optional.get is as (un)safe as Nullable.get is, which is the critical function when it comes to (memory) safety. Both may result in unexpected behaviour when compiled without asserts and called when isPresent is false.
This bug can at least be avoided if we instead use Nullable.apply and port it to Optional.
I agree that Optional is better naming, though. More similar to other languages naming, such as C++'s std::optional.
No, it's mostly because it has a range interface and safe access interface. I also think it's a better, more descriptive name.
We already have Nullable in Phobos, and this is exactly the sort of thing it's designed for - the case where a variable may or may not have a value.
How the is the interface
@safer?
I didn't say @safe, I said "safe access", without the at sign. "safe" might not be the best name but what I mean is you can do this:
class Bar { int a; }
class Foo { Bar bar; }
auto foo = none!Foo;
auto a = foo.bar.a; // safe access, "a" will be an optional int
auto b = a.or(3); // unwraps "a" if it has a value, otherwise returns 3
auto c = a.map!(i => i * 4); // it also supports the range interface
If the standard algorithms are used, the lambda will only be called if the optional has a value. When using map it's basically like apply for Nullable but with a standard interface.
How about until we all settle on the return type, we simply use auto? That way the exact return type becomes an implementation detail and in future, when we have made the necessary changes to Nullable or we have added an new type, we could switch from auto to an explicit return type.
Another option would be add these functions first to std.experimental, so we would be free to change the return type later.
So lets try adding your opDispatch to Nullable then!
Please no. I suggest we deprecate Nullable and add a new implementation called Optional. The name Nullable is poorly chosen. The name Nullable suggests it should only be used for value types which otherwise cannot be assigned null. But Optional is supposed to be used for all types, even those that can be assigned null. An optional pointer would mean that the pointer might not point to something, a plain pointer would mean that it always points to something, it can never be null (I know that this is not enforced by the language).
I agree Nullable can be used here but it's says nothing about "this value may be present" in a truely generic sense - null is a very valid part of some value type domains - e.g. pointers.
An optional is explicit in intent and it'd be a much nicer interface with an Optional. Also, once we start using Nullable in phobos for stuff like this it's going to be very hard to go back :/
So super big +1 for putting this in std.experimental until we have an Optional type.
If we put this in std.experimental.typecons shall std.typecons.parse reuse std.experimental.typecons.tryParse or shall we duplicate implementations?
Nothing outside std.experimental is allowed to use std.experimental. Perhaps create an internal function marked with package to be able to share it?
You mean an internal function. say tryParse(), marked with package inside std.experimental.typecons?
Nothing outside std.experimental is allowed to use std.experimental.
Nope, that's not true. You can't return experimental types, but Phobos is allowed to use things from experimental internally. We do so with checkedint and the allocator packages in a few places already.
You mean an internal function _tryParse() marked with package inside std.experimental.typecons?
No, inside std.typecons.
So, @wilzbach and @jacob-carlborg...should I put tryParseand tryTo
- in
std.experimental.typeconsand call them fromstd.typeconsor - in
std.typeconsand mark them withpackage?
And should we expose a documentation for the new functions?
@andralex?
I will let @wilzbach decide.
I would go with experimental because they are then at least partially exposed whereas package functions are entirely hidden from the user.
You mean an internal function. say tryParse(), marked with package inside std.experimental.typecons?
Why stdx.typecons and not std.experimental.conv?
I would go with experimental because they are then at least partially exposed whereas package functions are entirely hidden from the user.
What I meant was to put all the logic in a package function inside std.conv and expose it in std.experimental.conv.tryParse and std.conv.parse.
I have only one plea.
If we introduce an Optional as a possible alternative to Nullable, which I am all for, then whatever you do, please for the love of God do not alias get this! It was already a mistake with Nullable, though I suppose it did introduce feature parity with nullable pointers in the field of unexpected crashes...
