Add sub module inner attributes to module item attributes

Configurations.md

@@ -2159,7 +2159,7 @@ mod dolor;
 mod sit;
 ```
 
-**Note** `mod` with `#[macro_export]` will not be reordered since that could change the semantics
+**Note** `mod` with `#[macro_export]` or `#![macro_use]` will not be reordered since that could change the semantics
 of the original source code.
 
 ## `report_fixme`

src/modules.rs

@@ -18,6 +18,8 @@ use crate::parse::parser::{
 use crate::parse::session::ParseSess;
 use crate::utils::{contains_skip, mk_sp};
 
+#[cfg(test)]
+mod test;
 mod visitor;
 
 type FileModMap<'ast> = BTreeMap<FileName, Module<'ast>>;
@@ -71,6 +73,7 @@ pub(crate) struct ModResolver<'ast, 'sess> {
     directory: Directory,
     file_map: FileModMap<'ast>,
     recursive: bool,
+    current_mod_file: Option<PathBuf>,
 }
 
 /// Represents errors while trying to resolve modules.
@@ -106,6 +109,25 @@ enum SubModKind<'a, 'ast> {
     MultiExternal(Vec<(PathBuf, DirectoryOwnership, Module<'ast>)>),
     /// `mod foo {}`
     Internal(&'a ast::Item),
+    /// Just like External, but sub modules of this modules are not resolved,
+    /// and this module is not added to the FileModMap
+    SkippedExternal(PathBuf, DirectoryOwnership, Module<'ast>),
+}
+
+impl<'a, 'ast> SubModKind<'a, 'ast> {
+    fn attrs(&self) -> Vec<ast::Attribute> {
+        match self {
+            Self::External(_, _, module) | Self::SkippedExternal(_, _, module) => {
+                module.inner_attr.clone()
+            }
+            Self::MultiExternal(modules) => modules
+                .iter()
+                .map(|(_, _, module)| module.inner_attr.clone().into_iter())
+                .flatten()
+                .collect(),
+            Self::Internal(item) => item.attrs.clone(),
+        }
+    }
 }
 
 impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
@@ -122,6 +144,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
             },
             file_map: BTreeMap::new(),
             parse_sess,
+            current_mod_file: None,
             recursive,
         }
     }
@@ -137,10 +160,10 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
             _ => PathBuf::new(),
         };
 
-        // Skip visiting sub modules when the input is from stdin.
-        if self.recursive {
-            self.visit_mod_from_ast(&krate.items)?;
-        }
+        self.current_mod_file = match root_filename {
+            FileName::Real(ref p) => Some(p.clone()),
+            _ => None,
+        };
 
         let snippet_provider = self.parse_sess.snippet_provider(krate.spans.inner_span);
 
@@ -153,6 +176,12 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
                 Cow::Borrowed(&krate.attrs),
             ),
         );
+
+        // Skip visiting sub modules when the input is from stdin.
+        if self.recursive {
+            self.visit_mod_from_ast(&krate.items)?;
+        }
+
         Ok(self.file_map)
     }
 
@@ -235,29 +264,61 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
         sub_mod: Module<'ast>,
     ) -> Result<(), ModuleResolutionError> {
         let old_directory = self.directory.clone();
+        let old_mod_file = self.current_mod_file.clone();
+
         let sub_mod_kind = self.peek_sub_mod(item, &sub_mod)?;
         if let Some(sub_mod_kind) = sub_mod_kind {
+            let is_internal_mod = matches!(sub_mod_kind, SubModKind::Internal(_));
+            if !is_internal_mod {
+                self.update_mod_item_with_submod_attrs(item, &sub_mod_kind)
+            }
             self.insert_sub_mod(sub_mod_kind.clone())?;
             self.visit_sub_mod_inner(sub_mod, sub_mod_kind)?;
         }
         self.directory = old_directory;
+        self.current_mod_file = old_mod_file;
         Ok(())
     }
 
