callbacks for signal_start and signal_start_oneshot should receive a number

[adigitoleo]

Unless there is a strong reason for the current behavior, I propose that the callback which is given as the third argument to signal_start and signal_start_oneshot should receive/propagate the signal as a number. Currently, the signal is propagated as a lowercase string. This is confusing for several reasons:

• In libuv, those callbacks propagate the argument as an int, see here
• the variable is named signum, implying a numeric representation
• for neovim users, this presents a confusing switch in representations, see below
• the value of uv.constants.SIGINT, and other signals, is an integer

This example demonstrates the confusion for a NeoVim user. Sure, the outputs of vim.system have no obligation to be consistent with luv, and vice versa, but it would make things simpler for us. Requires NeoVim 0.10 for the vim.uv namespace. Note also that vim.system doesn't allow SIGINT hooks (only a timeout one) so this use case is more than hypothetical.

-- Run with nvim -u NORC -l <this_file>.
-- Interrupt with C-c before 5 seconds.
local uv = vim.uv
local sigint = uv.new_signal()

sigint:start(2, function(signal)
    vim.print(string.format("in handler: %s (type: %s)\n", signal, type(signal)))
end)

local job = vim.system({ "sleep", "5" }):wait()
vim.print(string.format("\nafter job exit: %s (type: %s)", job.signal, type(job.signal)))

[SinisterRectus]

Numbers were used when signals were added to an old luvit version. After luv was split into its own project, signals were reintroduced using uppercase strings. This implementation was eventually changed to using lowercase strings and moved to a dedicated constants file where it remains, along with other constants that are handled as lowercase strings.

This was all 10+ years ago and before I joined, so I can't speak for those who made the change, but luv generally uses strings instead of numbers where possible. I assume that this is for consistency with Lua's recommendation to use strings for options.

The usual convention in Lua libraries is to use strings instead of numbers to select options.

In any case, the return type is documented as a string, so we could not easily change this if we wanted. I think that we can do better to document string constants that are accepted and expected, though.

I do agree that the existence of numbers in the constants table is confusing. Perhaps we could also expose the strings in a constants table, or expose the string_to_num and/or num_to_string converters.

[squeek502]

A simple backwards compatible solution would be to pass a second parameter to the callback that contains the integer representation.

[adigitoleo]

Lua's recommendation to use strings for options

OK, this makes sense in a way. The change to lowercase is still strange considering what kill -l and such usually output, but what's done is done.

There seem to be three representations in the current API:

• signum: string for the callback functions already mentioned
• signal: integer for the on_exit handler in spawn
• signum: integer or string or nil for process_kill and kill

It seems like relaxing them all to accept either an integer or a string could be done in a backwards-compatible way. My opinion is that this is preferable to defining a second constants table + converters, but it doesn't exclude doing that as well. I suggest that the parameter be called signal across the whole API, not signum, and that this should accept either a numeric value or the lowercase string (from a list of possible values that could be listed somewhere in the docs). To avoid breaking changes, process_kill and kill can also accept nil (i.e. the signal arg is optional). If that sounds reasonable I can work on a PR over the weekend.

[squeek502]

It seems like relaxing them all to accept either an integer or a string could be done in a backwards-compatible way

I'm not sure I understand what you're proposing. There's no way to infer what type the callback is expecting, and process_kill and kill already accept the same types, so I'm not sure what you mean by 'relaxing them all'.

EDIT: Maybe you mean something like "if the signal is passed to the function as an integer, then the callback should be called with an integer"? But that would still be a breaking change, and it can't apply to spawn.

---

To give you an idea of the change I had in mind, for the callbacks, the backwards compatible approach would be something like:

For uv.spawn's on_exit callback, the parameters would now be:

• code: integer
• signum: integer (renamed parameter)
• signame: string (new parameter)

For uv.signal_start and uv.signal_start_oneshot's callbacks, the parameters would now be:

• signame: string (renamed parameter)
• signum: integer (new parameter)

It's a bit unfortunate that these parameters would have to be in a different order between spawn/signal_start, but not sure what alternative we have.

[adigitoleo]

if the signal is passed to the function as an integer, then the callback should be called with an integer

I confused myself a bit but yes, something like that is what I had in mind, and the same for strings. But if it doesn't work for spawn then no worries. I guess what you are proposing is the only real compromise in that case. Maybe this just boils down to a documentation issue in the end, I guess having the list of possible strings referenced is the most obvious thing.

[SinisterRectus]

Changing to signame is good, though I would recommend sigstr or sigstring if either of those provide any extra assurance that the value is e.g. "sigint" instead of "SIGINT".

We probably should document that the value can technically be nil for unknown signals as supported by lua_pushstring.

We can document the list of supported strings similarly to how we document FS modes. Since signals appear in multiple places, we can add a new section dedicated to constants to avoid duplication, and we can point the FS modes and other constants to there.

[SinisterRectus]

Adding the new parameters is also fine, if that helps things. The order difference is indeed unfortunate, though.

[SinisterRectus]

With #713, we now have documented the behavior encountered in this issue. As you can see by this change, there are places other than signals where the library optionally accepts lowercase strings or integer constants, but only returns lowercase strings. You may also find other places that only accept lowercase strings, or integer constants, but not both. When I get a chance, I'll make a list of all of these encounters, then we can think about how we can maximize UX without complicating library behavior. After documenting things, my belief at this point is that accepting multiple input types is already complicated enough, and providing multiple output types would be impractical in certain cases, but I'm open minded.

[adigitoleo]

Thanks, the list should help quite a lot and I think the simple examples are nice too. I'll do my best to keep an eye on the constants -> strings mapping in that file in case I notice any changes that update the constants table but miss the docs. These changes should propagate to :h luvref in NeoVim and that should help users like me to notice the difference in signal representations between luv and e.g. vim.system.

accepting multiple input types is already complicated enough, and providing multiple output types would be impractical in certain cases

I'm sympathetic to this concern, I wasn't aware of the lua string recommendation when creating the issue and that has softened my opinion. I'm happy for this to be closed in favour of more targeted future UX proposals.