Skip to content

Commit eb2bb07

Browse files
authored
Use versionsort for everything that's auto-reordered by name (#3764)
1 parent f1a44c5 commit eb2bb07

9 files changed

+203
-29
lines changed

rustfmt-core/rustfmt-lib/src/imports.rs

+8-22
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ use crate::config::{Edition, IndentStyle};
1111
use crate::lists::{
1212
definitive_tactic, itemize_list, write_list, ListFormatting, ListItem, Separator,
1313
};
14+
use crate::reorder::{compare_as_versions, compare_opt_ident_as_versions};
1415
use crate::rewrite::{Rewrite, RewriteContext};
1516
use crate::shape::Shape;
1617
use crate::source_map::SpanUtils;
@@ -654,12 +655,7 @@ impl Ord for UseSegment {
654655
match (self, other) {
655656
(&Slf(ref a), &Slf(ref b))
656657
| (&Super(ref a), &Super(ref b))
657-
| (&Crate(ref a), &Crate(ref b)) => match (a, b) {
658-
(Some(sa), Some(sb)) => {
659-
sa.trim_start_matches("r#").cmp(sb.trim_start_matches("r#"))
660-
}
661-
(_, _) => a.cmp(b),
662-
},
658+
| (&Crate(ref a), &Crate(ref b)) => compare_opt_ident_as_versions(&a, &b),
663659
(&Glob, &Glob) => Ordering::Equal,
664660
(&Ident(ref pia, ref aa), &Ident(ref pib, ref ab)) => {
665661
let ia = pia.trim_start_matches("r#");
@@ -677,18 +673,8 @@ impl Ord for UseSegment {
677673
if !is_upper_snake_case(ia) && is_upper_snake_case(ib) {
678674
return Ordering::Less;
679675
}
680-
let ident_ord = ia.cmp(ib);
681-
if ident_ord != Ordering::Equal {
682-
return ident_ord;
683-
}
684-
match (aa, ab) {
685-
(None, Some(_)) => Ordering::Less,
686-
(Some(_), None) => Ordering::Greater,
687-
(Some(aas), Some(abs)) => aas
688-
.trim_start_matches("r#")
689-
.cmp(abs.trim_start_matches("r#")),
690-
(None, None) => Ordering::Equal,
691-
}
676+
677+
compare_as_versions(&ia, &ib).then_with(|| compare_opt_ident_as_versions(&aa, &ab))
692678
}
693679
(&List(ref a), &List(ref b)) => {
694680
for (a, b) in a.iter().zip(b.iter()) {
@@ -716,17 +702,17 @@ impl Ord for UseSegment {
716702
impl Ord for UseTree {
717703
fn cmp(&self, other: &UseTree) -> Ordering {
718704
for (a, b) in self.path.iter().zip(other.path.iter()) {
719-
let ord = a.cmp(b);
720705
// The comparison without aliases is a hack to avoid situations like
721706
// comparing `a::b` to `a as c` - where the latter should be ordered
722707
// first since it is shorter.
723-
if ord != Ordering::Equal && a.remove_alias().cmp(&b.remove_alias()) != Ordering::Equal
724-
{
708+
let ord = a.remove_alias().cmp(&b.remove_alias());
709+
if ord != Ordering::Equal {
725710
return ord;
726711
}
727712
}
728713

729-
self.path.len().cmp(&other.path.len())
714+
Ord::cmp(&self.path.len(), &other.path.len())
715+
.then(Ord::cmp(&self.path.last(), &other.path.last()))
730716
}
731717
}
732718

rustfmt-core/rustfmt-lib/src/items.rs

+3-2
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ use crate::expr::{
2323
use crate::lists::{definitive_tactic, itemize_list, write_list, ListFormatting, Separator};
2424
use crate::macros::{rewrite_macro, MacroPosition};
2525
use crate::overflow;
26+
use crate::reorder::compare_as_versions;
2627
use crate::rewrite::{Rewrite, RewriteContext};
2728
use crate::shape::{Indent, Shape};
2829
use crate::source_map::{LineRangeUtils, SpanUtils};
@@ -701,10 +702,10 @@ impl<'a> FmtVisitor<'a> {
701702
(TyAlias(_, _, _, ref lty), TyAlias(_, _, _, ref rty))
702703
if both_type(lty, rty) || both_opaque(lty, rty) =>
703704
{
704-
a.ident.as_str().cmp(&b.ident.as_str())
705+
compare_as_versions(&a.ident.as_str(), &b.ident.as_str())
705706
}
706707
(Const(..), Const(..)) | (MacCall(..), MacCall(..)) => {
707-
a.ident.as_str().cmp(&b.ident.as_str())
708+
compare_as_versions(&a.ident.as_str(), &b.ident.as_str())
708709
}
709710
(Fn(..), Fn(..)) => a.span.lo().cmp(&b.span.lo()),
710711
(TyAlias(_, _, _, ref ty), _) if is_type(ty) => Ordering::Less,

rustfmt-core/rustfmt-lib/src/reorder.rs

+152-3
Original file line numberDiff line numberDiff line change
@@ -22,18 +22,133 @@ use crate::spanned::Spanned;
2222
use crate::utils::{contains_skip, mk_sp};
2323
use crate::visitor::FmtVisitor;
2424

25+
/// Compare strings according to version sort (roughly equivalent to `strverscmp`)
26+
pub(crate) fn compare_as_versions(left: &str, right: &str) -> Ordering {
27+
let mut left = left.chars().peekable();
28+
let mut right = right.chars().peekable();
29+
30+
loop {
31+
// The strings are equal so far and not inside a number in both sides
32+
let (l, r) = match (left.next(), right.next()) {
33+
// Is this the end of both strings?
34+
(None, None) => return Ordering::Equal,
35+
// If for one, the shorter one is considered smaller
36+
(None, Some(_)) => return Ordering::Less,
37+
(Some(_), None) => return Ordering::Greater,
38+
(Some(l), Some(r)) => (l, r),
39+
};
40+
let next_ordering = match (l.to_digit(10), r.to_digit(10)) {
41+
// If neither is a digit, just compare them
42+
(None, None) => Ord::cmp(&l, &r),
43+
// The one with shorter non-digit run is smaller
44+
// For `strverscmp` it's smaller iff next char in longer is greater than digits
45+
(None, Some(_)) => Ordering::Greater,
46+
(Some(_), None) => Ordering::Less,
47+
// If both start numbers, we have to compare the numbers
48+
(Some(l), Some(r)) => {
49+
if l == 0 || r == 0 {
50+
// Fraction mode: compare as if there was leading `0.`
51+
let ordering = Ord::cmp(&l, &r);
52+
if ordering != Ordering::Equal {
53+
return ordering;
54+
}
55+
loop {
56+
// Get next pair
57+
let (l, r) = match (left.peek(), right.peek()) {
58+
// Is this the end of both strings?
59+
(None, None) => return Ordering::Equal,
60+
// If for one, the shorter one is considered smaller
61+
(None, Some(_)) => return Ordering::Less,
62+
(Some(_), None) => return Ordering::Greater,
63+
(Some(l), Some(r)) => (l, r),
64+
};
65+
// Are they digits?
66+
match (l.to_digit(10), r.to_digit(10)) {
67+
// If out of digits, use the stored ordering due to equal length
68+
(None, None) => break Ordering::Equal,
69+
// If one is shorter, it's smaller
70+
(None, Some(_)) => return Ordering::Less,
71+
(Some(_), None) => return Ordering::Greater,
72+
// If both are digits, consume them and take into account
73+
(Some(l), Some(r)) => {
74+
left.next();
75+
right.next();
76+
let ordering = Ord::cmp(&l, &r);
77+
if ordering != Ordering::Equal {
78+
return ordering;
79+
}
80+
}
81+
}
82+
}
83+
} else {
84+
// Integer mode
85+
let mut same_length_ordering = Ord::cmp(&l, &r);
86+
loop {
87+
// Get next pair
88+
let (l, r) = match (left.peek(), right.peek()) {
89+
// Is this the end of both strings?
90+
(None, None) => return same_length_ordering,
91+
// If for one, the shorter one is considered smaller
92+
(None, Some(_)) => return Ordering::Less,
93+
(Some(_), None) => return Ordering::Greater,
94+
(Some(l), Some(r)) => (l, r),
95+
};
96+
// Are they digits?
97+
match (l.to_digit(10), r.to_digit(10)) {
98+
// If out of digits, use the stored ordering due to equal length
99+
(None, None) => break same_length_ordering,
100+
// If one is shorter, it's smaller
101+
(None, Some(_)) => return Ordering::Less,
102+
(Some(_), None) => return Ordering::Greater,
103+
// If both are digits, consume them and take into account
104+
(Some(l), Some(r)) => {
105+
left.next();
106+
right.next();
107+
same_length_ordering = same_length_ordering.then(Ord::cmp(&l, &r));
108+
}
109+
}
110+
}
111+
}
112+
}
113+
};
114+
if next_ordering != Ordering::Equal {
115+
return next_ordering;
116+
}
117+
}
118+
}
119+
120+
/// Compare identifiers, trimming `r#` if present, according to version sort
121+
pub(crate) fn compare_ident_as_versions(left: &str, right: &str) -> Ordering {
122+
compare_as_versions(
123+
left.trim_start_matches("r#"),
124+
right.trim_start_matches("r#"),
125+
)
126+
}
127+
128+
pub(crate) fn compare_opt_ident_as_versions<S>(left: &Option<S>, right: &Option<S>) -> Ordering
129+
where
130+
S: AsRef<str>,
131+
{
132+
match (left, right) {
133+
(None, None) => Ordering::Equal,
134+
(None, Some(_)) => Ordering::Less,
135+
(Some(_), None) => Ordering::Greater,
136+
(Some(left), Some(right)) => compare_ident_as_versions(left.as_ref(), right.as_ref()),
137+
}
138+
}
139+
25140
/// Choose the ordering between the given two items.
26141
fn compare_items(a: &ast::Item, b: &ast::Item) -> Ordering {
27142
match (&a.kind, &b.kind) {
28143
(&ast::ItemKind::Mod(..), &ast::ItemKind::Mod(..)) => {
29-
a.ident.as_str().cmp(&b.ident.as_str())
144+
compare_as_versions(&a.ident.as_str(), &b.ident.as_str())
30145
}
31146
(&ast::ItemKind::ExternCrate(ref a_name), &ast::ItemKind::ExternCrate(ref b_name)) => {
32147
// `extern crate foo as bar;`
33148
// ^^^ Comparing this.
34149
let a_orig_name = a_name.map_or_else(|| a.ident.as_str(), rustc_span::Symbol::as_str);
35150
let b_orig_name = b_name.map_or_else(|| b.ident.as_str(), rustc_span::Symbol::as_str);
36-
let result = a_orig_name.cmp(&b_orig_name);
151+
let result = compare_as_versions(&a_orig_name, &b_orig_name);
37152
if result != Ordering::Equal {
38153
return result;
39154
}
@@ -44,7 +159,7 @@ fn compare_items(a: &ast::Item, b: &ast::Item) -> Ordering {
44159
(Some(..), None) => Ordering::Greater,
45160
(None, Some(..)) => Ordering::Less,
46161
(None, None) => Ordering::Equal,
47-
(Some(..), Some(..)) => a.ident.as_str().cmp(&b.ident.as_str()),
162+
(Some(..), Some(..)) => compare_as_versions(&a.ident.as_str(), &b.ident.as_str()),
48163
}
49164
}
50165
_ => unreachable!(),
@@ -264,3 +379,37 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
264379
}
265380
}
266381
}
382+
383+
#[cfg(test)]
384+
mod tests {
385+
#[test]
386+
fn test_compare_as_versions() {
387+
use super::compare_as_versions;
388+
use std::cmp::Ordering;
389+
let mut strings: &[&'static str] = &[
390+
"9", "i8", "ia32", "u009", "u08", "u08", "u080", "u8", "u8", "u16", "u32", "u128",
391+
];
392+
while !strings.is_empty() {
393+
let (first, tail) = strings.split_first().unwrap();
394+
for second in tail {
395+
if first == second {
396+
assert_eq!(compare_as_versions(first, second), Ordering::Equal);
397+
assert_eq!(compare_as_versions(second, first), Ordering::Equal);
398+
} else {
399+
assert_eq!(compare_as_versions(first, second), Ordering::Less);
400+
assert_eq!(compare_as_versions(second, first), Ordering::Greater);
401+
}
402+
}
403+
strings = tail;
404+
}
405+
}
406+
#[test]
407+
fn test_compare_opt_ident_as_versions() {
408+
use super::compare_opt_ident_as_versions;
409+
use std::cmp::Ordering;
410+
let items: &[Option<&'static str>] = &[None, Some("a"), Some("r#a"), Some("a")];
411+
for (p, n) in items[..items.len() - 1].iter().zip(items[1..].iter()) {
412+
assert!(compare_opt_ident_as_versions(p, n) != Ordering::Greater);
413+
}
414+
}
415+
}

rustfmt-core/rustfmt-lib/tests/source/imports-reorder-lines.rs

+11
Original file line numberDiff line numberDiff line change
@@ -30,3 +30,14 @@ mod test {}
3030

3131
use test::{a as aa, c};
3232
use test::{self as bb, b};
33+
34+
#[path = "empty_file.rs"]
35+
mod v10;
36+
#[path = "empty_file.rs"]
37+
mod v2;
38+
39+
extern crate crate10;
40+
extern crate crate2;
41+
42+
extern crate my as c10;
43+
extern crate my as c2;

rustfmt-core/rustfmt-lib/tests/source/imports-reorder.rs

+6
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,9 @@
33
use path::{C,/*A*/ A, B /* B */, self /* self */};
44

55
use {ab, ac, aa, Z, b};
6+
7+
// The sort order shall follow versionsort
8+
use {u8, u128, u64, u16, u32};
9+
use {v1, v0200, v0030, v0002, v02000, v02001};
10+
// Order by alias should use versionsort too
11+
use {crate as crate10, crate as crate2, crate as crate1};

rustfmt-core/rustfmt-lib/tests/source/issue-2863.rs

+2
Original file line numberDiff line numberDiff line change
@@ -22,4 +22,6 @@ impl<T> IntoIterator for SafeVec<T> {
2222
type E = impl Trait;
2323
const AnotherConst: i32 = 100;
2424
fn foo8() {println!("hello, world");}
25+
const AnyConst10: i32 = 100;
26+
const AnyConst2: i32 = 100;
2527
}

rustfmt-core/rustfmt-lib/tests/target/imports-reorder-lines.rs

+13-2
Original file line numberDiff line numberDiff line change
@@ -15,17 +15,28 @@ use aaa::*;
1515
mod test {}
1616
// If item names are equal, order by rename
1717

18-
use test::{a as bb, b};
1918
use test::{a as aa, c};
19+
use test::{a as bb, b};
2020

2121
mod test {}
2222
// If item names are equal, order by rename - no rename comes before a rename
2323

24-
use test::{a as bb, b};
2524
use test::{a, c};
25+
use test::{a as bb, b};
2626

2727
mod test {}
2828
// `self` always comes first
2929

3030
use test::{self as bb, b};
3131
use test::{a as aa, c};
32+
33+
#[path = "empty_file.rs"]
34+
mod v2;
35+
#[path = "empty_file.rs"]
36+
mod v10;
37+
38+
extern crate crate2;
39+
extern crate crate10;
40+
41+
extern crate my as c2;
42+
extern crate my as c10;

rustfmt-core/rustfmt-lib/tests/target/imports-reorder.rs

+6
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,9 @@
33
use path::{self /* self */, /* A */ A, B /* B */, C};
44

55
use {aa, ab, ac, b, Z};
6+
7+
// The sort order shall follow versionsort
8+
use {u8, u16, u32, u64, u128};
9+
use {v0002, v0030, v0200, v02000, v02001, v1};
10+
// Order by alias should use versionsort too
11+
use {crate as crate1, crate as crate2, crate as crate10};

rustfmt-core/rustfmt-lib/tests/target/issue-2863.rs

+2
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ impl<T> IntoIterator for SafeVec<T> {
1313
type F = impl Trait;
1414

1515
const AnotherConst: i32 = 100;
16+
const AnyConst2: i32 = 100;
17+
const AnyConst10: i32 = 100;
1618
const SomeConst: i32 = 100;
1719

1820
// comment on foo()

0 commit comments

Comments
 (0)