Improve `Ord` violation help #128273

Voultapher · 2024-07-27T14:54:08Z

Recent experience in #128083 showed that the panic message when an Ord violation is detected by the new sort implementations can be confusing. So this PR aims to improve it, together with minor bug fixes in the doc comments for sort*, sort_unstable* and select_nth_unstable*.

Is it possible to get these changes into the 1.81 release? It doesn't change behavior and would greatly help when users encounter this panic for the first time, which they may after upgrading to 1.81.

Tagging @orlp

The new sort implementations have the ability to detect Ord violations in many cases. This commit improves the message in a way that should help users realize what went wrong in their program.

…ble* - Move panic information into # Panics section - Fix mentions of T: Ord that should be compare - Add missing information

rustbot · 2024-07-27T14:54:16Z

r? @joboet

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

library/core/src/slice/sort/shared/smallsort.rs

library/core/src/slice/mod.rs

tgross35 · 2024-07-27T18:37:17Z

Thanks for the followup, this will be great to have.

For all the uses of "total order", could we link somewhere? Either the Wikipedia page, or link somewhere on the Ord page.

Maybe Ord should just get a "total order" section that shows an example incorrect implementation since it's not always easy to compare code to equations.

workingjubilee

I already anticipate someone is going to read this note and say "but that can't happen because K is Ord and Ord is a total order". This should be clearer about that case.

library/alloc/src/slice.rs

workingjubilee

Sorry, noticed some typos also.

library/core/src/slice/sort/shared/smallsort.rs

Voultapher · 2024-07-28T08:50:41Z

Thanks for the followup, this will be great to have.

For all the uses of "total order", could we link somewhere? Either the Wikipedia page, or link somewhere on the Ord page.

Maybe Ord should just get a "total order" section that shows an example incorrect implementation since it's not always easy to compare code to equations.

Good idea, currently the Ord docs links to wikipedia, we could do the same. That said doing so might seem a little intimidating to users unfamiliar with the matter. I like the idea of having concrete code that demonstrates the ideas at play.

- Use if the implementation of [`Ord`] for `T` language - Link to total order wiki page - Rework total order help and examples - Improve language to be more precise and less prone to misunderstandings. - Fix usage of `sort_unstable_by` in `sort_by` example - Fix missing author mention - Use more consistent example input for sort - Use more idiomatic assert_eq! in examples - Use more natural "comparison function" language instead of "comparator function"

Voultapher · 2024-08-01T15:41:16Z

Changes since last review:

Use if the implementation of [Ord] for T
language
Link to total order wiki page
Rework total order help and examples
Improve language to be more precise and less
prone to misunderstandings.
Fix usage of sort_unstable_by in sort_by
example
Fix missing author mention
Use more consistent example input for sort
Use more idiomatic assert_eq! in examples
Use more natural "comparison function" language
instead of "comparator function"
Hide sort module

Voultapher · 2024-08-01T15:42:15Z

There are something like 10 places that document sort usage, so chances are I missed something, please carefully review for typos and mistakes.

Voultapher · 2024-08-01T15:45:14Z

I've also started making changes to the Ord docs to better help people understand how to correctly derive and implement them, but that has grown out of scope and I plan on creating a separate PR for it. The sort docs link people to relevant information and surface some of it where sensible, but it's a tricky balance between too little information and linking to the relevant places and overloading and duplicating information in the sort docs. I hope the current iteration strikes a decent balance between the two.

workingjubilee

This looks good. I noticed some remaining nits. They're just typos or non-blocking concerns because the original wording here was signed-off-on by libs already and they don't necessarily relate directly to the concern of the "total order" requirement.

library/alloc/src/slice.rs

library/core/src/slice/mod.rs

Voultapher · 2024-08-03T13:11:11Z

@workingjubilee thanks for the detailed review so far.

workingjubilee · 2024-08-05T23:32:21Z

Okie dokie! I think that everything else is stuff that we can defer to later. Thank you everyone.

@bors r+

bors · 2024-08-05T23:32:23Z

📌 Commit 613155c has been approved by workingjubilee

It is now in the queue for this repository.

…elp, r=workingjubilee Improve `Ord` violation help Recent experience in rust-lang#128083 showed that the panic message when an Ord violation is detected by the new sort implementations can be confusing. So this PR aims to improve it, together with minor bug fixes in the doc comments for sort*, sort_unstable* and select_nth_unstable*. Is it possible to get these changes into the 1.81 release? It doesn't change behavior and would greatly help when users encounter this panic for the first time, which they may after upgrading to 1.81. Tagging `@orlp`

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#122792 (Stabilize `min_exhaustive_patterns`) - rust-lang#124944 (On trait bound mismatch, detect multiple crate versions in dep tree) - rust-lang#128273 (Improve `Ord` violation help) - rust-lang#128406 (implement BufReader::peek) - rust-lang#128539 (Forbid unused unsafe in vxworks-specific std modules) - rust-lang#128692 (Add a triagebot mention for `library/Cargo.lock`) - rust-lang#128710 (Don't ICE when getting an input file name's stem fails) r? `@ghost` `@rustbot` modify labels: rollup

…elp, r=workingjubilee Improve `Ord` violation help Recent experience in rust-lang#128083 showed that the panic message when an Ord violation is detected by the new sort implementations can be confusing. So this PR aims to improve it, together with minor bug fixes in the doc comments for sort*, sort_unstable* and select_nth_unstable*. Is it possible to get these changes into the 1.81 release? It doesn't change behavior and would greatly help when users encounter this panic for the first time, which they may after upgrading to 1.81. Tagging ``@orlp``

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#125048 (PinCoerceUnsized trait into core) - rust-lang#128273 (Improve `Ord` violation help) - rust-lang#128406 (implement BufReader::peek) - rust-lang#128539 (Forbid unused unsafe in vxworks-specific std modules) - rust-lang#128687 (interpret: refactor function call handling to be better-abstracted) - rust-lang#128692 (Add a triagebot mention for `library/Cargo.lock`) - rust-lang#128710 (Don't ICE when getting an input file name's stem fails) - rust-lang#128718 (Consider `cfg_attr` checked by `CheckAttrVisitor`) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2024-08-06T22:32:59Z

@bors r-
guess this failed here: #128739 (comment)

Voultapher · 2024-08-09T13:06:58Z

While I can't reproduce the issue with ./x.py --stage 0 test linkchecker html-checker I made small tweak based on the error message that should hopefully fix the issue.

workingjubilee · 2024-08-09T19:00:11Z

@bors r+

bors · 2024-08-09T19:00:14Z

📌 Commit 1be60b5 has been approved by workingjubilee

It is now in the queue for this repository.

…llaumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#128273 (Improve `Ord` violation help) - rust-lang#128807 (run-make: explaing why fmt-write-bloat is ignore-windows) - rust-lang#128903 (rustdoc-json-types `Discriminant`: fix typo) - rust-lang#128905 (gitignore: Add Zed and Helix editors) - rust-lang#128908 (diagnostics: do not warn when a lifetime bound infers itself) - rust-lang#128909 (Fix dump-ice-to-disk for RUSTC_ICE=0 users) - rust-lang#128910 (Differentiate between methods and associated functions in diagnostics) - rust-lang#128923 ([rustdoc] Stop showing impl items for negative impls) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#128273 - Voultapher:improve-ord-violation-help, r=workingjubilee Improve `Ord` violation help Recent experience in rust-lang#128083 showed that the panic message when an Ord violation is detected by the new sort implementations can be confusing. So this PR aims to improve it, together with minor bug fixes in the doc comments for sort*, sort_unstable* and select_nth_unstable*. Is it possible to get these changes into the 1.81 release? It doesn't change behavior and would greatly help when users encounter this panic for the first time, which they may after upgrading to 1.81. Tagging `@orlp`

Voultapher · 2024-08-12T11:54:34Z

I see this PR hasn't made its way into the beta yet, what is needed for that?

ChrisDenton · 2024-08-12T12:03:27Z

Patience. Merging into beta is a manual process. This PR is labelled as beta-accepted so it'll be merged (possibly along with other beta-accepted PRs) the next time someone does the backports.

[beta] backports and bump stage0 - Disable jump threading of float equality rust-lang#128271 - Normalize when equating `dyn` tails in MIR borrowck rust-lang#128694 - Improve `Ord` violation help rust-lang#128273 - bump stage0 to stable 1.80.1 - Revert rust-lang#125915 on beta rust-lang#128760 - derive(SmartPointer): register helper attributes rust-lang#128925 - Fix bug in `Parser::look_ahead`. rust-lang#128994 r? cuviper

Voultapher added 2 commits July 27, 2024 16:47

Improve panic message and surrounding documentation for Ord violations

644550f

The new sort implementations have the ability to detect Ord violations in many cases. This commit improves the message in a way that should help users realize what went wrong in their program.

Improve panic sections for sort*, sort_unstable* and select_nth_unsta…

