Improve error message for std integer clamp() if min > max

[Dan54]

For #142309: change the error message for Ord::clamp() for std integer types if min > max to be more useful.

Message is now min > max. min = {min:?}, max = {max:?}

Also add #[track_caller] to clamp()

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
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

[hkBst]

I think you should instead just improve the default implementation of clamp to have a better error message...

View changes since this review

[Dan54]

The nicer error message requires Self: Debug, which is not required by Ord. Adding the trait bound would be a breaking change.

[scottmcm]

r? libs

[Mark-Simulacrum]

@bors r+

I think this seems reasonable. I suspect that in 90% of cases just adding #[track_caller] to Ord::clamp would already produce the desired improvement in the case that the values are constants - which at least in my experience is common - but I think the debugging is probably minimally impactful.

[bors]

📌 Commit 6dbea1f has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[bors]

⌛ Testing commit 6dbea1f with merge 5549523...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 5549523 to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing ba2142a (parent) -> 5549523 (this PR)

Test differences

No test diffs found

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 554952348a7dd13851f25789f6bb1061f45c4b60 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-aarch64-apple: 6324.6s -> 7240.4s (+14.5%)
2. x86_64-gnu-llvm-20-2: 5348.3s -> 6114.4s (+14.3%)
3. dist-x86_64-apple: 7314.8s -> 8181.4s (+11.8%)
4. tidy: 178.1s -> 159.0s (-10.7%)
5. x86_64-gnu-tools: 3348.4s -> 3667.3s (+9.5%)
6. dist-various-2: 2313.1s -> 2107.8s (-8.9%)
7. dist-x86_64-netbsd: 4586.3s -> 4983.6s (+8.7%)
8. aarch64-msvc-1: 7076.2s -> 6476.1s (-8.5%)
9. x86_64-gnu: 6710.3s -> 7215.4s (+7.5%)
10. dist-aarch64-llvm-mingw: 6210.8s -> 5777.0s (-7.0%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (5549523): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[0.6%, 0.6%]	1

Max RSS (memory usage)

Results (secondary -1.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.3%	[1.3%, 1.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.8%, -2.5%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 2.9%, secondary -2.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.9%	[2.3%, 3.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	2.9%	[2.3%, 3.6%]	2

Binary size

Results (primary 1.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.4%	[1.4%, 1.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.4%	[1.4%, 1.4%]	1

Bootstrap: 470.113s -> 471.938s (0.39%)
Artifact size: 388.98 MiB -> 388.98 MiB (0.00%)

[zmodem]

We're hitting build errors in Chromium after this change (https://crbug.com/467060286):

error: `compiler_builtins` cannot call functions through upstream monomorphizations; encountered invalid call from `core::fmt::num::<impl core::fmt::Debug for i32>::fmt` to `core::fmt::num::imp::<impl core::fmt::Display for i32>::fmt`
 --> ./../../third_party/rust-toolchain/lib/rustlib/src/rust/library/core/src/fmt/num.rs:79:0
 ::: ./../../third_party/rust-toolchain/lib/rustlib/src/rust/library/core/src/fmt/num.rs:97:1
  |
  = note: in this expansion of `impl_Debug!`
 ::: ./../../third_party/rust-toolchain/lib/rustlib/src/rust/library/core/src/fmt/num.rs:592:1
  |
  = note: in this macro invocation

Specifically caused by the const_assert! added in this change, though I don't really understand the chain of events that lead to the error.

My understanding is that "error: compiler_builtins cannot call functions through upstream monomorphizations" means compiler_builtins is not allowed to call (non-inlined) functions outside the library, and after this change we somehow end up doing that.

The failure seems sensitive to what flags we pass, like -Cpanic=immediate-abort or not.

Does this error make sense to anyone?

[Dan54]

A search of the rust-lang/compiler-builtins repository shows two calls to clamp, both in scalbn.rs (permalink). I assume this is the cause of the error.

[zmodem]

Thanks! Do you have any ideas for how to fix this? Could we somehow make the const_assert! not apply when building compiler-builtins, or at least not do the formatting in that setting?

[nico]

Looks like rust-lang/compiler-builtins#1047 will fix things.

[Dan54]

@zmodem this should have been fixed (it was rolled up in commit 58b270b, which would have been included in today's nightly), is it still an issue?