-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
Stable inline const using unstable intrinsics #149817
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
base: main
Are you sure you want to change the base?
Stable inline const using unstable intrinsics #149817
Conversation
|
Some changes occurred in compiler/rustc_attr_parsing Some changes occurred to constck cc @fee1-dead Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr Some changes occurred to the CTFE machinery
cc @rust-lang/wg-const-eval |
| #[rustc_intrinsic] | ||
| #[miri::intrinsic_fallback_is_spec] | ||
| #[rustc_intrinsic_const_stable_indirect] | ||
| pub const unsafe fn const_allocate(_size: usize, _align: usize) -> *mut u8 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this and make_global are needed in
rust/library/alloc/src/boxed/thin.rs
Line 334 in e2893f7
| let alloc: *mut u8 = const_allocate(alloc_size, alloc_align); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one really shouldn't be callable-from-stable, it is the very definition of not stable... same for const_make_global.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would strongly prefer to add allow_const_unstable to new_unsize_zst -- and we should consider if it's really safe to use these highly experimental features there.
That said, Thin is entirely unstable, so we can also add allow_const_unstable with a comment saying that this isn't actually stable -- but then we should also add that as a blocker on the tracking issue as it seems easy to miss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would strongly prefer to add allow_const_unstable to new_unsize_zst
That does feel nicer, to guard it against any other uses. With a comment there why specifically this call needs it and is safe
5640606 to
8666a33
Compare
|
Is this a fix for #132536? Or only a partial fix since it "just" applies to inline consts, not const items? |
| || self.tcx.sess.opts.unstable_opts.force_unstable_if_unmarked) | ||
| matches!( | ||
| self.const_kind, | ||
| Some(hir::ConstContext::ConstFn | hir::ConstContext::Const { inline: true }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only a partial fix. I can remove the inline limitation here, but it has a lot of fallout that I haven't analyzed yet. I'll do that in a follow-up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the remaining intrinsics, they aren't actually used from const fn either, are they? We just expose them via regular functions? The fact that those use a constant internally seems more of an implementation detail -- and we haven't had the t-lang FCP that would be required to actually expose them in a const fn.
| #[unstable(feature = "core_intrinsics", issue = "none")] | ||
| #[rustc_intrinsic] | ||
| #[rustc_intrinsic_const_stable_indirect] | ||
| pub const fn variant_count<T>() -> usize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't actually stable either, I think? std::mem::variant_count is still unstable.
| #[unstable(feature = "core_intrinsics", issue = "none")] | ||
| #[rustc_intrinsic] | ||
| #[rustc_intrinsic_const_stable_indirect] | ||
| pub const fn type_name<T: ?Sized>() -> &'static str; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't const-stable, it can only be invoked via a regular function.
| #[unstable(feature = "core_intrinsics", issue = "none")] | ||
| #[rustc_intrinsic] | ||
| #[rustc_intrinsic_const_stable_indirect] | ||
| pub const fn type_id<T: ?Sized + 'static>() -> crate::any::TypeId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And neither is this const-stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is const stable. TypeId::of is const stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah fair, somehow I didn't find that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, the t-lang FCP that justifies adding this attribute passed here.
Maybe we should have a separate PR just adding that attribute? This PR shouldn't add any more since we don't have FCP for any of the others.
| } | ||
| InlineAsmOperand::Const { anon_const } => hir::InlineAsmOperand::Const { | ||
| anon_const: self.lower_const_block(anon_const), | ||
| anon_const: self.lower_const_block(anon_const, &[]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can put at least some atributes on const operands. Iirc recently cfg was stabilized here? Though that one is of course expanded out early. Still wonder where those attributes go and whether they should be forwarded here 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even if in this case it only matters for their stability attrs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know where the attribute would go. If it's on the anon const, it will end up as part of its body's expression. You can't add attrs on operands, at least the ast has nowhere to put them. This change should be behaviour-preserving, but I could add a FIXME
| Allow(Target::AssocConst), | ||
| Allow(Target::Trait), | ||
| Allow(Target::Static), | ||
| Allow(Target::Expression), // FIXME: we really only want to allow inline consts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could make up a new target for that 🤷♀️ . This would be its current only use but it's not that hard to do otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea I had that, but I felt I already changed enough in this PR 😅
| #[rustc_intrinsic] | ||
| #[miri::intrinsic_fallback_is_spec] | ||
| #[rustc_intrinsic_const_stable_indirect] | ||
| pub const unsafe fn const_allocate(_size: usize, _align: usize) -> *mut u8 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would strongly prefer to add allow_const_unstable to new_unsize_zst
That does feel nicer, to guard it against any other uses. With a comment there why specifically this call needs it and is safe
|
Reminder, once the PR becomes ready for a review, use |
Remove the custom special case rejecting const stability on macros as it is now handled by the general check
I think it is important that non-const public items that contain const blocks using unstable things are also stability checked, otherwise we end up with situations where const features like const_allocate end up used in ways that we can only keep supporting in even hackier ways (worst case completely computing the const inside rustc via a new intrinsic). Limiting it to things that need to have a stability attribute added to it seems useful to me. But I agree that it's different from exposing calling it directly in const fns. const fns that just have a const block in their body are no different from non-const fns that have a const block in their body. And in case we don't do this kind of checking for const blocks, why would we do it for const items/assoc consts? |
8666a33 to
2bf4c76
Compare
|
cc @rust-lang/wg-const-eval |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| #[stable(feature = "asdf", since = "99.0.0")] | ||
| #[rustc_const_stable(feature = "asdf", since = "99.0.0")] | ||
| const fn my_fun4() { | ||
| #[rustc_const_unstable(feature = "abi_unadjusted", issue = "42")] | ||
| const { | ||
| my_fun() | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok this logic is borked 😆 I can make a const block const unstable and the function using it stable and it just keeps on working using very unstable things in the const block
If we can find the right rules then I agree, it's good to have some sort of check here -- However, the PR as-is (when I wrote those comments) requires too many annotations. For the very reasons you state, we should not mark anything that is related to const-heaps as indirectly-const-stable at the moment -- and the compiler shouldn't ask us to do that. Given that regular stability is not recursive, I am not sure it will be possible to fully check for indirect exposure of const intrinsics via const blocks in non-const functions. |
2bf4c76 to
5b232db
Compare
is what the compiler tells us. The latter comes with a suggestion, too (tho the span is a bit off). It doesn't actually suggest the option of using Either way, it correctly errors on calls to unstable const fns from const blocks now, with about the same UX as calling them from within const fns (see required changes to libcore) |
This comment has been minimized.
This comment has been minimized.
5b232db to
7958dfe
Compare
It used to suggest that. Then people did it all the time when it wasn't appropriate, so I removed the suggestion. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| #[rustc_const_unstable(feature = "const_type_name", issue = "63084")] | ||
| pub const fn type_name<T: ?Sized>() -> &'static str { | ||
| const { intrinsics::type_name::<T>() } | ||
| #[rustc_const_unstable(feature = "const_type_name", issue = "63084")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does rustc_const_unstable on a const block mean?
It would help to see e.g. a patch updating the dev guide on these attributes, to understand the intended behavior without having to reverse engineer it from the implementation.
cc @RalfJung
Having moved several intrinsics from sth that gets invoked directly in a const fn to sth invoked in a const block kind of broke our "indirectly usable intrinsic" logic. So now we're actually checking it again. This required changing some intrinsics to now be indirectly callable
r? @jdonszelmann