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

Conversation

Xanewok
Copy link
Member

@Xanewok Xanewok commented Dec 8, 2018

When using new_parser_from_source_str sometimes it can panic on incomplete input (e.g. bare }), so here we use the Result-returning maybe_new_parser_from_source_str variant to catch possible errors and diagnostics to emit later.

Closes #3239

related #753
cc @topecongiro

@Xanewok Xanewok changed the title Catch possible tokenizer panics WIP: Catch possible tokenizer panics Dec 9, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Dec 12, 2018
…r=petrochenkov

Add non-panicking `maybe_new_parser_from_file` variant

Add (seemingly?) missing `maybe_new_parser_from_file` constructor variant.

Disclaimer: I'm not certain this is the correct approach - just found out we don't have this when working on a Rustfmt PR to catch/prevent more Rust parser panics: rust-lang/rustfmt#3240 and tried to make it work somehow.
@Xanewok Xanewok changed the title WIP: Catch possible tokenizer panics Catch possible tokenizer panics Dec 13, 2018
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.

@Xanewok
Copy link
Member Author

Xanewok commented Dec 13, 2018

This should be ready to review now

kennytm added a commit to kennytm/rust that referenced this pull request Dec 13, 2018
…r=petrochenkov

Add non-panicking `maybe_new_parser_from_file` variant

Add (seemingly?) missing `maybe_new_parser_from_file` constructor variant.

Disclaimer: I'm not certain this is the correct approach - just found out we don't have this when working on a Rustfmt PR to catch/prevent more Rust parser panics: rust-lang/rustfmt#3240 and tried to make it work somehow.
kennytm added a commit to kennytm/rust that referenced this pull request Dec 14, 2018
…r=petrochenkov

Add non-panicking `maybe_new_parser_from_file` variant

Add (seemingly?) missing `maybe_new_parser_from_file` constructor variant.

Disclaimer: I'm not certain this is the correct approach - just found out we don't have this when working on a Rustfmt PR to catch/prevent more Rust parser panics: rust-lang/rustfmt#3240 and tried to make it work somehow.
kennytm added a commit to kennytm/rust that referenced this pull request Dec 14, 2018
…r=petrochenkov

Add non-panicking `maybe_new_parser_from_file` variant

Add (seemingly?) missing `maybe_new_parser_from_file` constructor variant.

Disclaimer: I'm not certain this is the correct approach - just found out we don't have this when working on a Rustfmt PR to catch/prevent more Rust parser panics: rust-lang/rustfmt#3240 and tried to make it work somehow.
@topecongiro
Copy link
Contributor

Thank you!

@topecongiro topecongiro merged commit 3f6ea77 into rust-lang:master Dec 18, 2018
@Xanewok Xanewok deleted the parser-panic branch December 18, 2018 21:49
@wada314
Copy link
Contributor

wada314 commented Dec 24, 2018

Hello,

I'm running "$ cargo test stdin_parser_panic_caught" in nightly Rust, Windows 10.
The test itself successfully finishes, but the panic!() outputs are not suppressed and displayed in my console.
Is this an expected behavior? Or is this something happening only in my environment?

thanks,

running 1 test
error: this file contains an un-closed delimiter
 --> <stdin>:1:2
  |
1 | {
  | -^
  | |
  | un-closed delimiter

error: expected item, found `{`
 --> <stdin>:1:1
  |
1 | {
  | ^ expected item

error: unexpected close delimiter: `}`
 --> <stdin>:1:1
  |
1 | }
  | ^ unexpected close delimiter

test test::stdin_parser_panic_caught ... ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants