From 9d281b44c3a080cabc570780088627752c5a4dd5 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Sun, 17 Mar 2019 12:21:21 +0900 Subject: [PATCH 1/6] Add a test for #3427 --- tests/source/path_clarity/foo.rs | 2 ++ tests/source/path_clarity/foo/bar.rs | 3 +++ tests/target/path_clarity/foo.rs | 2 ++ tests/target/path_clarity/foo/bar.rs | 3 +++ 4 files changed, 10 insertions(+) create mode 100644 tests/source/path_clarity/foo.rs create mode 100644 tests/source/path_clarity/foo/bar.rs create mode 100644 tests/target/path_clarity/foo.rs create mode 100644 tests/target/path_clarity/foo/bar.rs diff --git a/tests/source/path_clarity/foo.rs b/tests/source/path_clarity/foo.rs new file mode 100644 index 00000000000..cd247fabfe3 --- /dev/null +++ b/tests/source/path_clarity/foo.rs @@ -0,0 +1,2 @@ +// rustfmt-edition: 2018 +mod bar; diff --git a/tests/source/path_clarity/foo/bar.rs b/tests/source/path_clarity/foo/bar.rs new file mode 100644 index 00000000000..8c1be504c09 --- /dev/null +++ b/tests/source/path_clarity/foo/bar.rs @@ -0,0 +1,3 @@ +pub fn fn_in_bar( ) { + println!( "foo/bar.rs" ); +} diff --git a/tests/target/path_clarity/foo.rs b/tests/target/path_clarity/foo.rs new file mode 100644 index 00000000000..cd247fabfe3 --- /dev/null +++ b/tests/target/path_clarity/foo.rs @@ -0,0 +1,2 @@ +// rustfmt-edition: 2018 +mod bar; diff --git a/tests/target/path_clarity/foo/bar.rs b/tests/target/path_clarity/foo/bar.rs new file mode 100644 index 00000000000..b18a7d3499c --- /dev/null +++ b/tests/target/path_clarity/foo/bar.rs @@ -0,0 +1,3 @@ +pub fn fn_in_bar() { + println!("foo/bar.rs"); +} From bde77714e696602f52ca6d9378eabd9a642a1a27 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Sun, 17 Mar 2019 12:21:57 +0900 Subject: [PATCH 2/6] Disable self_tests on beta channel --- src/test/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/test/mod.rs b/src/test/mod.rs index 1992190bb62..20845b08f83 100644 --- a/src/test/mod.rs +++ b/src/test/mod.rs @@ -231,6 +231,10 @@ fn idempotence_tests() { // no warnings are emitted. #[test] fn self_tests() { + match option_env!("CFG_RELEASE_CHANNEL") { + None | Some("nightly") => {} + _ => return, // 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 { From 92f57211c1ed9a27fcd86bf05ee9c930b1b4449a Mon Sep 17 00:00:00 2001 From: topecongiro Date: Sun, 17 Mar 2019 12:25:59 +0900 Subject: [PATCH 3/6] Support path-clarity submodule --- src/formatting.rs | 31 ++++++- src/lib.rs | 20 +++- src/modules.rs | 229 +++++++++++++++++++++++++++++----------------- 3 files changed, 192 insertions(+), 88 deletions(-) diff --git a/src/formatting.rs b/src/formatting.rs index eee02f275cf..56adfa9b1a3 100644 --- a/src/formatting.rs +++ b/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( 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( let mut context = FormatContext::new(&krate, report, parse_session, config, handler); - let files = modules::list_files(&krate, context.parse_session.source_map())?; - for (path, module) in files { + let files = modules::ModResolver::new( + 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,11 +606,19 @@ fn parse_crate( parse_session: &ParseSess, config: &Config, report: &mut FormatReport, + directory_ownership: Option, ) -> Result { 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()), diff --git a/src/lib.rs b/src/lib.rs index 53e74210ede..b1090e95ff3 100644 --- a/src/lib.rs +++ b/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 { + 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)] diff --git a/src/modules.rs b/src/modules.rs index 7bb50356ed8..01a0784c6f4 100644 --- a/src/modules.rs +++ b/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. -/// If a file is used twice in a crate, it appears only once. -pub fn list_files<'a>( - krate: &'a ast::Crate, - source_map: &source_map::SourceMap, -) -> Result, io::Error> { - let mut result = BTreeMap::new(); // Enforce file order determinism - let root_filename = source_map.span_to_filename(krate.span); - { - let parent = match root_filename { - source_map::FileName::Real(ref path) => path.parent().unwrap(), - _ => Path::new(""), +type FileModMap<'a> = BTreeMap; + +/// Maps each module to the corresponding file. +pub struct ModResolver<'a, 'b> { + source_map: &'b source_map::SourceMap, + directory: Directory, + file_map: FileModMap<'a>, + is_input_stdin: bool, +} + +#[derive(Clone)] +struct Directory { + path: PathBuf, + 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, 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, }; - list_submodules(&krate.module, parent, None, source_map, &mut result)?; + 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 { @@ -43,69 +174,3 @@ fn path_value(attr: &ast::Attribute) -> Option { fn find_path_value(attrs: &[ast::Attribute]) -> Option { 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, - source_map: &source_map::SourceMap, - result: &mut BTreeMap, -) -> 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, - source_map: &source_map::SourceMap, -) -> Result<(PathBuf, Option), 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), - )), - } -} From d0e96ed3310a289354cf7358a19fce02254cba40 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Sun, 17 Mar 2019 12:26:45 +0900 Subject: [PATCH 4/6] Do not look for external modules when the input is from stdin --- src/formatting.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/formatting.rs b/src/formatting.rs index 56adfa9b1a3..a23c312bd2a 100644 --- a/src/formatting.rs +++ b/src/formatting.rs @@ -624,6 +624,10 @@ fn parse_crate( 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() From 7fc4e418bfcc77b2ff6fc63cd542442d203b5eab Mon Sep 17 00:00:00 2001 From: topecongiro Date: Sun, 17 Mar 2019 12:27:24 +0900 Subject: [PATCH 5/6] Tweak test settings --- src/test/mod.rs | 15 ++++++++++++++- tests/source/configs/skip_children/foo/mod.rs | 3 +++ tests/target/configs/skip_children/foo/mod.rs | 3 +++ 3 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 tests/source/configs/skip_children/foo/mod.rs create mode 100644 tests/target/configs/skip_children/foo/mod.rs diff --git a/src/test/mod.rs b/src/test/mod.rs index 20845b08f83..98f81cbe245 100644 --- a/src/test/mod.rs +++ b/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 { 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); } } diff --git a/tests/source/configs/skip_children/foo/mod.rs b/tests/source/configs/skip_children/foo/mod.rs new file mode 100644 index 00000000000..d7ff6cdb829 --- /dev/null +++ b/tests/source/configs/skip_children/foo/mod.rs @@ -0,0 +1,3 @@ +fn skip_formatting_this() { + println ! ( "Skip this" ) ; +} diff --git a/tests/target/configs/skip_children/foo/mod.rs b/tests/target/configs/skip_children/foo/mod.rs new file mode 100644 index 00000000000..d7ff6cdb829 --- /dev/null +++ b/tests/target/configs/skip_children/foo/mod.rs @@ -0,0 +1,3 @@ +fn skip_formatting_this() { + println ! ( "Skip this" ) ; +} From be0ada27f4d0686b77e8119b223129d104985187 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Campinas?= Date: Sun, 17 Mar 2019 12:30:06 +0900 Subject: [PATCH 6/6] Add comment which refers to an issue on nightly-only test Co-Authored-By: topecongiro --- src/test/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/mod.rs b/src/test/mod.rs index 98f81cbe245..61d7c88884a 100644 --- a/src/test/mod.rs +++ b/src/test/mod.rs @@ -246,7 +246,7 @@ fn idempotence_tests() { fn self_tests() { match option_env!("CFG_RELEASE_CHANNEL") { None | Some("nightly") => {} - _ => return, // these tests require 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"];