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

Catch possible tokenizer panics #3240

Merged
merged 3 commits into from
Dec 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions src/formatting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::time::{Duration, Instant};

use syntax::ast;
use syntax::errors::emitter::{ColorConfig, EmitterWriter};
use syntax::errors::Handler;
use syntax::errors::{DiagnosticBuilder, Handler};
use syntax::parse::{self, ParseSess};
use syntax::source_map::{FilePathMapping, SourceMap, Span};

Expand Down Expand Up @@ -73,7 +73,12 @@ 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 = parse_crate(input, &parse_session, config, &mut report)?;
let krate = match parse_crate(input, &parse_session, config, &mut report) {
Ok(krate) => krate,
// Surface parse error via Session (errors are merged there from report)
Err(ErrorKind::ParseError) => return Ok(report),
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this to return report here for parse errors, otherwise we eagerly ?-return an Err here:

let krate = parse_crate(input, &parse_session, config, &mut report)?;

but we only add errors to Session if we have Ok(FormatReport)
format_result.map(|report| {
{
let new_errors = &report.internal.borrow().1;
self.errors.add(new_errors);
}
report
})
})

I changed this because I expected session.has_parsing_errors() call to work:
https://github.com/rust-lang/rustfmt/pull/3240/files#diff-382d140f826c9bd99f62116b9712fad3R299
and it seemed like a special case that it didn't.

However, for IO errors we still will get Err(_) from Session::format(...) and session.has_operational_errors() will return false (should we also change that?).

Copy link
Contributor

@topecongiro topecongiro Dec 18, 2018

Choose a reason for hiding this comment

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

However, for IO errors we still will get Err(_) from Session::format(...) and session.has_operational_errors() will return false (should we also change that?).

I think so, but that can be done in a separate PR. I would rather fix this via refactoring the whole error handling around Session, as currently it is error prone.

Err(e) => return Err(e),
};
timer = timer.done_parsing();

// Suppress error output if we have to do any further parsing.
Expand Down Expand Up @@ -604,30 +609,41 @@ fn parse_crate(
) -> Result<ast::Crate, ErrorKind> {
let input_is_stdin = input.is_text();

let mut parser = match input {
Input::File(file) => parse::new_parser_from_file(parse_session, &file),
Input::Text(text) => parse::new_parser_from_source_str(
let parser = match input {
Input::File(file) => Ok(parse::new_parser_from_file(parse_session, &file)),
Xanewok marked this conversation as resolved.
Show resolved Hide resolved
Input::Text(text) => parse::maybe_new_parser_from_source_str(
parse_session,
syntax::source_map::FileName::Custom("stdin".to_owned()),
text,
),
)
.map_err(|diags| {
diags
.into_iter()
.map(|d| DiagnosticBuilder::new_diagnostic(&parse_session.span_diagnostic, d))
.collect::<Vec<_>>()
}),
};

parser.cfg_mods = false;
if config.skip_children() {
parser.recurse_into_file_modules = false;
}
let result = match parser {
Ok(mut parser) => {
parser.cfg_mods = false;
if config.skip_children() {
parser.recurse_into_file_modules = false;
}

let mut parser = AssertUnwindSafe(parser);
let result = catch_unwind(move || parser.0.parse_crate_mod());
let mut parser = AssertUnwindSafe(parser);
catch_unwind(move || parser.0.parse_crate_mod().map_err(|d| vec![d]))
}
Err(db) => Ok(Err(db)),
};

match result {
Ok(Ok(c)) => {
if !parse_session.span_diagnostic.has_errors() {
return Ok(c);
}
}
Ok(Err(mut e)) => e.emit(),
Ok(Err(mut diagnostics)) => diagnostics.iter_mut().for_each(|d| d.emit()),
Err(_) => {
// Note that if you see this message and want more information,
// then run the `parse_crate_mod` function above without
Expand Down
12 changes: 12 additions & 0 deletions src/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,18 @@ fn stdin_formatting_smoke_test() {
assert_eq!(buf, "fn main() {}\r\n".as_bytes());
}

#[test]
fn stdin_parser_panic_caught() {
// https://github.com/rust-lang/rustfmt/issues/3239
for text in ["{", "}"].iter().cloned().map(String::from) {
let mut buf = vec![];
let mut session = Session::new(Default::default(), Some(&mut buf));
let _ = session.format(Input::Text(text));

assert!(session.has_parsing_errors());
}
}

#[test]
fn stdin_disable_all_formatting_test() {
match option_env!("CFG_RELEASE_CHANNEL") {
Expand Down