Skip to content

Conversation

@FeepingCreature
Copy link
Contributor

It's basically a copypaste.

Follow-up to #7060 .

Changes: .get is now .value (.get was always a weird naming); behavior is well-defined in the absence of a value. Old toString methods have been removed. nullify has no equivalent. Everything else is the same, down to the unittests. apply has been changed to work with both.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @FeepingCreature! 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: and Returns:)

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#7065"

std/typecons.d Outdated
Constructor initializing `this` with `value`.
Params:
value = The value to initialize this `Nullable` with.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copypaste error in docs?

Copy link
Contributor Author

@FeepingCreature FeepingCreature Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops thanks
You saw nothing. NOTHING.

@FeepingCreature FeepingCreature force-pushed the feature/std-typecons-optional branch from 7cd307f to a5d7023 Compare June 11, 2019 07:52
@wilzbach
Copy link
Contributor

nullify has no equivalent

Could you please explain why .optional isn't possible.

@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Jun 11, 2019

.optional is possible, and is the equivalent of .nullable. .nullify, the method to explicitly empty out an existing Nullable, has no equivalent, because it's a weird operation whose purpose I don't understand. (I was tempted to remove opAssign!T as well.)

formatValue(writer, _value.payload, fmt);
else
put(writer, "Optional.absent");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a non const and const version needed and not just a const version?

Copy link
Contributor Author

@FeepingCreature FeepingCreature Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toString should still work if Optional embeds a data type with non-const toString (as is, sadly, the default).

{
if (!_present)
{
throw missingValueException_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be a recoverable error imo. It's more normal (and I think cleaner) for optional types to be more about asking for permission than asking for forgiveness. I'd suggest either an assertion or an Error as opposed to an Exception.

Here's an example showing how attempting to unwrap an empty optional value in Rust produces a panic (unrecoverable error) and accessing an Option's value is normally done via pattern matching (i.e. checking state first, and then accessing value): http://www.ameyalokare.com/rust/2017/10/23/rust-options.html

Copy link
Contributor Author

@FeepingCreature FeepingCreature Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the defining difference between Nullable and Optional. The entire reason why get can be alias this in Nullable is that accessing a Nullable that is null is undefined (check the doc for Nullable.get!), so from a certain perspective Nullable!T to T is an operation that "cannot fail" and is thus "safe". It is thus 100% deliberate for Optional.value to throw an Exception, because the state of "Optional without a value" is a fully defined one and thus one that must result in ordinary control flow in all methods. (That's why valueUnsafe says "unsafe".)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FeepingCreature this is not what normally distinguishes an optional type from a nullable type.

The core difference between option types and nullable types is that option types support nesting (Maybe (Maybe A) ≠ Maybe A), while nullable types do not (String?? = String?).

https://en.wikipedia.org/wiki/Option_type

Very often, the fundamental motivation for using an optional type is to not involve exceptions. Wrongly attempting to access the value of an optional type is a programmer error, in the same sense attempting to index an out-of-bounds array index is a programmer error. (In which case D produces an unrecoverable RangeError, not an exception.)

@FeepingCreature FeepingCreature force-pushed the feature/std-typecons-optional branch from a5d7023 to 46a5acd Compare June 11, 2019 10:28
Copy link
Contributor

@jacob-carlborg jacob-carlborg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Optional should provide a range interface
  • I think this should be added to a new module
  • Perhaps implement opDispatch to allow safe access to methods of the stored value
  • I would expect to be able to do something like this: some(3) and none!int, to indicate that a value is present or not
  • I'm questioning whether there should be a get/value method at all. Instead all access to the value should be performed using algorithms

@pineapplemachine
Copy link
Contributor

pineapplemachine commented Jun 12, 2019 via email

@jacob-carlborg
Copy link
Contributor

I don't believe this is a normal thing for optional types to do in the standard libraries of other languages which support them.

No, because they have built-in language support. Or the language does not support anything like opDispatch.

@pineapplemachine
Copy link
Contributor

@jacob-carlborg I'm not sure what you mean by built-in language support. Of the optional type implementations I've looked at in other languages, you always explicitly .get or .unwrap or etc. to refer to the value and its properties. I think that using opDispatch this way is bound to cause headaches when expanding the Optional interface with more functionality later on, and bound to cause confusion for people trying to use this type, especially those coming from C++ or Rust.

@jacob-carlborg
Copy link
Contributor

@jacob-carlborg I'm not sure what you mean by built-in language support

I mean that in other language like Swift and C# it's possible to do something like this (here with D syntax):

Object? a;
string b = a?.toString ?? "foobar";

In the above example, if a does not have a value "foobar" will be assigned to b. If a has a value, whatever toString returns will be assigned to b.

@pineapplemachine
Copy link
Contributor

I mean that in other language like Swift and C# it's possible to do something like this (here with D syntax):

In your example Object? is not an optional Object but a nullable one, and ?? is the null-coalesce operator.

@jacob-carlborg
Copy link
Contributor

Of the optional type implementations I've looked at in other languages, you always explicitly .get or .unwrap or etc. to refer to the value and its properties

That's not the recommend way to do it in Scala, for example. It's recommended to use algorithms like map, flatMap, filter and so on. Here are some examples [1].

Swift has a different approach. It supports optional chaining and optional nil-coalescing [2], which is basically my opDispatch suggestion. It also has a dedicated if-syntax:

if let starPath = imagePaths["star"] {
    print("The star image is at '\(starPath)'")
} else {
    print("Couldn't find the star image")
}

[1] https://danielwestheide.com/blog/2012/12/19/the-neophytes-guide-to-scala-part-5-the-option-type.html

[2] https://developer.apple.com/documentation/swift/optional

@jacob-carlborg
Copy link
Contributor

In your example Object? is not an optional Object but a nullable one

It's an optional Object in Swift: https://developer.apple.com/documentation/swift/optional.

@aliak00
Copy link
Contributor

aliak00 commented Jun 12, 2019

I agree with @jacob-carlborg on everything as well and also strongly with @pineapplemachine disagreement about opDispatch. It's iffy at best to have an opDispatch inside a wrapper type because as mentioned, it will hinder the api. What if I put a range in the wrapper (that's a range and has an opDispatch) and I call .front? Please no.

@aliak00
Copy link
Contributor

aliak00 commented Jun 12, 2019

That's not the recommend way to do it in Scala, for example. It's recommended to use algorithms like map, flatMap, filter and so on. Here are some examples [1].

Yes, and having optional be a range will enable the scala approach. We can't have Swift's approach without language support. The best I think I at least came up with is this

@pineapplemachine
Copy link
Contributor

pineapplemachine commented Jun 12, 2019

Was just reading about this in Swift after you posted some links. I stand corrected. I wasn't thinking about the safe member access or unwrapping member access operators. I for one would be happy to introduce ?. and !. to D, and then we might use these for accessing attributes of the wrapped value. But please let's not try to contort opDispatch into serving that purpose.

You can at least easily get most of the same if behavior with feep's implementation by adding a bool opCast(T: bool) method. (I would recommend this, actually, now seeing that it hasn't got this already.)

As for map and filter and so on, these are not about accessing properties of the enclosed value but are about using a range interface to enumerate the optional's zero-or-one element. You still need to actually unwrap the value at the end of the maps and filters and whatever. This is not a call for opDispatch but an argument for adding a range interface. (Like you mentioned before, and I agree with this.)

Potentially of interest is mach's optional type implementation: https://github.com/pineapplemachine/mach.d/blob/master/mach/types/option.d

@aliak00
Copy link
Contributor

aliak00 commented Jun 12, 2019

I'd much rather have #3915 revived and worked on instead of another nullable. Plus all the feature that are requested in this thread (except for jacob's opDispatch) are already in the optional dub package - and is already being used and proven. Which I'm more than happy to put in to phobos if desired.

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Jun 12, 2019

There should be a method that returns the value if present, otherwise the passed in value. It can be called or, orElse, getOrElse or valueOrElse.

@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Jun 12, 2019

It seems I have aesthetic disagreements about "how an Optional type should behave" with pretty much everybody here. Fun!

It seems a lot of people are looking at Optional types from functional languages, whereas I see the core examples for it as Nullable and Flag in the same file.

I would expect to be able to do something like this: some(3) and none!int, to indicate that a value is present or not

You are able to do this: 3.present and Optional!int.absent. absent!int can be added, I guess, for consistency with things like Yes and No. orElse seems like the sort of thing that should be its own PR. I really hate optional.front - what does "front" have to do with accessing a value? Why does Some a | None have a "front"? - so I'm not sure what to do about that. opDispatch seems like it's asking for all kinds of confusing errors.

Anyway, these all - aside range support, which I dislike for other reasons - seem like wishlist features, not necessary requirements.

@jacob-carlborg
Copy link
Contributor

I really hate optional.front - what does "front" have to do with accessing a value?

front should not be used directly to access the value. Then you're missing the point. There should be not direct access to the value. Technically front will give you direct access to the value but it's only to be used by the algorithms.

Anyway, these all - aside range support, which I dislike for other reasons - seem like wishlist features, not necessary requirements.

Well, if there are no improvements over Nullable I don't see a point in adding this.

@FeepingCreature
Copy link
Contributor Author

FeepingCreature commented Jun 12, 2019

The improvement over Nullable is the lack of the alias get this, basically. Which is a significant improvement, since alias get this keeps causing problems and wasting developer time on bugs undetected until runtime.

@aliak00
Copy link
Contributor

aliak00 commented Jun 13, 2019

The improvement over Nullable is the lack of the alias get this, basically. Which is a significant improvement, since alias get this keeps causing problems and wasting developer time on bugs undetected until runtime.

I don't think adding an exact copy of a whole type to fix a bug in the original type is the way to go. I think we should press harder on PR #7060 and get that in.

@FeepingCreature
Copy link
Contributor Author

Looks like we're going with improving Nullable instead. I'll close this once the deprecation PR is merged.

@FeepingCreature
Copy link
Contributor Author

Deprecation got merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants