http icon indicating copy to clipboard operation
http copied to clipboard

StatusCode TryFrom and PartialEq with integer types

Open dekellum opened this issue 3 years ago • 4 comments

As suggested in #230, I added TryFrom <usize>, <u32> and <u64> and From<StatusCode> for the same integer types, and re-implemented <u16> with the same macro. Oddly enough, if I stop with unsigned types, then previously working doctests for response::{Response, Builder} start failing with the following:

error[E0277]: the trait bound `StatusCode: From<i32>` is not satisfied
 --> src/response.rs:537:6
  |
7 |     .status(200)
  |      ^^^^^^ the trait `From<i32>` is not implemented for `StatusCode`
  |
  = help: the following implementations were found:
            <StatusCode as From<&'a StatusCode>>
  = note: required because of the requirements on the impl of `Into<StatusCode>` for `i32`
  = note: required because of the requirements on the impl of `TryFrom<i32>` for `StatusCode`

...~~which seems almost more like an obscure rustc or stdlib bug? (Fails on MSRV 1.39.0 as well as a recent nightly)~~ (edit: see comment below). As this was done with macro_rules!, its easy to extend to the signed types i16, i32, isize, and i64 types to avoid the above issue. Since its implemented via u16::try_from there is nothing particularly more unusual about including support for the signed types: negative values are just another case of InvalidStatusCode.

Also added bi-directional PartialEq for all of these integer types. This makes it so u16 doesn't have any particular favor in the public interface other then the existing StatusCode::from_u16 and as_u16. If a user sticks to TryInto, From and PartialEq then u16 (and NonZeroU16) is just a private implementation detail.

The PR includes some misc related test and doc improvements.

Given the warning of the about mentioned compile error, I would not suggest merging this for release in a patch release like 0.2.2. It warrants at least a MINOR bump 0.3.0 or 1.0.0.

Fixes #230

dekellum avatar Dec 11 '20 06:12 dekellum

Asked about the error mentioned above and got a helpful, quick answer here:

https://users.rust-lang.org/t/whats-so-special-about-the-i32-type-here/52613/3

So its known limitation of the compiler and i32 conversions need to be implemented to handle the i32 fallback for non-inferred integer literals. Inference no longer happens with the multiple TryFrom impls of this PR.

dekellum avatar Dec 11 '20 08:12 dekellum

@kaj how would this play out with #442, #454? Could you cross review?

dekellum avatar Dec 15 '20 18:12 dekellum

@kaj how would this play out with #442, #454? Could you cross review?

I think there is no relevant conflict. #442 has Builder2::status(code) which takes a StatusCode rather than a TryInto<StatusCode>, so it doesn't really matter to that which types implement TryInto<StatusCode>. I might amend it to also proivde a Builder2::try_status(...) method that taks a TryInto<StatusCode>, but I don't think that is a conflict either. Of course, if this i merged first and there is a conflict, I'll be happy to resolve it.

Also, I think this change looks good in itself (but I'm not a maintainer of this crate).

kaj avatar Dec 15 '20 23:12 kaj

Okay, thanks. That was the kind of sanity check on this PR I was hoping for.

dekellum avatar Dec 15 '20 23:12 dekellum