docs: most new_handle methods won't return fail #683

Bilal2453 · 2023-12-14T17:38:02Z

Since I've used this docs to generate my luvit-meta definitions, which will complain about not using assert on methods that could return nil (on failure), I've had to investigate into things like local pipe = uv.new_pipe() not using an assert in the examples.
And it appears like it and the methods listed below will never return a failure tuple when initializing a new handle. Even though the docs here explicitly imply otherwise as the return type is handle_type or fail.

Here are the details and how I came to the conclusion that each one of the methods patched don't return an error:

new_prepare and new_check and new_idle:
- on Luv side:
  - in Luv those are luv_new_prepare etc.
  - they make a call to uv_prepare_init (and uv_check_init and uv_idle_init).
  - a failure is returned if the return of this call is ret < 0.
- on libuv side:
  - the uv_name_init methods are defined in loop-watcher.c using a macro.
  - in int uv_##name##_init we see that the return is always 0 therefor never fails.

The following have been initially included in this PR, they have been reverted now. See the comments below.

new_pipe:

on Luv side:

in Luv it is defined as luv_new_pipe in pipe.c.

uv_pipe_init is called, if it returns a value less than 0 it is failure.

on libuv side:

the uv_pipe_init is defined in unix/pipe.c.

the return is always 0, therefor never fails.

new_fs_event:

on the Luv side:

it is luv_new_fs_event defined in fs_event.c.

returns failure when uv_fs_event_init returns a value less than 0.

on libuv side:

uv_fs_event_init is defined per platform. take for example unix/linux-inotify.c (this is weirdly enough called just linux.c nowadays in libuv, but that is unrelated).

always returns 0 therefor never fails.

new_fs_poll:

on Luv side:

it is luv_new_fs_poll defined in fs_poll.c.

uv_fs_poll_init is called, if it returns a value less than 0 it returns failure.

on libuv side:

uv_fs_poll_init is defined in fs-poll.c.

the return is always 0 therefor it never fails.

Bilal2453 · 2023-12-14T17:48:12Z

Fun note, uv.new_tcp never fails as well, as long as the passed flags are correct / or there are no passed flags, but can't do much about that.

Bilal2453 · 2023-12-14T23:50:56Z

oh sorry I forgot to mention, might need to take a look at the Windows version of those. I don't think libuv will be returning an error in the Windows version but not the unix one, but I honestly didn't check the Windows ones.

squeek502

I'm not certain that these changes make sense. Although it may be true that the current libuv implementation of these functions cannot fail, these functions are not defined by libuv to be unable to fail, and (as noted) the luv code allows for/handles failure.

For uv_fs_event_init specifically, it can fail if the platform doesn't support it: https://github.com/libuv/libuv/blob/1479b76310a38d98eda94db2b7f8a40e04b3ff32/src/unix/no-fsevents.c#L27 (used when targeting Haiku, Cygwin, and a few others as far as I can tell).

Bilal2453 · 2023-12-15T00:08:27Z

Yeah you are correct on both of these. It is one of two, either we document the possible failure return and also add an assert or nil check for things like uv.new_pipe in the examples, or we remove the fail type from the return, because otherwise it can be confusing for many as to when or how one of these could ever really fail.
I've got questions before (on the Discord server) to whether you should be asserting a call to uv.new_pipe or not, which started this for me.

I missed the second one, I will be reverting the changes on uv.new_fs_event.

Bilal2453 · 2023-12-15T00:28:28Z

So to be more specific, this tries to solve the issue of "the examples never seem to check for nil on (be it for the sake of the example) uv.new_pipe even though the return says it can fail, which one is it?" and in my case for an additional "the [LSP annotations] are complaining about a missing nil check for uv.new_pipe but the examples never check for that, it can't really fail!".

I think for the sake of clearance we can simply remove the possible fail return, and if at any moment libuv decides one of them can fail, we can put it back perhaps? Although frankly it is extremely unlikely that will ever happen in this stable version of libuv for the uv.new_pipe and the other first 3 listed methods, it seems to me like they are intended to not fail.

squeek502 · 2023-12-15T00:39:52Z

