interp: builtins skip the exec handler

[TorchedSammy]

im sure that this is definitely intentional, but this is actually undesirable in my use case im developing hilbish and use this module to run sh of course, but used to only use this as a last option fallback instead of running my custom builtins or lua code. but now im considering actually using an exec handler for the runner but realized cd wasnt behaving how i expected it to be since it's this module's cd and not mine

so is there a way for me to change how the builtin works from outside the package? i havent found anything in docs like that

[mvdan]

You're right in assuming this is not supported, right now.

The question is - if you replaced builtins like cd, how could our runner actually be aware of when the current directory changes? Many other builtins change the internal state of the runner, like set and exit, so the same applies to them.

So I struggle to imagine what an interface would look like. If it were truly complete, it would expose a lot of internals of the runner, at which point it stops being a useful interface :)

What are you trying to do with your custom builtins? If I understand your goal and use case better, maybe that's going to help suggest or design something we can actually pull off.

[TorchedSammy]

my goal, or what i want here is so that i can be able to define my shell's builtins in lua for easier changes, and that i can add any other enhancements whenever, and so i can send my hooks/events whenever something happens (example being the cd command throws a cd hook)

[mvdan]

To make sure I understand correctly - you want to run extra code when a builtin is run, but not replace the default behavior, right?

Would you want to run that extra code before, or after the built-in behavior?

[TorchedSammy]

oh no, i do want to replace it

able to define my shell's builtins in lua for easier changes, and that i can add any other enhancements whenever

[mvdan]

Right, and how would you implement that? If you entirely replace the runner's cd, then the runner will never actually change its working directory. If you replace the runner's built-ins, you're half way to replacing the entire runner.

[TorchedSammy]

i actually have no idea, what do u propose i do by myself instead? i only reported what happened, i have no idea how the runner works

[mvdan]

I'd be able to propose something if I clearly understood what you're trying to do.

A) If you're trying to reimplement the runner's builtins yourself, then you're basically reimplementing a big chunk of the runner. I don't see how we could ever support this well. A "handler" interface to implement all builtins externally would be huge.

B) If you're trying to alter the behavior of builtins, such as running extra code before or after, or changing their arguments in some way before having the runner interpret them, this could be doable. The interpreter would still run its own builtin logic, but you'd be able to have more control over it.

[TorchedSammy]

yes im trying to do A but B could work looking at the other builtins, cd is the only one i want to replace/reimplement

[mvdan]

Perhaps continue experimenting with external changes, or even forking and modifying interp temporarily. Then you'll have a clearer idea of whether you want A or B.

[TorchedSammy]

so yes, i definitely want A would you be able to make the default exec handler just handle builtins as well? instead of doing the check before running the exec handler

[mvdan]

Okay. For now I'd suggest to fork interp, because allowing an external package to implement builtins would be a major refactor, and right now I'm not even sure what such an API would look like.

[mvdan]

Throwing another idea out there: you could run some arbitrary code before our builtin runs, and change the list of words in the CallExpr to change the behavior of our builtin. You could use this to change cd foo into cd /some/root/foo, for example.

[josharian]

Somewhat related:

I'd like to be able to log everything being run, as I run it, for debugging. Params("-v") says invalid option: "-v". My next thought was to print everything as it passes through an ExecHandler, but built-ins don't arrive there.

[mvdan]

Did you mean Params("-x")? Because it's set -x that makes a shell print all commands as they are executed. I don't think that option has been implemented yet, but shouldn't be too hard.

Your use case seems much simpler than the original one here, in any case - it could fall under "run a callback before any syntax node is interpreted". We could do that as a separate bit of API, and it could be useful for lots of other use cases, I imagine.

[josharian]

set -v asks the shell to print all the commands exactly as it is provided them. set -x asks the shell to print all the commands exactly as it is about to execute them (after expansion, etc.).

I tried Params("-x"), and got invalid option: "-x". So I think they are perhaps two unimplemented modes. Should I file a new issue?

Either way, "run a callback before any syntax node is interpreted" does sounds fairly generally useful. For my purposes (debugging scripts), having line numbers available in such a callback would be extra useful.

[TorchedSammy]

Throwing another idea out there: you could run some arbitrary code before our builtin runs, and change the list of words in the CallExpr to change the behavior of our builtin. You could use this to change cd foo into cd /some/root/foo, for example.

coming back to this, it sounds like a good idea

[mvdan]

set -v asks the shell to print all the commands exactly as it is provided them. set -x asks the shell to print all the commands exactly as it is about to execute them (after expansion, etc.).

Ah, thanks - I had momentarily forgotten that both variants exist. They are both TODOs, unfortunately. Not hard to implement, though, if you're up for it to make your debugging easier. A new issue would also be fine :)

Either way, "run a callback before any syntax node is interpreted" does sounds fairly generally useful. For my purposes (debugging scripts), having line numbers available in such a callback would be extra useful.

Agreed on the callback being useful. I'll take a look after this particular handler is merged into master, as they'd run into conflicts. I also think it would be reasonable for HandlerContext to gain a "current position" field.

coming back to this, it sounds like a good idea

Great :) Please take a look at https://github.com/mvdan/sh/tree/interp-call-handler and give it a go. If it sounds good, I'll add a couple of tests and push it to master. If it doesn't work for you or you have any design feedback, I'd love to hear it.

[mvdan]

Friendly ping @TorchedSammy. I'll probably merge that branch into master even if you don't reply, but then I won't know if it actually solves your problem or not.

[TorchedSammy]

sorry for being dormant for that long

anyways, not 100% what i wanted but that works

[mvdan]

If it's not exactly what you want, maybe suggest another idea?

[TorchedSammy]

i really dont have any other ideas, and it works for me

[mvdan]

@TorchedSammy any particular reason for closing this? Do you not need this anymore?

I still plan on merging https://github.com/mvdan/sh/issues/705#issuecomment-864609280, so I'd like to keep this issue open. It just hasn't been a priority recently.

[TorchedSammy]

nope, don't need it anymore but i can imagine the call handle still being useful for something else

[mvdan]

Reopening as I'll be merging my branch soon; as a reminder.