Skip to content
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.

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

Use versionsort for everything that's auto-reordered by name #3764

Merged
merged 8 commits into from
Apr 18, 2020
30 changes: 8 additions & 22 deletions rustfmt-core/rustfmt-lib/src/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use crate::config::{Edition, IndentStyle};
use crate::lists::{
definitive_tactic, itemize_list, write_list, ListFormatting, ListItem, Separator,
};
use crate::reorder::{compare_as_versions, compare_opt_ident_as_versions};
use crate::rewrite::{Rewrite, RewriteContext};
use crate::shape::Shape;
use crate::source_map::SpanUtils;
Expand Down Expand Up @@ -654,12 +655,7 @@ impl Ord for UseSegment {
match (self, other) {
(&Slf(ref a), &Slf(ref b))
| (&Super(ref a), &Super(ref b))
| (&Crate(ref a), &Crate(ref b)) => match (a, b) {
(Some(sa), Some(sb)) => {
sa.trim_start_matches("r#").cmp(sb.trim_start_matches("r#"))
}
(_, _) => a.cmp(b),
},
| (&Crate(ref a), &Crate(ref b)) => compare_opt_ident_as_versions(&a, &b),
(&Glob, &Glob) => Ordering::Equal,
(&Ident(ref pia, ref aa), &Ident(ref pib, ref ab)) => {
let ia = pia.trim_start_matches("r#");
Expand All @@ -677,18 +673,8 @@ impl Ord for UseSegment {
if !is_upper_snake_case(ia) && is_upper_snake_case(ib) {
return Ordering::Less;
}
let ident_ord = ia.cmp(ib);
if ident_ord != Ordering::Equal {
return ident_ord;
}
match (aa, ab) {
(None, Some(_)) => Ordering::Less,
(Some(_), None) => Ordering::Greater,
(Some(aas), Some(abs)) => aas
.trim_start_matches("r#")
.cmp(abs.trim_start_matches("r#")),
(None, None) => Ordering::Equal,
}

compare_as_versions(&ia, &ib).then_with(|| compare_opt_ident_as_versions(&aa, &ab))
}
(&List(ref a), &List(ref b)) => {
for (a, b) in a.iter().zip(b.iter()) {
Expand Down Expand Up @@ -716,17 +702,17 @@ impl Ord for UseSegment {
impl Ord for UseTree {
fn cmp(&self, other: &UseTree) -> Ordering {
for (a, b) in self.path.iter().zip(other.path.iter()) {
let ord = a.cmp(b);
// The comparison without aliases is a hack to avoid situations like
// comparing `a::b` to `a as c` - where the latter should be ordered
// first since it is shorter.
if ord != Ordering::Equal && a.remove_alias().cmp(&b.remove_alias()) != Ordering::Equal
{
let ord = a.remove_alias().cmp(&b.remove_alias());
if ord != Ordering::Equal {
return ord;
}
}

self.path.len().cmp(&other.path.len())
Ord::cmp(&self.path.len(), &other.path.len())
.then(Ord::cmp(&self.path.last(), &other.path.last()))
}
}

Expand Down
5 changes: 3 additions & 2 deletions rustfmt-core/rustfmt-lib/src/items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use crate::expr::{
use crate::lists::{definitive_tactic, itemize_list, write_list, ListFormatting, Separator};
use crate::macros::{rewrite_macro, MacroPosition};
use crate::overflow;
use crate::reorder::compare_as_versions;
use crate::rewrite::{Rewrite, RewriteContext};
use crate::shape::{Indent, Shape};
use crate::source_map::{LineRangeUtils, SpanUtils};
Expand Down Expand Up @@ -701,10 +702,10 @@ impl<'a> FmtVisitor<'a> {
(TyAlias(_, _, _, ref lty), TyAlias(_, _, _, ref rty))
if both_type(lty, rty) || both_opaque(lty, rty) =>
{
a.ident.as_str().cmp(&b.ident.as_str())
compare_as_versions(&a.ident.as_str(), &b.ident.as_str())
}
(Const(..), Const(..)) | (MacCall(..), MacCall(..)) => {
a.ident.as_str().cmp(&b.ident.as_str())
compare_as_versions(&a.ident.as_str(), &b.ident.as_str())
}
(Fn(..), Fn(..)) => a.span.lo().cmp(&b.span.lo()),
(TyAlias(_, _, _, ref ty), _) if is_type(ty) => Ordering::Less,
Expand Down
155 changes: 152 additions & 3 deletions rustfmt-core/rustfmt-lib/src/reorder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,133 @@ use crate::spanned::Spanned;
use crate::utils::{contains_skip, mk_sp};
use crate::visitor::FmtVisitor;

/// Compare strings according to version sort (roughly equivalent to `strverscmp`)
pub(crate) fn compare_as_versions(left: &str, right: &str) -> Ordering {
let mut left = left.chars().peekable();
let mut right = right.chars().peekable();

loop {
// The strings are equal so far and not inside a number in both sides
let (l, r) = match (left.next(), right.next()) {
// Is this the end of both strings?
(None, None) => return Ordering::Equal,
// If for one, the shorter one is considered smaller
(None, Some(_)) => return Ordering::Less,
(Some(_), None) => return Ordering::Greater,
(Some(l), Some(r)) => (l, r),
};
let next_ordering = match (l.to_digit(10), r.to_digit(10)) {
// If neither is a digit, just compare them
(None, None) => Ord::cmp(&l, &r),
// The one with shorter non-digit run is smaller
// For `strverscmp` it's smaller iff next char in longer is greater than digits
(None, Some(_)) => Ordering::Greater,
(Some(_), None) => Ordering::Less,
// If both start numbers, we have to compare the numbers
(Some(l), Some(r)) => {
if l == 0 || r == 0 {
// Fraction mode: compare as if there was leading `0.`
let ordering = Ord::cmp(&l, &r);
if ordering != Ordering::Equal {
return ordering;
}
loop {
// Get next pair
let (l, r) = match (left.peek(), right.peek()) {
// Is this the end of both strings?
(None, None) => return Ordering::Equal,
// If for one, the shorter one is considered smaller
(None, Some(_)) => return Ordering::Less,
(Some(_), None) => return Ordering::Greater,
(Some(l), Some(r)) => (l, r),
};
// Are they digits?
match (l.to_digit(10), r.to_digit(10)) {
// If out of digits, use the stored ordering due to equal length
(None, None) => break Ordering::Equal,
// If one is shorter, it's smaller
(None, Some(_)) => return Ordering::Less,
(Some(_), None) => return Ordering::Greater,
// If both are digits, consume them and take into account
(Some(l), Some(r)) => {
left.next();
right.next();
let ordering = Ord::cmp(&l, &r);
if ordering != Ordering::Equal {
return ordering;
}
}
}
}
} else {
// Integer mode
let mut same_length_ordering = Ord::cmp(&l, &r);
loop {
// Get next pair
let (l, r) = match (left.peek(), right.peek()) {
// Is this the end of both strings?
(None, None) => return same_length_ordering,
// If for one, the shorter one is considered smaller
(None, Some(_)) => return Ordering::Less,
(Some(_), None) => return Ordering::Greater,
(Some(l), Some(r)) => (l, r),
};
// Are they digits?
match (l.to_digit(10), r.to_digit(10)) {
// If out of digits, use the stored ordering due to equal length
(None, None) => break same_length_ordering,
// If one is shorter, it's smaller
(None, Some(_)) => return Ordering::Less,
(Some(_), None) => return Ordering::Greater,
// If both are digits, consume them and take into account
(Some(l), Some(r)) => {
left.next();
right.next();
same_length_ordering = same_length_ordering.then(Ord::cmp(&l, &r));
}
}
}
}
}
};
if next_ordering != Ordering::Equal {
return next_ordering;
}
}
}

/// Compare identifiers, trimming `r#` if present, according to version sort
pub(crate) fn compare_ident_as_versions(left: &str, right: &str) -> Ordering {
compare_as_versions(
left.trim_start_matches("r#"),
right.trim_start_matches("r#"),
)
}

pub(crate) fn compare_opt_ident_as_versions<S>(left: &Option<S>, right: &Option<S>) -> Ordering
where
S: AsRef<str>,
{
match (left, right) {
(None, None) => Ordering::Equal,
(None, Some(_)) => Ordering::Less,
(Some(_), None) => Ordering::Greater,
(Some(left), Some(right)) => compare_ident_as_versions(left.as_ref(), right.as_ref()),
}
}

/// Choose the ordering between the given two items.
fn compare_items(a: &ast::Item, b: &ast::Item) -> Ordering {
match (&a.kind, &b.kind) {
(&ast::ItemKind::Mod(..), &ast::ItemKind::Mod(..)) => {
a.ident.as_str().cmp(&b.ident.as_str())
compare_as_versions(&a.ident.as_str(), &b.ident.as_str())
}
(&ast::ItemKind::ExternCrate(ref a_name), &ast::ItemKind::ExternCrate(ref b_name)) => {
// `extern crate foo as bar;`
// ^^^ Comparing this.
let a_orig_name = a_name.map_or_else(|| a.ident.as_str(), rustc_span::Symbol::as_str);
let b_orig_name = b_name.map_or_else(|| b.ident.as_str(), rustc_span::Symbol::as_str);
let result = a_orig_name.cmp(&b_orig_name);
let result = compare_as_versions(&a_orig_name, &b_orig_name);
if result != Ordering::Equal {
return result;
}
Expand All @@ -44,7 +159,7 @@ fn compare_items(a: &ast::Item, b: &ast::Item) -> Ordering {
(Some(..), None) => Ordering::Greater,
(None, Some(..)) => Ordering::Less,
(None, None) => Ordering::Equal,
(Some(..), Some(..)) => a.ident.as_str().cmp(&b.ident.as_str()),
(Some(..), Some(..)) => compare_as_versions(&a.ident.as_str(), &b.ident.as_str()),
}
}
_ => unreachable!(),
Expand Down Expand Up @@ -264,3 +379,37 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
}
}
}

#[cfg(test)]
mod tests {
#[test]
fn test_compare_as_versions() {
use super::compare_as_versions;
use std::cmp::Ordering;
let mut strings: &[&'static str] = &[
"9", "i8", "ia32", "u009", "u08", "u08", "u080", "u8", "u8", "u16", "u32", "u128",
];
while !strings.is_empty() {
let (first, tail) = strings.split_first().unwrap();
for second in tail {
if first == second {
assert_eq!(compare_as_versions(first, second), Ordering::Equal);
assert_eq!(compare_as_versions(second, first), Ordering::Equal);
} else {
assert_eq!(compare_as_versions(first, second), Ordering::Less);
assert_eq!(compare_as_versions(second, first), Ordering::Greater);
}
}
strings = tail;
}
}
#[test]
fn test_compare_opt_ident_as_versions() {
use super::compare_opt_ident_as_versions;
use std::cmp::Ordering;
let items: &[Option<&'static str>] = &[None, Some("a"), Some("r#a"), Some("a")];
for (p, n) in items[..items.len() - 1].iter().zip(items[1..].iter()) {
assert!(compare_opt_ident_as_versions(p, n) != Ordering::Greater);
}
}
}
11 changes: 11 additions & 0 deletions rustfmt-core/rustfmt-lib/tests/source/imports-reorder-lines.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,14 @@ mod test {}

use test::{a as aa, c};
use test::{self as bb, b};

#[path = "empty_file.rs"]
mod v10;
#[path = "empty_file.rs"]
mod v2;

extern crate crate10;
extern crate crate2;

extern crate my as c10;
extern crate my as c2;
6 changes: 6 additions & 0 deletions rustfmt-core/rustfmt-lib/tests/source/imports-reorder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,9 @@
use path::{C,/*A*/ A, B /* B */, self /* self */};

use {ab, ac, aa, Z, b};

// The sort order shall follow versionsort
use {u8, u128, u64, u16, u32};
use {v1, v0200, v0030, v0002, v02000, v02001};
// Order by alias should use versionsort too
use {crate as crate10, crate as crate2, crate as crate1};
2 changes: 2 additions & 0 deletions rustfmt-core/rustfmt-lib/tests/source/issue-2863.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,6 @@ impl<T> IntoIterator for SafeVec<T> {
type E = impl Trait;
const AnotherConst: i32 = 100;
fn foo8() {println!("hello, world");}
const AnyConst10: i32 = 100;
const AnyConst2: i32 = 100;
}
15 changes: 13 additions & 2 deletions rustfmt-core/rustfmt-lib/tests/target/imports-reorder-lines.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,28 @@ use aaa::*;
mod test {}
// If item names are equal, order by rename

use test::{a as bb, b};
use test::{a as aa, c};
use test::{a as bb, b};

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

use test::{a as bb, b};
use test::{a, c};
use test::{a as bb, b};

mod test {}
// `self` always comes first

use test::{self as bb, b};
use test::{a as aa, c};

#[path = "empty_file.rs"]
mod v2;
#[path = "empty_file.rs"]
mod v10;

extern crate crate2;
extern crate crate10;

extern crate my as c2;
extern crate my as c10;
6 changes: 6 additions & 0 deletions rustfmt-core/rustfmt-lib/tests/target/imports-reorder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,9 @@
use path::{self /* self */, /* A */ A, B /* B */, C};

use {aa, ab, ac, b, Z};

// The sort order shall follow versionsort
use {u8, u16, u32, u64, u128};
use {v0002, v0030, v0200, v02000, v02001, v1};
// Order by alias should use versionsort too
use {crate as crate1, crate as crate2, crate as crate10};
2 changes: 2 additions & 0 deletions rustfmt-core/rustfmt-lib/tests/target/issue-2863.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ impl<T> IntoIterator for SafeVec<T> {
type F = impl Trait;

const AnotherConst: i32 = 100;
const AnyConst2: i32 = 100;
const AnyConst10: i32 = 100;
const SomeConst: i32 = 100;

// comment on foo()
Expand Down