halt no longer directly calls std::process::exit

[a-n-d-r-e-w-l]

In one of my projects I used std::process::Command to shell out to jq for lots of fairly fiddly JSON processing - switching from "shell out to jq" to "use jaq as a library" saw speedups of something like 200x for my usecase (amazing, thank you for making this!). The only blocking issue I encountered was that some of my jq scripts used halt_error to signal invalid states - when shelling out and checking exit codes it all worked, but switching to jaq-as-a-library meant that halt_error in a script brought down the entire process rather than returning some kind of handleable error to the user.

This PR modifies the halt builtin to return a kind of Error that contains an exit code and cannot be caught by a try block.

This allows embedders (library users) to handle halt calls themselves - as an example, the CLI detects these Errors and calls std::process::exit at a more convenient location, preserving the same CLI behaviour, but other embedders can handle the error in different ways depending on their intended use case.

Checklist

• I have read the contribution guidelines.

[01mf02]

Thanks for your kind words --- I'm always happy about such experience reports! :)

Concerning your implementation: How about adding a Halt(usize) variant to exn::Inner instead? That would avoid increasing the size of the Error type.

[01mf02]

Hm, I see then that jaq_all::data::run in its current type signature would need to handle the Halt(...) variant, and reasonably, it can then only call std::process::exit.

If this is a concern, we could introduce a new jaq_all::data::run_exns, which takes impl FnMut(ValX) -> Result<(), E>, and have data::run call this new run_exns.

[a-n-d-r-e-w-l]

Regarding adding a new variant to Exn instead - I saw the /// If this can be observed by users, then this is a bug. doc comment within that enum and went "we want this new error/exception kind to be user-observable, so the Error enum is a better choice".

If adding a new variant to Exn, we'd also need to add:

impl<V> Exn<'_, V> {
    pub fn halt(exit_code: isize) -> Self;
    pub fn exit_code(&self) -> Option<isize>;
}

The current jaq_all::data::run could then be a thin convenience wrapper around the new jaq_all::data::run_exns that just checks the .exit_code() of exceptions and calls std::process::exit when necessary, which would preserve existing behaviour and not introduce code duplication.

Does this seem like a decent approach to you?

[a-n-d-r-e-w-l]

I've extended Exn and added the jaq_all::data::run_exns function as described. This moves the calling of process::exit into unwrap_valr, which (as far as I can tell) is the sole way that error-carrying Exns get converted into their errors, with explicit documentation about how to avoid this behaviour for other embedders.

[01mf02]

Regarding adding a new variant to Exn instead - I saw the /// If this can be observed by users, then this is a bug. doc comment within that enum and went "we want this new error/exception kind to be user-observable, so the Error enum is a better choice".

The comment that you write about, as you mentioned, is for the exn::Inner::TailCall variant, so it does not apply to the whole enum. Still, I appreciate the thought that you put into interpreting my comments. :)

I'm already pretty happy with the direction that your PR is taking. Still, this issue inspired me to think about whether this might be a good opportunity to extend this to some kind of effect system. Because there is not only Halt that we could add to exn::Inner, but also other things like Debug(V) and Stderr(V). If, for example, you would like to use jaq as library and make the debug filter not write to stderr, but to some other resource (e.g. because you are running multiple jaq programs in parallel), then this could come in handy.

This could be done e.g. by adding type Eff<'a>; to DataT and make a non-exhaustive enum Eff<V> { Halt(i32), Debug(V), Stderr(V) } or something like that. Then users of run_exns could handle these effects, i.e. by calling a default effect handler with special-casing for effects whose behaviour they want to modify, in your case Halt. I'm just not completely sure yet whether this whole approach works with resuming execution after an effect has been produced.

This approach might add a new layer of complexity to the jaq core, because every ValX would also need to store the Eff type. But perhaps, we could even get by this by passing the whole DataT instead of Eff and V.

I'm just thinking aloud here. What do you think about the whole idea of generalising this to a (light) output effect system? Because the effect system outlined here handles only the O of I/O --- for the I, the Data<'a> in DataT can already be (ab)used today.

[01mf02]

I've thought about this a bit more: The problem of the effect system I outlined above is that this can only handle effects that never fail; for example, errors from a hypothetic filter write_to_file($filename) could not be handled, because the effect is only executed at the very end, thus the jq program does not know whether it failed or not.

