Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't fail on recoverable parser errors in ignored files #3782

Merged
merged 9 commits into from
Oct 7, 2019
83 changes: 78 additions & 5 deletions src/formatting.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
// High level formatting functions.

use std::cell::RefCell;
use std::collections::HashMap;
use std::io::{self, Write};
use std::panic::{catch_unwind, AssertUnwindSafe};
use std::rc::Rc;
use std::time::{Duration, Instant};

use syntax::ast;
use syntax::errors::emitter::{ColorConfig, Emitter};
use syntax::errors::emitter::{ColorConfig, Emitter, EmitterWriter};
use syntax::errors::{Diagnostic, DiagnosticBuilder, Handler};
use syntax::parse::{self, ParseSess};
use syntax::source_map::{FilePathMapping, SourceMap, Span, DUMMY_SP};
Expand Down Expand Up @@ -67,16 +68,22 @@ fn format_project<T: FormatHandler>(
let input_is_stdin = main_file == FileName::Stdin;

let ignore_path_set = match IgnorePathSet::from_ignore_list(&config.ignore()) {
Ok(set) => set,
Ok(set) => Rc::new(set),
Err(e) => return Err(ErrorKind::InvalidGlobPattern(e)),
};
if config.skip_children() && ignore_path_set.is_match(&main_file) {
return Ok(FormatReport::new());
}

// Parse the crate.
let can_reset_parser_errors = Rc::new(RefCell::new(false));
let source_map = Rc::new(SourceMap::new(FilePathMapping::empty()));
let mut parse_session = make_parse_sess(source_map.clone(), config);
let mut parse_session = make_parse_sess(
source_map.clone(),
config,
Rc::clone(&ignore_path_set),
can_reset_parser_errors.clone(),
);
let mut report = FormatReport::new();
let directory_ownership = input.to_directory_ownership();
let krate = match parse_crate(
Expand All @@ -85,6 +92,7 @@ fn format_project<T: FormatHandler>(
config,
&mut report,
directory_ownership,
can_reset_parser_errors.clone(),
) {
Ok(krate) => krate,
// Surface parse error via Session (errors are merged there from report)
Expand Down Expand Up @@ -620,6 +628,7 @@ fn parse_crate(
config: &Config,
report: &mut FormatReport,
directory_ownership: Option<parse::DirectoryOwnership>,
can_reset_parser_errors: Rc<RefCell<bool>>,
) -> Result<ast::Crate, ErrorKind> {
let input_is_stdin = input.is_text();

Expand Down Expand Up @@ -667,6 +676,15 @@ fn parse_crate(
if !parse_session.span_diagnostic.has_errors() {
return Ok(c);
}
// This scenario occurs when the parser encountered errors
// but was still able to recover. If all of the parser errors
// occurred in files that are ignored, then reset
// the error count and continue.
// https://github.com/rust-lang/rustfmt/issues/3779
if *can_reset_parser_errors.borrow() {
parse_session.span_diagnostic.reset_err_count();
return Ok(c);
}
}
Ok(Err(mut diagnostics)) => diagnostics.iter_mut().for_each(DiagnosticBuilder::emit),
Err(_) => {
Expand All @@ -683,6 +701,40 @@ fn parse_crate(
Err(ErrorKind::ParseError)
}

struct SilentOnIgnoredFilesEmitter {
ignore_path_set: Rc<IgnorePathSet>,
source_map: Rc<SourceMap>,
emitter: EmitterWriter,
has_non_ignorable_parser_errors: bool,
can_reset: Rc<RefCell<bool>>,
}

impl Emitter for SilentOnIgnoredFilesEmitter {
fn emit_diagnostic(&mut self, db: &Diagnostic) {
if let Some(primary_span) = &db.span.primary_span() {
let file_name = self.source_map.span_to_filename(*primary_span);
match file_name {
syntax_pos::FileName::Real(ref path) => {
if self
.ignore_path_set
.is_match(&FileName::Real(path.to_path_buf()))
{
if !self.has_non_ignorable_parser_errors {
*self.can_reset.borrow_mut() = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I first thought that we could simply call db.handler.reset_err_count(); here instead, but looks like the rustc parser bumps the error count after calling emit_diagnostic:

https://github.com/rust-lang/rust/blob/f2023ac599c38a59f86552089e6791c5a73412d3/src/librustc_errors/lib.rs#L781-L791

Also like you mentioned, resetting the error count may cause skipping important parsing errors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I first thought that we could simply call db.handler.reset_err_count();

That's what I was expecting originally too 👍

here instead, but looks like the rustc parser bumps the error count after calling emit_diagnostic:

Thank you so much for finding that! I was baffled how calling reset_err_count() inside emit_diagnostic seemed to be doing nothing, but that makes much more sense now

}
return;
}
}
_ => (),
};
}

self.has_non_ignorable_parser_errors = true;
*self.can_reset.borrow_mut() = false;
self.emitter.emit_diagnostic(db);
}
}

/// Emitter which discards every error.
struct SilentEmitter;

Expand All @@ -694,7 +746,12 @@ fn silent_emitter() -> Box<SilentEmitter> {
Box::new(SilentEmitter {})
}

fn make_parse_sess(source_map: Rc<SourceMap>, config: &Config) -> ParseSess {
fn make_parse_sess(
source_map: Rc<SourceMap>,
config: &Config,
ignore_path_set: Rc<IgnorePathSet>,
can_reset: Rc<RefCell<bool>>,
) -> ParseSess {
let tty_handler = if config.hide_parse_errors() {
let silent_emitter = silent_emitter();
Handler::with_emitter(true, None, silent_emitter)
Expand All @@ -705,7 +762,23 @@ fn make_parse_sess(source_map: Rc<SourceMap>, config: &Config) -> ParseSess {
} else {
ColorConfig::Never
};
Handler::with_tty_emitter(color_cfg, true, None, Some(source_map.clone()))

let emitter_writer = EmitterWriter::stderr(
color_cfg,
Some(source_map.clone()),
false,
false,
None,
false,
);
let emitter = Box::new(SilentOnIgnoredFilesEmitter {
has_non_ignorable_parser_errors: false,
ignore_path_set: ignore_path_set,
source_map: Rc::clone(&source_map),
emitter: emitter_writer,
can_reset,
});
Handler::with_emitter(true, None, emitter)
};

ParseSess::with_span_handler(tty_handler, source_map)
Expand Down
2 changes: 2 additions & 0 deletions src/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ const SKIP_FILE_WHITE_LIST: &[&str] = &[
"configs/skip_children/foo/mod.rs",
"issue-3434/no_entry.rs",
"issue-3665/sub_mod.rs",
// Testing for issue-3779
"issue-3779/ice.rs",
// These files and directory are a part of modules defined inside `cfg_if!`.
"cfg_if/mod.rs",
"cfg_if/detect",
Expand Down
4 changes: 4 additions & 0 deletions tests/config/issue-3779.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
unstable_features = true
ignore = [
"tests/**/issue-3779/ice.rs"
]
3 changes: 3 additions & 0 deletions tests/source/issue-3779/ice.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pub fn bar() {
1x;
}
8 changes: 8 additions & 0 deletions tests/source/issue-3779/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// rustfmt-config: issue-3779.toml

#[path = "ice.rs"]
mod ice;

fn foo() {
println!("abc") ;
}
3 changes: 3 additions & 0 deletions tests/target/issue-3779/ice.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pub fn bar() {
1x;
}
8 changes: 8 additions & 0 deletions tests/target/issue-3779/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// rustfmt-config: issue-3779.toml

#[path = "ice.rs"]
mod ice;

fn foo() {
println!("abc");
}