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

Disable const-stability checking for intrinsics with fallback MIR? #122652

Closed
scottmcm opened this issue Mar 17, 2024 · 3 comments · Fixed by #132492
Closed

Disable const-stability checking for intrinsics with fallback MIR? #122652

scottmcm opened this issue Mar 17, 2024 · 3 comments · Fixed by #132492
Labels
A-intrinsics Area: Intrinsics C-discussion Category: Discussion or questions that doesn't represent real issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@scottmcm
Copy link
Member

As suggested in #122582 (comment), I moved my new intrinsic to core::intrinsics. But that meant I started getting a new error:

error: `typed_swap` is not yet stable as a const fn
   --> library\core\src\mem\mod.rs:731:14
    |
731 |     unsafe { intrinsics::typed_swap(x, y) }
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Is that error useful for things with fallback MIR that's already const-checked? Could we skip it?

(Maybe doing that in the same PR that would make CTFE capable of using fallback MIR?)

cc @oli-obk

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 17, 2024
@oli-obk
Copy link
Contributor

oli-obk commented Mar 18, 2024

This is just stability, you can mark intrinsics as const stable without the intrinsic itself becoming stable

@jieyouxu jieyouxu added A-intrinsics Area: Intrinsics T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 18, 2024
@jieyouxu jieyouxu added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Apr 4, 2024
@RalfJung
Copy link
Member

RalfJung commented Nov 2, 2024

Your fallback body uses a const-unstable function, so this is still not an intrinsic we'd want to allow calling from stable const fn. So IMO it makes sense that you would have to write #[rustc_const_unstable] here, you'd have to write the same if this was a regular function.

We could consider doing something special for intrinsics where the fallback body is entirely stable. But OTOH, adding a quick #[rustc_const_stable_intrinsic] (with #132492) isn't too much of a big deal either, I think.

@RalfJung
Copy link
Member

RalfJung commented Nov 2, 2024

Turns out with the new const stability checks this actually isn't that hard -- we just have to ensure that fallback bodies are recursively const-stability checked, and then we can take this into account when const-checking an intrinsic call.

@bors bors closed this as completed in 23ef001 Nov 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 5, 2024
Rollup merge of rust-lang#132492 - RalfJung:const-intrinsics, r=compiler-errors

remove support for extern-block const intrinsics

This converts all const-callable intrinsics into the "new" form of a regular `fn` with `#[rustc_intrinsic]` attribute. That simplifies some of the logic since those functions can be marked `const fn` like regular functions, so intrinsics no longer need a special case to be considered const-callable at all.

I also added a new attribute `#[rustc_const_stable_intrinsic]` to mark an intrinsic as being ready to be exposed on stable. Previously we used the `#[rustc_const_stable_indirect]` attribute for that, but that attribute had a dual role -- when used on a regular function, it is an entirely safe marker to make this function part of recursive const stability, but on an intrinsic it is a trusted marker requiring special care. It's not great for the same attribute to be sometimes fully checked and safe, and sometimes trusted and requiring special care, so I split this into two attributes.

This also fixes rust-lang#122652 by accepting intrinsics as const-stable if they have a fallback body that is recursively const-stable.

The library changes are best reviewed with whitespace hidden.

r? `@compiler-errors`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intrinsics Area: Intrinsics C-discussion Category: Discussion or questions that doesn't represent real issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants