From 42e38e2f058344ced98627d62275f1923335db2b Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Sun, 28 Mar 2021 18:59:24 +0300 Subject: [PATCH] Fix for issue 4499 - Skip in struct impl (v2) --- src/formatting/items.rs | 10 +++- src/formatting/missed_spans.rs | 28 +++++++-- src/formatting/visitor.rs | 57 +++++++++++++----- tests/source/skip/skip-and-fn.rs | 55 ++++++++++++++++++ tests/source/skip/skip-and-impl.rs | 86 ++++++++++++++++++++++++++++ tests/source/skip/skip-and-import.rs | 18 ++++++ tests/source/skip/skip-and-trait.rs | 34 +++++++++++ tests/target/issue-4398.rs | 2 +- tests/target/skip/skip-and-fn.rs | 55 ++++++++++++++++++ tests/target/skip/skip-and-impl.rs | 85 +++++++++++++++++++++++++++ tests/target/skip/skip-and-import.rs | 18 ++++++ tests/target/skip/skip-and-trait.rs | 34 +++++++++++ 12 files changed, 462 insertions(+), 20 deletions(-) create mode 100644 tests/source/skip/skip-and-fn.rs create mode 100644 tests/source/skip/skip-and-impl.rs create mode 100644 tests/source/skip/skip-and-import.rs create mode 100644 tests/source/skip/skip-and-trait.rs create mode 100644 tests/target/skip/skip-and-fn.rs create mode 100644 tests/target/skip/skip-and-impl.rs create mode 100644 tests/target/skip/skip-and-import.rs create mode 100644 tests/target/skip/skip-and-trait.rs diff --git a/src/formatting/items.rs b/src/formatting/items.rs index 82677696e6a..0b33a738e75 100644 --- a/src/formatting/items.rs +++ b/src/formatting/items.rs @@ -892,8 +892,14 @@ pub(crate) fn format_impl( let inner_indent_str = visitor.block_indent.to_string_with_newline(context.config); let outer_indent_str = offset.block_only().to_string_with_newline(context.config); - result.push_str(&inner_indent_str); - result.push_str(visitor.buffer.trim()); + if visitor.buffer.starts_with("\n") { + // Starts with kipped code - first line not formatted + result.push_str(visitor.buffer.trim_end()); + } else { + // First line not kipped code - first line is formatted + result.push_str(&inner_indent_str); + result.push_str(visitor.buffer.trim()); + } result.push_str(&outer_indent_str); } else if need_newline || !context.config.empty_item_single_line() { result.push_str(&sep); diff --git a/src/formatting/missed_spans.rs b/src/formatting/missed_spans.rs index 83af916c5aa..ff89870a044 100644 --- a/src/formatting/missed_spans.rs +++ b/src/formatting/missed_spans.rs @@ -50,13 +50,15 @@ impl<'a> FmtVisitor<'a> { self.last_pos = end; return; } - self.format_missing_inner(end, |this, last_snippet, _| this.push_str(last_snippet)); + self.format_missing_inner(end, true, |this, last_snippet, _| { + this.push_str(last_snippet) + }); self.normalize_vertical_spaces = false; } pub(crate) fn format_missing_with_indent(&mut self, end: BytePos) { let config = self.config; - self.format_missing_inner(end, |this, last_snippet, snippet| { + self.format_missing_inner(end, true, |this, last_snippet, snippet| { this.push_str(last_snippet.trim_end()); if last_snippet == snippet && !this.output_at_start() { // No new lines in the snippet. @@ -69,15 +71,23 @@ impl<'a> FmtVisitor<'a> { } pub(crate) fn format_missing_no_indent(&mut self, end: BytePos) { - self.format_missing_inner(end, |this, last_snippet, _| { + self.format_missing_inner(end, true, |this, last_snippet, _| { this.push_str(last_snippet.trim_end()); }); self.normalize_vertical_spaces = false; } + pub(crate) fn format_missing_before_skip(&mut self, end: BytePos) { + self.format_missing_inner(end, false, |this, last_snippet, _| { + this.push_str(last_snippet); + }); + self.normalize_vertical_spaces = false; + } + fn format_missing_inner, &str, &str)>( &mut self, end: BytePos, + trim_last_line: bool, // Keep last line contents when snippet is empty process_last_snippet: F, ) { let start = self.last_pos; @@ -110,7 +120,17 @@ impl<'a> FmtVisitor<'a> { if snippet.trim().is_empty() && !out_of_file_lines_range!(self, span) { // Keep vertical spaces within range. self.push_vertical_spaces(count_newlines(snippet)); - process_last_snippet(self, "", snippet); + if trim_last_line { + process_last_snippet(self, "", snippet); + } else { + let last_nl = snippet.rfind('\n'); + let first = if let Some(last) = last_nl { + last + 1 + } else { + 0 + }; + process_last_snippet(self, &snippet[first..], ""); + } } else { self.write_snippet(span, &process_last_snippet); } diff --git a/src/formatting/visitor.rs b/src/formatting/visitor.rs index 2a0af48c84a..85e4368241a 100644 --- a/src/formatting/visitor.rs +++ b/src/formatting/visitor.rs @@ -154,7 +154,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { ast::StmtKind::Local(..) | ast::StmtKind::Expr(..) | ast::StmtKind::Semi(..) => { let attrs = get_attrs_from_stmt(stmt.as_ast_node()); if contains_skip(attrs) { - self.push_skipped_with_span( + self.push_skipped_with_span_non_impl( attrs, stmt.span(), get_span_without_attrs(stmt.as_ast_node()), @@ -167,7 +167,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { } ast::StmtKind::MacCall(ref mac_stmt) => { if self.visit_attrs(&mac_stmt.attrs, ast::AttrStyle::Outer) { - self.push_skipped_with_span( + self.push_skipped_with_span_non_impl( &mac_stmt.attrs, stmt.span(), get_span_without_attrs(stmt.as_ast_node()), @@ -486,7 +486,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { // For use/extern crate items, skip rewriting attributes but check for a skip attribute. ast::ItemKind::Use(..) | ast::ItemKind::ExternCrate(_) => { if contains_skip(attrs) { - self.push_skipped_with_span(attrs.as_slice(), item.span(), item.span()); + self.push_skipped_with_span_non_impl(attrs.as_slice(), item.span(), item.span); false } else { true @@ -495,7 +495,11 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { // Module is inline, in this case we treat it like any other item. _ if !is_mod_decl(item) => { if self.visit_attrs(&item.attrs, ast::AttrStyle::Outer) { - self.push_skipped_with_span(item.attrs.as_slice(), item.span(), item.span()); + self.push_skipped_with_span_non_impl( + item.attrs.as_slice(), + item.span(), + item.span, + ); false } else { true @@ -515,7 +519,11 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { } _ => { if self.visit_attrs(&item.attrs, ast::AttrStyle::Outer) { - self.push_skipped_with_span(item.attrs.as_slice(), item.span(), item.span()); + self.push_skipped_with_span_non_impl( + item.attrs.as_slice(), + item.span(), + item.span, + ); false } else { true @@ -672,7 +680,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { skip_out_of_file_lines_range_visitor!(self, ti.span); if self.visit_attrs(&ti.attrs, ast::AttrStyle::Outer) { - self.push_skipped_with_span(ti.attrs.as_slice(), ti.span, ti.span); + self.push_skipped_with_span_non_impl(ti.attrs.as_slice(), ti.span(), ti.span); return; } let skip_context_outer = self.skip_context.clone(); @@ -732,7 +740,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { skip_out_of_file_lines_range_visitor!(self, ii.span); if self.visit_attrs(&ii.attrs, ast::AttrStyle::Outer) { - self.push_skipped_with_span(ii.attrs.as_slice(), ii.span, ii.span); + self.push_skipped_with_span_impl(ii.attrs.as_slice(), ii.span(), ii.span); return; } let skip_context_outer = self.skip_context.clone(); @@ -832,19 +840,22 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { } #[allow(clippy::needless_pass_by_value)] - fn push_rewrite_inner(&mut self, span: Span, rewrite: Option) { + fn push_rewrite_inner(&mut self, span: Span, rewrite: Option, trim_start: bool) { if let Some(ref s) = rewrite { self.push_str(s); } else { - let snippet = self.snippet(span); - self.push_str(snippet.trim()); + let mut snippet = self.snippet(span).trim_end(); + if trim_start { + snippet = snippet.trim_start() + }; + self.push_str(snippet); } self.last_pos = source!(self, span).hi(); } pub(crate) fn push_rewrite(&mut self, span: Span, rewrite: Option) { self.format_missing_with_indent(source!(self, span).lo()); - self.push_rewrite_inner(span, rewrite); + self.push_rewrite_inner(span, rewrite, true); } pub(crate) fn push_skipped_with_span( @@ -852,8 +863,8 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { attrs: &[ast::Attribute], item_span: Span, main_span: Span, + trim_start: bool, ) { - self.format_missing_with_indent(source!(self, item_span).lo()); // do not take into account the lines with attributes as part of the skipped range let attrs_end = attrs .iter() @@ -865,13 +876,33 @@ impl<'b, 'a: 'b> FmtVisitor<'a> { // or it can be on the same line as the last attribute. // So here we need to take a minimum between the two. let lo = std::cmp::min(attrs_end + 1, first_line); - self.push_rewrite_inner(item_span, None); + self.push_rewrite_inner(item_span, None, trim_start); let hi = self.line_number + 1; self.skipped_range .borrow_mut() .push(NonFormattedRange::new(lo, hi)); } + pub(crate) fn push_skipped_with_span_impl( + &mut self, + attrs: &[ast::Attribute], + item_span: Span, + main_span: Span, + ) { + self.format_missing_before_skip(source!(self, item_span).lo()); + self.push_skipped_with_span(attrs, item_span, main_span, false); + } + + pub(crate) fn push_skipped_with_span_non_impl( + &mut self, + attrs: &[ast::Attribute], + item_span: Span, + main_span: Span, + ) { + self.format_missing_with_indent(source!(self, item_span).lo()); + self.push_skipped_with_span(attrs, item_span, main_span, true); + } + pub(crate) fn from_context(ctx: &'a RewriteContext<'_>) -> FmtVisitor<'a> { let mut visitor = FmtVisitor::from_parse_sess( ctx.parse_sess, diff --git a/tests/source/skip/skip-and-fn.rs b/tests/source/skip/skip-and-fn.rs new file mode 100644 index 00000000000..537ab89c4a8 --- /dev/null +++ b/tests/source/skip/skip-and-fn.rs @@ -0,0 +1,55 @@ +// *** "rustfmt::skip" and Function +// *** All source lines starts with indent by 3 tabls. + +// "rustfmt::skip" as first line; + #[rustfmt::skip] + /** Comment line 1 + comment line 2. */ + fn main() { + x = y; + } + +// "rustfmt::skip" whithin a block of attributes + fn main() { + #[rustfmt::skip] + // Comment 1 + #[allow(non_snake_case)] + // Comment 2 + #[allow(non_snake_case)] + pub fn foo(&self) {} + } + + fn main() { + // Comment 1 + #[rustfmt::skip] + #[allow(non_snake_case)] + // Comment 2 + #[allow(non_snake_case)] + pub fn foo(&self) {} + } + + fn main() { + // Comment 1 + #[allow(non_snake_case)] + // Comment 2 + #[allow(non_snake_case)] + #[rustfmt::skip] + #[allow(non_snake_case)] + pub fn foo(&self) {} + } + + fn main() { + #[rustfmt::skip] + /** Comment line 1 + comment line 2. */ + #[allow(non_snake_case)] + x = y; + } + +// fn with Doc comment which is attribute vs one-line comment + fn main() { + // Comment for `foo` + #[rustfmt::skip] // comment on why use a skip here + #[allow(non_snake_case)] + pub fn foo(&self) {} + } diff --git a/tests/source/skip/skip-and-impl.rs b/tests/source/skip/skip-and-impl.rs new file mode 100644 index 00000000000..b19e0e244d4 --- /dev/null +++ b/tests/source/skip/skip-and-impl.rs @@ -0,0 +1,86 @@ +// *** "rustfmt::skip" and Impl + +// Original code from issue #4499 +#[rustfmt::skip] + /** Comment. + + Rustfmt skips this as expected. */ + fn main() { + for _ in 1..Foo::getRandomNumber() { + println!("Hello, world!"); + } +} + +struct Foo; + impl Foo { + #[rustfmt::skip] + /** Comment. + + Rustfmt unindents this despite the ::skip above. */ + #[allow(non_snake_case)] + pub fn getRandomNumber() -> i32 { 4 } +} + +// *** All source lines starts with indent by 3 tabls. + +// "rustfmt::skip" as first line; + #[rustfmt::skip] + /** Comment line 1 + comment line 2. */ + #[allow(non_snake_case)] + impl Foo { + pub fn getRandomNumber() -> i32 { 4 } + } + +// "rustfmt::skip" whithin a block of attributes + impl Foo { + #[rustfmt::skip] + // Comment 1 + #[allow(non_snake_case)] + // Comment 2 + #[allow(non_snake_case)] + pub fn foo(&self) {} + } + + impl Foo { + // Comment 1 + #[rustfmt::skip] + #[allow(non_snake_case)] + // Comment 2 + #[allow(non_snake_case)] + pub fn foo(&self) {} + } + + impl Foo { + // Comment 1 + #[allow(non_snake_case)] + // Comment 2 + #[allow(non_snake_case)] + #[rustfmt::skip] + #[allow(non_snake_case)] + pub fn foo(&self) {} + } + + impl Foo { + #[rustfmt::skip] + /** Comment line 1 + comment line 2. */ + #[allow(non_snake_case)] + pub fn getRandomNumber() -> i32 { 4 } + } + +// impl with Doc comment which is attribute vs one-line comment + impl Struct { + /// Documentation for `foo` + #[rustfmt::skip] + #[allow(non_snake_case)] + pub fn foo(&self) {} + } + + impl Struct { + // Comment for `foo` + #[rustfmt::skip] + #[allow(non_snake_case)] + pub fn foo(&self) {} + } + diff --git a/tests/source/skip/skip-and-import.rs b/tests/source/skip/skip-and-import.rs new file mode 100644 index 00000000000..96f162da7da --- /dev/null +++ b/tests/source/skip/skip-and-import.rs @@ -0,0 +1,18 @@ +// *** "rustfmt::skip" and Imports Use/Extern +// *** All source lines starts with indent by 3 tabls. + + #[rustfmt::skip] + use rustc_ast::{ast, attr::HasAttrs, token::DelimToken, visit}; + use crate::config::{BraceStyle, Config}; + use crate::formatting::{ + attr::*, + modules::{FileModMap, Module}, + }; + + use rustc_ast::{ast, attr::HasAttrs, token::DelimToken, visit}; + #[rustfmt::skip] + use crate::config::{BraceStyle, Config}; + use crate::formatting::{ + attr::*, + modules::{FileModMap, Module}, + }; diff --git a/tests/source/skip/skip-and-trait.rs b/tests/source/skip/skip-and-trait.rs new file mode 100644 index 00000000000..5ceafc4a8c7 --- /dev/null +++ b/tests/source/skip/skip-and-trait.rs @@ -0,0 +1,34 @@ +// *** "rustfmt::skip" and Trait +// *** All source lines starts with indent by 3 tabls. + +// Trait with Attributs + #[rustfmt::skip] + #[allow(non_snake_case)] + trait Animal1 { + // Static method signature; `Self` refers to the implementor type. + fn new(name: &'static str) -> Self; + + // Traits can provide default method definitions. + fn talk(&self) { + println!("{} says {}", self.name(), self.noise()); + } + } + + trait Animal2 { + // Static method signature; `Self` refers to the implementor type. + fn new(name: &'static str) -> Self; + + // Instance method signatures; these will return a string. + fn name(&self) -> &'static str; + #[rustfmt::skip] + #[allow(non_snake_case)] + fn noise(&self) -> &'static str; + + fn talk(&self) { + // Traits can provide default method definitions. + #[rustfmt::skip] + #[allow(non_snake_case)] + fn name(&self) -> &'static str; + println!("{} says {}", self.name(), self.noise()); + } + } diff --git a/tests/target/issue-4398.rs b/tests/target/issue-4398.rs index 2ca894528e9..b0095aaac79 100644 --- a/tests/target/issue-4398.rs +++ b/tests/target/issue-4398.rs @@ -6,7 +6,7 @@ impl Struct { impl Struct { /// Documentation for `foo` - #[rustfmt::skip] // comment on why use a skip here + #[rustfmt::skip] // comment on why use a skip here pub fn foo(&self) {} } diff --git a/tests/target/skip/skip-and-fn.rs b/tests/target/skip/skip-and-fn.rs new file mode 100644 index 00000000000..acd739582d9 --- /dev/null +++ b/tests/target/skip/skip-and-fn.rs @@ -0,0 +1,55 @@ +// *** "rustfmt::skip" and Function +// *** All source lines starts with indent by 3 tabls. + +// "rustfmt::skip" as first line; +#[rustfmt::skip] + /** Comment line 1 + comment line 2. */ + fn main() { + x = y; + } + +// "rustfmt::skip" whithin a block of attributes +fn main() { + #[rustfmt::skip] + // Comment 1 + #[allow(non_snake_case)] + // Comment 2 + #[allow(non_snake_case)] + pub fn foo(&self) {} +} + +fn main() { + // Comment 1 + #[rustfmt::skip] + #[allow(non_snake_case)] + // Comment 2 + #[allow(non_snake_case)] + pub fn foo(&self) {} +} + +fn main() { + // Comment 1 + #[allow(non_snake_case)] + // Comment 2 + #[allow(non_snake_case)] + #[rustfmt::skip] + #[allow(non_snake_case)] + pub fn foo(&self) {} +} + +fn main() { + #[rustfmt::skip] + /** Comment line 1 + comment line 2. */ + #[allow(non_snake_case)] + x = y; +} + +// fn with Doc comment which is attribute vs one-line comment +fn main() { + // Comment for `foo` + #[rustfmt::skip] // comment on why use a skip here + #[allow(non_snake_case)] + pub fn foo(&self) {} +} diff --git a/tests/target/skip/skip-and-impl.rs b/tests/target/skip/skip-and-impl.rs new file mode 100644 index 00000000000..62bce62d948 --- /dev/null +++ b/tests/target/skip/skip-and-impl.rs @@ -0,0 +1,85 @@ +// *** "rustfmt::skip" and Impl + +// Original code from issue #4499 +#[rustfmt::skip] + /** Comment. + + Rustfmt skips this as expected. */ + fn main() { + for _ in 1..Foo::getRandomNumber() { + println!("Hello, world!"); + } +} + +struct Foo; +impl Foo { + #[rustfmt::skip] + /** Comment. + + Rustfmt unindents this despite the ::skip above. */ + #[allow(non_snake_case)] + pub fn getRandomNumber() -> i32 { 4 } +} + +// *** All source lines starts with indent by 3 tabls. + +// "rustfmt::skip" as first line; +#[rustfmt::skip] + /** Comment line 1 + comment line 2. */ + #[allow(non_snake_case)] + impl Foo { + pub fn getRandomNumber() -> i32 { 4 } + } + +// "rustfmt::skip" whithin a block of attributes +impl Foo { + #[rustfmt::skip] + // Comment 1 + #[allow(non_snake_case)] + // Comment 2 + #[allow(non_snake_case)] + pub fn foo(&self) {} +} + +impl Foo { + // Comment 1 + #[rustfmt::skip] + #[allow(non_snake_case)] + // Comment 2 + #[allow(non_snake_case)] + pub fn foo(&self) {} +} + +impl Foo { + // Comment 1 + #[allow(non_snake_case)] + // Comment 2 + #[allow(non_snake_case)] + #[rustfmt::skip] + #[allow(non_snake_case)] + pub fn foo(&self) {} +} + +impl Foo { + #[rustfmt::skip] + /** Comment line 1 + comment line 2. */ + #[allow(non_snake_case)] + pub fn getRandomNumber() -> i32 { 4 } +} + +// impl with Doc comment which is attribute vs one-line comment +impl Struct { + /// Documentation for `foo` + #[rustfmt::skip] + #[allow(non_snake_case)] + pub fn foo(&self) {} +} + +impl Struct { + // Comment for `foo` + #[rustfmt::skip] + #[allow(non_snake_case)] + pub fn foo(&self) {} +} diff --git a/tests/target/skip/skip-and-import.rs b/tests/target/skip/skip-and-import.rs new file mode 100644 index 00000000000..8cd1cd78c89 --- /dev/null +++ b/tests/target/skip/skip-and-import.rs @@ -0,0 +1,18 @@ +// *** "rustfmt::skip" and Imports Use/Extern +// *** All source lines starts with indent by 3 tabls. + +#[rustfmt::skip] + use rustc_ast::{ast, attr::HasAttrs, token::DelimToken, visit}; +use crate::config::{BraceStyle, Config}; +use crate::formatting::{ + attr::*, + modules::{FileModMap, Module}, +}; + +use rustc_ast::{ast, attr::HasAttrs, token::DelimToken, visit}; +#[rustfmt::skip] + use crate::config::{BraceStyle, Config}; +use crate::formatting::{ + attr::*, + modules::{FileModMap, Module}, +}; diff --git a/tests/target/skip/skip-and-trait.rs b/tests/target/skip/skip-and-trait.rs new file mode 100644 index 00000000000..2ce29ff737e --- /dev/null +++ b/tests/target/skip/skip-and-trait.rs @@ -0,0 +1,34 @@ +// *** "rustfmt::skip" and Trait +// *** All source lines starts with indent by 3 tabls. + +// Trait with Attributs +#[rustfmt::skip] + #[allow(non_snake_case)] + trait Animal1 { + // Static method signature; `Self` refers to the implementor type. + fn new(name: &'static str) -> Self; + + // Traits can provide default method definitions. + fn talk(&self) { + println!("{} says {}", self.name(), self.noise()); + } + } + +trait Animal2 { + // Static method signature; `Self` refers to the implementor type. + fn new(name: &'static str) -> Self; + + // Instance method signatures; these will return a string. + fn name(&self) -> &'static str; + #[rustfmt::skip] + #[allow(non_snake_case)] + fn noise(&self) -> &'static str; + + fn talk(&self) { + // Traits can provide default method definitions. + #[rustfmt::skip] + #[allow(non_snake_case)] + fn name(&self) -> &'static str; + println!("{} says {}", self.name(), self.noise()); + } +}