Implement clamp_to by Kyuuhachi · Pull Request #150075 · rust-lang/rust

Kyuuhachi · 2025-12-16T22:31:41Z

Implements the revised version of #147781. Supersedes #147786.

Currently I restrict the ClampBounds trait using a second, perma-unstable feature. I don't know if that's the usual way to deal with this kind of traits, I'd be happy to change it if not.

~~I currently define NaN as equal to no bound. This is consistent with max and min, but is inconsistent with clamp, which panics.~~

Changed so that the float versions panic if any bound is NaN, just like clamp does.

rustbot · 2025-12-16T22:31:48Z

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

clarfonthey · 2025-12-17T03:23:53Z

+        fn clamp(self, value: $t) -> $t {
+            let (start, end) = self.into_inner();
+            // Deliberately avoid using `clamp` to handle NaN consistently
+            value.max(start).min(end)


I would be very surprised if x.clamp_to(a..=b) were to behave differently at all from x.clamp(a, b), regardless of the various values for x, a, and b.

Isn't clamp moving toward making its NaN behaviour consistent, anyway?

Yeah but imo it's worse if x.clamp_to(a..) and x.clamp_to(a..=b) handle nan differently. Making all of them panic on nan would be a plausible choice though. It's listed as an unresolved question on the tracking issue.

Looking at the current code, it does appear that clamp simply panics on NaN, so, I would say that it's reasonable to always panic on NaN for these methods to match that behaviour. But I guess we can discuss that in the tracking issue instead of blocking this.

(Side note: after a lot of poking around the changes to clamp and its original RFC, it appears that the panicking behaviour was explicitly decided as the best, and it was not an accident. So, I do think that panicking on NaN bounds like clamp does is the best behaviour.

You're right, if it's named like clamp it should behave like clamp. I'm leaving the question as unresolved, though it's pretty likely that it'll end up this way.

I don't currently have any tests that it does panic on nan; should I add that, and if so where?

I honestly would just look at whatever clamp's tests are and try to match them. Could even use some macros to just run the same tests on both functions to make sure the output matches.

Looks like there's some clamp tests in coretests, but I don't know how that system works. Should I just duplicate all the tests for clamp and adjust them for the different shapes of clamp_to? Or just not do that and leave as-is?

@Kyuuhachi Please do add tests for clamp_to that match the tests for clamp.

clarfonthey · 2025-12-18T23:09:46Z

+            assert!(!self.start.is_nan(), "min was NaN");
+            value.max(self.start)
+        }
+    }
+
+    #[unstable(feature = "clamp_bounds", issue = "147781")]
+    #[rustc_const_unstable(feature = "clamp_bounds", issue = "147781")]
+    impl const ClampBounds<$t> for RangeToInclusive<$t> {
+        fn clamp(self, value: $t) -> $t {
+            assert!(!self.end.is_nan(), "max was NaN");


Minor nit, but since these are ranges, you should probably use the start/end naming instead.

Note that std::range::RangeToInclusive uses the name last to represent an inclusive end instead of end, but since these are just for the regular ops ranges, we should match the actual field name.

These messages are consistent with clamp though. What is more important, consistency with clamp or with ranges?

@Kyuuhachi I'd suggest being consistent with ranges, and later we could potentially modify clamp to be consistent with this.

reddevilmidzy · 2026-03-24T08:25:11Z

@rustbot author

rustbot · 2026-03-24T08:25:17Z

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

Kyuuhachi · 2026-03-25T18:35:45Z

Oops, didn't realize this was still waiting for action from me. Do all the comments need to be resolved or what?

joshtriplett · 2026-05-08T02:46:50Z

@Kyuuhachi I responded in the two threads that remain open. With those two addressed, r=me.

Kyuuhachi · 2026-05-12T21:39:41Z

@bors r=joshtriplett

rust-bors · 2026-05-12T21:39:46Z

@Kyuuhachi: 🔑 Insufficient privileges: not in review users

Kyuuhachi · 2026-05-12T21:42:13Z

@joshtriplett Did I do that wrong?

conradludgate · 2026-05-27T13:42:38Z

Did I do that wrong?

I think you need to:

@rustbot ready

Kyuuhachi · 2026-05-28T07:13:04Z

@rustbot ready

ChrisDenton · 2026-05-28T07:31:31Z

Maybe squish your commits? I'm uncertain if one or two commits would be better it'd be nice to merge some of them at least.

Remove trivial bounds Panic on NaN Make assert messages consistent with field names Add clamp_to coretests Update to fmt style

rustbot · 2026-06-26T17:08:15Z

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.

ChrisDenton · 2026-06-26T20:22:59Z

I don't think the reviewer is currently active. I'll add this to my review queue so if nobody reviews this first I'll do it (I think it was more or less approved before, I just need to take a minute to double check).

r? @ChrisDenton

ChrisDenton · 2026-07-01T12:49:04Z

Thanks!

@bors r=joshtriplett,ChrisDenton

rust-bors · 2026-07-01T12:49:07Z

📌 Commit f31860e has been approved by joshtriplett,ChrisDenton

It is now in the queue for this repository.

rust-bors · 2026-07-01T13:41:40Z

⌛ Testing commit f31860e with merge 1c882cc...

Workflow: https://github.com/rust-lang/rust/actions/runs/28521960546

Implement clamp_to Implements the revised version of #147781. Supersedes #147786. Currently I restrict the ClampBounds trait using a second, perma-unstable feature. I don't know if that's the usual way to deal with this kind of traits, I'd be happy to change it if not. ~~I currently define NaN as equal to no bound. This is consistent with `max` and `min`, but is inconsistent with `clamp`, which panics.~~ Changed so that the float versions panic if any bound is NaN, just like `clamp` does.

…,ChrisDenton Implement clamp_to Implements the revised version of rust-lang#147781. Supersedes rust-lang#147786. Currently I restrict the ClampBounds trait using a second, perma-unstable feature. I don't know if that's the usual way to deal with this kind of traits, I'd be happy to change it if not. ~~I currently define NaN as equal to no bound. This is consistent with `max` and `min`, but is inconsistent with `clamp`, which panics.~~ Changed so that the float versions panic if any bound is NaN, just like `clamp` does.

JonathanBrouwer · 2026-07-01T13:46:07Z

@bors yield
Yielding to enclosing rollup

rust-bors · 2026-07-01T13:46:11Z

Auto build was cancelled. Cancelled workflows:

https://github.com/rust-lang/rust/actions/runs/28521960546

The next pull request likely to be tested is #158660.

…uwer Rollup of 8 pull requests Successful merges: - #150075 (Implement clamp_to) - #158169 (Fix debuginfo compression in bootstrap) - #158397 (delegation: support simplest output `Self` mapping) - #158613 (Fix getrandom fallback test on platforms with `panic=abort`) - #158620 (Remove skip_norm_w/i/p().def_id with a helper) - #158633 (Remove unnecessary `Clone` derives on resolver types) - #158634 (Add missing `needs_drop` check to `DroplessArena`.) - #158647 (Document `strip_circumfix` behavior on overlapping prefix and suffix.)

rust-bors · 2026-07-01T14:14:59Z

⌛ Testing commit f31860e with merge 8cd0b7c...

Workflow: https://github.com/rust-lang/rust/actions/runs/28524103392

Implement clamp_to Implements the revised version of #147781. Supersedes #147786. Currently I restrict the ClampBounds trait using a second, perma-unstable feature. I don't know if that's the usual way to deal with this kind of traits, I'd be happy to change it if not. ~~I currently define NaN as equal to no bound. This is consistent with `max` and `min`, but is inconsistent with `clamp`, which panics.~~ Changed so that the float versions panic if any bound is NaN, just like `clamp` does.

…,ChrisDenton Implement clamp_to Implements the revised version of rust-lang#147781. Supersedes rust-lang#147786. Currently I restrict the ClampBounds trait using a second, perma-unstable feature. I don't know if that's the usual way to deal with this kind of traits, I'd be happy to change it if not. ~~I currently define NaN as equal to no bound. This is consistent with `max` and `min`, but is inconsistent with `clamp`, which panics.~~ Changed so that the float versions panic if any bound is NaN, just like `clamp` does.

JonathanBrouwer · 2026-07-01T14:19:29Z

@bors yield
Yielding to enclosing rollup

rust-bors · 2026-07-01T14:19:34Z

Auto build was cancelled. Cancelled workflows:

https://github.com/rust-lang/rust/actions/runs/28524103392

The next pull request likely to be tested is #158661.

…uwer Rollup of 8 pull requests Successful merges: - #150075 (Implement clamp_to) - #156716 (tests: fix: parallel frontend test failures: different alloc ids) - #158397 (delegation: support simplest output `Self` mapping) - #158613 (Fix getrandom fallback test on platforms with `panic=abort`) - #158620 (Remove skip_norm_w/i/p().def_id with a helper) - #158633 (Remove unnecessary `Clone` derives on resolver types) - #158634 (Add missing `needs_drop` check to `DroplessArena`.) - #158647 (Document `strip_circumfix` behavior on overlapping prefix and suffix.)

JonathanBrouwer · 2026-07-01T15:44:17Z

💔 I suspect this PR failed tests as part of a rollup
@bors r-

After fixing the problem, consider running a try job for the failed job before re-approving.

Link to failure: #158661 (comment)

rust-bors · 2026-07-01T15:45:12Z

This PR was contained in a rollup (#158661), which was unapproved.

View changes since this unapproval

clarfonthey · 2026-07-01T16:35:23Z

The output is weird, but it appears that cfg(target_has_reliable_f128) didn't always mean the maximum intrinsic was available? Otherwise, I don't understand why this would happen:

---- library/core/src/num/f128.rs - f128::f128::clamp_to (line 1475) stdout ----
unexpected runtime library name: fmaximum_numl
UNREACHABLE executed at /checkout/src/llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp:952!
Couldn't compile the test.
\ (no newline at end of output)

Kyuuhachi · 2026-07-01T16:56:44Z

That's a fun one. Any idea what to do about it? Could always use the naive if a > b { a } else { b } I guess...

ChrisDenton · 2026-07-01T18:21:01Z

Seeing as it's wasm32-wasip1 and has_reliable_f128 defaults to true, my first thought is maybe the target spec is wrong? cc @alexcrichton

alexcrichton · 2026-07-01T18:27:46Z

I wouldn't be surprised if f128 isn't well supported on wasm, and it looks like wasm is hitting the fall-through here and I think it'd be reasonable to disable that for wasm.

tgross35 · 2026-07-01T22:56:55Z

+    /// ```
+    /// #![feature(f128, clamp_to)]
+    /// assert_eq!((-3.0f128).clamp_to(-2.0..=1.0), -2.0);
+    /// assert_eq!(0.0f128.clamp_to(-2.0..=1.0), 0.0);
+    /// assert_eq!(2.0f128.clamp_to(..=1.0), 1.0);
+    /// assert_eq!(5.0f128.clamp_to(7.0..), 7.0);
+    /// assert!(f128::NAN.clamp_to(1.0..=2.0).is_nan());
+    /// ```


Requires a gate behind #[cfg(target_has_reliable_f128_math)] (see the above tests, excluding clamp_magnitude which apparently hasn't gotten updated)

View changes since the review

tgross35 · 2026-07-01T22:57:42Z

+    /// #![feature(f16, clamp_to)]
+    /// assert_eq!((-3.0f16).clamp_to(-2.0..=1.0), -2.0);
+    /// assert_eq!(0.0f16.clamp_to(-2.0..=1.0), 0.0);
+    /// assert_eq!(2.0f16.clamp_to(..=1.0), 1.0);
+    /// assert_eq!(5.0f16.clamp_to(7.0..), 7.0);
+    /// assert!(f16::NAN.clamp_to(1.0..=2.0).is_nan());


Similar with #[cfg(target_has_reliable_f16_math)]

View changes since the review

tgross35 · 2026-07-01T23:24:17Z

+float_test! {
+    name: clamp_to_min_greater_than_max,
+    attrs: {
+        const: #[cfg(false)],
+        f16: #[should_panic, cfg(target_has_reliable_f16)],
+        f32: #[should_panic],
+        f64: #[should_panic],
+        f128: #[should_panic, cfg(target_has_reliable_f128)],
+    },
+    test {
+        let _ = Float::ONE.clamp_to(3.0..=1.0);
+    }
+}
+
+float_test! {
+    name: clamp_to_min_is_nan,
+    attrs: {
+        const: #[cfg(false)],
+        f16: #[should_panic, cfg(target_has_reliable_f16)],
+        f32: #[should_panic],
+        f64: #[should_panic],
+        f128: #[should_panic, cfg(target_has_reliable_f128)],
+    },
+    test {
+        let _ = Float::ONE.clamp_to(Float::NAN..=1.0);
+    }
+}
+
+float_test! {
+    name: clamp_to_max_is_nan,
+    attrs: {
+        const: #[cfg(false)],
+        f16: #[should_panic, cfg(target_has_reliable_f16)],
+        f32: #[should_panic],
+        f64: #[should_panic],
+        f128: #[should_panic, cfg(target_has_reliable_f128)],
+    },
+    test {
+        let _ = Float::ONE.clamp_to(3.0..=Float::NAN);
+    }
+}


Since these require libcalls, they need to be gated under target_has_reliable_fX_math rather than the non-_math version.

View changes since the review

tgross35 · 2026-07-01T23:28:53Z

I wouldn't be surprised if f128 isn't well supported on wasm, and it looks like wasm is hitting the fall-through here and I think it'd be reasonable to disable that for wasm.

It is supported reasonably well on wasm, but everything with LLVM and f128 libm functions is borked right now on all platforms except in specific circumstances (basically only glibc) llvm/llvm-project#44744. The target_has_reliable_f128_math gate is meant to guard this, as opposed to target_has_reliable_f128 for the basic functionality which works well.

rustbot assigned ibraheemdev Dec 16, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 16, 2025

This was referenced Dec 16, 2025

Implement clamp_min and clamp_max #147786

Closed

Tracking Issue for clamp_to #147781

Open

clarfonthey reviewed Dec 17, 2025

View reviewed changes

danielhenrymantilla reviewed Dec 18, 2025

View reviewed changes

Comment thread library/core/src/num/f64.rs Outdated

Comment thread library/core/src/num/f32.rs Outdated

Comment thread library/core/src/num/f16.rs Outdated

Comment thread library/core/src/num/f128.rs Outdated

clarfonthey reviewed Dec 18, 2025

View reviewed changes

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 Mar 24, 2026

This comment has been minimized.

Sign in to view

Kyuuhachi force-pushed the limit_to branch from 2229986 to 8786b0d Compare May 12, 2026 21:14

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 27, 2026

Implement clamp_to

f31860e

Remove trivial bounds Panic on NaN Make assert messages consistent with field names Add clamp_to coretests Update to fmt style

Kyuuhachi force-pushed the limit_to branch from 8786b0d to f31860e Compare June 26, 2026 17:08

rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 1, 2026

JonathanBrouwer mentioned this pull request Jul 1, 2026

Rollup of 8 pull requests #158660

Closed

JonathanBrouwer mentioned this pull request Jul 1, 2026

Rollup of 8 pull requests #158661

Closed

rust-bors Bot 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 1, 2026

This was referenced Jul 1, 2026

Disable f128 support on wasm #158672

Closed

f128 on wasm does not support maximum intrinsic #158673

Open

tgross35 reviewed Jul 1, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

Kyuuhachi commented Dec 16, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Dec 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reddevilmidzy commented Mar 24, 2026

Uh oh!

rustbot commented Mar 24, 2026

Uh oh!

Kyuuhachi commented Mar 25, 2026

Uh oh!

joshtriplett commented May 8, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

Kyuuhachi commented May 12, 2026

Uh oh!

rust-bors Bot commented May 12, 2026

Uh oh!

Kyuuhachi commented May 12, 2026

Uh oh!

conradludgate commented May 27, 2026

Uh oh!

Kyuuhachi commented May 28, 2026

Uh oh!

ChrisDenton commented May 28, 2026

Uh oh!

rustbot commented Jun 26, 2026

Uh oh!

ChrisDenton commented Jun 26, 2026

Uh oh!

ChrisDenton commented Jul 1, 2026

Uh oh!

rust-bors Bot commented Jul 1, 2026

Uh oh!

rust-bors Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonathanBrouwer commented Jul 1, 2026

Uh oh!

rust-bors Bot commented Jul 1, 2026

Uh oh!

rust-bors Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JonathanBrouwer commented Jul 1, 2026

Uh oh!

rust-bors Bot commented Jul 1, 2026

Uh oh!

Kyuuhachi commented Dec 16, 2025 •

edited by rustbot

Loading

clarfonthey Dec 18, 2025 •

edited

Loading

rust-bors Bot commented Jul 1, 2026 •

edited

Loading

rust-bors Bot commented Jul 1, 2026 •

edited

Loading

tgross35 Jul 1, 2026 •

edited by rustbot

Loading

tgross35 Jul 1, 2026 •

edited by rustbot

Loading

tgross35 Jul 1, 2026 •

edited by rustbot

Loading