-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Document that the SocketAddr memory representation is not stable #83524
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
@bors r+ rollup |
📌 Commit 147316a has been approved by |
…t, r=sfackler Document that the SocketAddr memory representation is not stable Intended to help out with rust-lang#78802. Work has been put into finding and fixing code that assumes the memory layout of `SocketAddrV4` and `SocketAddrV6`. But it turns out there are cases where new code continues to make the same assumption ([example](spacejam/seaslug@96927dc#diff-917db3d8ca6f862ebf42726b23c72a12b35e584e497ebdb24e474348d7c6ffb6R610-R621)). The memory layout of a type in `std` is never part of the public API. Unless explicitly stated I guess. But since that is invalidly relied upon by a considerable amount of code for these particular types, it might make sense to explicitly document this. This can be temporary. Once rust-lang#78802 lands it does not make sense to rely on the layout any longer, and this documentation can also be removed.
I am slightly concerned about explicitly mentioning the absence of a guarantee. Does that now mean that every type that doesn't say this implicitly has such a guarantee? Of course it doesn't, but it would be nice if it could be phrased in a way that makes it clear that we just happen to spell out what already is and has been the default policy. |
Rollup of 9 pull requests Successful merges: - rust-lang#83239 (Remove/replace some outdated crates from the dependency tree) - rust-lang#83328 (Fixes to inline assmebly tests) - rust-lang#83343 (Simplify and fix byte skipping in format! string parser) - rust-lang#83388 (Make # pretty print format easier to discover) - rust-lang#83431 (Tell GitHub to highlight `config.toml.example` as TOML) - rust-lang#83508 (Use the direct link to the platform support page) - rust-lang#83511 (compiletest: handle llvm_version with suffix like "12.0.0libcxx") - rust-lang#83524 (Document that the SocketAddr memory representation is not stable) - rust-lang#83525 (fix doc comment for `ty::Dynamic`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
@the8472 Is that properly documented somewhere? The fact that the memory layout is not part of a types stable API. If so, we could link to it maybe? The |
https://doc.rust-lang.org/reference/type-layout.html#the-default-representation
The rustonomicon says roughly the same. There are more details in the UCG, but they're not normative yet. But there is a much simpler fact that establishes that the internals of SocketAddr are not public: The fields are not |
I'm not disagreeing with you. But it's a fact that at least nine different crates have assumed they can rely on the memory layout of these types. So just as a temporary precaution while trying to make the change of implementation happen we can try to not continue making the same mistake. |
We'd have to ask the authors, but my guess it's due to a lack of conversion APIs between the std Looking at the fixup PRs that's 100LoC replicated over several crates. |
People should bring in
|
|
socket2 is owned by rust-lang, could we just link there? |
Intended to help out with #78802. Work has been put into finding and fixing code that assumes the memory layout of
SocketAddrV4
andSocketAddrV6
. But it turns out there are cases where new code continues to make the same assumption (example).The memory layout of a type in
std
is never part of the public API. Unless explicitly stated I guess. But since that is invalidly relied upon by a considerable amount of code for these particular types, it might make sense to explicitly document this. This can be temporary. Once #78802 lands it does not make sense to rely on the layout any longer, and this documentation can also be removed.