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

Import ordering #2535

Merged
merged 8 commits into from
Mar 22, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 14 additions & 14 deletions Configurations.md
Original file line number Diff line number Diff line change
Expand Up @@ -1297,28 +1297,28 @@ Reorder import statements in group

**Note:** This option takes effect only when [`reorder_imports`](#reorder_imports) is set to `true`.

#### `false` (default):
#### `true` (default):

```rust
use std::mem;
use std::io;
use std::mem;

use lorem;
use ipsum;
use dolor;
use ipsum;
use lorem;
use sit;
```

#### `true`:
#### `false`:

```rust
use std::io;
use std::mem;

```rust
use dolor;
use ipsum;
use lorem;
use sit;
use std::io;
use std::mem;
```

See also [`reorder_imports`](#reorder_imports).
Expand Down Expand Up @@ -1360,7 +1360,11 @@ Reorder `extern crate` statements in group
- **Possible values**: `true`, `false`
- **Stable**: No

#### `true` (default):
#### `false` (default):

This value has no influence beyond the effect of the [`reorder_extern_crates`](#reorder_extern_crates) option. Set [`reorder_extern_crates`](#reorder_extern_crates) to `false` if you do not want `extern crate` groups to be collapsed and ordered.

#### `true`:

**Note:** This only takes effect when [`reorder_extern_crates`](#reorder_extern_crates) is set to `true`.

Expand All @@ -1374,10 +1378,6 @@ extern crate lorem;
extern crate sit;
```

#### `false`:

This value has no influence beyond the effect of the [`reorder_extern_crates`](#reorder_extern_crates) option. Set [`reorder_extern_crates`](#reorder_extern_crates) to `false` if you do not want `extern crate` groups to be collapsed and ordered.

## `reorder_modules`

Reorder `mod` declarations alphabetically in group.
Expand All @@ -1386,7 +1386,7 @@ Reorder `mod` declarations alphabetically in group.
- **Possible values**: `true`, `false`
- **Stable**: No

#### `true`
#### `true` (default)

```rust
mod a;
Expand Down
5 changes: 2 additions & 3 deletions bootstrap.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@
cargo build --release

target/release/rustfmt --write-mode=overwrite src/lib.rs
target/release/rustfmt --write-mode=overwrite src/bin/rustfmt.rs
target/release/rustfmt --write-mode=overwrite src/bin/cargo-fmt.rs
target/release/rustfmt --write-mode=overwrite tests/system.rs
target/release/rustfmt --write-mode=overwrite src/bin/main.rs
target/release/rustfmt --write-mode=overwrite src/cargo-fmt/main.rs

for filename in tests/target/*.rs; do
if ! grep -q "rustfmt-" "$filename"; then
Expand Down
4 changes: 2 additions & 2 deletions src/bin/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,16 @@ extern crate env_logger;
extern crate getopts;
extern crate rustfmt_nightly as rustfmt;

use std::{env, error};
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if this and similar changes are correct. I feel that use std::{env, error}; should have higher precedence than use std::fs::File; and other imports that start with use std::.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I was pretty much making it up as I went along, but this was intentional. Looking at nested imports, I prefer use foo::{bar::baz, baz::{a, b}}; to the opposite ordering. I can't really justify why, but it felt to me like single imports should come before list imports. Do you have a reason for supporting the other order?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, actually inside nested imports I prefer to put list imports after single imports, but on top level I prefer the opposite.

Maybe it is just that I am used to the current behavior. To keep the rule consistent I agree that we should put the list imports after single imports.

use std::fs::File;
use std::io::{self, Read, Write};
use std::path::{Path, PathBuf};
use std::str::FromStr;
use std::{env, error};

use getopts::{Matches, Options};

use rustfmt::config::{get_toml_path, Color, Config, WriteMode};
use rustfmt::config::file_lines::FileLines;
use rustfmt::config::{get_toml_path, Color, Config, WriteMode};
use rustfmt::{run, FileName, Input, Summary};

type FmtError = Box<error::Error + Send + Sync>;
Expand Down
2 changes: 1 addition & 1 deletion src/chains.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ use std::borrow::Cow;
use std::cmp::min;
use std::iter;

use syntax::{ast, ptr};
use syntax::codemap::Span;
use syntax::{ast, ptr};

pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -> Option<String> {
debug!("rewrite_chain {:?}", shape);
Expand Down
2 changes: 1 addition & 1 deletion src/closures.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
// except according to those terms.

use config::lists::*;
use syntax::{ast, ptr};
use syntax::codemap::Span;
use syntax::parse::classify;
use syntax::{ast, ptr};

use codemap::SpanUtils;
use expr::{block_contains_comment, is_simple_block, is_unsafe_block, rewrite_cond, ToExpr};
Expand Down
2 changes: 1 addition & 1 deletion src/codemap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
//! This includes extension traits and methods for looking up spans and line ranges for AST nodes.

use config::file_lines::LineRange;
use visitor::SnippetProvider;
use syntax::codemap::{BytePos, CodeMap, Span};
use visitor::SnippetProvider;

use comment::FindUncommented;

Expand Down
2 changes: 1 addition & 1 deletion src/config/file_lines.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@

//! This module contains types and functions to support formatting specific line ranges.

use std::{cmp, iter, str};
use std::collections::HashMap;
use std::rc::Rc;
use std::{cmp, iter, str};

use serde::de::{Deserialize, Deserializer};
use serde_json as json;
Expand Down
2 changes: 1 addition & 1 deletion src/config/license.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::io;
use std::fmt;
use std::fs::File;
use std::io;
use std::io::Read;

use regex;
Expand Down
10 changes: 5 additions & 5 deletions src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use std::{env, fs};
use std::cell::Cell;
use std::default::Default;
use std::fs::File;
use std::io::{Error, ErrorKind, Read};
use std::path::{Path, PathBuf};
use std::{env, fs};

use regex::Regex;

Expand All @@ -23,9 +23,9 @@ mod config_type;
mod options;

pub mod file_lines;
pub mod license;
pub mod lists;
pub mod summary;
pub mod license;

use config::config_type::ConfigType;
use config::file_lines::FileLines;
Expand Down Expand Up @@ -70,11 +70,11 @@ create_config! {
// Ordering
reorder_extern_crates: bool, true, false, "Reorder extern crate statements alphabetically";
reorder_extern_crates_in_group: bool, true, false, "Reorder extern crate statements in group";
reorder_imports: bool, false, false, "Reorder import statements alphabetically";
reorder_imports_in_group: bool, false, false, "Reorder import statements in group";
reorder_imports: bool, true, false, "Reorder import statements alphabetically";
reorder_imports_in_group: bool, true, false, "Reorder import statements in group";
reorder_imported_names: bool, true, false,
"Reorder lists of names in import statements alphabetically";
reorder_modules: bool, false, false, "Reorder module statemtents alphabetically in group";
reorder_modules: bool, true, false, "Reorder module statemtents alphabetically in group";

// Spaces around punctuation
binop_separator: SeparatorPlace, SeparatorPlace::Front, false,
Expand Down
2 changes: 1 addition & 1 deletion src/config/summary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use std::time::{Duration, Instant};
use std::default::Default;
use std::time::{Duration, Instant};

#[must_use]
#[derive(Debug, Default, Clone, Copy)]
Expand Down
2 changes: 1 addition & 1 deletion src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ use std::cmp::min;
use std::iter::repeat;

use config::lists::*;
use syntax::{ast, ptr};
use syntax::codemap::{BytePos, CodeMap, Span};
use syntax::{ast, ptr};

use chains::rewrite_chain;
use closures;
Expand Down
2 changes: 1 addition & 1 deletion src/format-diff/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ extern crate regex;
extern crate serde_derive;
extern crate serde_json as json;

use std::{env, fmt, process};
use std::collections::HashSet;
use std::error::Error;
use std::io::{self, BufRead};
use std::{env, fmt, process};

use regex::Regex;

Expand Down
4 changes: 2 additions & 2 deletions src/items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ use std::cmp::min;

use config::lists::*;
use regex::Regex;
use syntax::{abi, ast, ptr, symbol};
use syntax::codemap::{self, BytePos, Span};
use syntax::visit;
use syntax::{abi, ast, ptr, symbol};

use codemap::{LineRangeUtils, SpanUtils};
use comment::{combine_strs_with_missing_comments, contains_comment, recover_comment_removed,
Expand All @@ -26,8 +26,8 @@ use config::{BraceStyle, Config, Density, IndentStyle};
use expr::{format_expr, is_empty_block, is_simple_block_stmt, rewrite_assign_rhs,
rewrite_assign_rhs_with, ExprType, RhsTactics};
use lists::{definitive_tactic, itemize_list, write_list, ListFormatting, ListItem, Separator};
use rewrite::{Rewrite, RewriteContext};
use overflow;
use rewrite::{Rewrite, RewriteContext};
use shape::{Indent, Shape};
use spanned::Spanned;
use types::TraitTyParamBounds;
Expand Down
6 changes: 3 additions & 3 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,11 @@ use std::path::PathBuf;
use std::rc::Rc;
use std::time::Duration;

use syntax::errors::{DiagnosticBuilder, Handler};
use syntax::errors::emitter::{ColorConfig, EmitterWriter};
use syntax::ast;
use syntax::codemap::{CodeMap, FilePathMapping};
pub use syntax::codemap::FileName;
use syntax::codemap::{CodeMap, FilePathMapping};
use syntax::errors::emitter::{ColorConfig, EmitterWriter};
use syntax::errors::{DiagnosticBuilder, Handler};
use syntax::parse::{self, ParseSess};

use checkstyle::{output_footer, output_header};
Expand Down
57 changes: 38 additions & 19 deletions src/lists.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ impl AsRef<ListItem> for ListItem {
}
}

#[derive(PartialEq, Eq)]
#[derive(PartialEq, Eq, Debug)]
pub enum ListItemCommentStyle {
// Try to keep the comment on the same line with the item.
SameLine,
Expand All @@ -66,6 +66,7 @@ pub enum ListItemCommentStyle {
None,
}

#[derive(Debug)]
pub struct ListItem {
// None for comments mean that they are not present.
pub pre_comment: Option<String>,
Expand Down Expand Up @@ -118,6 +119,18 @@ impl ListItem {
new_lines: false,
}
}

// true if the item causes something to be written.
fn is_substantial(&self) -> bool {
fn empty(s: &Option<String>) -> bool {
match *s {
Some(ref s) if !s.is_empty() => false,
_ => true,
}
}

!(empty(&self.pre_comment) && empty(&self.item) && empty(&self.post_comment))
}
}

/// The type of separator for lists.
Expand Down Expand Up @@ -220,6 +233,10 @@ where
item_last_line_width -= indent_str.len();
}

if !item.is_substantial() {
continue;
}

match tactic {
DefinitiveListTactic::Horizontal if !first => {
result.push(' ');
Expand Down Expand Up @@ -276,26 +293,28 @@ where
rewrite_comment(comment, block_mode, formatting.shape, formatting.config)?;
result.push_str(&comment);

if tactic == DefinitiveListTactic::Vertical {
// We cannot keep pre-comments on the same line if the comment if normalized.
let keep_comment = if formatting.config.normalize_comments()
|| item.pre_comment_style == ListItemCommentStyle::DifferentLine
{
false
if !inner_item.is_empty() {
if tactic == DefinitiveListTactic::Vertical {
// We cannot keep pre-comments on the same line if the comment if normalized.
let keep_comment = if formatting.config.normalize_comments()
|| item.pre_comment_style == ListItemCommentStyle::DifferentLine
{
false
} else {
// We will try to keep the comment on the same line with the item here.
// 1 = ` `
let total_width = total_item_width(item) + item_sep_len + 1;
total_width <= formatting.shape.width
};
if keep_comment {
result.push(' ');
} else {
result.push('\n');
result.push_str(indent_str);
}
} else {
// We will try to keep the comment on the same line with the item here.
// 1 = ` `
let total_width = total_item_width(item) + item_sep_len + 1;
total_width <= formatting.shape.width
};
if keep_comment {
result.push(' ');
} else {
result.push('\n');
result.push_str(indent_str);
}
} else {
result.push(' ');
}
item_max_width = None;
}
Expand All @@ -304,7 +323,7 @@ where
result.push_str(formatting.separator.trim());
result.push(' ');
}
result.push_str(&inner_item[..]);
result.push_str(inner_item);

// Post-comments
if tactic != DefinitiveListTactic::Vertical && item.post_comment.is_some() {
Expand Down
2 changes: 1 addition & 1 deletion src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
use std::collections::HashMap;

use config::lists::*;
use syntax::{ast, ptr};
use syntax::codemap::{BytePos, Span};
use syntax::parse::new_parser_from_tts;
use syntax::parse::parser::Parser;
Expand All @@ -31,6 +30,7 @@ use syntax::print::pprust;
use syntax::symbol;
use syntax::tokenstream::{Cursor, ThinTokenStream, TokenStream, TokenTree};
use syntax::util::ThinVec;
use syntax::{ast, ptr};

use codemap::SpanUtils;
use comment::{contains_comment, remove_trailing_white_spaces, CharClasses, FindUncommented,
Expand Down
Loading