refrect topecongiro reviews

- &Vec<syntax::ast::PathSegment> => &[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

@@ -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<S: Into<String>, R: Rewrite>(
         offset: shape.offset + last_line_width + 1,
         ..shape
     });
-    // dbg!(
     let rhs = choose_rhs(
         context,
         ex,

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.

src/macros.rs

@@ -208,12 +208,21 @@ pub fn rewrite_macro(
     shape: Shape,
     position: MacroPosition,
 ) -> Option<String> {
-    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<MacroArg> {

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<bool>,
     pub(crate) report: FormatReport,
-    pub skip_macro_names: Rc<RefCell<Vec<String>>>,
+    pub skip_macro_names: RefCell<Vec<String>>,
 }
 
 impl<'a> RewriteContext<'a> {

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 {

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<String> {
+    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::*;

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<RefCell<Vec<String>>>,
+    pub skip_macro_names: RefCell<Vec<String>>,
 }
 
 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<syntax::ast::PathSegment>) -> 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);
-                            }
-                        }
-                    }
-                }
-            }
-        }
-    }
 }

tests/source/issue-3434.rs

@@ -1,22 +0,0 @@
-#[rustfmt::skip::macros(html, skip_macro)]
-fn main() {
-    let macro_result1 = html! { <div>
-Hello</div>
-    }.to_string();
-
-    let macro_result2 = not_skip_macro! { <div>
-Hello</div>
-    }.to_string();
-
-    skip_macro! {
-this is a skip_macro here
-};
-
-  foo();
-}
-
-fn foo() {
-    let macro_result1 = html! { <div>
-Hello</div>
-    }.to_string();
-}

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! { <div>
+this should be skipped</div>
+    }
+    .to_string();
+
+    let macro_result2 = not_skip_macro! { <div>
+this should be mangled</div>
+        }
+    .to_string();
+
+    skip_macro! {
+this should be skipped
+};
+
+    foo();
+}
+
+fn foo() {
+    let macro_result1 = html! { <div>
+this should be mangled</div>
+            }
+    .to_string();
+}
+
+fn bar() {
+    let macro_result1 = skip_macro_mod! { <div>
+this should be skipped</div>
+        }
+    .to_string();
+}

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! { <div>
+this should be mangled</div>
+    }.to_string();
+}

tests/source/issue-3434/not_skip_macro.rs

@@ -0,0 +1,8 @@
+#[this::is::not::skip::macros(ouch)]
+
+fn main() {
+    let macro_result1 = ouch! { <div>
+this should be mangled</div>
+    }
+    .to_string();
+}

tests/target/issue-3434.rs

@@ -1,24 +0,0 @@
-#[rustfmt::skip::macros(html, skip_macro)]
-fn main() {
-    let macro_result1 = html! { <div>
-Hello</div>
-    }.to_string();
-
-    let macro_result2 = not_skip_macro! { <div>
-    Hello</div>
-        }
-    .to_string();
-
-    skip_macro! {
-this is a skip_macro here
-};
-
-    foo();
-}
-
-fn foo() {
-    let macro_result1 = html! { <div>
-    Hello</div>
-        }
-    .to_string();
-}

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! { <div>
+this should be skipped</div>
+    }
+    .to_string();
+
+    let macro_result2 = not_skip_macro! { <div>
+    this should be mangled</div>
+            }
+    .to_string();
+
+    skip_macro! {
+this should be skipped
+};
+
+    foo();
+}
+
+fn foo() {
+    let macro_result1 = html! { <div>
+    this should be mangled</div>
+                }
+    .to_string();
+}
+
+fn bar() {
+    let macro_result1 = skip_macro_mod! { <div>
+this should be skipped</div>
+        }
+    .to_string();
+}

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! { <div>
+    this should be mangled</div>
+        }
+    .to_string();
+}

tests/target/issue-3434/not_skip_macro.rs

@@ -0,0 +1,8 @@
+#[this::is::not::skip::macros(ouch)]
+
+fn main() {
+    let macro_result1 = ouch! { <div>
+    this should be mangled</div>
+        }
+    .to_string();
+}