Improve error message when module resolution failed (#4198)

topecongiro · web-flow · commit 8842c43903d4 · 2020-05-25T08:34:08.000+09:00
diff --git a/rustfmt-core/rustfmt-bin/src/bin/main.rs b/rustfmt-core/rustfmt-bin/src/bin/main.rs
@@ -323,16 +323,10 @@ pub enum OperationError {
     #[error("The `--print-config=minimal` option doesn't work with standard input.")]
     MinimalPathWithStdin,
     /// An io error during reading or writing.
-    #[error("io error: {0}")]
+    #[error("{0}")]
     IoError(IoError),
 }
 
-impl From<IoError> for OperationError {
-    fn from(e: IoError) -> OperationError {
-        OperationError::IoError(e)
-    }
-}
-
 impl CliOptions for Opt {
     fn apply_to(&self, config: &mut Config) {
         config.set().file_lines(self.file_lines.clone());
diff --git a/rustfmt-core/rustfmt-lib/src/formatting.rs b/rustfmt-core/rustfmt-lib/src/formatting.rs
@@ -1,6 +1,5 @@
 // High level formatting functions.
 
-use std::io;
 use std::time::{Duration, Instant};
 
 use rustc_ast::ast;
@@ -117,8 +116,7 @@ fn format_project(
         directory_ownership.unwrap_or(DirectoryOwnership::UnownedViaMod),
         !input_is_stdin && operation_setting.recursive,
     )
-    .visit_crate(&krate)
-    .map_err(|e| OperationError::IoError(io::Error::new(io::ErrorKind::Other, e)))?;
+    .visit_crate(&krate)?;
 
     for (path, module) in files {
         let should_ignore = !input_is_stdin && parse_session.ignore_file(&path);
diff --git a/rustfmt-core/rustfmt-lib/src/formatting/modules.rs b/rustfmt-core/rustfmt-lib/src/formatting/modules.rs
@@ -5,12 +5,13 @@ use std::path::{Path, PathBuf};
 use rustc_ast::ast;
 use rustc_ast::visit::Visitor;
 use rustc_span::symbol::{self, sym, Symbol};
+use thiserror::Error;
 
 use crate::config::FileName;
 use crate::formatting::{
     attr::MetaVisitor,
     items::is_mod_decl,
-    syntux::parser::{Directory, DirectoryOwnership, ModulePathSuccess, Parser},
+    syntux::parser::{Directory, DirectoryOwnership, ModulePathSuccess, Parser, ParserError},
     syntux::session::ParseSess,
     utils::contains_skip,
 };
@@ -31,6 +32,24 @@ pub(crate) struct ModResolver<'ast, 'sess> {
     recursive: bool,
 }
 
+/// Represents errors while trying to resolve modules.
+#[error("failed to resolve mod `{module}`: {kind}")]
+#[derive(Debug, Error)]
+pub struct ModuleResolutionError {
+    module: String,
+    kind: ModuleResolutionErrorKind,
+}
+
+#[derive(Debug, Error)]
+pub(crate) enum ModuleResolutionErrorKind {
+    /// Find a file that cannot be parsed.
+    #[error("cannot parse {file}")]
+    ParseError { file: PathBuf },
+    /// File cannot be found.
+    #[error("{file} does not exist")]
+    NotFound { file: PathBuf },
+}
+
 #[derive(Clone)]
 enum SubModKind<'a, 'ast> {
     /// `mod foo;`
@@ -63,7 +82,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
     pub(crate) fn visit_crate(
         mut self,
         krate: &'ast ast::Crate,
-    ) -> Result<FileModMap<'ast>, String> {
+    ) -> Result<FileModMap<'ast>, ModuleResolutionError> {
         let root_filename = self.parse_sess.span_to_filename(krate.span);
         self.directory.path = match root_filename {
             FileName::Real(ref p) => p.parent().unwrap_or_else(|| Path::new("")).to_path_buf(),
@@ -81,7 +100,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
     }
 
     /// Visit `cfg_if` macro and look for module declarations.
-    fn visit_cfg_if(&mut self, item: Cow<'ast, ast::Item>) -> Result<(), String> {
+    fn visit_cfg_if(&mut self, item: Cow<'ast, ast::Item>) -> Result<(), ModuleResolutionError> {
         let mut visitor = visitor::CfgIfVisitor::new(self.parse_sess);
         visitor.visit_item(&item);
         for module_item in visitor.mods() {
@@ -93,7 +112,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
     }
 
     /// Visit modules defined inside macro calls.
-    fn visit_mod_outside_ast(&mut self, module: ast::Mod) -> Result<(), String> {
+    fn visit_mod_outside_ast(&mut self, module: ast::Mod) -> Result<(), ModuleResolutionError> {
         for item in module.items {
             if is_cfg_if(&item) {
                 self.visit_cfg_if(Cow::Owned(item.into_inner()))?;
@@ -108,7 +127,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
     }
 
     /// Visit modules from AST.
-    fn visit_mod_from_ast(&mut self, module: &'ast ast::Mod) -> Result<(), String> {
+    fn visit_mod_from_ast(&mut self, module: &'ast ast::Mod) -> Result<(), ModuleResolutionError> {
         for item in &module.items {
             if is_cfg_if(item) {
                 self.visit_cfg_if(Cow::Borrowed(item))?;
@@ -125,7 +144,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
         &mut self,
         item: &'c ast::Item,
         sub_mod: Cow<'ast, ast::Mod>,
-    ) -> Result<(), String> {
+    ) -> Result<(), ModuleResolutionError> {
         let old_directory = self.directory.clone();
         let sub_mod_kind = self.peek_sub_mod(item, &sub_mod)?;
         if let Some(sub_mod_kind) = sub_mod_kind {
@@ -141,7 +160,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
         &self,
         item: &'c ast::Item,
         sub_mod: &Cow<'ast, ast::Mod>,
-    ) -> Result<Option<SubModKind<'c, 'ast>>, String> {
+    ) -> Result<Option<SubModKind<'c, 'ast>>, ModuleResolutionError> {
         if contains_skip(&item.attrs) {
             return Ok(None);
         }
@@ -160,7 +179,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
         &mut self,
         sub_mod_kind: SubModKind<'c, 'ast>,
         _sub_mod: Cow<'ast, ast::Mod>,
-    ) -> Result<(), String> {
+    ) -> Result<(), ModuleResolutionError> {
         match sub_mod_kind {
             SubModKind::External(mod_path, _, sub_mod) => {
                 self.file_map
@@ -183,7 +202,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
         &mut self,
         sub_mod: Cow<'ast, ast::Mod>,
         sub_mod_kind: SubModKind<'c, 'ast>,
-    ) -> Result<(), String> {
+    ) -> Result<(), ModuleResolutionError> {
         match sub_mod_kind {
             SubModKind::External(mod_path, directory_ownership, sub_mod) => {
                 let directory = Directory {
@@ -213,7 +232,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
         &mut self,
         sub_mod: Cow<'ast, ast::Mod>,
         directory: Option<Directory>,
-    ) -> Result<(), String> {
+    ) -> Result<(), ModuleResolutionError> {
         if let Some(directory) = directory {
             self.directory = directory;
         }
@@ -229,7 +248,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
         mod_name: symbol::Ident,
         attrs: &[ast::Attribute],
         sub_mod: &Cow<'ast, ast::Mod>,
-    ) -> Result<Option<SubModKind<'c, 'ast>>, String> {
+    ) -> Result<Option<SubModKind<'c, 'ast>>, ModuleResolutionError> {
         let relative = match self.directory.ownership {
             DirectoryOwnership::Owned { relative } => relative,
             DirectoryOwnership::UnownedViaBlock | DirectoryOwnership::UnownedViaMod => None,
@@ -239,15 +258,19 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
                 return Ok(None);
             }
             return match Parser::parse_file_as_module(self.parse_sess, &path, sub_mod.inner) {
-                Some(m) => Ok(Some(SubModKind::External(
+                Ok(m) => Ok(Some(SubModKind::External(
                     path,
                     DirectoryOwnership::Owned { relative: None },
                     Cow::Owned(m),
                 ))),
-                None => Err(format!(
-                    "Failed to find module {} in {:?} {:?}",
-                    mod_name, self.directory.path, relative,
-                )),
+                Err(ParserError::ParseError) => Err(ModuleResolutionError {
+                    module: mod_name.to_string(),
+                    kind: ModuleResolutionErrorKind::ParseError { file: path },
+                }),
+                Err(..) => Err(ModuleResolutionError {
+                    module: mod_name.to_string(),
+                    kind: ModuleResolutionErrorKind::NotFound { file: path },
+                }),
             };
         }
 
@@ -277,21 +300,25 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
                     }
                 }
                 match Parser::parse_file_as_module(self.parse_sess, &path, sub_mod.inner) {
-                    Some(m) if outside_mods_empty => {
+                    Ok(m) if outside_mods_empty => {
                         Ok(Some(SubModKind::External(path, ownership, Cow::Owned(m))))
                     }
-                    Some(m) => {
+                    Ok(m) => {
                         mods_outside_ast.push((path.clone(), ownership, Cow::Owned(m)));
                         if should_insert {
                             mods_outside_ast.push((path, ownership, sub_mod.clone()));
                         }
                         Ok(Some(SubModKind::MultiExternal(mods_outside_ast)))
                     }
-                    None if outside_mods_empty => Err(format!(
-                        "Failed to find module {} in {:?} {:?}",
-                        mod_name, self.directory.path, relative,
-                    )),
-                    None => {
+                    Err(ParserError::ParseError) => Err(ModuleResolutionError {
+                        module: mod_name.to_string(),
+                        kind: ModuleResolutionErrorKind::ParseError { file: path },
+                    }),
+                    Err(..) if outside_mods_empty => Err(ModuleResolutionError {
+                        module: mod_name.to_string(),
+                        kind: ModuleResolutionErrorKind::NotFound { file: path },
+                    }),
+                    Err(..) => {
                         if should_insert {
                             mods_outside_ast.push((path, ownership, sub_mod.clone()));
                         }
@@ -305,10 +332,12 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
             }
             Err(mut e) => {
                 e.cancel();
-                Err(format!(
-                    "Failed to find module {} in {:?} {:?}",
-                    mod_name, self.directory.path, relative,
-                ))
+                Err(ModuleResolutionError {
+                    module: mod_name.to_string(),
+                    kind: ModuleResolutionErrorKind::NotFound {
+                        file: self.directory.path.clone(),
+                    },
+                })
             }
         }
     }
@@ -367,8 +396,8 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> {
 
             let m = match Parser::parse_file_as_module(self.parse_sess, &actual_path, sub_mod.inner)
             {
-                Some(m) => m,
-                None => continue,
+                Ok(m) => m,
+                Err(..) => continue,
             };
 
             result.push((
diff --git a/rustfmt-core/rustfmt-lib/src/formatting/syntux/parser.rs b/rustfmt-core/rustfmt-lib/src/formatting/syntux/parser.rs
@@ -167,7 +167,7 @@ impl<'a> Parser<'a> {
         sess: &'a ParseSess,
         path: &Path,
         span: Span,
-    ) -> Option<ast::Mod> {
+    ) -> Result<ast::Mod, ParserError> {
         let result = catch_unwind(AssertUnwindSafe(|| {
             let mut parser = new_parser_from_file(sess.inner(), &path, Some(span));
 
@@ -192,8 +192,10 @@ impl<'a> Parser<'a> {
             }
         }));
         match result {
-            Ok(Some(m)) => Some(m),
-            _ => None,
+            Ok(Some(m)) => Ok(m),
+            Ok(None) => Err(ParserError::ParseError),
+            Err(..) if path.exists() => Err(ParserError::ParseError),
+            Err(_) => Err(ParserError::ParsePanicError),
         }
     }
 
diff --git a/rustfmt-core/rustfmt-lib/src/result.rs b/rustfmt-core/rustfmt-lib/src/result.rs
@@ -3,6 +3,7 @@
 use rustc_span::Span;
 use thiserror::Error;
 
+use crate::formatting::modules::ModuleResolutionError;
 use crate::{formatting::ParseSess, FileName};
 
 /// Represents the specific error kind of [`FormatError`].
@@ -110,9 +111,9 @@ pub enum OperationError {
     /// satisfy that requirement.
     #[error("version mismatch")]
     VersionMismatch,
-    /// An io error during reading or writing.
-    #[error("io error: {0}")]
-    IoError(std::io::Error),
+    /// Error during module resolution.
+    #[error("{0}")]
+    ModuleResolutionError(#[from] ModuleResolutionError),
     /// Invalid glob pattern in `ignore` configuration option.
     #[error("invalid glob pattern found in ignore list: {0}")]
     InvalidGlobPattern(ignore::Error),

Original file line number	Diff line number	Diff line change
`@@ -167,7 +167,7 @@ impl<'a> Parser<'a> {`
`167`	`167`	`sess: &'a ParseSess,`
`168`	`168`	`path: &Path,`
`169`	`169`	`span: Span,`
`170`		`- ) -> Option<ast::Mod> {`
	`170`	`+ ) -> Result<ast::Mod, ParserError> {`
`171`	`171`	`let result = catch_unwind(AssertUnwindSafe(\|\| {`
`172`	`172`	`let mut parser = new_parser_from_file(sess.inner(), &path, Some(span));`
`173`	`173`
`@@ -192,8 +192,10 @@ impl<'a> Parser<'a> {`
`192`	`192`	`}`
`193`	`193`	`}));`
`194`	`194`	`match result {`
`195`		`- Ok(Some(m)) => Some(m),`
`196`		`- _ => None,`
	`195`	`+ Ok(Some(m)) => Ok(m),`
	`196`	`+ Ok(None) => Err(ParserError::ParseError),`
	`197`	`+ Err(..) if path.exists() => Err(ParserError::ParseError),`
	`198`	`+ Err(_) => Err(ParserError::ParsePanicError),`
`197`	`199`	`}`
`198`	`200`	`}`
`199`	`201`