From db96eb48a19aa4b58791cf5dc3f473bbd7cf4a82 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Sun, 24 May 2020 23:43:04 +0900 Subject: [PATCH 1/2] Improve error message when module resolution failed --- rustfmt-core/rustfmt-bin/src/bin/main.rs | 8 +- rustfmt-core/rustfmt-lib/src/formatting.rs | 4 +- .../rustfmt-lib/src/formatting/modules.rs | 100 +++++++++++++----- .../src/formatting/syntux/parser.rs | 8 +- rustfmt-core/rustfmt-lib/src/result.rs | 7 +- 5 files changed, 82 insertions(+), 45 deletions(-) diff --git a/rustfmt-core/rustfmt-bin/src/bin/main.rs b/rustfmt-core/rustfmt-bin/src/bin/main.rs index 9839dfa7ad3..80df2c325dc 100644 --- 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 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 index 19a14dbcb7b..22842c8aef9 100644 --- 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 index eee19502906..718929e9cc4 100644 --- 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,31 @@ 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, + }, + /// IO error. + #[error("{0}")] + IOError(#[from] std::io::Error), +} + #[derive(Clone)] enum SubModKind<'a, 'ast> { /// `mod foo;` @@ -63,7 +89,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { pub(crate) fn visit_crate( mut self, krate: &'ast ast::Crate, - ) -> Result, String> { + ) -> Result, 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 +107,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 +119,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 +134,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 +151,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 +167,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { &self, item: &'c ast::Item, sub_mod: &Cow<'ast, ast::Mod>, - ) -> Result>, String> { + ) -> Result>, ModuleResolutionError> { if contains_skip(&item.attrs) { return Ok(None); } @@ -160,7 +186,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 +209,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 +239,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { &mut self, sub_mod: Cow<'ast, ast::Mod>, directory: Option, - ) -> Result<(), String> { + ) -> Result<(), ModuleResolutionError> { if let Some(directory) = directory { self.directory = directory; } @@ -229,7 +255,7 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { mod_name: symbol::Ident, attrs: &[ast::Attribute], sub_mod: &Cow<'ast, ast::Mod>, - ) -> Result>, String> { + ) -> Result>, ModuleResolutionError> { let relative = match self.directory.ownership { DirectoryOwnership::Owned { relative } => relative, DirectoryOwnership::UnownedViaBlock | DirectoryOwnership::UnownedViaMod => None, @@ -239,15 +265,23 @@ 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 +311,29 @@ 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 +347,10 @@ 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 +409,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 index 8b82e97f8fa..69bac50b5bd 100644 --- 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 { + ) -> Result { 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 index f212811fc88..712305d8073 100644 --- a/rustfmt-core/rustfmt-lib/src/result.rs +++ b/rustfmt-core/rustfmt-lib/src/result.rs @@ -4,6 +4,7 @@ use rustc_span::Span; use thiserror::Error; use crate::{formatting::ParseSess, FileName}; +use crate::formatting::modules::ModuleResolutionError; /// Represents the specific error kind of [`FormatError`]. #[derive(Error, Clone, Copy, Debug, Hash, Eq, PartialEq)] @@ -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), From 6142a9b78da9e08eae032e03c5002a2dce8a37e6 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Mon, 25 May 2020 07:37:06 +0900 Subject: [PATCH 2/2] Cargo fmt --- .../rustfmt-lib/src/formatting/modules.rs | 35 ++++++------------- rustfmt-core/rustfmt-lib/src/result.rs | 2 +- 2 files changed, 12 insertions(+), 25 deletions(-) diff --git a/rustfmt-core/rustfmt-lib/src/formatting/modules.rs b/rustfmt-core/rustfmt-lib/src/formatting/modules.rs index 718929e9cc4..f5c50529185 100644 --- a/rustfmt-core/rustfmt-lib/src/formatting/modules.rs +++ b/rustfmt-core/rustfmt-lib/src/formatting/modules.rs @@ -44,17 +44,10 @@ pub struct ModuleResolutionError { pub(crate) enum ModuleResolutionErrorKind { /// Find a file that cannot be parsed. #[error("cannot parse {file}")] - ParseError { - file: PathBuf, - }, + ParseError { file: PathBuf }, /// File cannot be found. #[error("{file} does not exist")] - NotFound { - file: PathBuf, - }, - /// IO error. - #[error("{0}")] - IOError(#[from] std::io::Error), + NotFound { file: PathBuf }, } #[derive(Clone)] @@ -272,15 +265,11 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { ))), Err(ParserError::ParseError) => Err(ModuleResolutionError { module: mod_name.to_string(), - kind: ModuleResolutionErrorKind::ParseError { - file: path, - }, + kind: ModuleResolutionErrorKind::ParseError { file: path }, }), Err(..) => Err(ModuleResolutionError { module: mod_name.to_string(), - kind: ModuleResolutionErrorKind::NotFound { - file: path, - } + kind: ModuleResolutionErrorKind::NotFound { file: path }, }), }; } @@ -323,15 +312,11 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { } Err(ParserError::ParseError) => Err(ModuleResolutionError { module: mod_name.to_string(), - kind: ModuleResolutionErrorKind::ParseError{ - file: path, - } - } ), + kind: ModuleResolutionErrorKind::ParseError { file: path }, + }), Err(..) if outside_mods_empty => Err(ModuleResolutionError { module: mod_name.to_string(), - kind: ModuleResolutionErrorKind::NotFound { - file: path, - }, + kind: ModuleResolutionErrorKind::NotFound { file: path }, }), Err(..) => { if should_insert { @@ -348,8 +333,10 @@ impl<'ast, 'sess, 'c> ModResolver<'ast, 'sess> { Err(mut e) => { e.cancel(); Err(ModuleResolutionError { - module: mod_name.to_string(), - kind: ModuleResolutionErrorKind::NotFound {file: self.directory.path.clone()}, + module: mod_name.to_string(), + kind: ModuleResolutionErrorKind::NotFound { + file: self.directory.path.clone(), + }, }) } } diff --git a/rustfmt-core/rustfmt-lib/src/result.rs b/rustfmt-core/rustfmt-lib/src/result.rs index 712305d8073..9d51c379684 100644 --- a/rustfmt-core/rustfmt-lib/src/result.rs +++ b/rustfmt-core/rustfmt-lib/src/result.rs @@ -3,8 +3,8 @@ use rustc_span::Span; use thiserror::Error; -use crate::{formatting::ParseSess, FileName}; use crate::formatting::modules::ModuleResolutionError; +use crate::{formatting::ParseSess, FileName}; /// Represents the specific error kind of [`FormatError`]. #[derive(Error, Clone, Copy, Debug, Hash, Eq, PartialEq)]