wryun / es-shell

es: a shell with higher-order functions
http://wryun.github.io/es-shell/
Other
313 stars 26 forks source link

Improve ergonomics and usefulness of `es -e`. #73

Open jpco opened 10 months ago

jpco commented 10 months ago

This pull request is meant to improve the usefulness of es' -e flag. It does a few things:

  1. Fixes the bug mentioned at https://github.com/wryun/es-shell/discussions/70#discussioncomment-7967441

This makes it so that exits-on-false only occur when $&exitonfalse is actually in the call stack. With this, REPL code, and functions called from the REPL like %parse or %write-history, can't cause the shell to implode, and so are much easier to write.

  1. Makes it so assignments can't trigger exits-on-false (unless you do something to intentionally trigger it like result <={var = value}).

The implementation seems a little sketchy to me, but as far as I have been able to test, it works. Before this PR, every one of the following lines causes the shell to exit; with it, none of them do.

; var = value one
; {var = value two}
; fn var {var = value three}
; var
  1. Turns exits-on-false into exceptions-on-false.

Instead of just calling exit(2), throw a false exception along with the triggering value. I strongly feel that using an exception is the Right Thing:

I picked false because it seems more useful to have a novel exception rather than re-using an existing one.

I would rename %exit-on-false and $&exitonfalse to %throw-on-false and $&throwonfalse if not for maintaining backwards compatibility.

If the user doesn't go out of their way catch false in a script, then the shell will quietly exit. In an interactive context...

  1. %interactive-loop catches the false exception, complains, and then re-prompts.

This is a fair bit friendlier than silently exiting -- not that it's very typical to use -e for an interactive session.

jpco commented 10 months ago

A thought that has just occurred to me, relevant for exception-based exit-on-false: Should false-exiting commands also cause an exception inside an exception handler? For example, if the following script is sourced with -e, should "finished the handler" be echoed or not?

catch @ e {
  result maybe throw an error here?
  echo finished the handler
} {
  result throw an error here
}
memreflect commented 9 months ago

Regarding your last suggestion, i would expect anything inside an exception handler that results in a false value to throw another exception when -e is in use. This would be expected behavior to me, but it certainly makes writing an exception handler more difficult when -e is used, particularly because catch @ {false} {result foo} will still trigger a false exception.

I haven't encountered any trouble yet, but why does a false exception cause the shell to exit in %interactive-loop? In fact, the only exceptions caught by %interactive-loop that should cause the shell to exit should be the exit and eof exceptions, so calling %dispatch after catching an error exception is wrong too... I can understand abandoning further evaluation and the shell complaining about the exception, but there's no need for an interactive session to die just because something went wrong is there? %batch-loop should exit if an exception is uncaught, but not %interactive-loop, right?

Or am i missing something glaringly obvious as usual? :-P

jpco commented 9 months ago

This would be expected behavior to me, but it certainly makes writing an exception handler more difficult when -e is used, particularly because catch @ {false} {result foo} will still trigger a false exception.

Yikes. Though I suppose that things like

; result = <={catch @ {false} {result foo}}

still work? This feels to me like a tradeoff between "more-surprising and more-useful" behavior and "less-surprising and less-useful" behavior. It's hard for me to judge the tradeoff without some actual experience. So I guess initially I'd say let's go with the less-surprising behavior, and if it ends up burdensome in practice, it can be revisited later.

Or am i missing something glaringly obvious as usual? :-P

You're missing nothing at all, and I actually prefer a complain-and-retry behavior in %interactive-loop for false results, and I initially implemented it that way -- but I got nervous about the idea of changing the "exit on false" flag to not actually cause the shell to, well, exit on false, so I changed it in a4f3b6d93afee2e47faca2f5df995c6f39ec10db. I'd happily revert that commit, though.

jpco commented 1 month ago

so calling %dispatch after catching an error exception is wrong too...

I only just got this (somehow I don't remember reading it at all before). You're 100% right, I missed that case. Though now that I look at it, I'm not entirely sure what the point of the $fn-%dispatch false line is in its current form, so I'm not sure exactly what the right behavior should be. Is it just there to make sure a shell invoked with -e will die on an error exception?