Implement apply for Nullable, in analogy to Haskell's monad operation #6182

FeepingCreature · 2018-02-15T17:31:52Z

bind is an operation for Nullables that unpacks the nullable, if it's non-null, and then repacks the result in another appropriately-typed nullable. This replaces a lot of awkward thingy.isNull ? Nullable!SomeType.init : op(thingy.get) with thingy.bind!op.

"Monads are underrated."

dlang-bot · 2018-02-15T17:31:53Z

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.

wilzbach

Thanks a lot for proposing this.

I added a few comments. In general: getting a new function into Phobos is hard, so be prepared for thorough reviews and making a strong case for its addition.
I made an initial pass with mostly nits.

BTW this rebinds me about safeDeref that @MetaLang posted a while ago on the forums: https://forum.dlang.org/post/[email protected]

Other random ideas:

any chance this could work with any kind of null pointers, e.g. classes? (then it could go into std.functional)
there's std.experimental.typecons, but it's more or less a graveyard (at the moment)
there's a discussion about orElse for ranges. I know, not the same, but as ideally all functions would magically work together so I thought it's worth mentioning

wilzbach · 2018-02-16T01:29:29Z

std/typecons.d

+{
+    import std.functional : unaryFun;
+
+    auto bind(T)(T t) if (isInstanceOf!(Nullable, T) && is(typeof(unaryFun!fun(T.init.get))))


https://dlang.org/dstyle.html#phobos_declarations

Constraints on declarations should have the same indentation level as their declaration:

wilzbach · 2018-02-16T01:58:32Z

std/typecons.d

+{
+    import std.meta : Alias;
+
+    alias toFloat = Alias!(i => cast(float) i);


alias toFloat = i => cast(float) i (same for the other uses)

wilzbach · 2018-02-16T02:03:41Z

std/typecons.d

+
+    sample = 3;
+    f = sample.bind!toFloat;
+    assert(!sample.isNull && !f.isNull);


assert(f.get == 3.0)

(similar for the examples below)

wilzbach · 2018-02-16T02:06:21Z

std/typecons.d

+}
+
+///
+@safe unittest


Please add nothrow pure @nogc too

Also this is quite a long example for user-facing docs. Split up into at least two and add comments.

wilzbach · 2018-02-16T02:06:48Z

std/typecons.d

+    result = sample.bind!greaterThree;
+    assert(!sample.isNull && !result.isNull);
+}
+


Nit: typically functions in Phobos are separated by only one line

wilzbach · 2018-02-16T02:09:07Z

std/typecons.d

+    sample = 4;
+    result = sample.bind!greaterThree;
+    assert(!sample.isNull && !result.isNull);
+}


Would be great to see more tests/examples here, e.g. pipes of bind and other real world examples to make your PR and case stronger.

I'm not sure why you'd want to pipe it. It's really just a convenience function for !foo.isNull ? fn(foo.get) : Nullable!(typeof(fn(typeof(foo.get).init))).init.

wilzbach · 2018-02-16T02:11:36Z

changelog/std-typecons-nullable-bind.dd

@@ -0,0 +1,13 @@
+`bind` was added to std.typecons.


Backticks for std.typecons too?

wilzbach · 2018-02-16T02:12:06Z

changelog/std-typecons-nullable-bind.dd

@@ -0,0 +1,13 @@
+`bind` was added to std.typecons.
+
+$(REF bind, std, typecons) is an operation for $(D Nullable) values that "unpacks" the $(D Nullable), performs some


Mix of style. New Phobos code uses backticks and you even you use them in the title ;-)
Also it makes sense to use $(REF Nullable, std, typecons)

wilzbach · 2018-02-16T02:18:00Z

changelog/std-typecons-nullable-bind.dd

+{
+    return n.bind!(i => i * i);
+}
+-----


I'm not sure that this is a good example on why bind is useful.
Sure, it shows what it does, but a user reading the release documentation will ask themselves: how can I use this in my code?

Nullable!int n; n = n.bind!log10; // does nothing if isNull assert(n.isNull); n = 100; assert(n.bind!log10.get == 2);

(this is subjective of course)

I mostly just wanted to show the syntax. log10 is reals, not ints..?

It's essentially a map operation. I think otherwise considering Nullable!T as a range of 0 or 1 elements is much cleaner design.

wilzbach · 2018-02-16T06:51:32Z

Also note that NullableRef already has a bind method.

JackStouffer · 2018-02-16T13:47:16Z

IMO this would be better setup as a method of Nullable.

FeepingCreature · 2018-02-16T15:33:50Z

I thought you couldn't do templated methods with alias parameters that take closures. (For no clear reason.)

MetaLang · 2018-02-16T17:03:47Z

I don't think bind is a good name, because NullableRef already has a bind method that does something different, and because it only makes sense in the context of a Haskell's particular implementation of Monadic containers. The (more or less) equivalent in D is a range that you apply a map function to, but Nullable is not a range (though it could easily be given a range interface).

JackStouffer · 2018-02-16T17:50:32Z

apply?

FeepingCreature · 2018-02-16T17:54:37Z

I had originally called it map, but I thought that might be too confusing. Should I change it to map? It is sort of like map if you think of a Nullable as a range that may have a length of 0 or 1.

MetaLang · 2018-02-16T17:56:10Z

@JackStouffer's suggestion sounds fine to me.

baklava151 · 2018-02-16T18:33:30Z

More analagous to Haskell's Maybe monad (well, exactly like Haskell's Maybe monad), monads overall are more general than that. Think it'd be interesting if they were added to Phobos at some point. Still, a good start.

MetaLang · 2018-02-16T19:32:34Z

The main problem with Nullable (and it has many problems) is that it is not like the Maybe monad. It implicitly converts to the underlying value which, it turns out, causes all sorts of problems when you try to use Nullable seriously.

FeepingCreature · 2018-02-16T23:48:46Z

I try my level best to never use that functionality.

FeepingCreature · 2018-02-19T07:50:55Z

Renamed to apply.

DmitryOlshansky · 2018-02-19T08:23:26Z

changelog/std-typecons-nullable-bind.dd

+{
+    return n.bind!(i => i * i);
+}
+-----


It's essentially a map operation. I think otherwise considering Nullable!T as a range of 0 or 1 elements is much cleaner design.

DmitryOlshansky · 2018-02-19T08:25:42Z

Think it'd be interesting if they were added to Phobos at some point. Still, a good start.

Instead of introducing a foreign concept, let's just reuse the range analogy. In fact range API + std.algorithm/std.range may work like a monad to an extent

jmdavis · 2018-02-19T08:39:08Z

I'd be very leery about treating Nullable as a range. Conceptually, I'd consider a Nullable and a range to be quite different, though I can see an argument for treating them similarly. However, it gets particularly iffy if Nullable is wrapped around a range, since then the range API on the Nullable suddenly wins out over the range API on the wrapped type, which could break code.

FeepingCreature · 2018-02-19T08:58:52Z

This only breaks code because Phobos is horribly broken in the first place in not forcing programmers to call .get explicitly. If anything, that's what should - must - be changed.

jmdavis · 2018-02-19T09:00:24Z

This only breaks code because Phobos is horribly broken in the first place in not forcing programmers to call .get explicitly.

Maybe having get is a design flaw, but the fact of the matter is that it's there, so we have to deal with that.

FeepingCreature · 2018-02-19T12:38:18Z

Having get is fine, the horrible misfeature is alias get this.

Anyway, if we could treat Nullable as a range (or both as Monads) that'd be great, but we can't, so apply should be fine I guess.

MetaLang · 2018-02-20T15:14:15Z

@jmdavis that's a good point that I had not considered... maybe an idiomatic D implementation of an Option-like range should actually invert the API such that the traditional member functions an Option type would have are just free functions that work on ranges. I think this would square better with Andrei's concerns that we already have std.range.Only which is basically Some!T.

FeepingCreature · 2018-02-26T08:48:21Z

Ping?

JackStouffer · 2018-03-09T14:44:13Z

std/typecons.d

+    Nullable!int sample;
+
+    // apply(null) results in a null $(D Nullable) of the function's return type.
+    auto f = sample.apply!toFloat;


for demonstration, change auto to Nullable!float

JackStouffer · 2018-03-09T14:52:56Z

