Skip to content

Stabilize funnel_shifts (including const) - #161015

Open
tgross35 wants to merge 1 commit into
rust-lang:mainfrom
tgross35:stabilize-funnel-shifts
Open

Stabilize funnel_shifts (including const)#161015
tgross35 wants to merge 1 commit into
rust-lang:mainfrom
tgross35:stabilize-funnel-shifts

Conversation

@tgross35

@tgross35 tgross35 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

funnel_shl and funnel_shr have been around for close to a year, the unchecked versions for a number of months. These are reasonably small and uncontroversial, and it can be tricky to get similar performance with a fallback; stabilize them here.

Newly stable API:

impl {u8, u16, u32, u64, u128, usize} {
    pub const fn funnel_shl(self, right: Self, shift: u32) -> Self;
    pub const fn funnel_shr(self, right: Self, shift: u32) -> Self;

    pub const unsafe fn unchecked_funnel_shl(self, right: Self, shift: u32) -> Self;
    pub const unsafe fn unchecked_funnel_shr(self, right: Self, shift: u32) -> Self;
}

The tracking issue also mentions a wrapping_ version but it has not been implemented.

Closes: #145686 (tracking issue, wrapping versions will need a new issue)

This involves an intrinsic stabilization so needs FCP from both @rust-lang/lang and @rust-lang/libs-api.

@rustbot label +I-lang-nominated +I-libs-api-nominated

Cc others who have been involved: @RalfJung, @folkertdev, @sayantn

r? libs

@rustbot

rustbot commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

⚠️ #[rustc_allow_const_fn_unstable] needs careful audit to avoid accidentally exposing unstable
implementation details on stable.

cc @rust-lang/wg-const-eval

⚠️ #[rustc_intrinsic_const_stable_indirect] controls whether intrinsics can be exposed to stable const
code; adding it needs t-lang approval.

cc @rust-lang/wg-const-eval

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

stdarch is developed in its own repository. If possible, consider making this change to rust-lang/stdarch instead.

cc @Amanieu, @folkertdev, @sayantn

@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 Aug 13, 2026
@rustbot rustbot added I-lang-nominated Nominated for discussion during a lang team meeting. I-libs-api-nominated [DEPRECATED; DO NOT USE] labels Aug 13, 2026
@tgross35 tgross35 added the needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. label Aug 13, 2026
Comment on lines +2327 to 2333
#[rustc_intrinsic_const_stable_indirect]
#[rustc_intrinsic]
#[rustc_nounwind]
#[rustc_const_unstable(feature = "funnel_shifts", issue = "145686")]
#[unstable(feature = "funnel_shifts", issue = "145686")]
#[track_caller]
#[rustc_allow_const_fn_unstable(const_trait_impl, core_intrinsics_fallbacks)]
#[miri::intrinsic_fallback_is_spec]
pub const unsafe fn unchecked_funnel_shl<T: [const] fallback::FunnelShift>(

@tgross35 tgross35 Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that const_trait_impl is only needed for the fallback. We could implement a version without it if needed.

The fallback traits are gated behind core_intrinsics_fallbacks, I'm not sure whether there's a cleaner way to do this.

View changes since the review

@nia-e nia-e added I-libs-nominated Nominated for discussion during a libs team meeting. and removed I-libs-api-nominated [DEPRECATED; DO NOT USE] labels Aug 13, 2026
@nia-e

This comment was marked as off-topic.

@tgross35

This comment was marked as off-topic.

@rust-bors

This comment has been minimized.

@tgross35
tgross35 force-pushed the stabilize-funnel-shifts branch from cf9c57e to 11cd98d Compare August 13, 2026 08:46
@rustbot

rustbot commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

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.

`funnel_shl` and `funnel_shr` have been around for close to a year, the
unchecked versions for a number of months. These are reasonably small
and uncontroversial, and it can be tricky to get similar performance
with a fallback; stabilize them here.

Newly stable API:

    impl {u8, u16, u32, u64, u128, usize} {
        pub const fn funnel_shl(self, right: Self, shift: u32) -> Self;
        pub const fn funnel_shr(self, right: Self, shift: u32) -> Self;

        pub const unsafe fn unchecked_funnel_shl(self, right: Self, shift: u32) -> Self;
        pub const unsafe fn unchecked_funnel_shr(self, right: Self, shift: u32) -> Self;
    }

The tracking issue also mentions a `wrapping_` version but it has not
been implemented.

Tracking issue: RUST-145686
@tgross35
tgross35 force-pushed the stabilize-funnel-shifts branch from 11cd98d to c21507f Compare August 13, 2026 08:51
/// This operation can be thought of as concatenating `self` and `right` into an
/// integer twice the size of
#[doc = concat!("`", stringify!($SelfT) , "`,")]
/// performing a right shift by `n`, and returning the **left half** of the result.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should be "right half", right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right; #161065

@traviscross traviscross added P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang T-lang Relevant to the language team labels Aug 13, 2026
@traviscross

Copy link
Copy Markdown
Contributor

Thanks @tgross35.

@rfcbot fcp merge lang,libs

@rust-rfcbot

rust-rfcbot commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

@traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rust-rfcbot rust-rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. and removed needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. labels Aug 13, 2026
@scottmcm

scottmcm commented Aug 13, 2026

Copy link
Copy Markdown
Member

Zero lang concerns; this is a value operation with fallback MIR showing it can be implemented with existing rust.

@rfcbot reviewed

@scottmcm

Copy link
Copy Markdown
Member

A concern for the libs(-api) part: Is this actually guaranteed to panic for out-of-range shifts?

It feels inconsistent with other things. I'd expect that funnel_shift would wrap in release and panic in debug, and that there'd be a strict_funnel_shift that would always panic for out-of-range shifts.


And it's a bit weird to not have the wrapping version of this. It'd be trivial to implement -- just mask the shift passed to unchecked -- so why not just add it before stabilizing the set?

@tgross35

Copy link
Copy Markdown
Contributor Author

Wrapping by default was the original proposal but was decided against, see rust-lang/libs-team#642 (comment).

The wrapping_ version was accepted as part of that ACP, but it never got added. I don't think we'd want to stabilize it right away but also don't want it to block these any longer since they're ready and useful.

@pthariensflame

Copy link
Copy Markdown
Contributor

We’ll implement the wrapping versions, independently of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. I-lang-nominated Nominated for discussion during a lang team meeting. I-libs-nominated Nominated for discussion during a libs team meeting. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. 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-lang Relevant to the language team 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.

Tracking Issue for Integer Funnel Shifts

8 participants