HeaderValue::from_static can be const

[jeromegn]

We have a few (lazy) statics for headers we reuse, e.g.

static HTTP_PROTO: Lazy<HeaderValue> = Lazy::new(|| HeaderValue::from_static("http"));

It dawned on me HeaderValue::from_static might be const with a few tweaks. With some help in avoiding the panic! (currently unstable, but stabilization incoming), I was able to make it work.

This change would allow us to just write:

const HTTP_PROTO: HeaderValue = HeaderValue::from_static("http");

It doesn't give a nice panic like before, but if the user follows the backtrace, they should see the comment explaining that a panic here means they've provided an invalid header value.

[dekellum]

Could you link any rustc tracking issue that stabilizes panic! in const fn? Maybe we just wait for that and then make these changes as part of http 0.3.0 or 1.0.0 with an MSRV increase?

Also it would seem that this should also include making HeaderName::from_static const? (For custom, non-standard header names.)

[jeromegn]

@dekellum Just linked this in the PR description: rust-lang/rust#51999 (comment)

I looked at HeaderName::from_static: it looks a lot more complicated to constantize.

[dekellum]

Re: HeaderName::from_static then it seems also fine to PR that separately, when and if it becomes more practical.

[jeromegn]

I'd like to get this merged in if the crate can stomach a higher MSRV. If not, what policy are we aiming for concerning MSRV?

[seanmonstar]

I think incrementing the MSRV is fine (as long as it's older than 6 months).

My personal nit is that the panic message here is much more cryptic. I know it points at the line and a user can go read the comment, but this nit personally makes me lean towards waiting for const-panics. I can be overruled if many think it's worth it, though.

[seanmonstar]

Why are we adding track_caller?

[jeromegn]

I believe this will provide a more useful line number when the panic occurs. Instead of pointing at hyper's code, it will point at the user's HeaderValue::from_static line

[seanmonstar]

Does removing this attribute make the panic message much worse?

[jeromegn]

Just tried without and it doesn't seem to change anything. Removed!

[jeromegn]

My personal nit is that the panic message here is much more cryptic.

I agree. I don't love it, but in practice that will almost never happen unless somebody passes a non-ascii &'static str.

[tobz]

I think it's reasonable to have a slightly-suboptimal panic message until const panic is stable. Given your statement, that's still 6+N months away from hyper being willing to make the MSRV bump to it, and who knows how long N is going to be.

In favor of merging as-is.

[seanmonstar]

@LucioFranco @hawkw @carllerche @sfackler @nox opinion on my nit/doing this now with a worse panic message vs waiting?

[nox]

No need to wait IMO.

[LucioFranco]

Is there an example of what the panic looks like?

[jeromegn]

Just triggered it with an invalid header value:

error: any use of this value will cause an error
  --> /Users/jerome/.cargo/git/checkouts/http-94277678a7841ff5/b6c783b/src/header/value.rs:67:17
   |
67 |                 ([] as [u8; 0])[0]; // Invalid header value
   |                 ^^^^^^^^^^^^^^^^^^
   |                 |
   |                 index out of bounds: the length is 0 but the index is 0
   |                 inside `HeaderValue::from_static` at /Users/jerome/.cargo/git/checkouts/http-94277678a7841ff5/b6c783b/src/header/value.rs:67:17
   |                 inside `HTTP_PROTO` at src/http/service.rs:73:33
   |
  ::: src/http/service.rs:73:1
   |
73 | const HTTP_PROTO: HeaderValue = HeaderValue::from_static("httpжsome value");
   | ----------------------------------------------------------------------------
   |
   = note: `#[deny(const_err)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #71800 <https://github.com/rust-lang/rust/issues/71800>

error: aborting due to previous error

[LucioFranco]

Yeah that error is quite cryptic but I think if we add some docs about it on the method then its likely people will find an explanation there and we can add a proper message later when that lands on stable. I don't think this is crazy enough to wait for it, so lets ship it w/ the doc comment?

[jeromegn]

I've added a panic example, not entirely sure this is the right way to document this though!

[jeromegn]

Looks like somebody needs to approve the workflow for it to run and test my changes.

[LucioFranco]

@jeromegn I just clicked the button.

[jeromegn]

I'm not sure how to best present the example panic error. Looks like rustdoc tries to parse it as code and errors.

I'm not all that familiar with rustdoc, anybody have any insight?

Maybe I don't need an example panic.

[tesaguri]

You might want to try this:

```text
error: any use of this value will cause an error
  --> http/src/header/value.rs:67:17
...
```

See https://doc.rust-lang.org/rustdoc/documentation-tests.html#attributes.

[jeromegn]

Can somebody press the approve CI run button :)

[nox]

I clicked the button but I don't actually have the rights to make the CI start hah.