Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

const assignment of HeaderName::from_static #599

Open
dodomorandi opened this issue Apr 14, 2023 · 6 comments
Open

const assignment of HeaderName::from_static #599

dodomorandi opened this issue Apr 14, 2023 · 6 comments

Comments

@dodomorandi
Copy link

dodomorandi commented Apr 14, 2023

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 #264 (comment)

@seanmonstar
Copy link
Member

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
Copy link
Author

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
Copy link
Contributor

rust-lang/rust-clippy#9776

@punkeel
Copy link

punkeel commented May 2, 2024

With rust-lang/rust-clippy#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
Copy link
Member

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
Copy link

rust-lang/rust-clippy#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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants