From bbbc1e86ebe3a003d41bd7b912ee3042086ff918 Mon Sep 17 00:00:00 2001 From: rchaser53 Date: Mon, 18 Mar 2019 21:41:31 +0900 Subject: [PATCH] refrect topecongiro reviews - &Vec => &[ast::PathSegment] - remove unnecessary implements - transfer skip logic to inside rewrite_macro - fix test - use util methods in libsyntax - use meta_item_list directly - avoid no_entry.rs for test using module system - add logic to skip rustfmt::skip::macros only - remove base_skip_macro_names - remove Rc - use clone to append skip_macro_names --- src/expr.rs | 23 +- src/formatting.rs | 10 +- src/macros.rs | 19 +- src/rewrite.rs | 3 +- src/test/mod.rs | 1 + src/utils.rs | 20 ++ src/visitor.rs | 279 +++++++++++----------- tests/source/issue-3434.rs | 22 -- tests/source/issue-3434/lib.rs | 36 +++ tests/source/issue-3434/no_entry.rs | 18 ++ tests/source/issue-3434/not_skip_macro.rs | 8 + tests/target/issue-3434.rs | 24 -- tests/target/issue-3434/lib.rs | 36 +++ tests/target/issue-3434/no_entry.rs | 19 ++ tests/target/issue-3434/not_skip_macro.rs | 8 + 15 files changed, 311 insertions(+), 215 deletions(-) delete mode 100644 tests/source/issue-3434.rs create mode 100644 tests/source/issue-3434/lib.rs create mode 100644 tests/source/issue-3434/no_entry.rs create mode 100644 tests/source/issue-3434/not_skip_macro.rs delete mode 100644 tests/target/issue-3434.rs create mode 100644 tests/target/issue-3434/lib.rs create mode 100644 tests/target/issue-3434/no_entry.rs create mode 100644 tests/target/issue-3434/not_skip_macro.rs diff --git a/src/expr.rs b/src/expr.rs index 247ec298b00..068b8f7fed6 100644 --- a/src/expr.rs +++ b/src/expr.rs @@ -190,21 +190,13 @@ pub fn format_expr( rewrite_chain(expr, context, shape) } ast::ExprKind::Mac(ref mac) => { - let should_skip = context - .skip_macro_names - .borrow() - .contains(&context.snippet(mac.node.path.span).to_owned()); - if should_skip { - None - } else { - rewrite_macro(mac, None, context, shape, MacroPosition::Expression).or_else(|| { - wrap_str( - context.snippet(expr.span).to_owned(), - context.config.max_width(), - shape, - ) - }) - } + rewrite_macro(mac, None, context, shape, MacroPosition::Expression).or_else(|| { + wrap_str( + context.snippet(expr.span).to_owned(), + context.config.max_width(), + shape, + ) + }) } ast::ExprKind::Ret(None) => Some("return".to_owned()), ast::ExprKind::Ret(Some(ref expr)) => { @@ -1928,7 +1920,6 @@ pub fn rewrite_assign_rhs_with, R: Rewrite>( offset: shape.offset + last_line_width + 1, ..shape }); - // dbg!( let rhs = choose_rhs( context, ex, diff --git a/src/formatting.rs b/src/formatting.rs index a23c312bd2a..545d2fe279f 100644 --- a/src/formatting.rs +++ b/src/formatting.rs @@ -15,6 +15,7 @@ use crate::comment::{CharClasses, FullCodeCharKind}; use crate::config::{Config, FileName, Verbosity}; use crate::issues::BadIssueSeeker; +use crate::utils::{count_newlines, get_skip_macro_names}; use crate::visitor::{FmtVisitor, SnippetProvider}; use crate::{modules, source_file, ErrorKind, FormatReport, Input, Session}; @@ -153,6 +154,10 @@ fn format_file( &snippet_provider, self.report.clone(), ); + visitor + .skip_macro_names + .borrow_mut() + .append(&mut get_skip_macro_names(&self.krate.attrs)); // Format inner attributes if available. if !self.krate.attrs.is_empty() && is_root { @@ -168,10 +173,7 @@ fn format_file( visitor.format_separate_mod(module, &*source_file); }; - debug_assert_eq!( - visitor.line_number, - crate::utils::count_newlines(&visitor.buffer) - ); + debug_assert_eq!(visitor.line_number, count_newlines(&visitor.buffer)); // For some reason, the source_map does not include terminating // newlines so we must add one on for each file. This is sad. diff --git a/src/macros.rs b/src/macros.rs index 60fdb9160a7..40a3b7edcd6 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -208,12 +208,21 @@ pub fn rewrite_macro( shape: Shape, position: MacroPosition, ) -> Option { - let guard = InsideMacroGuard::inside_macro_context(context); - let result = rewrite_macro_inner(mac, extra_ident, context, shape, position, guard.is_nested); - if result.is_none() { - context.macro_rewrite_failure.replace(true); + let should_skip = context + .skip_macro_names + .borrow() + .contains(&context.snippet(mac.node.path.span).to_owned()); + if should_skip { + None + } else { + let guard = InsideMacroGuard::inside_macro_context(context); + let result = + rewrite_macro_inner(mac, extra_ident, context, shape, position, guard.is_nested); + if result.is_none() { + context.macro_rewrite_failure.replace(true); + } + result } - result } fn check_keyword<'a, 'b: 'a>(parser: &'a mut Parser<'b>) -> Option { diff --git a/src/rewrite.rs b/src/rewrite.rs index 406dc64dccf..c736c2535a6 100644 --- a/src/rewrite.rs +++ b/src/rewrite.rs @@ -1,7 +1,6 @@ // A generic trait to abstract the rewriting of an element (of the AST). use std::cell::RefCell; -use std::rc::Rc; use syntax::parse::ParseSess; use syntax::ptr; @@ -40,7 +39,7 @@ pub struct RewriteContext<'a> { // Used for `format_snippet` pub(crate) macro_rewrite_failure: RefCell, pub(crate) report: FormatReport, - pub skip_macro_names: Rc>>, + pub skip_macro_names: RefCell>, } impl<'a> RewriteContext<'a> { diff --git a/src/test/mod.rs b/src/test/mod.rs index 61d7c88884a..0628094fdef 100644 --- a/src/test/mod.rs +++ b/src/test/mod.rs @@ -22,6 +22,7 @@ // We want to make sure that the `skip_children` is correctly working, // so we do not want to test this file directly. "configs/skip_children/foo/mod.rs", + "issue-3434/no_entry.rs", ]; fn is_file_skip(path: &Path) -> bool { diff --git a/src/utils.rs b/src/utils.rs index e42b5c8c7d1..743a8627612 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -603,6 +603,26 @@ pub(crate) fn unicode_str_width(s: &str) -> usize { s.width() } +pub fn get_skip_macro_names(attrs: &[ast::Attribute]) -> Vec { + let mut skip_macro_names = vec![]; + for attr in attrs { + // syntax::ast::Path is implemented partialEq + // but it is designed for segments.len() == 1 + if format!("{}", attr.path) != "rustfmt::skip::macros" { + continue; + } + + if let Some(list) = attr.meta_item_list() { + for spanned in list { + if let Some(name) = spanned.name() { + skip_macro_names.push(name.to_string()); + } + } + } + } + skip_macro_names +} + #[cfg(test)] mod test { use super::*; diff --git a/src/visitor.rs b/src/visitor.rs index 4b1bd5bc55e..441f7b4e75f 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -1,9 +1,7 @@ use std::cell::RefCell; -use std::rc::Rc; -use syntax::parse::{token, ParseSess}; +use syntax::parse::ParseSess; use syntax::source_map::{self, BytePos, Pos, SourceMap, Span}; -use syntax::tokenstream::TokenTree; use syntax::{ast, visit}; use crate::attr::*; @@ -22,8 +20,8 @@ use crate::source_map::{LineRangeUtils, SpanUtils}; use crate::spanned::Spanned; use crate::utils::{ - self, contains_skip, count_newlines, inner_attributes, mk_sp, ptr_vec_to_ref_vec, - rewrite_ident, stmt_expr, DEPR_SKIP_ANNOTATION, + self, contains_skip, count_newlines, get_skip_macro_names, inner_attributes, mk_sp, + ptr_vec_to_ref_vec, rewrite_ident, stmt_expr, DEPR_SKIP_ANNOTATION, }; use crate::{ErrorKind, FormatReport, FormattingError}; @@ -68,7 +66,7 @@ pub struct FmtVisitor<'a> { pub skipped_range: Vec<(usize, usize)>, pub macro_rewrite_failure: bool, pub(crate) report: FormatReport, - pub skip_macro_names: Rc>>, + pub skip_macro_names: RefCell>, } impl<'a> Drop for FmtVisitor<'a> { @@ -299,25 +297,32 @@ pub fn visit_item(&mut self, item: &ast::Item) { // the AST lumps them all together. let filtered_attrs; let mut attrs = &item.attrs; - match item.node { + let temp_skip_macro_names = self.skip_macro_names.clone(); + self.skip_macro_names + .borrow_mut() + .append(&mut get_skip_macro_names(&attrs)); + + let should_visit_node_again = match item.node { // For use items, skip rewriting attributes. Just check for a skip attribute. ast::ItemKind::Use(..) => { if contains_skip(attrs) { self.push_skipped_with_span(attrs.as_slice(), item.span(), item.span()); - return; + false + } else { + true } } // 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()); - return; + false + } else { + true } } // Module is not inline, but should be skipped. - ast::ItemKind::Mod(..) if contains_skip(&item.attrs) => { - return; - } + ast::ItemKind::Mod(..) if contains_skip(&item.attrs) => false, // Module is not inline and should not be skipped. We want // to process only the attributes in the current file. ast::ItemKind::Mod(..) => { @@ -326,123 +331,127 @@ pub fn visit_item(&mut self, item: &ast::Item) { // the above case. assert!(!self.visit_attrs(&filtered_attrs, ast::AttrStyle::Outer)); attrs = &filtered_attrs; + true } _ => { if self.visit_attrs(&item.attrs, ast::AttrStyle::Outer) { self.push_skipped_with_span(item.attrs.as_slice(), item.span(), item.span()); - return; + false + } else { + true } } - } - self.get_skip_macros(&attrs); - - match item.node { - ast::ItemKind::Use(ref tree) => self.format_import(item, tree), - ast::ItemKind::Impl(..) => { - let snippet = self.snippet(item.span); - let where_span_end = snippet - .find_uncommented("{") - .map(|x| BytePos(x as u32) + source!(self, item.span).lo()); - let block_indent = self.block_indent; - let rw = - self.with_context(|ctx| format_impl(&ctx, item, block_indent, where_span_end)); - self.push_rewrite(item.span, rw); - } - ast::ItemKind::Trait(..) => { - let block_indent = self.block_indent; - let rw = self.with_context(|ctx| format_trait(&ctx, item, block_indent)); - self.push_rewrite(item.span, rw); - } - ast::ItemKind::TraitAlias(ref generics, ref generic_bounds) => { - let shape = Shape::indented(self.block_indent, self.config); - let rw = format_trait_alias( - &self.get_context(), - item.ident, - &item.vis, - generics, - generic_bounds, - shape, - ); - self.push_rewrite(item.span, rw); - } - ast::ItemKind::ExternCrate(_) => { - let rw = rewrite_extern_crate(&self.get_context(), item); - self.push_rewrite(item.span, rw); - } - ast::ItemKind::Struct(..) | ast::ItemKind::Union(..) => { - self.visit_struct(&StructParts::from_item(item)); - } - ast::ItemKind::Enum(ref def, ref generics) => { - self.format_missing_with_indent(source!(self, item.span).lo()); - self.visit_enum(item.ident, &item.vis, def, generics, item.span); - self.last_pos = source!(self, item.span).hi(); - } - ast::ItemKind::Mod(ref module) => { - let is_inline = !is_mod_decl(item); - self.format_missing_with_indent(source!(self, item.span).lo()); - self.format_mod(module, &item.vis, item.span, item.ident, attrs, is_inline); - } - ast::ItemKind::Mac(ref mac) => { - self.visit_mac(mac, Some(item.ident), MacroPosition::Item); - } - ast::ItemKind::ForeignMod(ref foreign_mod) => { - self.format_missing_with_indent(source!(self, item.span).lo()); - self.format_foreign_mod(foreign_mod, item.span); - } - ast::ItemKind::Static(..) | ast::ItemKind::Const(..) => { - self.visit_static(&StaticParts::from_item(item)); - } - ast::ItemKind::Fn(ref decl, ref fn_header, ref generics, ref body) => { - let inner_attrs = inner_attributes(&item.attrs); - self.visit_fn( - visit::FnKind::ItemFn(item.ident, fn_header, &item.vis, body), - generics, - decl, - item.span, - ast::Defaultness::Final, - Some(&inner_attrs), - ) - } - ast::ItemKind::Ty(ref ty, ref generics) => { - let rewrite = rewrite_type_alias( - &self.get_context(), - self.block_indent, - item.ident, - ty, - generics, - &item.vis, - ); - self.push_rewrite(item.span, rewrite); - } - ast::ItemKind::Existential(ref generic_bounds, ref generics) => { - let rewrite = rewrite_existential_type( - &self.get_context(), - self.block_indent, - item.ident, - generic_bounds, - generics, - &item.vis, - ); - self.push_rewrite(item.span, rewrite); - } - ast::ItemKind::GlobalAsm(..) => { - let snippet = Some(self.snippet(item.span).to_owned()); - self.push_rewrite(item.span, snippet); - } - ast::ItemKind::MacroDef(ref def) => { - let rewrite = rewrite_macro_def( - &self.get_context(), - self.shape(), - self.block_indent, - def, - item.ident, - &item.vis, - item.span, - ); - self.push_rewrite(item.span, rewrite); - } }; - self.skip_macro_names.borrow_mut().clear(); + + if should_visit_node_again { + match item.node { + ast::ItemKind::Use(ref tree) => self.format_import(item, tree), + ast::ItemKind::Impl(..) => { + let snippet = self.snippet(item.span); + let where_span_end = snippet + .find_uncommented("{") + .map(|x| BytePos(x as u32) + source!(self, item.span).lo()); + let block_indent = self.block_indent; + let rw = self + .with_context(|ctx| format_impl(&ctx, item, block_indent, where_span_end)); + self.push_rewrite(item.span, rw); + } + ast::ItemKind::Trait(..) => { + let block_indent = self.block_indent; + let rw = self.with_context(|ctx| format_trait(&ctx, item, block_indent)); + self.push_rewrite(item.span, rw); + } + ast::ItemKind::TraitAlias(ref generics, ref generic_bounds) => { + let shape = Shape::indented(self.block_indent, self.config); + let rw = format_trait_alias( + &self.get_context(), + item.ident, + &item.vis, + generics, + generic_bounds, + shape, + ); + self.push_rewrite(item.span, rw); + } + ast::ItemKind::ExternCrate(_) => { + let rw = rewrite_extern_crate(&self.get_context(), item); + self.push_rewrite(item.span, rw); + } + ast::ItemKind::Struct(..) | ast::ItemKind::Union(..) => { + self.visit_struct(&StructParts::from_item(item)); + } + ast::ItemKind::Enum(ref def, ref generics) => { + self.format_missing_with_indent(source!(self, item.span).lo()); + self.visit_enum(item.ident, &item.vis, def, generics, item.span); + self.last_pos = source!(self, item.span).hi(); + } + ast::ItemKind::Mod(ref module) => { + let is_inline = !is_mod_decl(item); + self.format_missing_with_indent(source!(self, item.span).lo()); + self.format_mod(module, &item.vis, item.span, item.ident, attrs, is_inline); + } + ast::ItemKind::Mac(ref mac) => { + self.visit_mac(mac, Some(item.ident), MacroPosition::Item); + } + ast::ItemKind::ForeignMod(ref foreign_mod) => { + self.format_missing_with_indent(source!(self, item.span).lo()); + self.format_foreign_mod(foreign_mod, item.span); + } + ast::ItemKind::Static(..) | ast::ItemKind::Const(..) => { + self.visit_static(&StaticParts::from_item(item)); + } + ast::ItemKind::Fn(ref decl, ref fn_header, ref generics, ref body) => { + let inner_attrs = inner_attributes(&item.attrs); + self.visit_fn( + visit::FnKind::ItemFn(item.ident, fn_header, &item.vis, body), + generics, + decl, + item.span, + ast::Defaultness::Final, + Some(&inner_attrs), + ) + } + ast::ItemKind::Ty(ref ty, ref generics) => { + let rewrite = rewrite_type_alias( + &self.get_context(), + self.block_indent, + item.ident, + ty, + generics, + &item.vis, + ); + self.push_rewrite(item.span, rewrite); + } + ast::ItemKind::Existential(ref generic_bounds, ref generics) => { + let rewrite = rewrite_existential_type( + &self.get_context(), + self.block_indent, + item.ident, + generic_bounds, + generics, + &item.vis, + ); + self.push_rewrite(item.span, rewrite); + } + ast::ItemKind::GlobalAsm(..) => { + let snippet = Some(self.snippet(item.span).to_owned()); + self.push_rewrite(item.span, snippet); + } + ast::ItemKind::MacroDef(ref def) => { + let rewrite = rewrite_macro_def( + &self.get_context(), + self.shape(), + self.block_indent, + def, + item.ident, + &item.vis, + item.span, + ); + self.push_rewrite(item.span, rewrite); + } + }; + } + self.skip_macro_names = temp_skip_macro_names; } pub fn visit_trait_item(&mut self, ti: &ast::TraitItem) { @@ -597,6 +606,10 @@ pub fn from_context(ctx: &'a RewriteContext<'_>) -> FmtVisitor<'a> { ctx.snippet_provider, ctx.report.clone(), ); + visitor + .skip_macro_names + .borrow_mut() + .append(&mut ctx.skip_macro_names.borrow().clone()); visitor.set_parent_context(ctx); visitor } @@ -621,7 +634,7 @@ pub(crate) fn from_source_map( skipped_range: vec![], macro_rewrite_failure: false, report, - skip_macro_names: Rc::new(RefCell::new(vec![])), + skip_macro_names: RefCell::new(vec![]), } } @@ -674,7 +687,7 @@ pub fn visit_attrs(&mut self, attrs: &[ast::Attribute], style: ast::AttrStyle) - false } - fn is_rustfmt_macro_error(&self, segments: &Vec) -> bool { + fn is_rustfmt_macro_error(&self, segments: &[ast::PathSegment]) -> bool { if segments[0].ident.to_string() != "rustfmt" { return false; } @@ -837,22 +850,4 @@ pub fn get_context(&self) -> RewriteContext<'_> { skip_macro_names: self.skip_macro_names.clone(), } } - - pub fn get_skip_macros(&mut self, attrs: &[ast::Attribute]) { - for attr in attrs { - for token in attr.tokens.trees() { - if let TokenTree::Delimited(_, _, stream) = token { - for inner_token in stream.trees() { - if let TokenTree::Token(span, token) = inner_token { - if let token::Token::Ident(_, _) = token { - // FIXME ident.span.lo() and ident.span.hi() are 0 - let macro_name = self.get_context().snippet(span).to_owned(); - self.skip_macro_names.borrow_mut().push(macro_name); - } - } - } - } - } - } - } } diff --git a/tests/source/issue-3434.rs b/tests/source/issue-3434.rs deleted file mode 100644 index 5d753ef0c4d..00000000000 --- a/tests/source/issue-3434.rs +++ /dev/null @@ -1,22 +0,0 @@ -#[rustfmt::skip::macros(html, skip_macro)] -fn main() { - let macro_result1 = html! {
-Hello
- }.to_string(); - - let macro_result2 = not_skip_macro! {
-Hello
- }.to_string(); - - skip_macro! { -this is a skip_macro here -}; - - foo(); -} - -fn foo() { - let macro_result1 = html! {
-Hello
- }.to_string(); -} diff --git a/tests/source/issue-3434/lib.rs b/tests/source/issue-3434/lib.rs new file mode 100644 index 00000000000..d3f4056bd8a --- /dev/null +++ b/tests/source/issue-3434/lib.rs @@ -0,0 +1,36 @@ +#![rustfmt::skip::macros(skip_macro_mod)] + +mod no_entry; + +#[rustfmt::skip::macros(html, skip_macro)] +fn main() { + let macro_result1 = html! {
+this should be skipped
+ } + .to_string(); + + let macro_result2 = not_skip_macro! {
+this should be mangled
+ } + .to_string(); + + skip_macro! { +this should be skipped +}; + + foo(); +} + +fn foo() { + let macro_result1 = html! {
+this should be mangled
+ } + .to_string(); +} + +fn bar() { + let macro_result1 = skip_macro_mod! {
+this should be skipped
+ } + .to_string(); +} diff --git a/tests/source/issue-3434/no_entry.rs b/tests/source/issue-3434/no_entry.rs new file mode 100644 index 00000000000..0838829fed3 --- /dev/null +++ b/tests/source/issue-3434/no_entry.rs @@ -0,0 +1,18 @@ +#[rustfmt::skip::macros(another_macro)] +fn foo() { + another_macro!( +This should be skipped. + ); +} + +fn bar() { + skip_macro_mod!( +This should be skipped. + ); +} + +fn baz() { + let macro_result1 = no_skip_macro! {
+this should be mangled
+ }.to_string(); +} diff --git a/tests/source/issue-3434/not_skip_macro.rs b/tests/source/issue-3434/not_skip_macro.rs new file mode 100644 index 00000000000..1d7d73c523d --- /dev/null +++ b/tests/source/issue-3434/not_skip_macro.rs @@ -0,0 +1,8 @@ +#[this::is::not::skip::macros(ouch)] + +fn main() { + let macro_result1 = ouch! {
+this should be mangled
+ } + .to_string(); +} diff --git a/tests/target/issue-3434.rs b/tests/target/issue-3434.rs deleted file mode 100644 index 44171bb83fb..00000000000 --- a/tests/target/issue-3434.rs +++ /dev/null @@ -1,24 +0,0 @@ -#[rustfmt::skip::macros(html, skip_macro)] -fn main() { - let macro_result1 = html! {
-Hello
- }.to_string(); - - let macro_result2 = not_skip_macro! {
- Hello
- } - .to_string(); - - skip_macro! { -this is a skip_macro here -}; - - foo(); -} - -fn foo() { - let macro_result1 = html! {
- Hello
- } - .to_string(); -} diff --git a/tests/target/issue-3434/lib.rs b/tests/target/issue-3434/lib.rs new file mode 100644 index 00000000000..95bc75642f0 --- /dev/null +++ b/tests/target/issue-3434/lib.rs @@ -0,0 +1,36 @@ +#![rustfmt::skip::macros(skip_macro_mod)] + +mod no_entry; + +#[rustfmt::skip::macros(html, skip_macro)] +fn main() { + let macro_result1 = html! {
+this should be skipped
+ } + .to_string(); + + let macro_result2 = not_skip_macro! {
+ this should be mangled
+ } + .to_string(); + + skip_macro! { +this should be skipped +}; + + foo(); +} + +fn foo() { + let macro_result1 = html! {
+ this should be mangled
+ } + .to_string(); +} + +fn bar() { + let macro_result1 = skip_macro_mod! {
+this should be skipped
+ } + .to_string(); +} diff --git a/tests/target/issue-3434/no_entry.rs b/tests/target/issue-3434/no_entry.rs new file mode 100644 index 00000000000..a2ecf2c2f99 --- /dev/null +++ b/tests/target/issue-3434/no_entry.rs @@ -0,0 +1,19 @@ +#[rustfmt::skip::macros(another_macro)] +fn foo() { + another_macro!( +This should be skipped. + ); +} + +fn bar() { + skip_macro_mod!( +This should be skipped. + ); +} + +fn baz() { + let macro_result1 = no_skip_macro! {
+ this should be mangled
+ } + .to_string(); +} diff --git a/tests/target/issue-3434/not_skip_macro.rs b/tests/target/issue-3434/not_skip_macro.rs new file mode 100644 index 00000000000..c90d09744b2 --- /dev/null +++ b/tests/target/issue-3434/not_skip_macro.rs @@ -0,0 +1,8 @@ +#[this::is::not::skip::macros(ouch)] + +fn main() { + let macro_result1 = ouch! {
+ this should be mangled
+ } + .to_string(); +}