From ff301efa4e0f433dccaa9de920da19b5492e24e8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ga=C3=ABtan=20Cassiers?= <gaetan.cassiers@gmail.com>
Date: Sat, 25 Jul 2015 18:58:16 +0200
Subject: [PATCH 1/4] Fix width computation in rewrite_binary_op
Operator width was counted twice.
---
src/expr.rs | 2 +-
tests/source/expr.rs | 1 +
tests/target/expr.rs | 4 ++--
3 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/expr.rs b/src/expr.rs
index 1f92ed5d719..81cc2049081 100644
--- a/src/expr.rs
+++ b/src/expr.rs
@@ -608,7 +608,7 @@ fn rewrite_binary_op(context: &RewriteContext,
result.push_str(&operator_str);
// 1 = space between operator and rhs
- let used_width = result.len() + operator_str.len() + 1;
+ let used_width = result.len() + 1;
let remaining_width = match result.rfind('\n') {
Some(idx) => (offset + width + idx).checked_sub(used_width).unwrap_or(0),
None => width.checked_sub(used_width).unwrap_or(0)
diff --git a/tests/source/expr.rs b/tests/source/expr.rs
index 74538c13597..9a69d68987f 100644
--- a/tests/source/expr.rs
+++ b/tests/source/expr.rs
@@ -7,6 +7,7 @@ fn foo() -> bool {
let very_long_variable_name = ( a + first + simple + test );
let very_long_variable_name = (a + first + simple + test + AAAAAAAAAAAAA + BBBBBBBBBBBBBBBBB + b + c);
+ //FIXME this exceeds width limit. Needs assignments reformatting
let is_internalxxxx = self.codemap.span_to_filename(s) == self.codemap.span_to_filename(m.inner);
let some_val = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa * bbbb / (bbbbbb -
diff --git a/tests/target/expr.rs b/tests/target/expr.rs
index a52ad51b0c6..1ec5e0168fa 100644
--- a/tests/target/expr.rs
+++ b/tests/target/expr.rs
@@ -8,8 +8,8 @@ fn foo() -> bool {
let very_long_variable_name = (a + first + simple + test + AAAAAAAAAAAAA + BBBBBBBBBBBBBBBBB +
b + c);
- let is_internalxxxx = self.codemap.span_to_filename(s) ==
- self.codemap.span_to_filename(m.inner);
+ //FIXME this exceeds width limit. Needs assignments reformatting
+ let is_internalxxxx = self.codemap.span_to_filename(s) == self.codemap.span_to_filename(m.inner);
let some_val = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa * bbbb /
(bbbbbb - function_call(x, *very_long_pointer, y)) + 1000;
From 30b16bc47424f6aefcfbe7c2d9bb501262acbd24 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ga=C3=ABtan=20Cassiers?= <gaetan.cassiers@gmail.com>
Date: Fri, 17 Jul 2015 15:58:47 +0200
Subject: [PATCH 2/4] Move 'use' to Rewrite
Implements Rewrite for ViewPath
Behavior change: always use max_width instead of ideal_width for use
list rewrite. I think it looks better, was also suggested by @nrc in
https://github.com/nrc/rustfmt/issues/82#issuecomment-105314265
---
src/imports.rs | 229 +++++++++++++++++++++------------------
src/visitor.rs | 48 ++++----
tests/target/imports.rs | 4 +-
tests/target/multiple.rs | 7 +-
4 files changed, 148 insertions(+), 140 deletions(-)
diff --git a/src/imports.rs b/src/imports.rs
index 12ce3368eb3..722c6a87012 100644
--- a/src/imports.rs
+++ b/src/imports.rs
@@ -8,128 +8,147 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.
-use visitor::FmtVisitor;
use lists::{write_list, itemize_list, ListItem, ListFormatting, SeparatorTactic, ListTactic};
-use utils::{span_after, format_visibility};
+use utils::span_after;
+use rewrite::{Rewrite, RewriteContext};
+use config::Config;
use syntax::ast;
use syntax::parse::token;
use syntax::print::pprust;
-use syntax::codemap::Span;
+use syntax::codemap::{CodeMap, Span};
// TODO (some day) remove unused imports, expand globs, compress many single imports into a list import
-fn rewrite_single_use_list(path_str: String, vpi: ast::PathListItem, vis: &str) -> String {
- if let ast::PathListItem_::PathListIdent{ name, .. } = vpi.node {
- let name_str = token::get_ident(name).to_string();
- if path_str.len() == 0 {
- format!("{}use {};", vis, name_str)
- } else {
- format!("{}use {}::{};", vis, path_str, name_str)
- }
- } else {
- if path_str.len() != 0 {
- format!("{}use {};", vis, path_str)
- } else {
- // This catches the import: use {self}, which is a compiler error, so we just
- // leave it alone.
- format!("{}use {{self}};", vis)
+impl Rewrite for ast::ViewPath {
+ // Returns an empty string when the ViewPath is empty (like foo::bar::{})
+ fn rewrite(&self, context: &RewriteContext, width: usize, offset: usize) -> Option<String> {
+ match self.node {
+ ast::ViewPath_::ViewPathList(ref path, ref path_list) => {
+ Some(rewrite_use_list(width,
+ offset,
+ path,
+ path_list,
+ self.span,
+ context.codemap,
+ context.config).unwrap_or("".to_owned()))
+ }
+ ast::ViewPath_::ViewPathGlob(_) => {
+ // FIXME convert to list?
+ None
+ }
+ ast::ViewPath_::ViewPathSimple(_,_) => {
+ None
+ }
}
}
}
-impl<'a> FmtVisitor<'a> {
- // Basically just pretty prints a multi-item import.
- // Returns None when the import can be removed.
- pub fn rewrite_use_list(&self,
- block_indent: usize,
- one_line_budget: usize, // excluding indentation
- multi_line_budget: usize,
- path: &ast::Path,
- path_list: &[ast::PathListItem],
- visibility: ast::Visibility,
- span: Span)
- -> Option<String> {
- let path_str = pprust::path_to_string(path);
- let vis = format_visibility(visibility);
-
- match path_list.len() {
- 0 => return None,
- 1 => return Some(rewrite_single_use_list(path_str, path_list[0], vis)),
- _ => ()
- }
-
- // 2 = ::
- let path_separation_w = if path_str.len() > 0 {
- 2
+fn rewrite_single_use_list(path_str: String, vpi: ast::PathListItem) -> String {
+ if let ast::PathListItem_::PathListIdent{ name, .. } = vpi.node {
+ let name_str = token::get_ident(name).to_string();
+ if path_str.len() == 0 {
+ name_str
} else {
- 0
- };
- // 5 = "use " + {
- let indent = path_str.len() + 5 + path_separation_w + vis.len();
-
- // 2 = } + ;
- let used_width = indent + 2;
-
- // Break as early as possible when we've blown our budget.
- let remaining_line_budget = one_line_budget.checked_sub(used_width).unwrap_or(0);
- let remaining_multi_budget = multi_line_budget.checked_sub(used_width).unwrap_or(0);
-
- let fmt = ListFormatting {
- tactic: ListTactic::Mixed,
- separator: ",",
- trailing_separator: SeparatorTactic::Never,
- indent: block_indent + indent,
- h_width: remaining_line_budget,
- v_width: remaining_multi_budget,
- ends_with_newline: true,
- };
-
- let mut items = itemize_list(self.codemap,
- vec![ListItem::from_str("")], /* Dummy value, explanation
- * below */
- path_list.iter(),
- ",",
- "}",
- |vpi| vpi.span.lo,
- |vpi| vpi.span.hi,
- |vpi| match vpi.node {
- ast::PathListItem_::PathListIdent{ name, .. } => {
- token::get_ident(name).to_string()
- }
- ast::PathListItem_::PathListMod{ .. } => {
- "self".to_owned()
- }
- },
- span_after(span, "{", self.codemap),
- span.hi);
-
- // We prefixed the item list with a dummy value so that we can
- // potentially move "self" to the front of the vector without touching
- // the rest of the items.
- // FIXME: Make more efficient by using a linked list? That would
- // require changes to the signatures of itemize_list and write_list.
- let has_self = move_self_to_front(&mut items);
- let first_index = if has_self {
- 0
- } else {
- 1
- };
-
- if self.config.reorder_imports {
- items[1..].sort_by(|a, b| a.item.cmp(&b.item));
+ format!("{}::{}", path_str, name_str)
+ }
+ } else {
+ if path_str.len() != 0 {
+ path_str
+ } else {
+ // This catches the import: use {self}, which is a compiler error, so we just
+ // leave it alone.
+ "{self}".to_owned()
}
-
- let list = write_list(&items[first_index..], &fmt);
-
- Some(if path_str.len() == 0 {
- format!("{}use {{{}}};", vis, list)
- } else {
- format!("{}use {}::{{{}}};", vis, path_str, list)
- })
}
}
+// Basically just pretty prints a multi-item import.
+// Returns None when the import can be removed.
+pub fn rewrite_use_list(width: usize,
+ offset: usize,
+ path: &ast::Path,
+ path_list: &[ast::PathListItem],
+ span: Span,
+ codemap: &CodeMap,
+ config: &Config)
+ -> Option<String> {
+ let path_str = pprust::path_to_string(path);
+
+ match path_list.len() {
+ 0 => return None,
+ 1 => return Some(rewrite_single_use_list(path_str, path_list[0])),
+ _ => ()
+ }
+
+ // 2 = ::
+ let path_separation_w = if path_str.len() > 0 {
+ 2
+ } else {
+ 0
+ };
+ // 1 = {
+ let supp_indent = path_str.len() + path_separation_w + 1;
+ // 1 = }
+ let remaining_width = width.checked_sub(supp_indent + 1).unwrap_or(0);
+
+ let fmt = ListFormatting {
+ tactic: ListTactic::Mixed,
+ separator: ",",
+ trailing_separator: SeparatorTactic::Never,
+ indent: offset + supp_indent,
+ h_width: remaining_width,
+ // FIXME This is too conservative, and will not use all width
+ // available
+ // (loose 1 column (";"))
+ v_width: remaining_width,
+ ends_with_newline: true,
+ };
+
+ let mut items = itemize_list(codemap,
+ vec![ListItem::from_str("")], /* Dummy value, explanation
+ * below */
+ path_list.iter(),
+ ",",
+ "}",
+ |vpi| vpi.span.lo,
+ |vpi| vpi.span.hi,
+ |vpi| match vpi.node {
+ ast::PathListItem_::PathListIdent{ name, .. } => {
+ token::get_ident(name).to_string()
+ }
+ ast::PathListItem_::PathListMod{ .. } => {
+ "self".to_owned()
+ }
+ },
+ span_after(span, "{", codemap),
+ span.hi);
+
+ // We prefixed the item list with a dummy value so that we can
+ // potentially move "self" to the front of the vector without touching
+ // the rest of the items.
+ // FIXME: Make more efficient by using a linked list? That would
+ // require changes to the signatures of itemize_list and write_list.
+ let has_self = move_self_to_front(&mut items);
+ let first_index = if has_self {
+ 0
+ } else {
+ 1
+ };
+
+ if config.reorder_imports {
+ items[1..].sort_by(|a, b| a.item.cmp(&b.item));
+ }
+
+ let list = write_list(&items[first_index..], &fmt);
+
+ Some(if path_str.len() == 0 {
+ format!("{{{}}}", list)
+ } else {
+ format!("{}::{{{}}}", path_str, list)
+ })
+}
+
// Returns true when self item was found.
fn move_self_to_front(items: &mut Vec<ListItem>) -> bool {
match items.iter().position(|item| item.item == "self") {
diff --git a/src/visitor.rs b/src/visitor.rs
index 594d3328595..375107c00ea 100644
--- a/src/visitor.rs
+++ b/src/visitor.rs
@@ -164,39 +164,29 @@ impl<'a, 'v> visit::Visitor<'v> for FmtVisitor<'a> {
match item.node {
ast::Item_::ItemUse(ref vp) => {
- match vp.node {
- ast::ViewPath_::ViewPathList(ref path, ref path_list) => {
- let block_indent = self.block_indent;
- let one_line_budget = self.config.max_width - block_indent;
- let multi_line_budget = self.config.ideal_width - block_indent;
- let formatted = self.rewrite_use_list(block_indent,
- one_line_budget,
- multi_line_budget,
- path,
- path_list,
- item.vis,
- item.span);
-
- if let Some(new_str) = formatted {
- self.format_missing_with_indent(item.span.lo);
- self.changes.push_str_span(item.span, &new_str);
- } else {
- // Format up to last newline
- let span = codemap::mk_sp(self.last_pos, item.span.lo);
- let span_end = match self.snippet(span).rfind('\n') {
- Some(offset) => self.last_pos + BytePos(offset as u32),
- None => item.span.lo
- };
- self.format_missing(span_end);
- }
-
+ let vis = utils::format_visibility(item.vis);
+ let offset = self.block_indent + vis.len() + "use ".len();
+ let context = RewriteContext {
+ codemap: self.codemap, config: self.config, block_indent: self.block_indent };
+ // 1 = ";"
+ match vp.rewrite(&context, self.config.max_width - offset - 1, offset) {
+ Some(ref s) if s.len() == 0 => {
+ // Format up to last newline
+ let span = codemap::mk_sp(self.last_pos, item.span.lo);
+ let span_end = match self.snippet(span).rfind('\n') {
+ Some(offset) => self.last_pos + BytePos(offset as u32),
+ None => item.span.lo
+ };
+ self.format_missing(span_end);
self.last_pos = item.span.hi;
}
- ast::ViewPath_::ViewPathGlob(_) => {
+ Some(ref s) => {
+ let s = format!("{}use {};", vis, s);
self.format_missing_with_indent(item.span.lo);
- // FIXME convert to list?
+ self.changes.push_str_span(item.span, &s);
+ self.last_pos = item.span.hi;
}
- ast::ViewPath_::ViewPathSimple(_,_) => {
+ None => {
self.format_missing_with_indent(item.span.lo);
}
}
diff --git a/tests/target/imports.rs b/tests/target/imports.rs
index 372b4f2051f..a799c096210 100644
--- a/tests/target/imports.rs
+++ b/tests/target/imports.rs
@@ -23,8 +23,8 @@ mod Foo {
pub use syntax::ast::{ItemForeignMod, ItemImpl, ItemMac, ItemMod, ItemStatic, ItemDefaultImpl};
mod Foo2 {
- pub use syntax::ast::{self, ItemForeignMod, ItemImpl, ItemMac, ItemMod,
- ItemStatic, ItemDefaultImpl};
+ pub use syntax::ast::{self, ItemForeignMod, ItemImpl, ItemMac, ItemMod, ItemStatic,
+ ItemDefaultImpl};
}
}
diff --git a/tests/target/multiple.rs b/tests/target/multiple.rs
index 0fb0c94e3a8..5573ea7e96e 100644
--- a/tests/target/multiple.rs
+++ b/tests/target/multiple.rs
@@ -14,10 +14,9 @@ extern crate foo;
extern crate foo;
use std::cell::*;
-use std::{self, any, ascii, borrow, boxed, char, borrow, boxed, char, borrow,
- borrow, boxed, char, borrow, boxed, char, borrow, boxed, char,
- borrow, boxed, char, borrow, boxed, char, borrow, boxed, char,
- borrow, boxed, char, borrow, boxed, char};
+use std::{self, any, ascii, borrow, boxed, char, borrow, boxed, char, borrow, borrow, boxed, char,
+ borrow, boxed, char, borrow, boxed, char, borrow, boxed, char, borrow, boxed, char,
+ borrow, boxed, char, borrow, boxed, char, borrow, boxed, char};
mod doc;
mod other;
From 5168d7458a6fc532a3fcb672246898d696867264 Mon Sep 17 00:00:00 2001
From: cassiersg <cassiersg@users.noreply.github.com>
Date: Mon, 20 Jul 2015 21:14:45 +0200
Subject: [PATCH 3/4] Indent fix
---
src/imports.rs | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/imports.rs b/src/imports.rs
index 722c6a87012..37b1d9e5d46 100644
--- a/src/imports.rs
+++ b/src/imports.rs
@@ -26,12 +26,12 @@ impl Rewrite for ast::ViewPath {
match self.node {
ast::ViewPath_::ViewPathList(ref path, ref path_list) => {
Some(rewrite_use_list(width,
- offset,
- path,
- path_list,
- self.span,
- context.codemap,
- context.config).unwrap_or("".to_owned()))
+ offset,
+ path,
+ path_list,
+ self.span,
+ context.codemap,
+ context.config).unwrap_or("".to_owned()))
}
ast::ViewPath_::ViewPathGlob(_) => {
// FIXME convert to list?
From 92b3f69934efb61433223fc10888746e0c705afe Mon Sep 17 00:00:00 2001
From: cassiersg <cassiersg@users.noreply.github.com>
Date: Mon, 20 Jul 2015 21:33:23 +0200
Subject: [PATCH 4/4] Add a helper method to format imports
---
src/visitor.rs | 62 +++++++++++++++++++++++++++-----------------------
1 file changed, 34 insertions(+), 28 deletions(-)
diff --git a/src/visitor.rs b/src/visitor.rs
index 375107c00ea..6e550c2f330 100644
--- a/src/visitor.rs
+++ b/src/visitor.rs
@@ -164,33 +164,7 @@ impl<'a, 'v> visit::Visitor<'v> for FmtVisitor<'a> {
match item.node {
ast::Item_::ItemUse(ref vp) => {
- let vis = utils::format_visibility(item.vis);
- let offset = self.block_indent + vis.len() + "use ".len();
- let context = RewriteContext {
- codemap: self.codemap, config: self.config, block_indent: self.block_indent };
- // 1 = ";"
- match vp.rewrite(&context, self.config.max_width - offset - 1, offset) {
- Some(ref s) if s.len() == 0 => {
- // Format up to last newline
- let span = codemap::mk_sp(self.last_pos, item.span.lo);
- let span_end = match self.snippet(span).rfind('\n') {
- Some(offset) => self.last_pos + BytePos(offset as u32),
- None => item.span.lo
- };
- self.format_missing(span_end);
- self.last_pos = item.span.hi;
- }
- Some(ref s) => {
- let s = format!("{}use {};", vis, s);
- self.format_missing_with_indent(item.span.lo);
- self.changes.push_str_span(item.span, &s);
- self.last_pos = item.span.hi;
- }
- None => {
- self.format_missing_with_indent(item.span.lo);
- }
- }
- visit::walk_item(self, item);
+ self.format_import(item.vis, vp, item.span);
}
ast::Item_::ItemImpl(..) |
ast::Item_::ItemTrait(..) => {
@@ -211,7 +185,6 @@ impl<'a, 'v> visit::Visitor<'v> for FmtVisitor<'a> {
def,
generics,
item.span);
- self.last_pos = item.span.hi;
}
ast::Item_::ItemEnum(ref def, ref generics) => {
self.format_missing_with_indent(item.span.lo);
@@ -408,4 +381,37 @@ impl<'a> FmtVisitor<'a> {
self.last_pos = last_pos;
self.block_indent = block_indent;
}
+
+ fn format_import(&mut self, vis: ast::Visibility, vp: &ast::ViewPath, span: Span) {
+ let vis = utils::format_visibility(vis);
+ let offset = self.block_indent + vis.len() + "use ".len();
+ let context = RewriteContext {
+ codemap: self.codemap,
+ config: self.config,
+ block_indent: self.block_indent,
+ };
+ // 1 = ";"
+ match vp.rewrite(&context, self.config.max_width - offset - 1, offset) {
+ Some(ref s) if s.len() == 0 => {
+ // Format up to last newline
+ let prev_span = codemap::mk_sp(self.last_pos, span.lo);
+ let span_end = match self.snippet(prev_span).rfind('\n') {
+ Some(offset) => self.last_pos + BytePos(offset as u32),
+ None => span.lo
+ };
+ self.format_missing(span_end);
+ self.last_pos = span.hi;
+ }
+ Some(ref s) => {
+ let s = format!("{}use {};", vis, s);
+ self.format_missing_with_indent(span.lo);
+ self.changes.push_str_span(span, &s);
+ self.last_pos = span.hi;
+ }
+ None => {
+ self.format_missing_with_indent(span.lo);
+ self.format_missing(span.hi);
+ }
+ }
+ }
}