The reservation I have is because luv doesn't really/can't really control/know whether or not a libuv function can return a negative number if its API allows for a negative number to be returned. That is, luv can be linked against any libuv implementation that satisfies the API/ABI, including e.g. a forked version for some operating system that doesn't support new_pipe and returns ENOSYS if it's called. This is admittedly very unlikely / mostly theoretical, but I think it might make more sense to err on the side of "if the function's API allows it to return an error, that function can fail, even if it's basically impossible in normal usage" than document something that may not actually be true.

Bilal2453 · 2023-12-15T14:15:39Z

Hm, that is understandable, didn't think in case of a fork. Would it change much if this change was applied upstream on libuv docs themselves? even though honestly that wouldn't be accepted there either, and it still wouldn't change the fact the ABI will still be compatible with such a fork.

I guess for now I will put this aside, it will come haunting me again if I ever start working on the new doc gen, but that is a problem for later (possibly never).

truemedian · 2023-12-15T20:31:30Z

After diving through the documentation, I see a few things here are actually valid. But in general we can only assume failure is impossible only when explicitly defined by the documentation. Additionally, Luv code itself should mirror these changes, a function that is documented to never fail should not in any case be able to throw an error (ie. the dead path should be removed).

All other functions you've described here are following the Libuv guidelines on errors:

In libuv errors are negative numbered constants. As a rule of thumb, whenever
there is a status parameter, or an API functions returns an integer, a negative
number will imply an error. (emphasis mine)

Only the following are documented to always succeed:

uv_prepare_init
uv_check_init
uv_idle_init

Everything else has in the past, or may in the future throw an error.

Bilal2453 · 2023-12-15T21:24:10Z

That is interesting! I just looked at the docs, kind of surprised uv_pipe_init isn't documented to never fail as well, but that makes sense I guess. Everything that may depend on the platform may error, but those 3 specifically do not depend on the platform, their implementation and purpose is entirely dependent on the event loop, so it can make guarantees about them.

I will revert the changes on everything except those three for now then!

Note: I believe what Truemedian is talking about here is the explicit RETURNS: 0.

Bilal2453 · 2024-01-17T17:59:23Z

@squeek502 What do you think should be done about the libuv docs's RETURNS: 0? Should we go with this change or something else?

squeek502

I'm good with this, thanks!

Bilal2453 added 2 commits December 14, 2023 18:40

docs: most new_handle methods won't return fail

5cf1269

docs: new_thread may fail

71bed92

squeek502 reviewed Dec 14, 2023

View reviewed changes

docs: new_fs_event may fail

ab0c3ec

docs: follow explicit docs on what cannot fail

d460f95

squeek502 approved these changes Jan 17, 2024

View reviewed changes

squeek502 merged commit adff530 into luvit:master Jan 17, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: most new_handle methods won't return fail #683

docs: most new_handle methods won't return fail #683

Bilal2453 commented Dec 14, 2023 •

edited

Loading

Bilal2453 commented Dec 14, 2023

Bilal2453 commented Dec 14, 2023

squeek502 left a comment •

edited

Loading

Bilal2453 commented Dec 15, 2023 •

edited

Loading

Bilal2453 commented Dec 15, 2023

squeek502 commented Dec 15, 2023 •

edited

Loading

Bilal2453 commented Dec 15, 2023

truemedian commented Dec 15, 2023

Bilal2453 commented Dec 15, 2023 •

edited

Loading

Bilal2453 commented Jan 17, 2024

squeek502 left a comment

docs: most new_handle methods won't return fail #683

docs: most new_handle methods won't return fail #683

Conversation

Bilal2453 commented Dec 14, 2023 • edited Loading

Bilal2453 commented Dec 14, 2023

Bilal2453 commented Dec 14, 2023

squeek502 left a comment • edited Loading

Choose a reason for hiding this comment

Bilal2453 commented Dec 15, 2023 • edited Loading

Bilal2453 commented Dec 15, 2023

squeek502 commented Dec 15, 2023 • edited Loading

Bilal2453 commented Dec 15, 2023

truemedian commented Dec 15, 2023

Bilal2453 commented Dec 15, 2023 • edited Loading

Bilal2453 commented Jan 17, 2024

squeek502 left a comment

Choose a reason for hiding this comment

Bilal2453 commented Dec 14, 2023 •

edited

Loading

squeek502 left a comment •

edited

Loading

Bilal2453 commented Dec 15, 2023 •

edited

Loading

squeek502 commented Dec 15, 2023 •

edited

Loading

Bilal2453 commented Dec 15, 2023 •

edited

Loading