00ce238

…ble* - Move panic information into # Panics section - Fix mentions of T: Ord that should be compare - Add missing information

Voultapher changed the title ~~Improve ord violation help~~ Improve Ord violation help Jul 27, 2024

rustbot assigned joboet Jul 27, 2024

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 Jul 27, 2024

compiler-errors added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 27, 2024

tgross35 reviewed Jul 27, 2024

View reviewed changes

library/core/src/slice/sort/shared/smallsort.rs Show resolved Hide resolved

tgross35 reviewed Jul 27, 2024

View reviewed changes

library/core/src/slice/mod.rs Show resolved Hide resolved

workingjubilee reviewed Jul 27, 2024

View reviewed changes

library/alloc/src/slice.rs Outdated Show resolved Hide resolved

workingjubilee requested changes Jul 27, 2024

View reviewed changes

Voultapher changed the title ~~Improve Ord violation help~~ Improve Ord violation help Jul 31, 2024

Amanieu added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jul 31, 2024

Hide internal sort module

eae7a18

Voultapher requested review from workingjubilee and tgross35 August 1, 2024 15:48

workingjubilee reviewed Aug 1, 2024

View reviewed changes

joboet assigned workingjubilee and unassigned joboet Aug 1, 2024

Apply review comments to PartialOrd section

613155c

matthiaskrgr mentioned this pull request Aug 6, 2024

Rollup of 7 pull requests #128719

Closed

matthiaskrgr mentioned this pull request Aug 6, 2024

Rollup of 8 pull requests #128739

Closed

bors 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 Aug 6, 2024

Fix linkchecker issue

1be60b5

bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 9, 2024

GuillaumeGomez mentioned this pull request Aug 10, 2024

Rollup of 8 pull requests #128927

Merged

bors merged commit 65875b2 into rust-lang:master Aug 10, 2024
6 checks passed

rustbot added this to the 1.82.0 milestone Aug 10, 2024

Voultapher deleted the improve-ord-violation-help branch August 12, 2024 12:35

cuviper mentioned this pull request Aug 13, 2024

[beta] backports and bump stage0 #129033

Merged

cuviper modified the milestones: 1.82.0, 1.81.0 Aug 13, 2024

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 13, 2024

Voultapher mentioned this pull request Sep 6, 2024

New panics from sort detecting Ord/PartialOrd violations #129561

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `Ord` violation help #128273

Improve `Ord` violation help #128273

Voultapher commented Jul 27, 2024

rustbot commented Jul 27, 2024

tgross35 commented Jul 27, 2024 •

edited

Loading

workingjubilee left a comment

workingjubilee left a comment

Voultapher commented Jul 28, 2024

Voultapher commented Aug 1, 2024

Voultapher commented Aug 1, 2024 •

edited

Loading

Voultapher commented Aug 1, 2024 •

edited

Loading

workingjubilee left a comment

Voultapher commented Aug 3, 2024

workingjubilee commented Aug 5, 2024

bors commented Aug 5, 2024

matthiaskrgr commented Aug 6, 2024

Voultapher commented Aug 9, 2024

workingjubilee commented Aug 9, 2024

bors commented Aug 9, 2024

Voultapher commented Aug 12, 2024

ChrisDenton commented Aug 12, 2024

Improve Ord violation help #128273

Improve Ord violation help #128273

Conversation

Voultapher commented Jul 27, 2024

rustbot commented Jul 27, 2024

tgross35 commented Jul 27, 2024 • edited Loading

workingjubilee left a comment

Choose a reason for hiding this comment

workingjubilee left a comment

Choose a reason for hiding this comment

Voultapher commented Jul 28, 2024

Voultapher commented Aug 1, 2024

Voultapher commented Aug 1, 2024 • edited Loading

Voultapher commented Aug 1, 2024 • edited Loading

workingjubilee left a comment

Choose a reason for hiding this comment

Voultapher commented Aug 3, 2024

workingjubilee commented Aug 5, 2024

bors commented Aug 5, 2024

matthiaskrgr commented Aug 6, 2024

Voultapher commented Aug 9, 2024

workingjubilee commented Aug 9, 2024

bors commented Aug 9, 2024

Voultapher commented Aug 12, 2024

ChrisDenton commented Aug 12, 2024

Improve `Ord` violation help #128273

Improve `Ord` violation help #128273

tgross35 commented Jul 27, 2024 •

edited

Loading

Voultapher commented Aug 1, 2024 •

edited

Loading

Voultapher commented Aug 1, 2024 •

edited

Loading