Signal.mergeMany [] throws runtime exception

[rtfeldman]

Making a separate issue for this so it can be listed under https://github.com/elm-lang/core/issues/377 - clearly in scope, since this is a runtime exception that a Core API change could avoid.

[TheSeamau5]

I'm for turning this into

Signal.mergeMany : Signal (List a) -> Signal a -> Signal a

[TheSeamau5]

I'm for turning this into

Signal.mergeMany : Signal (List a) -> Signal a -> Signal a

[rtfeldman]

The use case I usually have for this is merging a dozen or so port Signals into one "user input" Signal.

For that purpose, something along the lines of andMap (or, my personal preference, a custom pipeline operator) would presumably suffice.

[rtfeldman]

The use case I usually have for this is merging a dozen or so port Signals into one "user input" Signal.

For that purpose, something along the lines of andMap (or, my personal preference, a custom pipeline operator) would presumably suffice.

[mgold]

I am of the opinion that, because the function will never be used with Signal.map and therefore a runtime error will occur immediately, this is acceptable as-is.

The most common use of mergeMany is to gather signals of the action type into one signal:

arrows : Signal Action
clicks : Signal Action
clock : Signal Action

actions : Signal Action
actions = Signal.mergeMany [arrows, clicks, clock]

-- and often
state = Signal.foldp step state0 actions

There's absolutely no possibility of encountering the error in this code, so there's no reason to encumber the programmer with a Maybe (Signal Action) or similar.

Hassan's version could be named mergeInto, and it might be neat in some edge cases. However, it it can be implemented on top of existing function, I'd say put it in and -extra library. (And if it can't it's beyond the scope of core.)

[mgold]

I am of the opinion that, because the function will never be used with Signal.map and therefore a runtime error will occur immediately, this is acceptable as-is.

The most common use of mergeMany is to gather signals of the action type into one signal:

arrows : Signal Action
clicks : Signal Action
clock : Signal Action

actions : Signal Action
actions = Signal.mergeMany [arrows, clicks, clock]

-- and often
state = Signal.foldp step state0 actions

There's absolutely no possibility of encountering the error in this code, so there's no reason to encumber the programmer with a Maybe (Signal Action) or similar.

Hassan's version could be named mergeInto, and it might be neat in some edge cases. However, it it can be implemented on top of existing function, I'd say put it in and -extra library. (And if it can't it's beyond the scope of core.)

[rtfeldman]

Personally I think if we rename <~ to |+ we no longer need mergeMany.

From the mergeMany docs:

type Update = MouseMove (Int,Int) | TimeDelta Float | Click

updates : Signal Update
updates =
    mergeMany
        [ map MouseMove Mouse.position
        , map TimeDelta (fps 40)
        , map (always Click) Mouse.clicks
        ]

Compare to:

type Update = MouseMove (Int,Int) | TimeDelta Float | Click

updates : Signal Update
updates =
    map MouseMove Mouse.position
      |+ map TimeDelta (fps 40)
      |+ map (always Click) Mouse.clicks

(|+) = Signal.merge

I think custom pipe operators are really nice, and worth exploring more. Whereas other custom infix operators make code more dense and difficult to scan, the conventions around how pipes are used in Elm mean they make code more organized and easier to scan.

Granted, mergeMany is more self-documenting, and may have nontrivial performance benefits (I actually have no idea), but this is both crash-proof and very readable to me. I look at it and I think "oh yeah, it's combining them."

[rtfeldman]

Personally I think if we rename <~ to |+ we no longer need mergeMany.

From the mergeMany docs:

type Update = MouseMove (Int,Int) | TimeDelta Float | Click

updates : Signal Update
updates =
    mergeMany
        [ map MouseMove Mouse.position
        , map TimeDelta (fps 40)
        , map (always Click) Mouse.clicks
        ]

Compare to:

type Update = MouseMove (Int,Int) | TimeDelta Float | Click

updates : Signal Update
updates =
    map MouseMove Mouse.position
      |+ map TimeDelta (fps 40)
      |+ map (always Click) Mouse.clicks

(|+) = Signal.merge

I think custom pipe operators are really nice, and worth exploring more. Whereas other custom infix operators make code more dense and difficult to scan, the conventions around how pipes are used in Elm mean they make code more organized and easier to scan.

Granted, mergeMany is more self-documenting, and may have nontrivial performance benefits (I actually have no idea), but this is both crash-proof and very readable to me. I look at it and I think "oh yeah, it's combining them."

[rtfeldman]

Perhaps a simpler alternative: replace Signal.mergeMany with Signal.mergeMap:

Before:

type Update = MouseMove (Int,Int) | TimeDelta Float | Click

updates : Signal Update
updates =
    mergeMany
        [ map MouseMove Mouse.position
        , map TimeDelta (fps 40)
        , map (always Click) Mouse.clicks
        ]

After:

type Update = MouseMove (Int,Int) | TimeDelta Float | Click

updates : Signal Update
updates =
    map MouseMove Mouse.position
      |> mergeMap TimeDelta (fps 40)
      |> mergeMap (always Click) Mouse.clicks

mergeMap : (a -> b) -> Signal a -> Signal b -> Signal b
mergeMap fn signal =
  Signal.merge (Signal.map fn signal)

[rtfeldman]

Perhaps a simpler alternative: replace Signal.mergeMany with Signal.mergeMap:

Before:

type Update = MouseMove (Int,Int) | TimeDelta Float | Click

updates : Signal Update
updates =
    mergeMany
        [ map MouseMove Mouse.position
        , map TimeDelta (fps 40)
        , map (always Click) Mouse.clicks
        ]

After:

type Update = MouseMove (Int,Int) | TimeDelta Float | Click

updates : Signal Update
updates =
    map MouseMove Mouse.position
      |> mergeMap TimeDelta (fps 40)
      |> mergeMap (always Click) Mouse.clicks

mergeMap : (a -> b) -> Signal a -> Signal b -> Signal b
mergeMap fn signal =
  Signal.merge (Signal.map fn signal)

[mgold]

These approaches look promising. It feels, just a little, like removing multi-way if in favor of repeated single ifs.

I'd like to see some benchmarks. For examples, having many signals that update at 60fps instead of 1 or 2 could plausibly be a problem. I'd hate to pick a notation we all love only to find it's not performant.

[mgold]

These approaches look promising. It feels, just a little, like removing multi-way if in favor of repeated single ifs.

I'd like to see some benchmarks. For examples, having many signals that update at 60fps instead of 1 or 2 could plausibly be a problem. I'd hate to pick a notation we all love only to find it's not performant.

[rtfeldman]

Seems unlikely to be significantly off, given that mergeMany isn't Native; it's just a foldl over whatever you give it. 😉

[rtfeldman]

Seems unlikely to be significantly off, given that mergeMany isn't Native; it's just a foldl over whatever you give it. 😉

[jvoigtlaender]

@rtfeldman, as a means of stress-testing your proposed notation/functions:

How would you replace something like the following code?

updates : Signal Time
updates =
    mergeMany (makeTimers 10)

makeTimers n =
    List.map (\i -> every (100 * i)) [1 .. n]

Where the point is that makeTimers could be any function that "creates" some list of signals that is not trivially just a list literal explicitly written out in the program code to begin with. For example, maybe imagine even a situation where makeTimers does not take the n as a parameter (here fixed to 10 at the call site), but instead that number comes in from the JavaScript side via a port (value, not signal) as a configuration parameter for the Elm program?

So my test is: You cannot change the makeTimers function. How do you change the updates signal definition to avoid use of mergeMany?

[jvoigtlaender]

@rtfeldman, as a means of stress-testing your proposed notation/functions:

How would you replace something like the following code?

updates : Signal Time
updates =
    mergeMany (makeTimers 10)

makeTimers n =
    List.map (\i -> every (100 * i)) [1 .. n]

Where the point is that makeTimers could be any function that "creates" some list of signals that is not trivially just a list literal explicitly written out in the program code to begin with. For example, maybe imagine even a situation where makeTimers does not take the n as a parameter (here fixed to 10 at the call site), but instead that number comes in from the JavaScript side via a port (value, not signal) as a configuration parameter for the Elm program?

So my test is: You cannot change the makeTimers function. How do you change the updates signal definition to avoid use of mergeMany?

[rtfeldman]

Fortunately, there's Core precedent for exactly this situation!

We already know that you can't turn List (Signal a) into Signal a in the general case, because [] cannot be converted into a Signal a in the general case. However, the fact that you can't safely implement List (Signal a) -> Signal a is just a special case of the fact that you can't safely implement List a -> a.

Back in Core v1.1.1, when Signal.mergeMany and List.head were both partial, this test could just as easily have been asking how to implement the following if the crashy version of List.head were to go away:

timeValue : Time
timeValue =
    head (makeTime 10)

makeTime n =
    List.map (\i -> (100 * i)) [1 .. n]

This isn't a hypothetical anymore, because the crashy version of List.head indeed did go away! The short answer is that I am now responsible for either handling [] gracefully or pulling the Debug.crash trigger myself.

As in the Signal version of the test, I don't have any context about what to do in the case where I'm given [], so by process of elimination there is only one solution to either test: call Debug.crash in the event that I receive [].

timeValue : Time
timeValue =
    case makeTime 10 of
        [] ->
            Debug.crash ":("

        timeValue :: timeValues ->
            timeValue


updates : Signal Time
updates =
    case makeTimers 10 of
        [] ->
            Debug.crash ":("

        timer :: timers ->
            List.foldl merge timer timers

So if this is the only behavior that can pass that test, the question becomes whether forcing people to write this out is better or worse than offering it as a built-in library function.

We have precedent for that; List.head now requires exactly this, because we determined that if you really want something to crash, you should have to ask for a crash by name.

[rtfeldman]

Fortunately, there's Core precedent for exactly this situation!

We already know that you can't turn List (Signal a) into Signal a in the general case, because [] cannot be converted into a Signal a in the general case. However, the fact that you can't safely implement List (Signal a) -> Signal a is just a special case of the fact that you can't safely implement List a -> a.

Back in Core v1.1.1, when Signal.mergeMany and List.head were both partial, this test could just as easily have been asking how to implement the following if the crashy version of List.head were to go away:

timeValue : Time
timeValue =
    head (makeTime 10)

makeTime n =
    List.map (\i -> (100 * i)) [1 .. n]

This isn't a hypothetical anymore, because the crashy version of List.head indeed did go away! The short answer is that I am now responsible for either handling [] gracefully or pulling the Debug.crash trigger myself.

As in the Signal version of the test, I don't have any context about what to do in the case where I'm given [], so by process of elimination there is only one solution to either test: call Debug.crash in the event that I receive [].

timeValue : Time
timeValue =
    case makeTime 10 of
        [] ->
            Debug.crash ":("

        timeValue :: timeValues ->
            timeValue


updates : Signal Time
updates =
    case makeTimers 10 of
        [] ->
            Debug.crash ":("

        timer :: timers ->
            List.foldl merge timer timers

So if this is the only behavior that can pass that test, the question becomes whether forcing people to write this out is better or worse than offering it as a built-in library function.

We have precedent for that; List.head now requires exactly this, because we determined that if you really want something to crash, you should have to ask for a crash by name.

[rtfeldman]

That reminds me...I vaguely recall seeing this on a mailing list thread somewhere, so it's probably worth noting here.

This would be total and wouldn't require any Result or Maybe stuff:

Signal.mergeAll : Signal a -> List (Signal a) -> Signal a

[rtfeldman]

That reminds me...I vaguely recall seeing this on a mailing list thread somewhere, so it's probably worth noting here.

This would be total and wouldn't require any Result or Maybe stuff:

Signal.mergeAll : Signal a -> List (Signal a) -> Signal a

[jvoigtlaender]

@rtfeldman, it's actually not true we don't have context for doing anything else than crash in the two cases you give. For example, you could do:

timeValue : Time
timeValue =
    case makeTime 10 of
        [] ->
            0

        timeValue :: timeValues ->
            timeValue

and something similar for the signal case. But of course that's not the point.

The point was whether it is good to get rid of mergeMany and instead use something like your binary operators. As I expected and now see, for my "test" you didn't use the notation you previously suggested. Instead, turned to a List.foldl merge. That suggests there is a role for a named function here that does more than combine only two signals.

The next question is then what type that function should have. Signal a -> List (Signal a) -> Signal a is an option. You have probably seen it in a comment by @TheSeamau5 in earlier discussions of Signal.mergeMany (and also above in this thread here, though there the type is a bit mixed up).

Anyway, my main point with my "test" above was: an API with only binary andMap or mergeMap or |+ seems not very desirable. We want something that can work on a list. How to deal with the case of an empty list then is a separate question. But do we agree now that simply dropping Signal.mergeMany is not so nice?

[jvoigtlaender]

@rtfeldman, it's actually not true we don't have context for doing anything else than crash in the two cases you give. For example, you could do:

timeValue : Time
timeValue =
    case makeTime 10 of
        [] ->
            0

        timeValue :: timeValues ->
            timeValue

and something similar for the signal case. But of course that's not the point.

The point was whether it is good to get rid of mergeMany and instead use something like your binary operators. As I expected and now see, for my "test" you didn't use the notation you previously suggested. Instead, turned to a List.foldl merge. That suggests there is a role for a named function here that does more than combine only two signals.

The next question is then what type that function should have. Signal a -> List (Signal a) -> Signal a is an option. You have probably seen it in a comment by @TheSeamau5 in earlier discussions of Signal.mergeMany (and also above in this thread here, though there the type is a bit mixed up).

Anyway, my main point with my "test" above was: an API with only binary andMap or mergeMap or |+ seems not very desirable. We want something that can work on a list. How to deal with the case of an empty list then is a separate question. But do we agree now that simply dropping Signal.mergeMany is not so nice?

[jvoigtlaender]

Rereading your comment, I see you might actually prefer to write List.foldl merge. I'd prefer if this were given a name, so that people don't have to use a higher-order combinator "just" to merge their list of input signals into one signal.

[jvoigtlaender]

Rereading your comment, I see you might actually prefer to write List.foldl merge. I'd prefer if this were given a name, so that people don't have to use a higher-order combinator "just" to merge their list of input signals into one signal.

[rtfeldman]

I'd prefer if this were given a name, so that people don't have to use a higher-order combinator "just" to merge their list of input signals into one signal.

Seems reasonable to me!

I've made a PR to elm-signal-extra for the option I like best (mergeAll), so anyone who'd like a named total version of mergeMany can start using it today, regardless of how this issue shakes out.

[rtfeldman]

I'd prefer if this were given a name, so that people don't have to use a higher-order combinator "just" to merge their list of input signals into one signal.

Seems reasonable to me!

I've made a PR to elm-signal-extra for the option I like best (mergeAll), so anyone who'd like a named total version of mergeMany can start using it today, regardless of how this issue shakes out.

[mgold]

Well, that is indeed a convincing argument about performance. The rest of the discussion cements the idea of "mergeMany = merge + fold + handling the empty case".

As Janis brings up, when the type is monomorphic, one does not have to crash. This suggests that, in cases like makeTimers, one could supply a default.

safeMergeMany : a -> List (Signal a) -> Signal a
safeMergeMany default signals =
    case signals of
        [] ->
            Signal.constant default

        x :: xs ->
            List.foldl Signal.merge x xs

However, in the common case of a list literal, one is required to provide a default that is never used (and this is known, to the programmer but not the compiler, at compile time).

I think some kind of binary operator is the best way to handle the common case. Part of me thinks that mergeMap is too trivial to add to core (uncurry Signal.map >> Signal.merge |> curry), but it works really well.

[mgold]

Well, that is indeed a convincing argument about performance. The rest of the discussion cements the idea of "mergeMany = merge + fold + handling the empty case".

As Janis brings up, when the type is monomorphic, one does not have to crash. This suggests that, in cases like makeTimers, one could supply a default.

safeMergeMany : a -> List (Signal a) -> Signal a
safeMergeMany default signals =
    case signals of
        [] ->
            Signal.constant default

        x :: xs ->
            List.foldl Signal.merge x xs

However, in the common case of a list literal, one is required to provide a default that is never used (and this is known, to the programmer but not the compiler, at compile time).

I think some kind of binary operator is the best way to handle the common case. Part of me thinks that mergeMap is too trivial to add to core (uncurry Signal.map >> Signal.merge |> curry), but it works really well.

[sindikat]

We can't have a total function of type List a -> a, but we can have a total function of type List (List a) -> List a:

f : [[a]] -> [a]
f xs =
  case xs of
    [] -> []
    (x::_) -> x

Is it possible in Elm type system to have a Signal that emits nothing, never, and have a polymorphic type of Signal a? This “empty” might be the result of mergeMany [].

[sindikat]

We can't have a total function of type List a -> a, but we can have a total function of type List (List a) -> List a:

f : [[a]] -> [a]
f xs =
  case xs of
    [] -> []
    (x::_) -> x

Is it possible in Elm type system to have a Signal that emits nothing, never, and have a polymorphic type of Signal a? This “empty” might be the result of mergeMany [].

[mgold]

I'm afraid not. One of the premises of signals is that they're always defined. If I map over this signal, what do I get? Does the mapping just not happen (a silent error)?

I have no problem leaving this as it is; it almost never comes up in practice and is always an immediate error. Failing that, I'll say again that an easy binary merge could work as well.

[mgold]

I'm afraid not. One of the premises of signals is that they're always defined. If I map over this signal, what do I get? Does the mapping just not happen (a silent error)?

I have no problem leaving this as it is; it almost never comes up in practice and is always an immediate error. Failing that, I'll say again that an easy binary merge could work as well.

[jvoigtlaender]

@sindikat, what you suggest would be perfectly possible if there were different concepts for Signals and Streams, where Streams are the ones that need not have an initial value. In fact, when such a separation was discussed for the core API a while ago, mergeing operated on Streams, not on Signals, so there we would have had an approriate "empty" to use as result of mergeMany []. But not with Signals.

@mgold, "immediate" in what sense? Always happening on the developer's machine before the code is even shipped (so that the developer would notice, and not ship the code, but instead repair the bug first), or possibly happening only after deployment (but then immediately on page load)? I think the latter can happen here, because the behavior (whether mergeMany gets called on an empty list or not) can depend on an incoming port value, thus be decided at page load time, not before.

[jvoigtlaender]

@sindikat, what you suggest would be perfectly possible if there were different concepts for Signals and Streams, where Streams are the ones that need not have an initial value. In fact, when such a separation was discussed for the core API a while ago, mergeing operated on Streams, not on Signals, so there we would have had an approriate "empty" to use as result of mergeMany []. But not with Signals.

@mgold, "immediate" in what sense? Always happening on the developer's machine before the code is even shipped (so that the developer would notice, and not ship the code, but instead repair the bug first), or possibly happening only after deployment (but then immediately on page load)? I think the latter can happen here, because the behavior (whether mergeMany gets called on an empty list or not) can depend on an incoming port value, thus be decided at page load time, not before.

[mgold]

@jvoigtlaender Okay, I hadn't considered that you could do something like

port numbers : List Int

possiblyBadSignal = Signal.mergeMany <| List.map Signal.constant numbers

But that seems really contrived to me. Almost always you'd have a fixed number of ports of signals, and combine them in a list literal.

[mgold]

@jvoigtlaender Okay, I hadn't considered that you could do something like

port numbers : List Int

possiblyBadSignal = Signal.mergeMany <| List.map Signal.constant numbers

But that seems really contrived to me. Almost always you'd have a fixed number of ports of signals, and combine them in a list literal.

[jvoigtlaender]

@mgold As already hinted above, I was thinking more of something like

port configParameter : Int

updates : Signal Time
updates =
    mergeMany (makeTimers configParameter)

makeTimers n =
    List.map (\i -> every (100 * i)) [1 .. n]

(where of course the makeTimers could be a more interesting function).

I don't find this particularly contrived.

As soon as the HTML that calls/embeds the Elm code passes a config parameter that is not positive, the error will be triggered.

[jvoigtlaender]

@mgold As already hinted above, I was thinking more of something like

port configParameter : Int

updates : Signal Time
updates =
    mergeMany (makeTimers configParameter)

makeTimers n =
    List.map (\i -> every (100 * i)) [1 .. n]

(where of course the makeTimers could be a more interesting function).

I don't find this particularly contrived.

As soon as the HTML that calls/embeds the Elm code passes a config parameter that is not positive, the error will be triggered.

[mgold]

Ah, right. Yeah, I agree that's problematic. How would you implement this in a safe way, perhaps using only merge?

[mgold]

Ah, right. Yeah, I agree that's problematic. How would you implement this in a safe way, perhaps using only merge?

[jvoigtlaender]

@mgold Addressing this would go like in this or this comment

[jvoigtlaender]

@mgold Addressing this would go like in this or this comment

[rtfeldman]

🎉🎉🎉🎉 🏆 🏆 🏆 🏆 🏆 😸 😸 😸 😸 😸

[rtfeldman]

🎉🎉🎉🎉 🏆 🏆 🏆 🏆 🏆 😸 😸 😸 😸 😸

[mgold]

OBE, as they say. (Overcome By Events)

[mgold]

OBE, as they say. (Overcome By Events)