Skip to content

Fix Ord violations in rustdoc items sorting #128139

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix Ord violations in rustdoc items sorting #128139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 39 additions & 42 deletions src/librustdoc/html/render/print_item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,8 @@ trait ItemTemplate<'a, 'cx: 'a>: rinja::Template + fmt::Display {
fn item_module(w: &mut Buffer, cx: &mut Context<'_>, item: &clean::Item, items: &[clean::Item]) {
write!(w, "{}", document(cx, item, None, HeadingOffset::H2));

let mut indices = (0..items.len()).filter(|i| !items[*i].is_stripped()).collect::<Vec<usize>>();
let mut not_stripped_items =
items.iter().filter(|i| !i.is_stripped()).enumerate().collect::<Vec<_>>();

// the order of item types in the listing
fn reorder(ty: ItemType) -> u8 {
Expand All @@ -338,37 +339,30 @@ fn item_module(w: &mut Buffer, cx: &mut Context<'_>, item: &clean::Item, items:
}
}

fn cmp(
i1: &clean::Item,
i2: &clean::Item,
idx1: usize,
idx2: usize,
tcx: TyCtxt<'_>,
) -> Ordering {
fn cmp(i1: &clean::Item, i2: &clean::Item, tcx: TyCtxt<'_>) -> Ordering {
let ty1 = i1.type_();
let ty2 = i2.type_();
if item_ty_to_section(ty1) != item_ty_to_section(ty2)
|| (ty1 != ty2 && (ty1 == ItemType::ExternCrate || ty2 == ItemType::ExternCrate))
{
return (reorder(ty1), idx1).cmp(&(reorder(ty2), idx2));
}
let s1 = i1.stability(tcx).as_ref().map(|s| s.level);
let s2 = i2.stability(tcx).as_ref().map(|s| s.level);
if let (Some(a), Some(b)) = (s1, s2) {
match (a.is_stable(), b.is_stable()) {
(true, true) | (false, false) => {}
(false, true) => return Ordering::Greater,
(true, false) => return Ordering::Less,
if ty1 != ty2 {
return reorder(ty1).cmp(&reorder(ty2));
}
let is_stable1 = i1.stability(tcx).as_ref().map(|s| s.level.is_stable()).unwrap_or(true);
let is_stable2 = i2.stability(tcx).as_ref().map(|s| s.level.is_stable()).unwrap_or(true);
if is_stable1 != is_stable2 {
if !is_stable1 {
return Ordering::Greater;
}
return Ordering::Less;
}
let lhs = i1.name.unwrap_or(kw::Empty);
let rhs = i2.name.unwrap_or(kw::Empty);
compare_names(lhs.as_str(), rhs.as_str())
}

let tcx = cx.tcx();

match cx.shared.module_sorting {
ModuleSorting::Alphabetical => {
indices.sort_by(|&i1, &i2| cmp(&items[i1], &items[i2], i1, i2, cx.tcx()));
not_stripped_items.sort_by(|(_, i1), (_, i2)| cmp(i1, i2, tcx));
}
ModuleSorting::DeclarationOrder => {}
}
Expand All @@ -391,24 +385,19 @@ fn item_module(w: &mut Buffer, cx: &mut Context<'_>, item: &clean::Item, items:
// can be identical even if the elements are different (mostly in imports).
// So in case this is an import, we keep everything by adding a "unique id"
// (which is the position in the vector).
indices.dedup_by_key(|i| {
not_stripped_items.dedup_by_key(|(idx, i)| {
(
items[*i].item_id,
if items[*i].name.is_some() { Some(full_path(cx, &items[*i])) } else { None },
items[*i].type_(),
if items[*i].is_import() { *i } else { 0 },
i.item_id,
if i.name.is_some() { Some(full_path(cx, i)) } else { None },
i.type_(),
if i.is_import() { *idx } else { 0 },
)
});

debug!("{indices:?}");
debug!("{not_stripped_items:?}");
let mut last_section = None;

for &idx in &indices {
let myitem = &items[idx];
if myitem.is_stripped() {
continue;
}

for (_, myitem) in &not_stripped_items {
let my_section = item_ty_to_section(myitem.type_());
if Some(my_section) != last_section {
if last_section.is_some() {
Expand All @@ -424,7 +413,6 @@ fn item_module(w: &mut Buffer, cx: &mut Context<'_>, item: &clean::Item, items:
);
}

let tcx = cx.tcx();
match *myitem.kind {
clean::ExternCrateItem { ref src } => {
use crate::html::format::anchor;
Expand Down Expand Up @@ -453,7 +441,7 @@ fn item_module(w: &mut Buffer, cx: &mut Context<'_>, item: &clean::Item, items:
let stab_tags = if let Some(import_def_id) = import.source.did {
// Just need an item with the correct def_id and attrs
let import_item =
clean::Item { item_id: import_def_id.into(), ..myitem.clone() };
clean::Item { item_id: import_def_id.into(), ..(*myitem).clone() };

let stab_tags = Some(extra_info_tags(&import_item, item, tcx).to_string());
stab_tags
Expand Down Expand Up @@ -2026,19 +2014,28 @@ pub(crate) fn compare_names(mut lhs: &str, mut rhs: &str) -> Ordering {
let (ra, rb) = take_parts(&mut rhs);
// First process the non-numeric part.
match la.cmp(ra) {
Ordering::Equal => (),
Ordering::Equal => {}
x => return x,
}
match (lb.is_empty(), rb.is_empty()) {
(true, false) => return Ordering::Less,
(false, true) => return Ordering::Greater,
(true, true) => continue,
(false, false) => {}
}
// Then process the numeric part, if both sides have one (and they fit in a u64).
if let (Ok(ln), Ok(rn)) = (lb.parse::<u64>(), rb.parse::<u64>()) {
match ln.cmp(&rn) {
Ordering::Equal => (),
match (lb.parse::<u64>(), rb.parse::<u64>()) {
(Err(_), Err(_)) => return Ordering::Equal,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this result, either.

Let's just use the same function the style-guide recommends for rustfmt, unless there's a compelling reason not to.

(Ok(_), Err(_)) => return Ordering::Less,
(Err(_), Ok(_)) => return Ordering::Greater,
(Ok(nb1), Ok(nb2)) => match nb1.cmp(&nb2) {
Ordering::Equal => {}
x => return x,
}
},
}
// Then process the numeric part again, but this time as strings.
// If both numbers are equal, then we compare the strings.
match lb.cmp(rb) {
Ordering::Equal => (),
Ordering::Equal => {}
x => return x,
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/html/render/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ fn test_compare_names() {
("hello", "world"),
("", "world"),
("123", "hello"),
("123", ""),
// ("123", ""),
("123test", "123"),
("hello", ""),
("hello", "hello"),
Expand Down
Loading