From 7359c4801f5beec5d99d1b6e9eec06db44c1f74b Mon Sep 17 00:00:00 2001 From: Yacin Tmimi Date: Fri, 19 Nov 2021 23:05:22 -0500 Subject: [PATCH] Treat empty doc comments that precede doc attributes as significant Fixes 5073 Normally we would want to remove any empty trailing doc comments however, as highlighted by 5073 by removing these trailing comments rustfmt is interfering with tools like rustdoc. Checks have been added to allow rustfmt to keep empty trailing comments when using ``wrap_comments = true`` if and only if the empty trailing comments immideatly precede a doc attribute. --- src/attr.rs | 36 +++++++++++++++--- src/comment.rs | 37 ++++++++++++++++--- ...oc_comment_and_attr_wrap_comments_false.rs | 8 ++++ ...doc_comment_and_attr_wrap_comments_true.rs | 8 ++++ ...oc_comment_and_attr_wrap_comments_false.rs | 8 ++++ ...doc_comment_and_attr_wrap_comments_true.rs | 7 ++++ ...ificant_empty_comment_wrap_commet_false.rs | 7 ++++ ...nificant_empty_comment_wrap_commet_true.rs | 7 ++++ 8 files changed, 106 insertions(+), 12 deletions(-) create mode 100644 tests/source/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_false.rs create mode 100644 tests/source/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_true.rs create mode 100644 tests/target/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_false.rs create mode 100644 tests/target/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_true.rs create mode 100644 tests/target/issue-5073/significant_empty_comment_wrap_commet_false.rs create mode 100644 tests/target/issue-5073/significant_empty_comment_wrap_commet_true.rs diff --git a/src/attr.rs b/src/attr.rs index 76b66e9da80..65bca2b8f3f 100644 --- a/src/attr.rs +++ b/src/attr.rs @@ -199,16 +199,19 @@ fn format_derive( /// Returns the first group of attributes that fills the given predicate. /// We consider two doc comments are in different group if they are separated by normal comments. -fn take_while_with_pred<'a, P>( +fn take_while_with_pred<'a, P, N>( context: &RewriteContext<'_>, attrs: &'a [ast::Attribute], pred: P, -) -> &'a [ast::Attribute] + is_next_significant: N, +) -> (&'a [ast::Attribute], bool) where P: Fn(&ast::Attribute) -> bool, + N: Fn(&ast::Attribute, &ast::Attribute) -> bool, { let mut len = 0; let mut iter = attrs.iter().peekable(); + let mut next_attr_significant = false; while let Some(attr) = iter.next() { if pred(attr) { @@ -217,6 +220,7 @@ where break; } if let Some(next_attr) = iter.peek() { + next_attr_significant = is_next_significant(attr, next_attr); // Extract comments between two attributes. let span_between_attr = mk_sp(attr.span.hi(), next_attr.span.lo()); let snippet = context.snippet(span_between_attr); @@ -226,7 +230,7 @@ where } } - &attrs[..len] + (&attrs[..len], next_attr_significant) } /// Rewrite the any doc comments which come before any other attributes. @@ -239,7 +243,20 @@ fn rewrite_initial_doc_comments( return Some((0, None)); } // Rewrite doc comments - let sugared_docs = take_while_with_pred(context, attrs, |a| a.is_doc_comment()); + // We want to treat trailing empty comments as significant if they precede a doc attribute + // see https://github.com/rust-lang/rustfmt/issues/5073 + let (sugared_docs, keep_empty_trailing_comment) = take_while_with_pred( + context, + attrs, + |a| a.is_doc_comment(), + |curr_attr, next_attr| { + let is_curr_doc_comment = curr_attr.is_doc_comment(); + let symbol = next_attr.name_or_empty(); + let is_next_doc_attr = next_attr.has_name(symbol) && symbol.as_str() == "doc"; + let next_immediately_after_current = (next_attr.span.lo() - curr_attr.span.hi()).0 == 1; + is_curr_doc_comment && is_next_doc_attr && next_immediately_after_current + }, + ); if !sugared_docs.is_empty() { let snippet = sugared_docs .iter() @@ -252,6 +269,7 @@ fn rewrite_initial_doc_comments( &snippet, shape.comment(context.config), context.config, + keep_empty_trailing_comment, )?), )); } @@ -333,7 +351,12 @@ impl Rewrite for ast::Attribute { fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option { let snippet = context.snippet(self.span); if self.is_doc_comment() { - rewrite_doc_comment(snippet, shape.comment(context.config), context.config) + rewrite_doc_comment( + snippet, + shape.comment(context.config), + context.config, + false, + ) } else { let should_skip = self .ident() @@ -362,6 +385,7 @@ impl Rewrite for ast::Attribute { &doc_comment, shape.comment(context.config), context.config, + false, ); } } @@ -432,7 +456,7 @@ impl Rewrite for [ast::Attribute] { // Handle derives if we will merge them. if context.config.merge_derives() && is_derive(&attrs[0]) { - let derives = take_while_with_pred(context, attrs, is_derive); + let (derives, _) = take_while_with_pred(context, attrs, is_derive, |_, _| false); let derive_str = format_derive(derives, shape, context)?; result.push_str(&derive_str); diff --git a/src/comment.rs b/src/comment.rs index 7b76c232937..b03eb2ae3f7 100644 --- a/src/comment.rs +++ b/src/comment.rs @@ -240,8 +240,20 @@ pub(crate) fn combine_strs_with_missing_comments( Some(result) } -pub(crate) fn rewrite_doc_comment(orig: &str, shape: Shape, config: &Config) -> Option { - identify_comment(orig, false, shape, config, true) +pub(crate) fn rewrite_doc_comment( + orig: &str, + shape: Shape, + config: &Config, + keep_empty_trailing_comment: bool, +) -> Option { + identify_comment( + orig, + false, + shape, + config, + true, + keep_empty_trailing_comment, + ) } pub(crate) fn rewrite_comment( @@ -250,7 +262,7 @@ pub(crate) fn rewrite_comment( shape: Shape, config: &Config, ) -> Option { - identify_comment(orig, block_style, shape, config, false) + identify_comment(orig, block_style, shape, config, false, false) } fn identify_comment( @@ -259,6 +271,7 @@ fn identify_comment( shape: Shape, config: &Config, is_doc_comment: bool, + keep_empty_trailing_comment: bool, ) -> Option { let style = comment_style(orig, false); @@ -365,6 +378,7 @@ fn identify_comment( shape, config, is_doc_comment || style.is_doc_comment(), + keep_empty_trailing_comment, )? }; if rest.is_empty() { @@ -376,6 +390,7 @@ fn identify_comment( shape, config, is_doc_comment, + keep_empty_trailing_comment, ) .map(|rest_str| { format!( @@ -618,6 +633,7 @@ impl<'a> CommentRewrite<'a> { i: usize, line: &'a str, has_leading_whitespace: bool, + keep_empty_trailing_comment: bool, ) -> bool { let is_last = i == count_newlines(orig); @@ -700,8 +716,10 @@ impl<'a> CommentRewrite<'a> { } } else if self.is_prev_line_multi_line && !line.is_empty() { self.result.push(' ') - } else if is_last && line.is_empty() { - // trailing blank lines are unwanted + } else if is_last && line.is_empty() && !keep_empty_trailing_comment { + // trailing blank lines are unwanted (in most cases) + // see https://github.com/rust-lang/rustfmt/issues/5073 for an example + // where empty comment lines are significant if !self.closer.is_empty() { self.result.push_str(&self.indent_str); } @@ -777,6 +795,7 @@ fn rewrite_comment_inner( shape: Shape, config: &Config, is_doc_comment: bool, + keep_empty_trailing_comment: bool, ) -> Option { let mut rewriter = CommentRewrite::new(orig, block_style, shape, config); @@ -806,7 +825,13 @@ fn rewrite_comment_inner( }); for (i, (line, has_leading_whitespace)) in lines.enumerate() { - if rewriter.handle_line(orig, i, line, has_leading_whitespace) { + if rewriter.handle_line( + orig, + i, + line, + has_leading_whitespace, + keep_empty_trailing_comment, + ) { break; } } diff --git a/tests/source/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_false.rs b/tests/source/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_false.rs new file mode 100644 index 00000000000..dcc4e50b20d --- /dev/null +++ b/tests/source/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_false.rs @@ -0,0 +1,8 @@ +// rustfmt-wrap_comments: false + +//! Text +//! + +#![doc = include_str!("something.md")] +//! +//! More text diff --git a/tests/source/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_true.rs b/tests/source/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_true.rs new file mode 100644 index 00000000000..314a9d675b6 --- /dev/null +++ b/tests/source/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_true.rs @@ -0,0 +1,8 @@ +// rustfmt-wrap_comments: true + +//! Text +//! + +#![doc = include_str!("something.md")] +//! +//! More text diff --git a/tests/target/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_false.rs b/tests/target/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_false.rs new file mode 100644 index 00000000000..dcc4e50b20d --- /dev/null +++ b/tests/target/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_false.rs @@ -0,0 +1,8 @@ +// rustfmt-wrap_comments: false + +//! Text +//! + +#![doc = include_str!("something.md")] +//! +//! More text diff --git a/tests/target/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_true.rs b/tests/target/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_true.rs new file mode 100644 index 00000000000..3616370d419 --- /dev/null +++ b/tests/target/issue-5073/non_consecutive_doc_comment_and_attr_wrap_comments_true.rs @@ -0,0 +1,7 @@ +// rustfmt-wrap_comments: true + +//! Text + +#![doc = include_str!("something.md")] +//! +//! More text diff --git a/tests/target/issue-5073/significant_empty_comment_wrap_commet_false.rs b/tests/target/issue-5073/significant_empty_comment_wrap_commet_false.rs new file mode 100644 index 00000000000..ea997c09137 --- /dev/null +++ b/tests/target/issue-5073/significant_empty_comment_wrap_commet_false.rs @@ -0,0 +1,7 @@ +// rustfmt-wrap_comments: false + +//! Text +//! +#![doc = include_str!("something.md")] +//! +//! More text diff --git a/tests/target/issue-5073/significant_empty_comment_wrap_commet_true.rs b/tests/target/issue-5073/significant_empty_comment_wrap_commet_true.rs new file mode 100644 index 00000000000..42468d854a5 --- /dev/null +++ b/tests/target/issue-5073/significant_empty_comment_wrap_commet_true.rs @@ -0,0 +1,7 @@ +// rustfmt-wrap_comments: true + +//! Text +//! +#![doc = include_str!("something.md")] +//! +//! More text