Skip to content

Improve error handling in libsyntax #29285

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

Merged
merged 7 commits into from
Nov 3, 2015
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
6 changes: 3 additions & 3 deletions src/librustc/session/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -657,15 +657,15 @@ pub fn build_target_config(opts: &Options, sp: &SpanHandler) -> Config {
let target = match Target::search(&opts.target_triple) {
Ok(t) => t,
Err(e) => {
sp.handler().fatal(&format!("Error loading target specification: {}", e));
panic!(sp.handler().fatal(&format!("Error loading target specification: {}", e)));
}
};

let (int_type, uint_type) = match &target.target_pointer_width[..] {
"32" => (ast::TyI32, ast::TyU32),
"64" => (ast::TyI64, ast::TyU64),
w => sp.handler().fatal(&format!("target specification was invalid: unrecognized \
target-pointer-width {}", w))
w => panic!(sp.handler().fatal(&format!("target specification was invalid: \
unrecognized target-pointer-width {}", w))),
};

Config {
Expand Down
6 changes: 3 additions & 3 deletions src/librustc/session/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ impl Session {
if self.opts.treat_err_as_bug {
self.bug(msg);
}
self.diagnostic().handler().fatal(msg)
panic!(self.diagnostic().handler().fatal(msg))
}
pub fn span_err_or_warn(&self, is_warning: bool, sp: Span, msg: &str) {
if is_warning {
Expand Down Expand Up @@ -415,8 +415,8 @@ pub fn build_session_(sopts: config::Options,
let host = match Target::search(config::host_triple()) {
Ok(t) => t,
Err(e) => {
span_diagnostic.handler()
.fatal(&format!("Error loading host specification: {}", e));
panic!(span_diagnostic.handler()
.fatal(&format!("Error loading host specification: {}", e)));
}
};
let target_cfg = config::build_target_config(&sopts, &span_diagnostic);
Expand Down
6 changes: 4 additions & 2 deletions src/librustc_back/target/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,10 @@ impl Target {
.map(|s| s.as_string())
.and_then(|os| os.map(|s| s.to_string())) {
Some(val) => val,
None =>
handler.fatal(&format!("Field {} in target specification is required", name))
None => {
panic!(handler.fatal(&format!("Field {} in target specification is required",
name)))
}
}
};

Expand Down
6 changes: 2 additions & 4 deletions src/librustc_trans/back/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,12 @@ pub fn llvm_err(handler: &diagnostic::Handler, msg: String) -> ! {
unsafe {
let cstr = llvm::LLVMRustGetLastError();
if cstr == ptr::null() {
handler.fatal(&msg[..]);
panic!(handler.fatal(&msg[..]));
} else {
let err = CStr::from_ptr(cstr).to_bytes();
let err = String::from_utf8_lossy(err).to_string();
libc::free(cstr as *mut _);
handler.fatal(&format!("{}: {}",
&msg[..],
&err[..]));
panic!(handler.fatal(&format!("{}: {}", &msg[..], &err[..])));
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to panic in these places? We're already declaring a fatal exception, what happens if we just carry on? Don't we exit any way?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you're changing fatal errors to not panic. That seems maybe not a good idea? Could you use non-fatal errors in libsyntax instead? Or maybe it is a good idea since it makes the panicking more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, on master, we're in the slightly odd situation where fatal() panics, but span_fatal() doesn't panic; it probably doesn't make sense for it to stay that way.

In terms of how this should work long-term... libsyntax has basically settled on a "don't panic on fatal errors" approach, so it doesn't really make sense for libsyntax to provide a panicking fatal() method. On the other hand, libsyntax also provides the canonical implementation of fatal errors... so maybe fatal errors should panic, but libsyntax just shouldn't use the fatal() functions.

Copy link
Member

Choose a reason for hiding this comment

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

OK, this sounds reasonable, thanks for the explanantion

}
}
}
Expand Down
23 changes: 10 additions & 13 deletions src/libsyntax/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,13 +206,9 @@ impl Handler {
can_emit_warnings: can_emit_warnings
}
}
pub fn fatal(&self, msg: &str) -> ! {
pub fn fatal(&self, msg: &str) -> FatalError {
self.emit.borrow_mut().emit(None, msg, None, Fatal);

// Suppress the fatal error message from the panic below as we've
// already terminated in our own "legitimate" fashion.
io::set_panic(Box::new(io::sink()));
panic!(FatalError);
FatalError
}
pub fn err(&self, msg: &str) {
self.emit.borrow_mut().emit(None, msg, None, Error);
Expand All @@ -230,14 +226,15 @@ impl Handler {
pub fn abort_if_errors(&self) {
let s;
match self.err_count.get() {
0 => return,
1 => s = "aborting due to previous error".to_string(),
_ => {
s = format!("aborting due to {} previous errors",
self.err_count.get());
}
0 => return,
1 => s = "aborting due to previous error".to_string(),
_ => {
s = format!("aborting due to {} previous errors",
self.err_count.get());
}
}
self.fatal(&s[..]);

panic!(self.fatal(&s[..]));
Copy link
Member

Choose a reason for hiding this comment

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

could be &s instead of &s[..] probably

}
pub fn warn(&self, msg: &str) {
self.emit.borrow_mut().emit(None, msg, None, Warning);
Expand Down
6 changes: 3 additions & 3 deletions src/libsyntax/ext/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ pub fn expand_asm<'cx>(cx: &'cx mut ExtCtxt, sp: Span, tts: &[ast::TokenTree])
cx.span_err(sp, "malformed inline assembly");
return DummyResult::expr(sp);
}
let (s, style) = match expr_to_string(cx, p.parse_expr(),
let (s, style) = match expr_to_string(cx, panictry!(p.parse_expr_nopanic()),
"inline assembly must be a string literal") {
Some((s, st)) => (s, st),
// let compilation continue
Expand All @@ -102,7 +102,7 @@ pub fn expand_asm<'cx>(cx: &'cx mut ExtCtxt, sp: Span, tts: &[ast::TokenTree])
let span = p.last_span;

panictry!(p.expect(&token::OpenDelim(token::Paren)));
let out = p.parse_expr();
let out = panictry!(p.parse_expr_nopanic());
panictry!(p.expect(&token::CloseDelim(token::Paren)));

// Expands a read+write operand into two operands.
Expand Down Expand Up @@ -146,7 +146,7 @@ pub fn expand_asm<'cx>(cx: &'cx mut ExtCtxt, sp: Span, tts: &[ast::TokenTree])
}

panictry!(p.expect(&token::OpenDelim(token::Paren)));
let input = p.parse_expr();
let input = panictry!(p.parse_expr_nopanic());
panictry!(p.expect(&token::CloseDelim(token::Paren)));

inputs.push((constraint, input));
Expand Down
4 changes: 2 additions & 2 deletions src/libsyntax/ext/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -809,7 +809,7 @@ pub fn get_single_str_from_tts(cx: &mut ExtCtxt,
cx.span_err(sp, &format!("{} takes 1 argument", name));
return None
}
let ret = cx.expander().fold_expr(p.parse_expr());
let ret = cx.expander().fold_expr(panictry!(p.parse_expr_nopanic()));
if p.token != token::Eof {
cx.span_err(sp, &format!("{} takes 1 argument", name));
}
Expand All @@ -826,7 +826,7 @@ pub fn get_exprs_from_tts(cx: &mut ExtCtxt,
let mut p = cx.new_parser_from_tts(tts);
let mut es = Vec::new();
while p.token != token::Eof {
es.push(cx.expander().fold_expr(p.parse_expr()));
es.push(cx.expander().fold_expr(panictry!(p.parse_expr_nopanic())));
if panictry!(p.eat(&token::Comma)){
continue;
}
Expand Down
3 changes: 1 addition & 2 deletions src/libsyntax/ext/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,14 @@ use ext::base;
use ext::build::AstBuilder;
use attr;
use attr::*;
use parse::attr::ParserAttr;
use parse::token;

pub fn expand_cfg<'cx>(cx: &mut ExtCtxt,
sp: Span,
tts: &[ast::TokenTree])
-> Box<base::MacResult+'static> {
let mut p = cx.new_parser_from_tts(tts);
let cfg = p.parse_meta_item();
let cfg = panictry!(p.parse_meta_item());

if !panictry!(p.eat(&token::Eof)){
cx.span_err(sp, "expected 1 cfg-pattern");
Expand Down
6 changes: 3 additions & 3 deletions src/libsyntax/ext/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ fn parse_args(ecx: &mut ExtCtxt, sp: Span, tts: &[ast::TokenTree])
ecx.span_err(sp, "requires at least a format string argument");
return None;
}
let fmtstr = p.parse_expr();
let fmtstr = panictry!(p.parse_expr_nopanic());
let mut named = false;
while p.token != token::Eof {
if !panictry!(p.eat(&token::Comma)) {
Expand Down Expand Up @@ -124,7 +124,7 @@ fn parse_args(ecx: &mut ExtCtxt, sp: Span, tts: &[ast::TokenTree])
let name: &str = &ident.name.as_str();

panictry!(p.expect(&token::Eq));
let e = p.parse_expr();
let e = panictry!(p.parse_expr_nopanic());
match names.get(name) {
None => {}
Some(prev) => {
Expand All @@ -138,7 +138,7 @@ fn parse_args(ecx: &mut ExtCtxt, sp: Span, tts: &[ast::TokenTree])
order.push(name.to_string());
names.insert(name.to_string(), e);
} else {
args.push(p.parse_expr());
args.push(panictry!(p.parse_expr_nopanic()));
}
}
Some((fmtstr, args, order, names))
Expand Down
16 changes: 8 additions & 8 deletions src/libsyntax/ext/quote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -327,55 +327,55 @@ pub fn expand_quote_expr<'cx>(cx: &'cx mut ExtCtxt,
sp: Span,
tts: &[ast::TokenTree])
-> Box<base::MacResult+'cx> {
let expanded = expand_parse_call(cx, sp, "parse_expr", vec!(), tts);
let expanded = expand_parse_call(cx, sp, "parse_expr_panic", vec!(), tts);
base::MacEager::expr(expanded)
}

pub fn expand_quote_item<'cx>(cx: &mut ExtCtxt,
sp: Span,
tts: &[ast::TokenTree])
-> Box<base::MacResult+'cx> {
let expanded = expand_parse_call(cx, sp, "parse_item", vec!(), tts);
let expanded = expand_parse_call(cx, sp, "parse_item_panic", vec!(), tts);
base::MacEager::expr(expanded)
}

pub fn expand_quote_pat<'cx>(cx: &'cx mut ExtCtxt,
sp: Span,
tts: &[ast::TokenTree])
-> Box<base::MacResult+'cx> {
let expanded = expand_parse_call(cx, sp, "parse_pat", vec!(), tts);
let expanded = expand_parse_call(cx, sp, "parse_pat_panic", vec!(), tts);
base::MacEager::expr(expanded)
}

pub fn expand_quote_arm(cx: &mut ExtCtxt,
sp: Span,
tts: &[ast::TokenTree])
-> Box<base::MacResult+'static> {
let expanded = expand_parse_call(cx, sp, "parse_arm", vec!(), tts);
let expanded = expand_parse_call(cx, sp, "parse_arm_panic", vec!(), tts);
base::MacEager::expr(expanded)
}

pub fn expand_quote_ty(cx: &mut ExtCtxt,
sp: Span,
tts: &[ast::TokenTree])
-> Box<base::MacResult+'static> {
let expanded = expand_parse_call(cx, sp, "parse_ty", vec!(), tts);
let expanded = expand_parse_call(cx, sp, "parse_ty_panic", vec!(), tts);
base::MacEager::expr(expanded)
}