@FeepingCreature Can you please email @andralex (email can be found at https://erdani.com/index.php/contact/) about reviewing this name addition? Thanks.

andralex · 2018-03-22T21:48:11Z

Wait a second. The title says add bind but the code adds apply. What gives?

I'm okay with adding apply but as a method of Nullable, not an alembicated free function.

andralex

Please make a method.

FeepingCreature · 2018-03-22T22:21:18Z

Gladly, just as soon as that actually works.

struct S
{
    void apply(alias fun)() { fun(); }
}

void main()
{
    int k = 5;
    S().apply!(() => k);
}

Error: template instance `apply!(delegate () => k)` cannot use local __lambda1 as
parameter to non-global template apply(alias fun)()`

FeepingCreature · 2018-03-27T08:15:39Z

@andralex ping

JackStouffer · 2018-03-27T12:39:23Z

I'm going to stick my neck out here and move forward with this.

@andralex Feel free to revert if you disagree.

Outdated

andralex · 2018-03-27T12:45:42Z

Cool. How about taking auto ref? Presumably some functions might want to modify their argument.

FeepingCreature · 2018-03-27T13:37:34Z

Since this is pretty much copied from Haskell's monads, it's very much a pure functional approach, meaning you're supposed to construct a new Nullable and assign it instead of modifying in-place. I feel like if you're modifying the parameter, you may be doing the idiom wrong.

Not sure if that's a sufficient reason not to do it?

andralex · 2018-03-27T14:05:23Z

@FeepingCreature the only problem is we're not disallowing functions that take ref. Ideally we'd either disallow them or let them run with the expected behavior. Right now the results may be surprising.

Generally: principles of FP apply slightly differently (and not always elegantly) to languages with mutation.

wilzbach · 2018-03-27T16:55:23Z

Can we please instead of sticking our necks out, have an experimental stage for new functions?

Rust is showing the way here by making news additions "unstable" first and then stabilizing them after this testing period, e.g. https://blog.rust-lang.org/2018/02/15/Rust-1.24.html (they do the same thing for language features too: https://doc.rust-lang.org/unstable-book)
I proposed a few things at #6178 (comment) (I don't care which one is picked).

However, any mechanism that allows us to mark it unstable for one or two versions is a lot better than immediately releasing it and releasing that we have made a mistake / slight oversight which happens far too frequently.

JackStouffer · 2018-03-27T16:58:45Z

Sounds like a good topic for a DIP.

andralex · 2018-03-27T18:42:32Z

For new free entities great, for methods marking something as unstable is more difficult. Perhaps we could add a new attribute @unstable.

wilzbach · 2018-03-30T12:02:11Z

Can we please instead of sticking our necks out, have an experimental stage for new functions?

-> #6389 (a start anyhow)

Implement bind for Nullable, in analogy to Haskell's monad operation

659dcc2

FeepingCreature requested review from MetaLang and PetarKirov as code owners February 15, 2018 17:31

FeepingCreature added 2 commits February 15, 2018 18:36

add changelog entry

cbf7917

std.typecons.bind: fix syntax error

b0c382a

FeepingCreature force-pushed the feature/nullable-bind branch from 9c43129 to b235ecd Compare February 15, 2018 18:02

std.typecons.bind: improve documentation

680e5c7

FeepingCreature force-pushed the feature/nullable-bind branch from b235ecd to 680e5c7 Compare February 15, 2018 19:14

wilzbach reviewed Feb 16, 2018

View reviewed changes

std.typecons.bind: style fixes, address github feedback

28047d6

std.typecons.bind: rename to apply

beef0a9

DmitryOlshansky previously requested changes Feb 19, 2018

View reviewed changes

DmitryOlshansky added the @andralex label Feb 19, 2018

JackStouffer approved these changes Mar 9, 2018

View reviewed changes

std.typecons.apply: make unittest clearer

057386f

andralex removed the @andralex label Mar 22, 2018

andralex previously requested changes Mar 22, 2018

View reviewed changes

JackStouffer changed the title ~~Implement bind for Nullable, in analogy to Haskell's monad operation~~ Implement apply for Nullable, in analogy to Haskell's monad operation Mar 23, 2018

JackStouffer added Severity:Enhancement Merge:auto-merge labels Mar 27, 2018

dlang-bot merged commit 0c9a10b into dlang:master Mar 27, 2018

wilzbach mentioned this pull request Mar 30, 2018

Move std.typecons.apply to std.experimental #6389

Closed

andralex mentioned this pull request Mar 30, 2018

Followup to addition of apply #6390

Merged

		@@ -0,0 +1,13 @@
		`bind` was added to std.typecons.

		$(REF bind, std, typecons) is an operation for $(D Nullable) values that "unpacks" the $(D Nullable), performs some

Implement apply for Nullable, in analogy to Haskell's monad operation #6182

Implement apply for Nullable, in analogy to Haskell's monad operation #6182

Conversation

FeepingCreature commented Feb 15, 2018 • edited Loading

dlang-bot commented Feb 15, 2018

Bugzilla references

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Feb 16, 2018

JackStouffer commented Feb 16, 2018

FeepingCreature commented Feb 16, 2018 • edited Loading

MetaLang commented Feb 16, 2018

JackStouffer commented Feb 16, 2018

FeepingCreature commented Feb 16, 2018 • edited Loading

MetaLang commented Feb 16, 2018

baklava151 commented Feb 16, 2018

MetaLang commented Feb 16, 2018

FeepingCreature commented Feb 16, 2018

FeepingCreature commented Feb 19, 2018

Choose a reason for hiding this comment

DmitryOlshansky commented Feb 19, 2018 • edited Loading

jmdavis commented Feb 19, 2018

FeepingCreature commented Feb 19, 2018 • edited Loading

jmdavis commented Feb 19, 2018

FeepingCreature commented Feb 19, 2018 • edited Loading

MetaLang commented Feb 20, 2018

FeepingCreature commented Feb 26, 2018

Choose a reason for hiding this comment

JackStouffer commented Mar 9, 2018

andralex commented Mar 22, 2018

andralex left a comment

Choose a reason for hiding this comment

FeepingCreature commented Mar 22, 2018 • edited Loading

FeepingCreature commented Mar 27, 2018

JackStouffer commented Mar 27, 2018

andralex commented Mar 27, 2018

FeepingCreature commented Mar 27, 2018

andralex commented Mar 27, 2018

wilzbach commented Mar 27, 2018

JackStouffer commented Mar 27, 2018

andralex commented Mar 27, 2018

wilzbach commented Mar 30, 2018

FeepingCreature commented Feb 15, 2018 •

edited

Loading

FeepingCreature commented Feb 16, 2018 •

edited

Loading

FeepingCreature commented Feb 16, 2018 •

edited

Loading

DmitryOlshansky commented Feb 19, 2018 •

edited

Loading

FeepingCreature commented Feb 19, 2018 •

edited

Loading

FeepingCreature commented Feb 19, 2018 •

edited

Loading

FeepingCreature commented Mar 22, 2018 •

edited

Loading