Skip to content
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

Improve ergonomics and usefulness of es -e. #73

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

jpco
Copy link
Collaborator

@jpco jpco commented Dec 29, 2023

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

  1. Fixes the bug mentioned at Does anybody here use `es -e`? #70 (comment)

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:

  • most other ways to cause the shell to exit do so via exceptions -- including the exit command itself! It is unpleasantly surprising to lack the ability to handle and clean up after errors just for this.

  • exit-on-false is essentially a crappy imitation of exceptions in shells that lack them. Es actually has exceptions, so it should use them for this like it does for internal errors, signals, and the like. Exceptions are a far more powerful mechanism for error handling than just killing the process.

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 added 5 commits November 30, 2023 20:20
… catch, complain, and retry); and limit the exit-on-false behavior to only be triggered in the context of %exit-on-false.
This feels like a hack, but it seems to work, and I can't come up with a way to break it right now.
@jpco
Copy link
Collaborator Author

jpco commented Dec 29, 2023

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
Copy link
Contributor

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
Copy link
Collaborator Author

jpco commented Jan 22, 2024

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 a4f3b6d. I'd happily revert that commit, though.

@jpco
Copy link
Collaborator Author

jpco commented Sep 12, 2024

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?

jpco added a commit to jpco/es-shell that referenced this pull request Sep 14, 2024
This causes %exit-on-false, as part of %dispatch, to be the actual trigger for the exit-on-false behavior.  The implementation context strongly implies that the current behavior is a bug.  (Why would we use %exit-on-false, whose only job is to set eval_exitonfalse to 1, and also just leave eval_exitonfalse as 1 ourselves?)

This also makes using `es -e` with any kind of overriden REPL much easier, because %prompt and other REPL hooks can no longer trigger the exit-on-false behavior.

This is one part of wryun#73, but because this part is almost definitely just a bug, while the rest is more about design improvements, I am merging this part more unilaterally.
jpco added a commit that referenced this pull request Sep 14, 2024
This causes %exit-on-false, as part of %dispatch, to be the actual trigger for the exit-on-false behavior.  The implementation context strongly implies that the current behavior is a bug.  (Why would we use %exit-on-false, whose only job is to set eval_exitonfalse to 1, and also just leave eval_exitonfalse as 1 ourselves?)

This also makes using `es -e` with any kind of overriden REPL much easier, because %prompt and other REPL hooks can no longer trigger the exit-on-false behavior.

This is one part of #73, but because this part is almost definitely just a bug, while the rest is more about design improvements, I am merging this part more unilaterally.
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.

2 participants