Clap: Potentially unjustified `unsafe`

[H2CO3]

There are a couple of uses of unsafe in the code that I think are not strictly necessary. The prime example is this:

https://github.com/kbknapp/clap-rs/blob/6e948994a61e389c8f3b6726435d3d14bdd328bb/src/app/parser.rs#L1350-L1362

I understand that this instance of unsafe might have been introduced for performance reasons, however I don't think giving up safety is a clear win here. When printing help text for the user, I/O time probably dominates UTF-8 validation time, and help texts aren't enormously long either, usually (typically at most in the tens of kilobytes).

Similarly, in https://github.com/kbknapp/clap-rs/blob/6e948994a61e389c8f3b6726435d3d14bdd328bb/src/app/help.rs#L850-L855
it seems highly unlikely to me that a debug build would experience a bottleneck in the UTF-8 validation of this one method.

[kbknapp]

Agreed! I think those were either left over from old code, or under assumptions that only ascii code would be supported. Both of which seem wrong now.

The debug code one I actually want to look up git blame and see if that's something I did :stuck_out_tongue_winking_eye: as I normally don't care at all about debug performance. Even if I didn't write it, I still accepted it, so there's no escaping the blame :laughing:

I'd be good with a PR removing these uses if someone wants a quick PR!

[H2CO3]

Awesome, in this case I'll open a PR soon!

[H2CO3]

There are two more instances of unsafe I'd like to address separately, if possible. First one is:

https://github.com/kbknapp/clap-rs/blob/1ef6e62e1e05f3a1c5dee4d946eb85c9778e4065/src/macros.rs#L430-L442

If this is static for performance reasons, it could be realized using the dedicated lazy_static crate instead to avoid reinventing the wheel.

However, better yet, staticness could also be removed altogether, with the following additional advantages:

1. This gives more control to the user (if they wanted static, they could wrap it into a lazy_static, and if they preferred no unsafe, they could move it to a place in their code so that no static is necessary). This is in accordance with the common Rust design guideline that the consumer of an API should be able to decide how they want to use it, potentially based on context (for example, functions that need a value should generally take it by move, not by reference and then magically clone()ing inside).
2. The current implementation of the macro is such that when called multiple times, it will always return a result that respects the separator used in the first call. This might be counter-intuitive when it's called more than once with different separator arguments. I wouldn't necessarily call it a bug as long as it's documented, but it can be surprising.

[H2CO3]

The second one is:

https://github.com/kbknapp/clap-rs/blob/53f3b65d2469a00eefa3cbad5a2650d602dbbfc8/src/osstringext.rs#L26-L32

Wouldn't be it better to, instead of creating our own from_bytes method, use the already-existing one provided by OsStr itself, and use OsStrs instead of &OsStrs? There aren't many places this is being used (https://github.com/kbknapp/clap-rs/search?utf8=%E2%9C%93&q=from_bytes&type=), so it should also be a relatively painless change.

[kbknapp]

[crate authors]

I'm good trying this method if it can be implemented in a non-breaking manner. Even as unfortunate as it is, if we can't do this in a non-breaking way it'll have to wait.

[OsStrExt3]

This was written before the actual from_bytes was stablized. Now that we're well beyond the stable minus two releases from when from_bytes was stablized, I'm good with updating it so long as we ensure we bump the minor version of this lib and document that a newer version of Rust is required.

[H2CO3]

[crate authors]

"Non-breaking" meaning retaining the incorrect behavior with regards to the separator, or leaving it static? And if the former, wouldn't this classify as a bugfix rather than a breaking change? (AFAIK semver allows breaking changes without major version bump if they are actually bugfixes.)

[OsStrExt3]

Sure, will try to do that as well, then!

[kbknapp]

"Non-breaking" meaning retaining the incorrect behavior with regards to the separator, or leaving it static? And if the former, wouldn't this classify as a bugfix rather than a breaking change? (AFAIK semver allows breaking changes without major version bump if they are actually bugfixes.)

I mean non breaking, meaning change of user code with the exception of correcting a bugfix like you stated. Specifically, I'm talking about removing staticness altogether. For the bug about using crate_authors multiple times, it seems unlikely to me. I agree, its a bug per-se but only due to implementation or lack of being documented. I have yet to have anyone actually complain about this bug. The only thing I'm trying to avoid (not that you're advocating this, I'm just making my position clear 😄 ) is justifying a breaking change behind this one bug might be a stretch of allowable semver.

Thanks for tackling all this by the way!

[H2CO3]

Oooh, alright. Sure thing – of course I'm advocating for making these changes without user code having to change. My suggestion about "giving users the control over staticness" might have sounded contradictory to that, in fact.

And indeed there may be no non-breaking way to remove staticness since the macro yields a pointer. I'll see — but I'll try very hard to do it anyway.

And, as always — you are welcome! I'm happy to improve libraries that I love.

[H2CO3]

This has been resolved via https://github.com/kbknapp/clap-rs/pull/1058.