From 6899471497614d73f8bc13ac074d7624b4554091 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Fri, 20 Jul 2018 14:27:56 +1200 Subject: [PATCH 1/2] Check for comments after the `=>` in a match arm Closes #2188 --- rustfmt.toml | 2 +- src/matches.rs | 70 +++++++++++++++++++-------- tests/target/comment-not-disappear.rs | 4 +- 3 files changed, 54 insertions(+), 22 deletions(-) diff --git a/rustfmt.toml b/rustfmt.toml index c1f797409c39..6abb5a9ef6d1 100644 --- a/rustfmt.toml +++ b/rustfmt.toml @@ -1,3 +1,3 @@ error_on_line_overflow = true error_on_unformatted = true -edition = "Edition2018" \ No newline at end of file +edition = "Edition2018" diff --git a/src/matches.rs b/src/matches.rs index 201e10d5ccf9..791f34cefe8d 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -17,7 +17,7 @@ use syntax::codemap::{BytePos, Span}; use syntax::{ast, ptr}; use codemap::SpanUtils; -use comment::combine_strs_with_missing_comments; +use comment::{combine_strs_with_missing_comments, rewrite_comment}; use config::{Config, ControlBraceStyle, IndentStyle}; use expr::{ format_expr, is_empty_block, is_simple_block, is_unsafe_block, prefer_next_line, @@ -267,12 +267,15 @@ fn rewrite_match_arm( false, ) })?; + + let arrow_span = mk_sp(arm.pats.last().unwrap().span.hi(), arm.body.span.lo()); rewrite_match_body( context, &arm.body, &pats_str, shape, arm.guard.is_some(), + arrow_span, is_last, ) } @@ -345,6 +348,7 @@ fn rewrite_match_body( pats_str: &str, shape: Shape, has_guard: bool, + arrow_span: Span, is_last: bool, ) -> Option { let (extend, body) = flatten_arm_body(context, body); @@ -369,24 +373,43 @@ fn rewrite_match_body( Some(format!("{} =>{}{}{}", pats_str, block_sep, body_str, comma)) }; - let forbid_same_line = has_guard && pats_str.contains('\n') && !is_empty_block; let next_line_indent = if !is_block || is_empty_block { shape.indent.block_indent(context.config) } else { shape.indent }; + + let forbid_same_line = has_guard && pats_str.contains('\n') && !is_empty_block; + + // Look for comments between `=>` and the start of the body. + let arrow_comment = { + let arrow_snippet = context.snippet(arrow_span).trim(); + let arrow_index = arrow_snippet.find("=>").unwrap(); + // 2 = `=>` + let comment_str = arrow_snippet[arrow_index + 2..].trim(); + if comment_str.is_empty() { + String::new() + } else { + rewrite_comment(comment_str, false, shape, &context.config)? + } + }; + let combine_next_line_body = |body_str: &str| { + let nested_indent_str = next_line_indent.to_string_with_newline(context.config); + if is_block { - return Some(format!( - "{} =>{}{}", - pats_str, - next_line_indent.to_string_with_newline(context.config), - body_str - )); + let mut result = pats_str.to_owned(); + result.push_str(" =>"); + if !arrow_comment.is_empty() { + result.push_str(&nested_indent_str); + result.push_str(&arrow_comment); + } + result.push_str(&nested_indent_str); + result.push_str(&body_str); + return Some(result); } let indent_str = shape.indent.to_string_with_newline(context.config); - let nested_indent_str = next_line_indent.to_string_with_newline(context.config); let (body_prefix, body_suffix) = if context.config.match_arm_blocks() { let comma = if context.config.match_block_trailing_comma() { "," @@ -401,14 +424,22 @@ fn rewrite_match_body( let block_sep = match context.config.control_brace_style() { ControlBraceStyle::AlwaysNextLine => format!("{}{}", alt_block_sep, body_prefix), _ if body_prefix.is_empty() => "".to_owned(), - _ if forbid_same_line => format!("{}{}", alt_block_sep, body_prefix), + _ if forbid_same_line || !arrow_comment.is_empty() => { + format!("{}{}", alt_block_sep, body_prefix) + } _ => format!(" {}", body_prefix), } + &nested_indent_str; - Some(format!( - "{} =>{}{}{}", - pats_str, block_sep, body_str, body_suffix - )) + let mut result = pats_str.to_owned(); + result.push_str(" =>"); + if !arrow_comment.is_empty() { + result.push_str(&indent_str); + result.push_str(&arrow_comment); + } + result.push_str(&block_sep); + result.push_str(&body_str); + result.push_str(&body_suffix); + Some(result) }; // Let's try and get the arm body on the same line as the condition. @@ -416,7 +447,9 @@ fn rewrite_match_body( let orig_body_shape = shape .offset_left(extra_offset(pats_str, shape) + 4) .and_then(|shape| shape.sub_width(comma.len())); - let orig_body = if let Some(body_shape) = orig_body_shape { + let orig_body = if forbid_same_line || !arrow_comment.is_empty() { + None + } else if let Some(body_shape) = orig_body_shape { let rewrite = nop_block_collapse( format_expr(body, ExprType::Statement, context, body_shape), body_shape.width, @@ -424,9 +457,7 @@ fn rewrite_match_body( match rewrite { Some(ref body_str) - if !forbid_same_line - && (is_block - || (!body_str.contains('\n') && body_str.len() <= body_shape.width)) => + if is_block || (!body_str.contains('\n') && body_str.len() <= body_shape.width) => { return combine_orig_body(body_str); } @@ -445,8 +476,7 @@ fn rewrite_match_body( ); match (orig_body, next_line_body) { (Some(ref orig_str), Some(ref next_line_str)) - if forbid_same_line - || prefer_next_line(orig_str, next_line_str, RhsTactics::Default) => + if prefer_next_line(orig_str, next_line_str, RhsTactics::Default) => { combine_next_line_body(next_line_str) } diff --git a/tests/target/comment-not-disappear.rs b/tests/target/comment-not-disappear.rs index 646b37e46e2e..b1fa0ff6fe11 100644 --- a/tests/target/comment-not-disappear.rs +++ b/tests/target/comment-not-disappear.rs @@ -11,8 +11,10 @@ fn a() { fn b() { match x { X => - // A comment + // A comment + { y + } } } From b085113cbe77b558624bbd2acb098956f5d6f266 Mon Sep 17 00:00:00 2001 From: Nick Cameron Date: Fri, 20 Jul 2018 14:42:48 +1200 Subject: [PATCH 2/2] Trigger an internal error if we skip formatting due to a lost comment --- src/comment.rs | 13 ++++++++++++- src/lib.rs | 28 ++++++++++++++++++++++------ 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/comment.rs b/src/comment.rs index 53e496bf4574..3d55f6560e3e 100644 --- a/src/comment.rs +++ b/src/comment.rs @@ -20,6 +20,7 @@ use rewrite::RewriteContext; use shape::{Indent, Shape}; use string::{rewrite_string, StringFormat}; use utils::{count_newlines, first_line_width, last_line_width}; +use {ErrorKind, FormattingError}; fn is_custom_comment(comment: &str) -> bool { if !comment.starts_with("//") { @@ -1124,7 +1125,17 @@ pub fn recover_comment_removed( ) -> Option { let snippet = context.snippet(span); if snippet != new && changed_comment_content(snippet, &new) { - // We missed some comments. Keep the original text. + // We missed some comments. Warn and keep the original text. + if context.config.error_on_unformatted() { + context.report.append( + context.codemap.span_to_filename(span).into(), + vec![FormattingError::from_span( + &span, + &context.codemap, + ErrorKind::LostComment, + )], + ); + } Some(snippet.to_owned()) } else { Some(new) diff --git a/src/lib.rs b/src/lib.rs index b856c66dd71e..820134c2085d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -140,8 +140,20 @@ pub enum ErrorKind { ParseError, /// The user mandated a version and the current version of Rustfmt does not /// satisfy that requirement. - #[fail(display = "Version mismatch")] + #[fail(display = "version mismatch")] VersionMismatch, + /// If we had formatted the given node, then we would have lost a comment. + #[fail(display = "not formatted because a comment would be lost")] + LostComment, +} + +impl ErrorKind { + fn is_comment(&self) -> bool { + match self { + ErrorKind::LostComment => true, + _ => false, + } + } } impl From for ErrorKind { @@ -162,8 +174,8 @@ impl FormattingError { fn from_span(span: &Span, codemap: &CodeMap, kind: ErrorKind) -> FormattingError { FormattingError { line: codemap.lookup_char_pos(span.lo()).line, + is_comment: kind.is_comment(), kind, - is_comment: false, is_string: false, line_buffer: codemap .span_to_lines(*span) @@ -181,7 +193,8 @@ impl FormattingError { ErrorKind::LineOverflow(..) | ErrorKind::TrailingWhitespace | ErrorKind::IoError(_) - | ErrorKind::ParseError => "internal error:", + | ErrorKind::ParseError + | ErrorKind::LostComment => "internal error:", ErrorKind::LicenseCheck | ErrorKind::BadAttr | ErrorKind::VersionMismatch => "error:", ErrorKind::BadIssue(_) | ErrorKind::DeprecatedAttr => "warning:", } @@ -200,7 +213,10 @@ impl FormattingError { fn format_len(&self) -> (usize, usize) { match self.kind { ErrorKind::LineOverflow(found, max) => (max, found - max), - ErrorKind::TrailingWhitespace | ErrorKind::DeprecatedAttr | ErrorKind::BadAttr => { + ErrorKind::TrailingWhitespace + | ErrorKind::DeprecatedAttr + | ErrorKind::BadAttr + | ErrorKind::LostComment => { let trailing_ws_start = self .line_buffer .rfind(|c: char| !c.is_whitespace()) @@ -501,7 +517,7 @@ fn should_report_error( is_string: bool, error_kind: &ErrorKind, ) -> bool { - let allow_error_report = if char_kind.is_comment() || is_string { + let allow_error_report = if char_kind.is_comment() || is_string || error_kind.is_comment() { config.error_on_unformatted() } else { true @@ -509,7 +525,7 @@ fn should_report_error( match error_kind { ErrorKind::LineOverflow(..) => config.error_on_line_overflow() && allow_error_report, - ErrorKind::TrailingWhitespace => allow_error_report, + ErrorKind::TrailingWhitespace | ErrorKind::LostComment => allow_error_report, _ => true, } }