luvit / luv

Bare libuv bindings for lua
Apache License 2.0
832 stars 187 forks source link

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

Closed Bilal2453 closed 10 months ago

Bilal2453 commented 11 months ago

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:

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 commented 11 months ago

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 commented 11 months ago

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.

Bilal2453 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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:

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

Bilal2453 commented 11 months ago

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 commented 10 months ago

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