`const` assignment of `HeaderName::from_static`

dodomorandi commented 1 year ago

When using HeaderName::from_static in in order to create aconst(instead of astatic` variable), clippy triggers a warning. Indeed, the following code:

pub const NAME: HeaderName = HeaderName::from_static("test");

Produces the following warning:

warning: a const item should never be interior mutable

Playground

I could just ignore the issue and use static instead of const, but I think that the lint exposes an interesting fact I would like to discuss.

First of all, the reason behind the lint: there is an AtomicPtr deep down. The dependency chain is:

HeaderName -> Repr<Custom> -> Custom -> ByteStr -> Bytes -> AtomicPtr

Now, the issue itself... It is a non-problem, except for one detail: the fact that potentially we have internal mutability in a header string that obviously is meant to never change is unexpected. Don't get me wrong, I understand the reason behind this: we want to be able to reuse memory when receiving headers (thanks to Bytes) and at the same time it is extremely nice to have just one HeaderName abstraction (instead of HeaderName and ConstHeaderName, for instance). However, following the principle of least surprise, probably you would expect a const HeaderName::from_static() function to not have interior mutability.

Now, what should be done? To be honest, I am not sure. I personally don't see a practical problem with the current approach, which is pretty ergonomic. Maybe we could consider adding a note in the documentation of HeaderName::from_static, suggesting to use a static variable instead of a const to avoid being flooded by clippy warnings.

Do you have better ideas? CC @paolobarbolini

Originally posted by @dodomorandi in https://github.com/hyperium/http/issues/264#issuecomment-1508257859

seanmonstar commented 1 year ago

I believe the clippy lint is wrong.

Yes, there's an AtomicPtr in there. But when made via from_static, it only ever points at a &'static str, and it will never change.

dodomorandi commented 1 year ago

You are right, HeaderName does not expose any method to let you change the inner value. But I still think that two issues still exist:

Unexpected warning by the user. As a user, you don't have any desire (or possibility) to change the inner value of HeaderName, but if you assign it to a const you get an unexpected warning. I think that we should at least document the behavior.
Unexpected cost. Notice that this point is the original reason behind the clippy lint. If you notice from [this trivial example], it is noticeable that using the const value brings to some memory copying (it uses 4 128 bit instructions in x86_64).

Because of this, I don't thing that clippy is wrong. But, as I said, I also think that this is not a real problem since we inform the user that the warning is expected when assigning a HeaderName to a const.

sfackler commented 1 year ago

https://github.com/rust-lang/rust-clippy/issues/9776

punkeel commented 2 months ago

With https://github.com/rust-lang/rust-clippy/pull/12691 (using nightly), the error is now a warn:

~/Code/... ❯ cargo +nightly clippy
warning: a `const` item should never be interior mutable
  --> crates/my_http/src/....rs:19:1
   |
19 |   const CROSS_ORIGIN_RESOURCE_POLICY: HeaderName =
   |   ^----
   |   |
   |  _make this a static item (maybe with lazy_static)
   | |
20 | |     HeaderName::from_static("cross-origin-resource-policy");
   | |____________________________________________________________^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#declare_interior_mutable_const
   = note: `#[warn(clippy::declare_interior_mutable_const)]` on by default

warning: `my_http` (lib) generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.20s
~/Code/... ❯ cargo +nightly clippy --version
clippy 0.1.79 (c987ad52 2024-05-01)

Is there anything that can be done in hyper/http to smoothe it further, or are we happy closing once this hits stable?

seanmonstar commented 2 months ago

The lint is still wrong. I mean, the idea behind it makes sense. But the header name does not use interior mutability. So it's a false positive.

Alexendoo commented 5 days ago

https://github.com/rust-lang/rust-clippy/pull/12691 hadn't quite reached the 2024-05-01 nightly as Clippy updates are synced with the main repo periodically

This false positive no longer occurs on nightly and the fix will hit stable in a few days when 1.80 releases

hyperium / http

`const` assignment of `HeaderName::from_static` #599