I have yet another wild idea: If we consider only halt, then we could implement it with techniques from #426. In particular, we could implement halt_error by throwing an exn::Inner::Yield(0, error_code). For this, we would have to start filter execution with Ctx { labels: 1, ... }, such that the label 0 can never be produced by the user. Then, a user could find out whether an exception is a halt instruction by checking whether the yield label is 0. (This mechanism, by the way, could also be extended to basically be equivalent to the effect system above.)

I have to think a bit more whether this is all too much voodoo. Perhaps the Halt variant is fine as well. I just do not like special cases that much if I can express them via a more general mechanism, but as I said, this may be too much voodoo.

[a-n-d-r-e-w-l]

Personally, I think it's better to keep Halt and Yield as separate variants, if only for clarity: someone looking through the codebase for the first time can fairly easily guess what ::Halt(i32) would mean on an error enum, while hiding it behind effectively a "niche optimized ::Yield(0, data_value_containing_i32)" is a lot harder to understand at a glance.

If you do want to go the route of the 0 label meaning the entirety of filter execution, I'd say it's probably worth redefining labels to be Option<NonZeroUsize> (or a newtype around it) to make it very clear that the "zero" label has special semantics.

---

In terms of a more general "effect system", Exn::halt(exit_code) being public actually fills the last significant gap in what native filters can do (as far as I'm aware); when writing any sort of native filter function, an embedder already can add side effects to it being called, and can implement fallible filters. One of the major building blocks that was missing, however, was the ability for a native filter function to end filter execution entirely - which custom filters can now do by constructing an Exn::halt.

Adding some kind of effect payload that returns to a top-level handler, which then needs to feed back a (possible) return value back into the filter runtime, which then needs to be able to resume filter execution (kind of similar to an async fns statemachine) is quite complicated. The benefits that would add are that embedders can choose to not resume filter execution (basically what an Exn::halt does with this PR), or to suspend, store and re-enter filter execution at some later time.

---

Most importantly, however, is the fact that the other impure filters aren't surprising (to an embedder), and that the other impure filters can be implemented by an embedder in 3.0.0, but halt($exit_code) breaks both of those.

The impure filters - those that "escape the filter context" in some manner - are:

• debug, debug(f), stderr - it makes sense that calling any of these would produce something on stderr, so aren't surprising; embedders can use the pattern shown below to override this with e.g. "write to file" or "add to in-memory buffer" etc.
• env, $ENV - it makes sense that these can read environment variables, so aren't surprising; embedders can override these if needed
• now - it makes sense that this returns the real timestamp, so isn't surprising; embedders can override this if needed
• repl - this deliberately involves interaction on a TTY, so what it does isn't surprising; as this is implemented in jaq rather than jaq-core or jaq-std, it is by definition implementable by other embedders
• halt - doesn't "halt filter execution", but does "halt process instead" - not surprising in the jaq CLI, but yes surprising for other embedders (a rogue filter calling halt can't be caught or recovered); see below for override possibilities

The actual project I was working on when I encountered this currently has a snippet like:

let funs = jaq_all::data::funs()
  .filter(|fun| fun.0 != "halt")
  .chain(std::iter::once(jaq_all::jaq_std::run((
      "halt",
      jaq_all::jaq_std::v(1),
      |mut cv| {
          let val = jaq_all::jaq_core::Ctx::<jaq_all::data::DataKind>::pop_var(&mut cv.0);
          let exit_code = jaq_all::jaq_std::ValT::as_isize(&val).ok_or_else(|| jaq_all::jaq_core::Error::typ(val, "integer"));
          jaq_all::jaq_std::bome(exit_code.and_then(|exit_code| Err(jaq_all::jaq_core::Error::str(format_args!("halt({exit_code})")))))
      },
  ))));

Note that this can't terminate filter execution, as that primitive isn't in 3.0.0 - it can either call std::process::exit (i.e. the existing implementation), return an error (but that's catchable within the filter context, which isn't great), or do something like a longjmp to completely abandon filter execution (unsafe and really not a good idea).

In short, most other useful potentially-fallible effects (except "suspend execution and re-enter at a later time") can already be implemented by embedders in 3.0.0, with the exception (heh) of there not being an existing primitive for ending filter execution - based on that, I don't think an effect-like system is needed.

[01mf02]

