Skip to content

Stabilize offset_of_slice #139673

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jdonszelmann
Copy link
Contributor

Tracking issue: #126151

This stabilizes using offset_of! for dynamically sized fields (i.e. the last field of a DST) which are a slice or a wrapper around a slice. If the field is, for example, dyn Trait we cannot know ifs offset, because the offset can depend on the alignment of the underlying type. But if the dynamically sized field is a slice, the alignment is constant and the offset of that field can be calculated.

There did not seem to be any problems with the implementation of this feature, and no known issues. Discussed with @WaffleLapkin to move for stabilization. I updated the documentation for the offset_of!().

r? @RalfJung

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 11, 2025
@jdonszelmann jdonszelmann added the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 11, 2025
@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Apr 11, 2025

Not sure why you are assigning this to me, I have little to do with this feature and with these parts of the compiler. Someone from @rust-lang/types should have a look regarding this "statically known alignment" check.

r? compiler

@rustbot rustbot assigned jieyouxu and unassigned RalfJung Apr 11, 2025
@jieyouxu
Copy link
Member

Since you may have more context...
r? @WaffleLapkin

@rustbot rustbot assigned WaffleLapkin and unassigned jieyouxu Apr 11, 2025
@compiler-errors
Copy link
Member

This should have a proper stabilization report written up for it. I think there's a template somewhere for some of the questions to ask--I'm on my phone so I cannot find it.

I can take a look at the HIR typeck implementation later too.

@compiler-errors compiler-errors self-assigned this Apr 11, 2025
@jdonszelmann
Copy link
Contributor Author

@RalfJung mostly because you wrote the tracking issue, but maybe someone else is better suited yea. @compiler-errors looked for a template but couldn't find one, so I wrote something informal. However, I'm happy to extend it! Let me take a look at some more other stabilization reports around. @WaffleLapkin agreed with the statically known alignment, though if we want to avoid that term we can keep it to just mentioning slices for now.

@RalfJung
Copy link
Member

The stabilization template draft is at rust-lang/rustc-dev-guide#2219.

@jdonszelmann
Copy link
Contributor Author

Alright, I'm working on a proper stabilization report using that template, thanks Ralf!

ObligationCauseCode::Misc,
);
}
self.require_type_has_static_alignment(field_ty, expr.span);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a comment onto require_type_has_static_alignment that notes that changing the behavior will extend the functionality of offset_of and is insta-stable?

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
---- library/core/src/mem/mod.rs - mem::offset_of (line 1271) stdout ----
error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
  --> library/core/src/mem/mod.rs:1285:12
   |
17 | assert_eq!(mem::offset_of!(Struct<[u8]>, b), 1); // OK — the last field is a slice
   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
   |
   = help: the trait `Sized` is not implemented for `[u8]`
   = note: this error originates in the macro `mem::offset_of` (in Nightly builds, run with -Z macro-backtrace for more info)

@jdonszelmann
Copy link
Contributor Author

ugh, I believe the CI failure is because it tries to compile the docs with an older compiler? like a #[cfg(bootstrap)] issue? could be wrong but I'm unsure how to fix that if that's the case.

@@ -1263,10 +1263,10 @@ impl<T> SizedTypeProperties for T {}
///
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate this item, mark one as #[cfg(bootstrap)] and one as #[cfg(not(bootstrap))]. Give the #[cfg(bootstrap)] one dummy documentation like /// This item will not be documented so it doesn't get doc-tested in stage 0.

@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 13, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 13, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

Please renominate when the stabilization report is available.

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants