New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`always` evaluates tasks right away, ignoring `andThen` chains #453

Closed
eeue56 opened this Issue Dec 1, 2015 · 22 comments

Comments

Projects
None yet
5 participants
@eeue56
Contributor

eeue56 commented Dec 1, 2015

Problem definition

Code using always in an andThen chain will execute before the previous value in the andThen chain is called.

An example taken from the minimal test case found here -

port run : Task x ()
port run = writeFileNotOK file "testomatic"
  `andThen` (always (readFileAndLog file))

The problem is that always will evaluate readFileAndLog file before writeFileNotOK file "testomatic" is run, meaning that the file will not exist and readFileAndLog file will die.

always is passed readFileAndLog(file), rather than a function object which hasn't been called yet through use of bind.

(\_ -> (readFileAndLog file)) on the other hand produces a function containing the readFileAndLog call, so the function isn't called until after the first Task is stepped through.

The fix in current Elm

Use lambda functions ignoring the argument instead of always. For example,

port run : Task x ()
port run = writeFileNotOK file "testomatic"
  `andThen` (\_ -> (readFileAndLog file))

instead of

port run : Task x ()
port run = writeFileNotOK file "testomatic"
  `andThen` (always (readFileAndLog file))

​### In brief

Using always will evaluate a task. If it's wrapped in Native with Task.asyncFunction then it returns an obj without having executed any of it

Possible fixes

I'm not sure if this should even be fixed. The behaviour of always kind of makes sense. It could be fixed by making always evaluate when it used rather, than when it's defined. Or it could be a modification to andThen that wraps the following function in a function call, so that andThen always (readFileAndLog file) becomes andThen(.., function() { always(readFileAndLog(file)) })

The andThen modification makes most sense to me and should be fairly simple to implement. It will sadly introduce another nesting of functions, but for correct behaviour I think it's worth it.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Dec 1, 2015

Contributor

Elm has strict evaluation, so always must behave the way it does.

My interpretation here is that you are misusing Native, not following the implicit contract that is expected to hold when you wrap some native code in a Task. For example, in Native.FS.readFileAndLog, you shouldn't simply issue a fs.readFile-call. Instead, you should wrap that in a Task.asyncFunction (on the JavaScript) side.

Contributor

jvoigtlaender commented Dec 1, 2015

Elm has strict evaluation, so always must behave the way it does.

My interpretation here is that you are misusing Native, not following the implicit contract that is expected to hold when you wrap some native code in a Task. For example, in Native.FS.readFileAndLog, you shouldn't simply issue a fs.readFile-call. Instead, you should wrap that in a Task.asyncFunction (on the JavaScript) side.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Dec 1, 2015

Contributor

In other words, your native code in the present form wouldn't pass the review that is required before it would be whitelisted for publishing to http://package.elm-lang.org/.

If you "follow the rules" (which are not really written down anywhere, I think) for wrapping impure stuff in Tasks, then the native code might get whitelisted, and also the code would not suffer from the issue you observe.

(Or I am totally off with most everything I've just written in this and the previous comment.)

Contributor

jvoigtlaender commented Dec 1, 2015

In other words, your native code in the present form wouldn't pass the review that is required before it would be whitelisted for publishing to http://package.elm-lang.org/.

If you "follow the rules" (which are not really written down anywhere, I think) for wrapping impure stuff in Tasks, then the native code might get whitelisted, and also the code would not suffer from the issue you observe.

(Or I am totally off with most everything I've just written in this and the previous comment.)

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman Dec 1, 2015

Member

My impression is in line with @jvoigtlaender's - if that code doesn't work, then readFileAndLog must have a side effect (which it idiomatically shouldn't), and all bets are off. 😄

Member

rtfeldman commented Dec 1, 2015

My impression is in line with @jvoigtlaender's - if that code doesn't work, then readFileAndLog must have a side effect (which it idiomatically shouldn't), and all bets are off. 😄

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Dec 1, 2015

Contributor

Concretely, why does readFileAndLog not look as follows?

  const readFileAndLog = function (path) {
      return Task.asyncFunction(function (callback) {
        fs.readFile(path, function(err, data){
          if(err){
            throw err;
          }else{
            console.log("data: ", data);
          }        
        });
        return callback(Task.succeed(Utils.Tuple0));
      });
    };
Contributor

jvoigtlaender commented Dec 1, 2015

Concretely, why does readFileAndLog not look as follows?

  const readFileAndLog = function (path) {
      return Task.asyncFunction(function (callback) {
        fs.readFile(path, function(err, data){
          if(err){
            throw err;
          }else{
            console.log("data: ", data);
          }        
        });
        return callback(Task.succeed(Utils.Tuple0));
      });
    };
@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Dec 1, 2015

Contributor

Ah, yes, you're right. The lack of the wrapping with asyncFunction is the problem.

In that case, I feel like the fix I proposed to the author shouldn't work, either. always should behave like (\_ -> a), and having the two separate behaviours for seemingly identical definitions is confusing, while they compile down to two different behaviours.

Contributor

eeue56 commented Dec 1, 2015

Ah, yes, you're right. The lack of the wrapping with asyncFunction is the problem.

In that case, I feel like the fix I proposed to the author shouldn't work, either. always should behave like (\_ -> a), and having the two separate behaviours for seemingly identical definitions is confusing, while they compile down to two different behaviours.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Dec 1, 2015

Contributor

I'm not clear on that last paragraph. Do you mean always a should behave like (_ -> a)? Not in Elm. Test with a = Debug.crash "Bang".

Contributor

jvoigtlaender commented Dec 1, 2015

I'm not clear on that last paragraph. Do you mean always a should behave like (_ -> a)? Not in Elm. Test with a = Debug.crash "Bang".

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Dec 1, 2015

Contributor

Also, I hadn't previously noticed that "you" (who I addressed the comments to) are not the author of that native code. 😄

Contributor

jvoigtlaender commented Dec 1, 2015

Also, I hadn't previously noticed that "you" (who I addressed the comments to) are not the author of that native code. 😄

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman Dec 1, 2015

Member

Yeah, although of course Debug.crash has a side effect by design. 😄

There are other cases where they aren't equivalent, having to do with recursive types. I don't remember offhand, but there was definitely a case I came across where something worked with an anonymous function but not with always.

Given that always is less concise than \_ -> and has some edge cases, our style guide recommends against ever using it - which I agree with.

Member

rtfeldman commented Dec 1, 2015

Yeah, although of course Debug.crash has a side effect by design. 😄

There are other cases where they aren't equivalent, having to do with recursive types. I don't remember offhand, but there was definitely a case I came across where something worked with an anonymous function but not with always.

Given that always is less concise than \_ -> and has some edge cases, our style guide recommends against ever using it - which I agree with.

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Dec 1, 2015

Contributor

So consider the following:

always : a -> b -> a
always a _ = a

ignoreEverything : b -> a
ignoreEverything = always (readSomeFile file)

vs

ignoreEverything : b -> a
ignoreEverything = (\_ -> readSomeFile file)

To some extent, I feel like these should be equivalent. I know they're not, because you pass readSomeFile file as a value to always, not as a function, whereas the lambda version will evaluate readSomeFile file when it's called.

Perhaps the docs should be updated to reflect? It's always a surprise when I see these kinds of problems, because always seems like it should be the same as the lambda version.

Contributor

eeue56 commented Dec 1, 2015

So consider the following:

always : a -> b -> a
always a _ = a

ignoreEverything : b -> a
ignoreEverything = always (readSomeFile file)

vs

ignoreEverything : b -> a
ignoreEverything = (\_ -> readSomeFile file)

To some extent, I feel like these should be equivalent. I know they're not, because you pass readSomeFile file as a value to always, not as a function, whereas the lambda version will evaluate readSomeFile file when it's called.

Perhaps the docs should be updated to reflect? It's always a surprise when I see these kinds of problems, because always seems like it should be the same as the lambda version.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Dec 1, 2015

Contributor

To me, this really boils down to "This is Elm, which chose strict evaluation."

The difference you are observing here is why lazy languages like Haskell are sometimes preferred for having better equational theories. In Haskell, always a and \_ -> a are equivalent, and actually this example is exactly what differentiates the evaluation semantics of Haskell from that of Elm.

Contributor

jvoigtlaender commented Dec 1, 2015

To me, this really boils down to "This is Elm, which chose strict evaluation."

The difference you are observing here is why lazy languages like Haskell are sometimes preferred for having better equational theories. In Haskell, always a and \_ -> a are equivalent, and actually this example is exactly what differentiates the evaluation semantics of Haskell from that of Elm.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Dec 1, 2015

Contributor

And again, if we stick to pure code (except for possible nontermination, which might also be considered a side-effect), then there's no problem. In that world (modulo nontermination), always a and \_ -> a are equivalent even in Elm.

I don't really know what documentation should be added to address the problem. If everybody plays nice (no side effects through cheating with native code), the problem does not exist.

Contributor

jvoigtlaender commented Dec 1, 2015

And again, if we stick to pure code (except for possible nontermination, which might also be considered a side-effect), then there's no problem. In that world (modulo nontermination), always a and \_ -> a are equivalent even in Elm.

I don't really know what documentation should be added to address the problem. If everybody plays nice (no side effects through cheating with native code), the problem does not exist.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Dec 1, 2015

Contributor

Sorry for writing comment after comment. The above "vs" does not show a real problem if you agree to not give readSomeFile file a non-Task type.

Contributor

jvoigtlaender commented Dec 1, 2015

Sorry for writing comment after comment. The above "vs" does not show a real problem if you agree to not give readSomeFile file a non-Task type.

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman Dec 1, 2015

Member

Right. Put another way, the reason they're not equivalent is that ignoreEverything has a bug in its Native implementation.

I think there are compelling reasons to deprecate always, but I actually think this is one of the arguments to keep it: it's a kind of canary in the coal mine. Any code that doesn't work the same way when using always versus \_ -> because of a Native implementation detail is necessarily broken!

Member

rtfeldman commented Dec 1, 2015

Right. Put another way, the reason they're not equivalent is that ignoreEverything has a bug in its Native implementation.

I think there are compelling reasons to deprecate always, but I actually think this is one of the arguments to keep it: it's a kind of canary in the coal mine. Any code that doesn't work the same way when using always versus \_ -> because of a Native implementation detail is necessarily broken!

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 Dec 1, 2015

Contributor

Coming from a Haskell background, perhaps that's why I'm always surprised by how always works.

I agree if you play nice, then it's not a problem. Except there does seem to be some really rare cases where even if you don't introduce Native code, it happens to be a problem. Basing this just off what people we work with have said.

That being said, until there are some concrete examples of this that are easily reproducible, then this is pretty much up in the air. Next time I come across one I'll revisit this issue - I had thought for sure it was this problem, but now in future I'll know I can check this.

Any code that doesn't work the same way when using always versus _ -> because of a Native implementation detail is necessarily broken!

This is a great point. I've added it to Native docs I'm building on here

Feel free to to close this issue as I think it's not an actual issue.

Contributor

eeue56 commented Dec 1, 2015

Coming from a Haskell background, perhaps that's why I'm always surprised by how always works.

I agree if you play nice, then it's not a problem. Except there does seem to be some really rare cases where even if you don't introduce Native code, it happens to be a problem. Basing this just off what people we work with have said.

That being said, until there are some concrete examples of this that are easily reproducible, then this is pretty much up in the air. Next time I come across one I'll revisit this issue - I had thought for sure it was this problem, but now in future I'll know I can check this.

Any code that doesn't work the same way when using always versus _ -> because of a Native implementation detail is necessarily broken!

This is a great point. I've added it to Native docs I'm building on here

Feel free to to close this issue as I think it's not an actual issue.

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman Dec 1, 2015

Member

I'd go ahead and close it then...nobody else in the current discussion has
permission to close it as far as I know!

On Tue, Dec 1, 2015, 10:30 AM Noah notifications@github.com wrote:

Coming from a Haskell background, perhaps that's why I'm always surprised
by how always works.

I agree if you play nice, then it's not a problem. Except there does seem
to be some really rare cases where even if you don't introduce Native
code, it happens to be a problem. Basing this just off what people we work
with have said.

That being said, until there are some concrete examples of this that are
easily reproducible, then this is pretty much up in the air. Next time I
come across one I'll revisit this issue - I had thought for sure it was
this problem, but now in future I'll know I can check this.

Any code that doesn't work the same way when using always versus _ ->
because of a Native implementation detail is necessarily broken!

This is a great point. I've added it to Native docs I'm building on here
https://github.com/NoRedInk/take-home/wiki/Native-best-practices

Feel free to to close this issue as I think it's not an actual issue.


Reply to this email directly or view it on GitHub
https://github.com/elm-lang/core/issues/453#issuecomment-161055945.

Member

rtfeldman commented Dec 1, 2015

I'd go ahead and close it then...nobody else in the current discussion has
permission to close it as far as I know!

On Tue, Dec 1, 2015, 10:30 AM Noah notifications@github.com wrote:

Coming from a Haskell background, perhaps that's why I'm always surprised
by how always works.

I agree if you play nice, then it's not a problem. Except there does seem
to be some really rare cases where even if you don't introduce Native
code, it happens to be a problem. Basing this just off what people we work
with have said.

That being said, until there are some concrete examples of this that are
easily reproducible, then this is pretty much up in the air. Next time I
come across one I'll revisit this issue - I had thought for sure it was
this problem, but now in future I'll know I can check this.

Any code that doesn't work the same way when using always versus _ ->
because of a Native implementation detail is necessarily broken!

This is a great point. I've added it to Native docs I'm building on here
https://github.com/NoRedInk/take-home/wiki/Native-best-practices

Feel free to to close this issue as I think it's not an actual issue.


Reply to this email directly or view it on GitHub
https://github.com/elm-lang/core/issues/453#issuecomment-161055945.

@eeue56 eeue56 closed this Dec 1, 2015

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Dec 1, 2015

Contributor

I'd be interested in seeing any case like this:

Except there does seem to be some really rare cases where even if you don't introduce Native code, it happens to be a problem. Basing this just off what people we work with have said.

as well.

Any case where always exp is different from \_ -> exp and where this does not come from either of:

  • nontermination inside exp,
  • Debug.crash inside exp,
  • native code inside exp that wouldn't get (or shouldn't have gotten) through the review process,

would require a fix.

The way the language is designed, there shouldn't be such cases other than of the above three kinds.

Contributor

jvoigtlaender commented Dec 1, 2015

I'd be interested in seeing any case like this:

Except there does seem to be some really rare cases where even if you don't introduce Native code, it happens to be a problem. Basing this just off what people we work with have said.

as well.

Any case where always exp is different from \_ -> exp and where this does not come from either of:

  • nontermination inside exp,
  • Debug.crash inside exp,
  • native code inside exp that wouldn't get (or shouldn't have gotten) through the review process,

would require a fix.

The way the language is designed, there shouldn't be such cases other than of the above three kinds.

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman Dec 1, 2015

Member

There was one case having to do with recursively defined types and decoders
where Evan pointed out the fix was to switch...can't remember it though.
Don't think it was broken per se though.

On Tue, Dec 1, 2015, 11:14 AM Janis Voigtländer notifications@github.com
wrote:

I'd be interested in seeing any case like this:

Except there does seem to be some really rare cases where even if you
don't introduce Native code, it happens to be a problem. Basing this just
off what people we work with have said.

as well.

Any case where always exp is different from _ -> exp and where this does
not come from either of:

  • nontermination inside exp,
  • Debug.crash inside exp,
  • native code inside exp that wouldn't get (or shouldn't have gotten)
    through the review process,

would require a fix.

The way the language is designed, there shouldn't be such cases other than
of the above three kinds.


Reply to this email directly or view it on GitHub
https://github.com/elm-lang/core/issues/453#issuecomment-161066552.

Member

rtfeldman commented Dec 1, 2015

There was one case having to do with recursively defined types and decoders
where Evan pointed out the fix was to switch...can't remember it though.
Don't think it was broken per se though.

On Tue, Dec 1, 2015, 11:14 AM Janis Voigtländer notifications@github.com
wrote:

I'd be interested in seeing any case like this:

Except there does seem to be some really rare cases where even if you
don't introduce Native code, it happens to be a problem. Basing this just
off what people we work with have said.

as well.

Any case where always exp is different from _ -> exp and where this does
not come from either of:

  • nontermination inside exp,
  • Debug.crash inside exp,
  • native code inside exp that wouldn't get (or shouldn't have gotten)
    through the review process,

would require a fix.

The way the language is designed, there shouldn't be such cases other than
of the above three kinds.


Reply to this email directly or view it on GitHub
https://github.com/elm-lang/core/issues/453#issuecomment-161066552.

@Fresheyeball

This comment has been minimized.

Show comment
Hide comment
@Fresheyeball

Fresheyeball Dec 1, 2015

so, I've updated this in a branch https://github.com/Fresheyeball/isolating-task-bug/blob/asyncFunction-solution/Native/FS.js

Adding .asyncFunction to readFileAndLog seems to fix this problem. However I don't understand why it fixes it. Is it reasonable to say that when writing Native code in Elm, that task creation should always be done with .asyncFunction?

Fresheyeball commented Dec 1, 2015

so, I've updated this in a branch https://github.com/Fresheyeball/isolating-task-bug/blob/asyncFunction-solution/Native/FS.js

Adding .asyncFunction to readFileAndLog seems to fix this problem. However I don't understand why it fixes it. Is it reasonable to say that when writing Native code in Elm, that task creation should always be done with .asyncFunction?

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Dec 1, 2015

Contributor

It fixes it because by wrapping the impure stuff in that asyncFunction call, you give the Elm runtime (specifically this module) control over when the task is run. So that is what "task creation" actually means: you create the task, but only in so far as you express what work should be done, then passing that over to the runtime. The runtime can sequence the actions, so you don't get out-of-order effects. In contrast, if you omit that wrapping, you don't really "create" a task for later execution. Instead, you do the execution right away, with all that entails about out-of-order execution of effects and the problems that can result from that.

Whether it is really always the case that you should wrap tasks in asyncFunction? I think yes, but maybe there are some cases I'm not thinking of right now where something weaker is okay. Evan might be able to tell.

(Of course, there's also the other way of bringing native stuff into the Elm world, via signals.)

Contributor

jvoigtlaender commented Dec 1, 2015

It fixes it because by wrapping the impure stuff in that asyncFunction call, you give the Elm runtime (specifically this module) control over when the task is run. So that is what "task creation" actually means: you create the task, but only in so far as you express what work should be done, then passing that over to the runtime. The runtime can sequence the actions, so you don't get out-of-order effects. In contrast, if you omit that wrapping, you don't really "create" a task for later execution. Instead, you do the execution right away, with all that entails about out-of-order execution of effects and the problems that can result from that.

Whether it is really always the case that you should wrap tasks in asyncFunction? I think yes, but maybe there are some cases I'm not thinking of right now where something weaker is okay. Evan might be able to tell.

(Of course, there's also the other way of bringing native stuff into the Elm world, via signals.)

@Fresheyeball

This comment has been minimized.

Show comment
Hide comment
@Fresheyeball

Fresheyeball Dec 1, 2015

Ok better heuristic:

Impure effects, even if synchronous, should be wrapped with Task.asyncFunction on the Native side.

Fresheyeball commented Dec 1, 2015

Ok better heuristic:

Impure effects, even if synchronous, should be wrapped with Task.asyncFunction on the Native side.

@rgrempel

This comment has been minimized.

Show comment
Hide comment
@rgrempel

rgrempel Dec 1, 2015

Contributor

Here's one way to think about it. When you write a native function which returns a Task, your native function executes at the time the Task is created, not at the time the task is performed. So, if you need the Task to do something each time it is performed (which is usually the case -- otherwise, it might not need to be a Task), then you'll need to arrange for that somehow. One way is to use Task.asyncFunction to provide a function to be called when the task is performed.

Contributor

rgrempel commented Dec 1, 2015

Here's one way to think about it. When you write a native function which returns a Task, your native function executes at the time the Task is created, not at the time the task is performed. So, if you need the Task to do something each time it is performed (which is usually the case -- otherwise, it might not need to be a Task), then you'll need to arrange for that somehow. One way is to use Task.asyncFunction to provide a function to be called when the task is performed.

@Fresheyeball

This comment has been minimized.

Show comment
Hide comment
@Fresheyeball

Fresheyeball Dec 1, 2015

Vunderbratski! Thank you all for helping me come up to speed on Elm Native!

Fresheyeball commented Dec 1, 2015

Vunderbratski! Thank you all for helping me come up to speed on Elm Native!

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