I agree with your reasoning, @a-n-d-r-e-w-l. I have also thought about this over the weekend and think that Halt is special enough to warrant special treatment.

A few more comments for the API:

• I would move the halt implementation from jaq-std to jaq-core, because jaq-core has all required functionality to implement halt now. Same goes for the definitions that rely on halt.
• unwrap_valr has a problem now; it will not work when compiled without the std feature. What should we do in that case? I do not currently see a viable alternative to panicking. (In jaq, features should never change the code that is executed, only which functions are compiled.) This function should probably be removed or adapted in a future version of jaq, but what to substitute it with?
• I have remembered that jaq-all has no stable API guarantee (yet), so it is fine to change the API (unlike for jaq-core). Therefore, I would suggest to remove run_exns again (sorry for the work!) and adapt run such that it returns Result<Option<i32>, E>. Here, the Option will be Some(..) if halt was called, else None.

[a-n-d-r-e-w-l]

re: points 1 (move from -std to -core) & 3 (remove run_exns, modify run) - yep, will do

re: point 2:

unwrap_valr only gets called in the following places: jaq_all::data::run (which always has std available), the -core "repl" example (which uses std::io::stdin and family, so std can be assumed), and a single function in -core that only ever gets called in tests. The test function could just use an .unwrap() while the example can manually terminate the process, so the only "important" usage is the one in jaq_all - which is the main one we care about, regardless.

I *think* the easiest solution is just to make Exn::get_err pub instead of pub(crate) - that way, each of the three usages of unwrap_valr can then implement their own version (each a fairly simple closure), removing the need for unwrap_valr in -core to begin with. They can't currently implement this as Exn is opaque outside the crate and this crate-private function is currently only exposed publicly by unwrap_valr.

Thoughts?

---

Edit on point 1: getting the exit code uses <_ as ValTx>::try_as_i32(), which uses <_ as ValT>; that trait is only defined in -std, so I don't think there's any way to convert the generic-over-D: DataT D::V<'a> into an i32 within jaq-core.

If moving the filter definition to -core is a must, then Exn<V>::Halt would need to hold a V instead of an i32, with it then being up to embedders to extract their desired value out of it. That is, strictly speaking, the more flexible approach, but would complicate the ""normal path"" of just containing an integral exit code.

[01mf02]

Just a little thought on unwrap_valr here: We unfortunately cannot just remove that, unless we bump the major version number, which I would rather like to avoid ATM. I think it would be better to mark it deprecated, and if jaq is compiled with std feature, then we can call process::exit in there, and otherwise, we have to panic. I think that is the proper way forward.

I'll have to think about the other things a bit first. But I think that I like your handle method. Let me think about the i32 issue a bit as well.

[01mf02]

Edit on point 1: getting the exit code uses <_ as ValTx>::try_as_i32(), which uses <_ as ValT>; that trait is only defined in -std, so I don't think there's any way to convert the generic-over-D: DataT D::V<'a> into an i32 within jaq-core.

You're right. I thought that jaq_core::ValT provides something like as_isize(), but I was wrong. So with the current API, the halt definition cannot reside in jaq-core. So let's leave it in jaq-std.

If moving the filter definition to -core is a must, then Exn<V>::Halt would need to hold a V instead of an i32, with it then being up to embedders to extract their desired value out of it. That is, strictly speaking, the more flexible approach, but would complicate the ""normal path"" of just containing an integral exit code.

Yes, and there would also be the problem that with this approach, users could not reliably catch errors coming from halt_error, as in:

$ jq -n 'halt_error("")?, 1'; echo $?
1
0

Conclusion: Let's keep Halt(i32) in jaq-core, and keep halt implementation in jaq-std.

[01mf02]

This looks like an advanced block to me.

[01mf02]

OK, I think I'll take this from here. The API is now quite clear to me. I'll polish things to my liking before merging.

[01mf02]

Hi @a-n-d-r-e-w-l. I have now finished my first polishing round. In particular, I reintroduced unwrap_valr to prevent API breakage as described in #433 (comment). I also changed the Exn method a bit to return Result<T, Self> instead of T, to prevent panicking.

Do you have feedback for the current state of the PR?

[a-n-d-r-e-w-l]

Sorry for the delayed response, I just didn't see the comment.

Yep, the current state of the PR looks good!

[01mf02]

Thanks for your heads up, @a-n-d-r-e-w-l.