Reword incorrect documentation about SocketAddr having varying layout #136230
Conversation
rustbot
added
S-waiting-on-review
| @@ -42,10 +39,6 @@ pub enum SocketAddr { | |||
| /// | |||
| /// See [`SocketAddr`] for a type encompassing both IPv4 and IPv6 socket addresses. | |||
| /// | |||
| /// The size of a `SocketAddrV4` struct may vary depending on the target operating | |||
| /// system. Do not assume that this type has the same memory layout as the underlying | |||
| /// system representation. | |||
There was a problem hiding this comment.
I'm ok with removing the size comment, but isn't the other layout note still relevant? (e.g. don't try to treat this like a C struct sockaddr!)
There was a problem hiding this comment.
So, it's technically relevant, but it feels unprecedented in the standard library, since no other type adds a comment like this. I actually took a bit to look through the existing types, though, and I guess these are the types where this would technically be most applicable. (You wouldn't expect a File to be equivalent to a file handler, for example.)
Like, most people are not going to try transmuting random standard library structs to libc structs unless it's explicitly marked as allowed, and since transmuting is unsafe, they're also kind of admitting fault if they do this.
But also, is there actually a reason we couldn't make this equivalent to a C sockaddr anyway? Since no padding is required, and we don't really gain anything from letting the fields be randomly arranged.
There was a problem hiding this comment.
We used to wrap sockaddr directly, and prominent crates did make unsafe assumptions about that. We changed to Rust-native types in #78802, which also documented many of those problems along the way.
So yes, in general users shouldn't make any assumptions about the underlying type, but they did in this case. Therefore, I think it's worth keeping some warning in the docs, even though it would be obviously broken if you tried it anymore.
There was a problem hiding this comment.
So, for some reason, I had naïvely assumed that sockaddr just used the IETF definition on all platforms, which is clearly not correct. And besides that, it's not the same definition as the one we use.
I'll update to remark on this.
|
@rustbot author |
rustbot
added
S-waiting-on-author
clarfonthey
changed the title
clarfonthey
force-pushed
the
net-memory-layout-assumptions
branch
from
91c2b28 to
58de5a8
Compare
clarfonthey
force-pushed
the
net-memory-layout-assumptions
branch
from
58de5a8 to
ffa86bf
Compare
|
Sorry for taking so long, but I've finally gotten around to revising the wording. It's longer than the original, but at least it's accurate now. |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
LGTM, thanks! @bors r+ rollup |
bors
added
S-waiting-on-bors
Rollup of 16 pull requests Successful merges: - rust-lang#136001 (Overhaul examples for PermissionsExt) - rust-lang#136230 (Reword incorrect documentation about SocketAddr having varying layout) - rust-lang#136892 (Sync Fuchsia target spec with clang Fuchsia driver) - rust-lang#136911 (Add documentation URL to selected jobs) - rust-lang#137870 ( Improve HashMap docs for const and static initializers) - rust-lang#138179 (Add `src/tools/x` to the main workspace) - rust-lang#138389 (use `expect` instead of `allow`) - rust-lang#138396 (Enable metrics and verbose tests in PR CI) - rust-lang#138398 (atomic intrinsics: clarify which types are supported and (if applicable) what happens with provenance) - rust-lang#138432 (fix: remove the check of lld not supporting `@response-file)` - rust-lang#138434 (Visit `PatField` when collecting lint levels) - rust-lang#138441 (update error message) - rust-lang#138442 (EUV: fix place of deref pattern's interior's scrutinee) - rust-lang#138457 (Remove usage of legacy scheme paths on RedoxOS) - rust-lang#138461 (Remove an outdated line from a test comment) - rust-lang#138466 (Remove myself from libs review) Failed merges: - rust-lang#138452 (Remove `RUN_CHECK_WITH_PARALLEL_QUERIES`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#136230 - clarfonthey:net-memory-layout-assumptions, r=cuviper Reword incorrect documentation about SocketAddr having varying layout This has no longer been the case since these types were moved to `core`. The note on portability remains, but it is reworded to not imply that the size varies by target.
…mptions, r=cuviper Reword incorrect documentation about SocketAddr having varying layout This has no longer been the case since these types were moved to `core`. The note on portability remains, but it is reworded to not imply that the size varies by target.
Rollup of 16 pull requests Successful merges: - rust-lang#136001 (Overhaul examples for PermissionsExt) - rust-lang#136230 (Reword incorrect documentation about SocketAddr having varying layout) - rust-lang#136892 (Sync Fuchsia target spec with clang Fuchsia driver) - rust-lang#136911 (Add documentation URL to selected jobs) - rust-lang#137870 ( Improve HashMap docs for const and static initializers) - rust-lang#138179 (Add `src/tools/x` to the main workspace) - rust-lang#138389 (use `expect` instead of `allow`) - rust-lang#138396 (Enable metrics and verbose tests in PR CI) - rust-lang#138398 (atomic intrinsics: clarify which types are supported and (if applicable) what happens with provenance) - rust-lang#138432 (fix: remove the check of lld not supporting `@response-file)` - rust-lang#138434 (Visit `PatField` when collecting lint levels) - rust-lang#138441 (update error message) - rust-lang#138442 (EUV: fix place of deref pattern's interior's scrutinee) - rust-lang#138457 (Remove usage of legacy scheme paths on RedoxOS) - rust-lang#138461 (Remove an outdated line from a test comment) - rust-lang#138466 (Remove myself from libs review) Failed merges: - rust-lang#138452 (Remove `RUN_CHECK_WITH_PARALLEL_QUERIES`) r? `@ghost` `@rustbot` modify labels: rollup
This has no longer been the case since these types were moved to
core. The note on portability remains, but it is reworded to not imply that the size varies by target.