Skip to content

Fix a bunch of misc. bugs I found #245

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

Merged
merged 4 commits into from
Sep 2, 2015
Merged

Fix a bunch of misc. bugs I found #245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
97e92b3
Preserve some whitespace between struct fields etc.
nrc Sep 2, 2015
a5f8b37
Format match expressions properly when they appear on an overflowing …
nrc Sep 2, 2015
ce4d5aa
Fix a bug with alignment in one-line match arms
nrc Sep 2, 2015
183dac9
Fix a bug where struct lits nested in fn calls were over-indented
nrc Sep 2, 2015
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
42 changes: 22 additions & 20 deletions src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ fn rewrite_closure(capture: ast::CaptureClause,

let fmt = ListFormatting::for_fn(argument_budget, argument_offset);
let prefix = format!("{}|{}|", mover, write_list(&arg_items.collect::<Vec<_>>(), &fmt));
let block_indent = closure_block_indent(context, offset);
let closure_indent = closure_indent(context, offset);

// Try to format closure body as a single line expression without braces.
if body.stmts.is_empty() {
Expand Down Expand Up @@ -232,11 +232,11 @@ fn rewrite_closure(capture: ast::CaptureClause,

// We couldn't format the closure body as a single line expression; fall
// back to block formatting.
let inner_context = &RewriteContext { block_indent: block_indent, ..*context };
let inner_context = context.overflow_context(closure_indent - context.block_indent);
let body_rewrite = if let ast::Expr_::ExprBlock(ref inner) = body.expr.as_ref().unwrap().node {
inner.rewrite(inner_context, 0, 0)
inner.rewrite(&inner_context, 0, 0)
} else {
body.rewrite(inner_context, 0, 0)
body.rewrite(&inner_context, 0, 0)
};

Some(format!("{} {}", prefix, try_opt!(body_rewrite)))
Expand All @@ -250,7 +250,7 @@ impl Rewrite for ast::Block {
}

let mut visitor = FmtVisitor::from_codemap(context.codemap, context.config);
visitor.block_indent = context.block_indent;
visitor.block_indent = context.block_indent + context.overflow_indent;

let prefix = match self.rules {
ast::BlockCheckMode::PushUnsafeBlock(..) |
Expand Down Expand Up @@ -541,9 +541,9 @@ fn rewrite_match(context: &RewriteContext,
let cond_str = try_opt!(cond.rewrite(context, cond_budget, offset + 6));
let mut result = format!("match {} {{", cond_str);

let block_indent = context.block_indent;
let nested_context = context.nested_context();
let arm_indent_str = make_indent(nested_context.block_indent);
let arm_indent = nested_context.block_indent + context.overflow_indent;
let arm_indent_str = make_indent(arm_indent);

let open_brace_pos = span_after(mk_sp(cond.span.hi, arm_start_pos(&arms[0])),
"{",
Expand Down Expand Up @@ -578,9 +578,8 @@ fn rewrite_match(context: &RewriteContext,
result.push_str(&arm_indent_str);

let arm_str = arm.rewrite(&nested_context,
context.config.max_width -
nested_context.block_indent,
nested_context.block_indent);
context.config.max_width - arm_indent,
arm_indent);
if let Some(ref arm_str) = arm_str {
result.push_str(arm_str);
} else {
Expand All @@ -594,7 +593,7 @@ fn rewrite_match(context: &RewriteContext,
// match expression, but meh.

result.push('\n');
result.push_str(&make_indent(block_indent));
result.push_str(&make_indent(context.block_indent + context.overflow_indent));
result.push('}');
Some(result)
}
Expand Down Expand Up @@ -694,15 +693,14 @@ impl Rewrite for ast::Arm {
} else {
","
};
let nested_indent = context.block_indent + context.config.tab_spaces;

// Let's try and get the arm body on the same line as the condition.
// 4 = ` => `.len()
if context.config.max_width > line_start + comma.len() + 4 {
let budget = context.config.max_width - line_start - comma.len() - 4;
if let Some(ref body_str) = body.rewrite(context,
budget,
nested_indent) {
line_start + 4) {
if first_line_width(body_str) <= budget {
return Some(format!("{}{} => {}{}",
attr_str.trim_left(),
Expand All @@ -720,7 +718,9 @@ impl Rewrite for ast::Arm {
}

let body_budget = try_opt!(width.checked_sub(context.config.tab_spaces));
let body_str = try_opt!(body.rewrite(context, body_budget, nested_indent));
let body_str = try_opt!(body.rewrite(context,
body_budget,
context.block_indent));
Some(format!("{}{} =>\n{}{},",
attr_str.trim_left(),
pats_str,
Expand Down Expand Up @@ -868,8 +868,8 @@ fn rewrite_call(context: &RewriteContext,
// 2 is for parens.
let remaining_width = try_opt!(width.checked_sub(extra_offset + 2));
let offset = offset + extra_offset + 1;
let block_indent = expr_block_indent(context, offset);
let inner_context = &RewriteContext { block_indent: block_indent, ..*context };
let inner_indent = expr_indent(context, offset);
let inner_context = context.overflow_context(inner_indent - context.block_indent);

let items = itemize_list(context.codemap,
args.iter(),
Expand All @@ -878,7 +878,7 @@ fn rewrite_call(context: &RewriteContext,
|item| item.span.hi,
// Take old span when rewrite fails.
|item| {
item.rewrite(inner_context, remaining_width, offset)
item.rewrite(&inner_context, remaining_width, offset)
.unwrap_or(context.snippet(item.span))
},
callee.span.hi + BytePos(1),
Expand All @@ -901,8 +901,8 @@ macro_rules! block_indent_helper {
);
}

block_indent_helper!(expr_block_indent, expr_indent_style);
block_indent_helper!(closure_block_indent, closure_indent_style);
block_indent_helper!(expr_indent, expr_indent_style);
block_indent_helper!(closure_indent, closure_indent_style);

fn rewrite_paren(context: &RewriteContext,
subexpr: &ast::Expr,
Expand Down Expand Up @@ -1192,7 +1192,9 @@ pub fn rewrite_assign_rhs<S: Into<String>>(context: &RewriteContext,
result.push_str(&format!("\n{}", make_indent(new_offset)));

let max_width = try_opt!(context.config.max_width.checked_sub(new_offset + 1));
let rhs = try_opt!(ex.rewrite(&context, max_width, new_offset));
let rhs = try_opt!(ex.rewrite(&context.overflow_context(context.config.tab_spaces),
max_width,
new_offset));

result.push_str(&rhs);
}
Expand Down
4 changes: 1 addition & 3 deletions src/items.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,13 +632,11 @@ impl<'a> FmtVisitor<'a> {
let break_line = !is_tuple || generics_str.contains('\n') ||
single_line_cost as usize + used_budget > self.config.max_width;

if break_line {
let tactic = if break_line {
let indentation = make_indent(offset + self.config.tab_spaces);
result.push('\n');
result.push_str(&indentation);
}

let tactic = if break_line {
ListTactic::Vertical
} else {
ListTactic::Horizontal
Expand Down
62 changes: 45 additions & 17 deletions src/lists.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,11 @@ impl<'a> ListFormatting<'a> {

pub struct ListItem {
pub pre_comment: Option<String>,
// Item should include attributes and doc comments
// Item should include attributes and doc comments.
pub item: String,
pub post_comment: Option<String>,
// Whether there is extra whitespace before this item.
pub new_lines: bool,
}

impl ListItem {
Expand All @@ -86,7 +88,7 @@ impl ListItem {
}

pub fn from_str<S: Into<String>>(s: S) -> ListItem {
ListItem { pre_comment: None, item: s.into(), post_comment: None }
ListItem { pre_comment: None, item: s.into(), post_comment: None, new_lines: false }
}
}

Expand Down Expand Up @@ -206,10 +208,8 @@ pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> St

// Post-comments
if tactic != ListTactic::Vertical && item.post_comment.is_some() {
let formatted_comment = rewrite_comment(item.post_comment.as_ref().unwrap(),
true,
formatting.v_width,
0);
let comment = item.post_comment.as_ref().unwrap();
let formatted_comment = rewrite_comment(comment, true, formatting.v_width, 0);

result.push(' ');
result.push_str(&formatted_comment);
Expand All @@ -234,6 +234,10 @@ pub fn write_list<'b>(items: &[ListItem], formatting: &ListFormatting<'b>) -> St
result.push(' ');
result.push_str(&formatted_comment);
}

if !last && tactic == ListTactic::Vertical && item.new_lines {
result.push('\n');
}
}

result
Expand Down Expand Up @@ -264,13 +268,14 @@ impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3>
let white_space: &[_] = &[' ', '\t'];

self.inner.next().map(|item| {
let mut new_lines = false;
// Pre-comment
let pre_snippet = self.codemap.span_to_snippet(codemap::mk_sp(self.prev_span_end,
(self.get_lo)(&item)))
.unwrap();
let pre_snippet = pre_snippet.trim();
let pre_comment = if !pre_snippet.is_empty() {
Some(pre_snippet.to_owned())
let trimmed_pre_snippet = pre_snippet.trim();
let pre_comment = if !trimmed_pre_snippet.is_empty() {
Some(trimmed_pre_snippet.to_owned())
} else {
None
};
Expand Down Expand Up @@ -307,7 +312,7 @@ impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3>
separator_index + 1)
}
// Potential *single* line comment.
(_, Some(j)) => { j + 1 }
(_, Some(j)) => j + 1,
_ => post_snippet.len()
}
},
Expand All @@ -317,18 +322,40 @@ impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3>
}
};

if !post_snippet.is_empty() && comment_end > 0 {
// Account for extra whitespace between items. This is fiddly
// because of the way we divide pre- and post- comments.

// Everything from the separator to the next item.
let test_snippet = &post_snippet[comment_end-1..];
let first_newline = test_snippet.find('\n').unwrap_or(test_snippet.len());
// From the end of the first line of comments.
let test_snippet = &test_snippet[first_newline..];
let first = test_snippet.find(|c: char| !c.is_whitespace())
.unwrap_or(test_snippet.len());
// From the end of the first line of comments to the next non-whitespace char.
let test_snippet = &test_snippet[..first];

if test_snippet.chars().filter(|c| c == &'\n').count() > 1 {
// There were multiple line breaks which got trimmed to nothing.
new_lines = true;
}
}

// Cleanup post-comment: strip separators and whitespace.
self.prev_span_end = (self.get_hi)(&item) + BytePos(comment_end as u32);
let mut post_snippet = post_snippet[..comment_end].trim();
let post_snippet = post_snippet[..comment_end].trim();

if post_snippet.starts_with(',') {
post_snippet = post_snippet[1..].trim_matches(white_space);
let post_snippet_trimmed = if post_snippet.starts_with(',') {
post_snippet[1..].trim_matches(white_space)
} else if post_snippet.ends_with(",") {
post_snippet = post_snippet[..(post_snippet.len() - 1)].trim_matches(white_space);
}
post_snippet[..(post_snippet.len() - 1)].trim_matches(white_space)
} else {
post_snippet
};

let post_comment = if !post_snippet.is_empty() {
Some(post_snippet.to_owned())
let post_comment = if !post_snippet_trimmed.is_empty() {
Some(post_snippet_trimmed.to_owned())
} else {
None
};
Expand All @@ -337,6 +364,7 @@ impl<'a, T, I, F1, F2, F3> Iterator for ListItems<'a, I, F1, F2, F3>
pre_comment: pre_comment,
item: (self.get_item_string)(&item),
post_comment: post_comment,
new_lines: new_lines,
}
})
}
Expand Down
17 changes: 17 additions & 0 deletions src/rewrite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,14 @@ pub trait Rewrite {
pub struct RewriteContext<'a> {
pub codemap: &'a CodeMap,
pub config: &'a Config,

// Indentation due to nesting of blocks.
pub block_indent: usize,
// *Extra* indentation due to overflowing to the next line, e.g.,
// let foo =
// bar();
// The extra 4 spaces when formatting `bar()` is overflow_indent.
pub overflow_indent: usize,
}

impl<'a> RewriteContext<'a> {
Expand All @@ -37,6 +44,16 @@ impl<'a> RewriteContext<'a> {
codemap: self.codemap,
config: self.config,
block_indent: self.block_indent + self.config.tab_spaces,
overflow_indent: self.overflow_indent,
}
}

pub fn overflow_context(&self, overflow: usize) -> RewriteContext<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there a separation between regular block indentation and overflow indentation? Can't we simply set block_indent += config.tab_spaces?

Copy link
Member Author

Choose a reason for hiding this comment

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

The intention with block indent is that it tracks only that component of indentation, as opposed to the offset/indent which we pass around which is the current indent. It is useful to carry round the block indent in case we want to block indent some part of a sub-expression.

E.g.,

let foo = match {
    bar => {
        foo();
    }
    baz => qux,
};

let foo2 =
    bar2();

In the first case we ignore the overflow, in the second case we use it. If we saved the overflow into the block indent, then we could not ignore it and match expressions would be over-indented. (Note that this case is easier because the overflow is the block indent, but it might be a visual indent, in which case there is no way to recover the block indent).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the thorough explanation. That makes sense.

RewriteContext {
codemap: self.codemap,
config: self.config,
block_indent: self.block_indent,
overflow_indent: overflow,
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/visitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ impl<'a> FmtVisitor<'a> {
codemap: self.codemap,
config: self.config,
block_indent: self.block_indent,
overflow_indent: 0,
};
// 1 = ";"
match vp.rewrite(&context, self.config.max_width - offset - 1, offset) {
Expand Down Expand Up @@ -369,6 +370,7 @@ impl<'a> FmtVisitor<'a> {
codemap: self.codemap,
config: self.config,
block_indent: self.block_indent,
overflow_indent: 0,
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions tests/source/struct_lits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ fn main() {
second: Item
};

Some(Data::MethodCallData(MethodCallData {
span: sub_span.unwrap(),
scope: self.enclosing_scope(id),
ref_id: def_id,
decl_id: Some(decl_id),
}));

Diagram { /* o This graph demonstrates how
* / \ significant whitespace is
* o o preserved.
Expand Down
28 changes: 28 additions & 0 deletions tests/source/structs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,43 @@ pub struct Foo<'a, Y: Baz>
}

struct Baz {

a: A, // Comment A
b: B, // Comment B
c: C, // Comment C

}

struct Baz {
a: A, // Comment A

b: B, // Comment B




c: C, // Comment C
}

struct Baz {

a: A,

b: B,
c: C,




d: D

}

struct Baz
{
// Comment A
a: A,

// Comment B
b: B,
// Comment C
Expand Down
Loading