pub fn expand_quote_stmt(cx: &mut ExtCtxt,
sp: Span,
tts: &[ast::TokenTree])
-> Box<base::MacResult+'static> {
let expanded = expand_parse_call(cx, sp, "parse_stmt", vec!(), tts);
let expanded = expand_parse_call(cx, sp, "parse_stmt_panic", vec!(), tts);
base::MacEager::expr(expanded)
}

pub fn expand_quote_attr(cx: &mut ExtCtxt,
sp: Span,
tts: &[ast::TokenTree])
-> Box<base::MacResult+'static> {
let expanded = expand_parse_call(cx, sp, "parse_attribute",
let expanded = expand_parse_call(cx, sp, "parse_attribute_panic",
vec!(cx.expr_bool(sp, true)), tts);

base::MacEager::expr(expanded)
Expand Down Expand Up @@ -694,7 +694,7 @@ fn parse_arguments_to_quote(cx: &ExtCtxt, tts: &[ast::TokenTree])
let mut p = cx.new_parser_from_tts(tts);
p.quote_depth += 1;

let cx_expr = p.parse_expr();
let cx_expr = panictry!(p.parse_expr_nopanic());
if !panictry!(p.eat(&token::Comma)) {
panic!(p.fatal("expected token `,`"));
}
Expand Down
4 changes: 2 additions & 2 deletions src/libsyntax/ext/source_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,13 @@ pub fn expand_include<'cx>(cx: &'cx mut ExtCtxt, sp: Span, tts: &[ast::TokenTree
}
impl<'a> base::MacResult for ExpandResult<'a> {
fn make_expr(mut self: Box<ExpandResult<'a>>) -> Option<P<ast::Expr>> {
Some(self.p.parse_expr())
Some(panictry!(self.p.parse_expr_nopanic()))
}
fn make_items(mut self: Box<ExpandResult<'a>>)
-> Option<SmallVector<P<ast::Item>>> {
let mut ret = SmallVector::zero();
while self.p.token != token::Eof {
match self.p.parse_item() {
match panictry!(self.p.parse_item_nopanic()) {
Some(item) => ret.push(item),
None => panic!(self.p.span_fatal(
self.p.span,
Expand Down
13 changes: 6 additions & 7 deletions src/libsyntax/ext/tt/macro_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ use codemap::{BytePos, mk_sp, Span};
use codemap;
use parse::lexer::*; //resolve bug?
use parse::ParseSess;
use parse::attr::ParserAttr;
use parse::parser::{LifetimeAndTypesWithoutColons, Parser};
use parse::token::{Eof, DocComment, MatchNt, SubstNt};
use parse::token::{Token, Nonterminal};
Expand Down Expand Up @@ -503,18 +502,18 @@ pub fn parse_nt(p: &mut Parser, sp: Span, name: &str) -> Nonterminal {
// check at the beginning and the parser checks after each bump
panictry!(p.check_unknown_macro_variable());
match name {
"item" => match p.parse_item() {
"item" => match panictry!(p.parse_item_nopanic()) {
Some(i) => token::NtItem(i),
None => panic!(p.fatal("expected an item keyword"))
},
"block" => token::NtBlock(panictry!(p.parse_block())),
"stmt" => match p.parse_stmt() {
"stmt" => match panictry!(p.parse_stmt_nopanic()) {
Some(s) => token::NtStmt(s),
None => panic!(p.fatal("expected a statement"))
},
"pat" => token::NtPat(p.parse_pat()),
"expr" => token::NtExpr(p.parse_expr()),
"ty" => token::NtTy(p.parse_ty()),
"pat" => token::NtPat(panictry!(p.parse_pat_nopanic())),
"expr" => token::NtExpr(panictry!(p.parse_expr_nopanic())),
"ty" => token::NtTy(panictry!(p.parse_ty_nopanic())),
// this could be handled like a token, since it is one
"ident" => match p.token {
token::Ident(sn,b) => { panictry!(p.bump()); token::NtIdent(Box::new(sn),b) }
Expand All @@ -527,7 +526,7 @@ pub fn parse_nt(p: &mut Parser, sp: Span, name: &str) -> Nonterminal {
"path" => {
token::NtPath(Box::new(panictry!(p.parse_path(LifetimeAndTypesWithoutColons))))
},
"meta" => token::NtMeta(p.parse_meta_item()),
"meta" => token::NtMeta(panictry!(p.parse_meta_item())),
_ => {
panic!(p.span_fatal_help(sp,
&format!("invalid fragment specifier `{}`", name),
Expand Down
8 changes: 4 additions & 4 deletions src/libsyntax/ext/tt/macro_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,18 +66,18 @@ impl<'a> ParserAnyMacro<'a> {

impl<'a> MacResult for ParserAnyMacro<'a> {
fn make_expr(self: Box<ParserAnyMacro<'a>>) -> Option<P<ast::Expr>> {
let ret = self.parser.borrow_mut().parse_expr();
let ret = panictry!(self.parser.borrow_mut().parse_expr_nopanic());
self.ensure_complete_parse(true);
Some(ret)
}
fn make_pat(self: Box<ParserAnyMacro<'a>>) -> Option<P<ast::Pat>> {
let ret = self.parser.borrow_mut().parse_pat();
let ret = panictry!(self.parser.borrow_mut().parse_pat_nopanic());
self.ensure_complete_parse(false);
Some(ret)
}
fn make_items(self: Box<ParserAnyMacro<'a>>) -> Option<SmallVector<P<ast::Item>>> {
let mut ret = SmallVector::zero();
while let Some(item) = self.parser.borrow_mut().parse_item() {
while let Some(item) = panictry!(self.parser.borrow_mut().parse_item_nopanic()) {
ret.push(item);
}
self.ensure_complete_parse(false);
Expand Down Expand Up @@ -119,7 +119,7 @@ impl<'a> MacResult for ParserAnyMacro<'a> {
}

fn make_ty(self: Box<ParserAnyMacro<'a>>) -> Option<P<ast::Ty>> {
let ret = self.parser.borrow_mut().parse_ty();
let ret = panictry!(self.parser.borrow_mut().parse_ty_nopanic());
self.ensure_complete_parse(true);
Some(ret)
}
Expand Down
1 change: 0 additions & 1 deletion src/libsyntax/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
#![feature(filling_drop)]
#![feature(libc)]
#![feature(rustc_private)]
#![feature(set_stdio)]
#![feature(staged_api)]
#![feature(str_char)]
#![feature(str_escape)]
Expand Down
Loading