+    fn update_mod_item_with_submod_attrs(
+        &mut self,
+        item: &'c ast::Item,
+        sub_mod_kind: &SubModKind<'_, '_>,
+    ) {
+        if let Some(path) = &self.current_mod_file {
+            let entry = self.file_map.entry(FileName::Real(path.clone()));
+
+            entry.and_modify(|module| {
+                if let Some(mod_item) = module
+                    .items
+                    .to_mut()
+                    .iter_mut()
+                    .filter(|i| matches!(i.kind, ast::ItemKind::Mod(..)))
+                    .find(|i| {
+                        // The module names need to be the same, and to account for multiple modules
+                        // with the same name (e.g. those annotated with `#[cfg(..)]` we also check
+                        // the spans to make sure we're adding attributes to the correct item.
+                        i.ident == item.ident && i.span == item.span
+                    })
+                {
+                    mod_item.attrs.extend(sub_mod_kind.attrs())
+                }
+            });
+        }
+    }
+
     /// Inspect the given sub-module which we are about to visit and returns its kind.
     fn peek_sub_mod(
         &self,
         item: &'c ast::Item,
         sub_mod: &Module<'ast>,
     ) -> Result<Option<SubModKind<'c, 'ast>>, ModuleResolutionError> {
-        if contains_skip(&item.attrs) {
-            return Ok(None);
-        }
-
         if is_mod_decl(item) {
             // mod foo;
             // Look for an extern file.
             self.find_external_module(item.ident, &item.attrs, sub_mod)
+        } else if contains_skip(&item.attrs) {
+            return Ok(None);
         } else {
             // An internal module (`mod foo { /* ... */ }`);
             Ok(Some(SubModKind::Internal(item)))
@@ -297,6 +358,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
                     path: mod_path.parent().unwrap().to_path_buf(),
                     ownership: directory_ownership,
                 };
+                self.current_mod_file = Some(mod_path);
                 self.visit_sub_mod_after_directory_update(sub_mod, Some(directory))
             }
             SubModKind::Internal(item) => {
@@ -309,10 +371,12 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
                         path: mod_path.parent().unwrap().to_path_buf(),
                         ownership: directory_ownership,
                     };
+                    self.current_mod_file = Some(mod_path);
                     self.visit_sub_mod_after_directory_update(sub_mod, Some(directory))?;
                 }
                 Ok(())
             }
+            SubModKind::SkippedExternal(..) => Ok(()),
         }
     }
 
@@ -351,7 +415,18 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
                 return Ok(None);
             }
             return match Parser::parse_file_as_module(self.parse_sess, &path, sub_mod.span) {
-                Ok((ref attrs, _, _)) if contains_skip(attrs) => Ok(None),
+                Ok((attrs, items, span)) if contains_skip(&attrs) => {
+                    Ok(Some(SubModKind::SkippedExternal(
+                        path,
+                        DirectoryOwnership::Owned { relative: None },
+                        Module::new(
+                            span,
+                            Some(Cow::Owned(ast::ModKind::Unloaded)),
+                            Cow::Owned(items),
+                            Cow::Owned(attrs),
+                        ),
+                    )))
+                }
                 Ok((attrs, items, span)) => Ok(Some(SubModKind::External(
                     path,
                     DirectoryOwnership::Owned { relative: None },
@@ -400,7 +475,18 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
                     }
                 }
                 match Parser::parse_file_as_module(self.parse_sess, &file_path, sub_mod.span) {
-                    Ok((ref attrs, _, _)) if contains_skip(attrs) => Ok(None),
+                    Ok((attrs, items, span)) if contains_skip(&attrs) => {
+                        Ok(Some(SubModKind::SkippedExternal(
+                            file_path,
+                            dir_ownership,
+                            Module::new(
+                                span,
+                                Some(Cow::Owned(ast::ModKind::Unloaded)),
+                                Cow::Owned(items),
+                                Cow::Owned(attrs),
+                            ),
+                        )))
+                    }
                     Ok((attrs, items, span)) if outside_mods_empty => {
                         Ok(Some(SubModKind::External(
                             file_path,

src/modules/test.rs

@@ -0,0 +1,86 @@
+use rustc_span::symbol::{sym, Ident};
+
+use std::path::PathBuf;
+
+use super::{FileModMap, ModResolver, Module};
+use crate::attr::contains_name;
+use crate::config::{Config, FileName};
+use crate::parse::parser::{DirectoryOwnership, Parser};
+use crate::parse::session::ParseSess;
+use crate::utils::contains_skip;
+use crate::Input;
+
+fn validate_file_mod_map<F: Fn(&FileModMap<'_>)>(mod_path: &PathBuf, recursive: bool, f: F) {
+    let config = Config::default();
+    let input = Input::File(mod_path.clone());
+    rustc_span::create_session_if_not_set_then(config.edition().into(), |_| {
+        let parse_session = ParseSess::new(&config).unwrap();
+
+        let directory_ownership = input
+            .to_directory_ownership()
+            .unwrap_or(DirectoryOwnership::UnownedViaBlock);
+        let krate = Parser::parse_crate(input, &parse_session).unwrap();
+
+        let mod_resolver = ModResolver::new(&parse_session, directory_ownership, recursive);
+        let file_map = mod_resolver.visit_crate(&krate).unwrap();
+
+        f(&file_map)
+    })
+}
+
+fn get_submodule<'a>(module: &'a Module<'_>, mod_name: &str) -> &'a rustc_ast::ast::Item {
+    get_nth_submodule(module, mod_name, 0)
+}
+
+fn get_nth_submodule<'a>(
+    module: &'a Module<'_>,
+    mod_name: &str,
+    position: usize,
+) -> &'a rustc_ast::ast::Item {
+    module
+        .items
+        .iter()
+        .filter(|i| i.ident == Ident::from_str(mod_name))
+        .nth(position)
+        .unwrap()
+}
+
+#[test]
+fn external_sub_module_inner_attrs_are_present_in_mod_item_attrs_list() {
+    // Ensure we can access external submodule inner attributes from the module items.
+    //
+    // Some inner attributes have formatting implications. For example, `#![rustfmt::skip]`
+    // informs rustfmt not to format the module, and `#![macro_use]` informs rustfmt that it can't
+    // safely reorder the module for fear of macro name collisions.
+
+    let path = PathBuf::from("tests/mod-resolver/issue-4959-sub-mod-inner-attr/lib.rs");
+    validate_file_mod_map(&path, true, |file_map| {
+        let module = file_map.get(&FileName::Real(path.clone())).unwrap();
+
+        let mod_a = get_submodule(module, "a");
+        assert!(contains_name(&mod_a.attrs, sym::macro_use));
+
+        let mod_b = get_submodule(module, "b");
+        assert!(contains_name(&mod_b.attrs, sym::macro_use));
+
+        // mod c is annotated with `#[rustfmt::skip]`, but we should still have access to
+        // the inner attributes
+        let mod_c = get_submodule(module, "c");
+        assert!(contains_name(&mod_c.attrs, sym::macro_use));
+        assert!(contains_skip(&mod_c.attrs));
+
+        // mod d is annotated with an inner `#![rustfmt::skip]` attribute.
+        let mod_d = get_submodule(module, "d");
+        assert!(contains_skip(&mod_d.attrs));
+
+        // mod e is defined in both e1.rs and e2.rs depending on the operating system.
+        // We should ensure that attributes from one path aren't added to the other.
+        let mod_e1 = get_nth_submodule(module, "e", 0);
+        assert!(contains_name(&mod_e1.attrs, sym::no_std));
+        assert!(!contains_name(&mod_e1.attrs, sym::no_implicit_prelude));
+
+        let mod_e2 = get_nth_submodule(module, "e", 1);
+        assert!(contains_name(&mod_e2.attrs, sym::no_implicit_prelude));
+        assert!(!contains_name(&mod_e2.attrs, sym::no_std));
+    });
+}

tests/mod-resolver/issue-4959-sub-mod-inner-attr/a.rs

@@ -0,0 +1 @@
+#![macro_use]

tests/mod-resolver/issue-4959-sub-mod-inner-attr/b/mod.rs

@@ -0,0 +1 @@
+#![macro_use]

tests/mod-resolver/issue-4959-sub-mod-inner-attr/c.rs

@@ -0,0 +1 @@
+#![macro_use]

tests/mod-resolver/issue-4959-sub-mod-inner-attr/d.rs

@@ -0,0 +1 @@
+#![rustfmt::skip]

tests/mod-resolver/issue-4959-sub-mod-inner-attr/e1.rs

@@ -0,0 +1 @@
+#![no_std]

tests/mod-resolver/issue-4959-sub-mod-inner-attr/e2.rs

@@ -0,0 +1 @@
+#![no_implicit_prelude]

tests/mod-resolver/issue-4959-sub-mod-inner-attr/lib.rs

@@ -0,0 +1,12 @@
+mod b;
+mod a;
+#[rustfmt::skip]
+mod c;
+mod d;
+
+#[cfg(linux)]
+#[path ="e1.rs"]
+mod e;
+#[cfg(not(linux))]
+#[path ="e2.rs"]
+mod e;

tests/target/issue-4959/a.rs

@@ -0,0 +1 @@
+#![macro_use]

tests/target/issue-4959/b.rs

@@ -0,0 +1 @@
+#![macro_use]

tests/target/issue-4959/cant_reorder_mods_with_inner_macro_use.rs

@@ -0,0 +1,3 @@
+// rustfmt-reorder_modules: true
+mod b;
+mod a;