Merge pull request #3448 from topecongiro/use-new_sub_parser_from_file

Support path clarity module even when we start from internal module

src/formatting.rs

@@ -10,7 +10,7 @@ use syntax::ast;
 use syntax::errors::emitter::{ColorConfig, EmitterWriter};
 use syntax::errors::{DiagnosticBuilder, Handler};
 use syntax::parse::{self, ParseSess};
-use syntax::source_map::{FilePathMapping, SourceMap, Span};
+use syntax::source_map::{FilePathMapping, SourceMap, Span, DUMMY_SP};
 
 use crate::comment::{CharClasses, FullCodeCharKind};
 use crate::config::{Config, FileName, Verbosity};
@@ -73,7 +73,14 @@ fn format_project<T: FormatHandler>(
     let source_map = Rc::new(SourceMap::new(FilePathMapping::empty()));
     let mut parse_session = make_parse_sess(source_map.clone(), config);
     let mut report = FormatReport::new();
-    let krate = match parse_crate(input, &parse_session, config, &mut report) {
+    let directory_ownership = input.to_directory_ownership();
+    let krate = match parse_crate(
+        input,
+        &parse_session,
+        config,
+        &mut report,
+        directory_ownership,
+    ) {
         Ok(krate) => krate,
         // Surface parse error via Session (errors are merged there from report)
         Err(ErrorKind::ParseError) => return Ok(report),
@@ -87,8 +94,14 @@ fn format_project<T: FormatHandler>(
 
     let mut context = FormatContext::new(&krate, report, parse_session, config, handler);
 
-    let files = modules::list_files(&krate, context.parse_session.source_map())?;
+    let files = modules::ModResolver::new(
-    for (path, module) in files {
+        context.parse_session.source_map(),
+        directory_ownership.unwrap_or(parse::DirectoryOwnership::UnownedViaMod(false)),
+        input_is_stdin,
+    )
+    .visit_crate(&krate)
+    .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;
+    for (path, (module, _)) in files {
         if (config.skip_children() && path != main_file) || config.ignore().skip_file(&path) {
             continue;
         }
@@ -593,16 +606,28 @@ fn parse_crate(
     parse_session: &ParseSess,
     config: &Config,
     report: &mut FormatReport,
+    directory_ownership: Option<parse::DirectoryOwnership>,
 ) -> Result<ast::Crate, ErrorKind> {
     let input_is_stdin = input.is_text();
 
     let parser = match input {
-        Input::File(file) => Ok(parse::new_parser_from_file(parse_session, &file)),
+        Input::File(ref file) => {
+            // Use `new_sub_parser_from_file` when we the input is a submodule.
+            Ok(if let Some(dir_own) = directory_ownership {
+                parse::new_sub_parser_from_file(parse_session, file, dir_own, None, DUMMY_SP)
+            } else {
+                parse::new_parser_from_file(parse_session, file)
+            })
+        }
         Input::Text(text) => parse::maybe_new_parser_from_source_str(
             parse_session,
             syntax::source_map::FileName::Custom("stdin".to_owned()),
             text,
         )
+        .map(|mut parser| {
+            parser.recurse_into_file_modules = false;
+            parser
+        })
         .map_err(|diags| {
             diags
                 .into_iter()

src/lib.rs

@@ -20,7 +20,7 @@ use std::path::PathBuf;
 use std::rc::Rc;
 
 use failure::Fail;
-use syntax::ast;
+use syntax::{ast, parse::DirectoryOwnership};
 
 use crate::comment::LineClasses;
 use crate::formatting::{FormatErrorMap, FormattingError, ReportedErrors, SourceFile};
@@ -586,6 +586,24 @@ impl Input {
             Input::Text(..) => FileName::Stdin,
         }
     }
+
+    fn to_directory_ownership(&self) -> Option<DirectoryOwnership> {
+        match self {
+            Input::File(ref file) => {
+                // If there exists a directory with the same name as an input,
+                // then the input should be parsed as a sub module.
+                let file_stem = file.file_stem()?;
+                if file.parent()?.to_path_buf().join(file_stem).is_dir() {
+                    Some(DirectoryOwnership::Owned {
+                        relative: file_stem.to_str().map(ast::Ident::from_str),
+                    })
+                } else {
+                    None
+                }
+            }
+            _ => None,
+        }
+    }
 }
 
 #[cfg(test)]

src/modules.rs

@@ -1,5 +1,4 @@
 use std::collections::BTreeMap;
-use std::io;
 use std::path::{Path, PathBuf};
 
 use syntax::ast;
@@ -8,25 +7,157 @@ use syntax::source_map;
 use syntax_pos::symbol::Symbol;
 
 use crate::config::FileName;
+use crate::items::is_mod_decl;
 use crate::utils::contains_skip;
 
-/// List all the files containing modules of a crate.
+type FileModMap<'a> = BTreeMap<FileName, (&'a ast::Mod, &'a str)>;
-/// If a file is used twice in a crate, it appears only once.
+
-pub fn list_files<'a>(
+/// Maps each module to the corresponding file.
-    krate: &'a ast::Crate,
+pub struct ModResolver<'a, 'b> {
-    source_map: &source_map::SourceMap,
+    source_map: &'b source_map::SourceMap,
-) -> Result<BTreeMap<FileName, &'a ast::Mod>, io::Error> {
+    directory: Directory,
-    let mut result = BTreeMap::new(); // Enforce file order determinism
+    file_map: FileModMap<'a>,
-    let root_filename = source_map.span_to_filename(krate.span);
+    is_input_stdin: bool,
-    {
+}
-        let parent = match root_filename {
+
-            source_map::FileName::Real(ref path) => path.parent().unwrap(),
+#[derive(Clone)]
-            _ => Path::new(""),
+struct Directory {
-        };
+    path: PathBuf,
-        list_submodules(&krate.module, parent, None, source_map, &mut result)?;
+    ownership: DirectoryOwnership,
+}
+
+impl<'a, 'b> ModResolver<'a, 'b> {
+    /// Creates a new `ModResolver`.
+    pub fn new(
+        source_map: &'b source_map::SourceMap,
+        directory_ownership: DirectoryOwnership,
+        is_input_stdin: bool,
+    ) -> Self {
+        ModResolver {
+            directory: Directory {
+                path: PathBuf::new(),
+                ownership: directory_ownership,
+            },
+            file_map: BTreeMap::new(),
+            source_map,
+            is_input_stdin,
+        }
+    }
+
+    /// Creates a map that maps a file name to the module in AST.
+    pub fn visit_crate(mut self, krate: &'a ast::Crate) -> Result<FileModMap<'a>, String> {
+        let root_filename = self.source_map.span_to_filename(krate.span);
+        self.directory.path = match root_filename {
+            source_map::FileName::Real(ref path) => path
+                .parent()
+                .expect("Parent directory should exists")
+                .to_path_buf(),
+            _ => PathBuf::new(),
+        };
+
+        // Skip visiting sub modules when the input is from stdin.
+        if !self.is_input_stdin {
+            self.visit_mod(&krate.module)?;
+        }
+
+        self.file_map
+            .insert(root_filename.into(), (&krate.module, ""));
+        Ok(self.file_map)
+    }
+
+    fn visit_mod(&mut self, module: &'a ast::Mod) -> Result<(), String> {
+        for item in &module.items {
+            if let ast::ItemKind::Mod(ref sub_mod) = item.node {
+                if contains_skip(&item.attrs) {
+                    continue;
+                }
+
+                let old_direcotry = self.directory.clone();
+                if is_mod_decl(item) {
+                    // mod foo;
+                    // Look for an extern file.
+                    let (mod_path, directory_ownership) =
+                        self.find_external_module(item.ident, &item.attrs)?;
+                    self.file_map.insert(
+                        FileName::Real(mod_path.clone()),
+                        (sub_mod, item.ident.name.as_str().get()),
+                    );
+                    self.directory = Directory {
+                        path: mod_path.parent().unwrap().to_path_buf(),
+                        ownership: directory_ownership,
+                    }
+                } else {
+                    // An internal module (`mod foo { /* ... */ }`);
+                    if let Some(path) = find_path_value(&item.attrs) {
+                        // All `#[path]` files are treated as though they are a `mod.rs` file.
+                        self.directory = Directory {
+                            path: Path::new(&path.as_str()).to_path_buf(),
+                            ownership: DirectoryOwnership::Owned { relative: None },
+                        };
+                    } else {
+                        self.push_inline_mod_directory(item.ident, &item.attrs);
+                    }
+                }
+                self.visit_mod(sub_mod)?;
+                self.directory = old_direcotry;
+            }
+        }
+        Ok(())
+    }
+
+    fn find_external_module(
+        &self,
+        mod_name: ast::Ident,
+        attrs: &[ast::Attribute],
+    ) -> Result<(PathBuf, DirectoryOwnership), String> {
+        if let Some(path) = parser::Parser::submod_path_from_attr(attrs, &self.directory.path) {
+            return Ok((path, DirectoryOwnership::Owned { relative: None }));
+        }
+
+        let relative = match self.directory.ownership {
+            DirectoryOwnership::Owned { relative } => relative,
+            DirectoryOwnership::UnownedViaBlock | DirectoryOwnership::UnownedViaMod(_) => None,
+        };
+        match parser::Parser::default_submod_path(
+            mod_name,
+            relative,
+            &self.directory.path,
+            self.source_map,
+        )
+        .result
+        {
+            Ok(parser::ModulePathSuccess {
+                path,
+                directory_ownership,
+                ..
+            }) => Ok((path, directory_ownership)),
+            Err(_) => Err(format!(
+                "Failed to find module {} in {:?} {:?}",
+                mod_name, self.directory.path, relative,
+            )),
+        }
+    }
+
+    fn push_inline_mod_directory(&mut self, id: ast::Ident, attrs: &[ast::Attribute]) {
+        if let Some(path) = find_path_value(attrs) {
+            self.directory.path.push(&path.as_str());
+            self.directory.ownership = DirectoryOwnership::Owned { relative: None };
+        } else {
+            // We have to push on the current module name in the case of relative
+            // paths in order to ensure that any additional module paths from inline
+            // `mod x { ... }` come after the relative extension.
+            //
+            // For example, a `mod z { ... }` inside `x/y.rs` should set the current
+            // directory path to `/x/y/z`, not `/x/z` with a relative offset of `y`.
+            if let DirectoryOwnership::Owned { relative } = &mut self.directory.ownership {
+                if let Some(ident) = relative.take() {
+                    // remove the relative offset
+                    self.directory.path.push(ident.as_str());
+                }
+            }
+            self.directory.path.push(&id.as_str());
+        }
     }
-    result.insert(root_filename.into(), &krate.module);
-    Ok(result)
 }
 
 fn path_value(attr: &ast::Attribute) -> Option<Symbol> {
@@ -43,69 +174,3 @@ fn path_value(attr: &ast::Attribute) -> Option<Symbol> {
 fn find_path_value(attrs: &[ast::Attribute]) -> Option<Symbol> {
     attrs.iter().flat_map(path_value).next()
 }
-
-/// Recursively list all external modules included in a module.
-fn list_submodules<'a>(
-    module: &'a ast::Mod,
-    search_dir: &Path,
-    relative: Option<ast::Ident>,
-    source_map: &source_map::SourceMap,
-    result: &mut BTreeMap<FileName, &'a ast::Mod>,
-) -> Result<(), io::Error> {
-    debug!("list_submodules: search_dir: {:?}", search_dir);
-    for item in &module.items {
-        if let ast::ItemKind::Mod(ref sub_mod) = item.node {
-            if !contains_skip(&item.attrs) {
-                let is_internal = source_map.span_to_filename(item.span)
-                    == source_map.span_to_filename(sub_mod.inner);
-                let (dir_path, relative) = if is_internal {
-                    if let Some(path) = find_path_value(&item.attrs) {
-                        (search_dir.join(&path.as_str()), None)
-                    } else {
-                        (search_dir.join(&item.ident.to_string()), None)
-                    }
-                } else {
-                    let (mod_path, relative) =
-                        module_file(item.ident, &item.attrs, search_dir, relative, source_map)?;
-                    let dir_path = mod_path.parent().unwrap().to_owned();
-                    result.insert(FileName::Real(mod_path), sub_mod);
-                    (dir_path, relative)
-                };
-                list_submodules(sub_mod, &dir_path, relative, source_map, result)?;
-            }
-        }
-    }
-    Ok(())
-}
-
-/// Finds the file corresponding to an external mod
-fn module_file(
-    id: ast::Ident,
-    attrs: &[ast::Attribute],
-    dir_path: &Path,
-    relative: Option<ast::Ident>,
-    source_map: &source_map::SourceMap,
-) -> Result<(PathBuf, Option<ast::Ident>), io::Error> {
-    if let Some(path) = parser::Parser::submod_path_from_attr(attrs, dir_path) {
-        return Ok((path, None));
-    }
-
-    match parser::Parser::default_submod_path(id, relative, dir_path, source_map).result {
-        Ok(parser::ModulePathSuccess {
-            path,
-            directory_ownership,
-            ..
-        }) => {
-            let relative = if let DirectoryOwnership::Owned { relative } = directory_ownership {
-                relative
-            } else {
-                None
-            };
-            Ok((path, relative))
-        }
-        Err(_) => Err(io::Error::new(
-            io::ErrorKind::Other,
-            format!("Couldn't find module {}", id),
-        )),
-    }
-}

src/test/mod.rs

@@ -17,6 +17,19 @@ use crate::{FormatReport, Input, Session};
 const DIFF_CONTEXT_SIZE: usize = 3;
 const CONFIGURATIONS_FILE_NAME: &str = "Configurations.md";
 
+// A list of files on which we want to skip testing.
+const SKIP_FILE_WHITE_LIST: &[&str] = &[
+    // 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",
+];
+
+fn is_file_skip(path: &Path) -> bool {
+    SKIP_FILE_WHITE_LIST
+        .iter()
+        .any(|file_path| path.ends_with(file_path))
+}
+
 // Returns a `Vec` containing `PathBuf`s of files with an  `rs` extension in the
 // given path. The `recursive` argument controls if files from subdirectories
 // are also returned.
@@ -31,7 +44,7 @@ fn get_test_files(path: &Path, recursive: bool) -> Vec<PathBuf> {
             let path = entry.path();
             if path.is_dir() && recursive {
                 files.append(&mut get_test_files(&path, recursive));
-            } else if path.extension().map_or(false, |f| f == "rs") {
+            } else if path.extension().map_or(false, |f| f == "rs") && !is_file_skip(&path) {
                 files.push(path);
             }
         }
@@ -231,6 +244,10 @@ fn idempotence_tests() {
 // no warnings are emitted.
 #[test]
 fn self_tests() {
+    match option_env!("CFG_RELEASE_CHANNEL") {
+        None | Some("nightly") => {}
+        _ => return, // Issue-3443: these tests require nightly
+    }
     let mut files = get_test_files(Path::new("tests"), false);
     let bin_directories = vec!["cargo-fmt", "git-rustfmt", "bin", "format-diff"];
     for dir in bin_directories {

tests/source/configs/skip_children/foo/mod.rs

@@ -0,0 +1,3 @@
+fn skip_formatting_this() {
+    println ! ( "Skip this" ) ;
+}

tests/source/path_clarity/foo.rs

@@ -0,0 +1,2 @@
+// rustfmt-edition: 2018
+mod bar;

tests/source/path_clarity/foo/bar.rs

@@ -0,0 +1,3 @@
+pub fn fn_in_bar( ) {
+    println!( "foo/bar.rs" );
+}

tests/target/configs/skip_children/foo/mod.rs

@@ -0,0 +1,3 @@
+fn skip_formatting_this() {
+    println ! ( "Skip this" ) ;
+}

tests/target/path_clarity/foo.rs

@@ -0,0 +1,2 @@
+// rustfmt-edition: 2018
+mod bar;

tests/target/path_clarity/foo/bar.rs

@@ -0,0 +1,3 @@
+pub fn fn_in_bar() {
+    println!("foo/bar.rs");
+}