Skip to content

make panictry! private to libsyntax #56897

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 1 commit into from
Jan 4, 2019
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
10 changes: 8 additions & 2 deletions src/librustdoc/core.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use syntax::json::JsonEmitter;
use syntax::ptr::P;
use syntax::symbol::keywords;
use syntax_pos::DUMMY_SP;
use errors;
use errors::{self, FatalError};
use errors::emitter::{Emitter, EmitterWriter};
use parking_lot::ReentrantMutex;

Expand Down Expand Up @@ -429,7 +429,13 @@ pub fn run_core(options: RustdocOptions) -> (clean::Crate, RenderInfo, RenderOpt

let control = &driver::CompileController::basic();

let krate = panictry!(driver::phase_1_parse_input(control, &sess, &input));
let krate = match driver::phase_1_parse_input(control, &sess, &input) {
Ok(krate) => krate,
Err(mut e) => {
e.emit();
FatalError.raise();
}
};

let name = match crate_name {
Some(ref crate_name) => crate_name.clone(),
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ extern crate rustc_metadata;
extern crate rustc_target;
extern crate rustc_typeck;
extern crate serialize;
#[macro_use] extern crate syntax;
extern crate syntax;
extern crate syntax_pos;
extern crate test as testing;
#[macro_use] extern crate log;
Expand Down
13 changes: 9 additions & 4 deletions src/librustdoc/test.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use errors;
use errors::{self, FatalError};
use errors::emitter::ColorConfig;
use rustc_data_structures::sync::Lrc;
use rustc_lint;
Expand Down Expand Up @@ -84,9 +84,14 @@ pub fn run(mut options: Options) -> isize {
target_features::add_configuration(&mut cfg, &sess, &*codegen_backend);
sess.parse_sess.config = cfg;

let krate = panictry!(driver::phase_1_parse_input(&driver::CompileController::basic(),
&sess,
&input));
let krate =
match driver::phase_1_parse_input(&driver::CompileController::basic(), &sess, &input) {
Ok(krate) => krate,
Err(mut e) => {
e.emit();
FatalError.raise();
}
};
let driver::ExpansionResult { defs, mut hir_forest, .. } = {
phase_2_configure_and_expand(
&sess,
Expand Down
2 changes: 0 additions & 2 deletions src/libsyntax/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,6 @@ use ast::AttrId;
// way towards a non-panic!-prone parser. It should be used for fatal parsing
// errors; eventually we plan to convert all code using panictry to just use
// normal try.
// Exported for syntax_ext, not meant for general use.
#[macro_export]
macro_rules! panictry {
($e:expr) => ({
use std::result::Result::{Ok, Err};
Expand Down
116 changes: 71 additions & 45 deletions src/libsyntax_ext/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use self::State::*;

use rustc_data_structures::thin_vec::ThinVec;

use errors::DiagnosticBuilder;
use syntax::ast;
use syntax::ext::base;
use syntax::ext::base::*;
Expand Down Expand Up @@ -51,6 +52,34 @@ pub fn expand_asm<'cx>(cx: &'cx mut ExtCtxt,
feature_gate::EXPLAIN_ASM);
}

let mut inline_asm = match parse_inline_asm(cx, sp, tts) {
Ok(Some(inline_asm)) => inline_asm,
Ok(None) => return DummyResult::expr(sp),
Err(mut err) => {
err.emit();
return DummyResult::expr(sp);
}
};

// If there are no outputs, the inline assembly is executed just for its side effects,
// so ensure that it is volatile
if inline_asm.outputs.is_empty() {
inline_asm.volatile = true;
}

MacEager::expr(P(ast::Expr {
id: ast::DUMMY_NODE_ID,
node: ast::ExprKind::InlineAsm(P(inline_asm)),
span: sp,
attrs: ThinVec::new(),
}))
}

fn parse_inline_asm<'a>(
cx: &mut ExtCtxt<'a>,
sp: Span,
tts: &[tokenstream::TokenTree],
) -> Result<Option<ast::InlineAsm>, DiagnosticBuilder<'a>> {
// Split the tts before the first colon, to avoid `asm!("x": y)` being
// parsed as `asm!(z)` with `z = "x": y` which is type ascription.
let first_colon = tts.iter()
Expand Down Expand Up @@ -80,22 +109,33 @@ pub fn expand_asm<'cx>(cx: &'cx mut ExtCtxt,
if asm_str_style.is_some() {
// If we already have a string with instructions,
// ending up in Asm state again is an error.
span_err!(cx, sp, E0660, "malformed inline assembly");
return DummyResult::expr(sp);
return Err(struct_span_err!(
cx.parse_sess.span_diagnostic,
sp,
E0660,
"malformed inline assembly"
));
}
// Nested parser, stop before the first colon (see above).
let mut p2 = cx.new_parser_from_tts(&tts[..first_colon]);
let (s, style) = match expr_to_string(cx,
panictry!(p2.parse_expr()),
"inline assembly must be a string literal") {
Some((s, st)) => (s, st),
// let compilation continue
None => return DummyResult::expr(sp),
};

if p2.token == token::Eof {
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we handling EOF specially here compared to how we did it before? Is this to preserve the error message?

(if so, it seems a bit unfortunate that we have to duplicate the error here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't a span associated with the EOF error, so the diagnostics are very poor unless we specifically check for it. See #55547 for an example..

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, seems like a problem, but an orthogonal one.

let mut err =
cx.struct_span_err(sp, "macro requires a string literal as an argument");
err.span_label(sp, "string literal required");
return Err(err);
}

let expr = p2.parse_expr()?;
let (s, style) =
match expr_to_string(cx, expr, "inline assembly must be a string literal") {
Some((s, st)) => (s, st),
None => return Ok(None),
};

// This is most likely malformed.
if p2.token != token::Eof {
let mut extra_tts = panictry!(p2.parse_all_token_trees());
let mut extra_tts = p2.parse_all_token_trees()?;
extra_tts.extend(tts[first_colon..].iter().cloned());
p = parse::stream_to_parser(cx.parse_sess, extra_tts.into_iter().collect());
}
Expand All @@ -105,18 +145,17 @@ pub fn expand_asm<'cx>(cx: &'cx mut ExtCtxt,
}
Outputs => {
while p.token != token::Eof && p.token != token::Colon && p.token != token::ModSep {

if !outputs.is_empty() {
p.eat(&token::Comma);
}

let (constraint, _str_style) = panictry!(p.parse_str());
let (constraint, _) = p.parse_str()?;

let span = p.prev_span;

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

// Expands a read+write operand into two operands.
//
Expand All @@ -143,20 +182,19 @@ pub fn expand_asm<'cx>(cx: &'cx mut ExtCtxt,
let is_indirect = constraint_str.contains("*");
outputs.push(ast::InlineAsmOutput {
constraint: output.unwrap_or(constraint),
expr: out,
expr,
is_rw,
is_indirect,
});
}
}
Inputs => {
while p.token != token::Eof && p.token != token::Colon && p.token != token::ModSep {

if !inputs.is_empty() {
p.eat(&token::Comma);
}

let (constraint, _str_style) = panictry!(p.parse_str());
let (constraint, _) = p.parse_str()?;

if constraint.as_str().starts_with("=") {
span_err!(cx, p.prev_span, E0662,
Expand All @@ -166,21 +204,20 @@ pub fn expand_asm<'cx>(cx: &'cx mut ExtCtxt,
"input operand constraint contains '+'");
}

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

inputs.push((constraint, input));
}
}
Clobbers => {
while p.token != token::Eof && p.token != token::Colon && p.token != token::ModSep {

if !clobs.is_empty() {
p.eat(&token::Comma);
}

let (s, _str_style) = panictry!(p.parse_str());
let (s, _) = p.parse_str()?;

if OPTIONS.iter().any(|&opt| s == opt) {
cx.span_warn(p.prev_span, "expected a clobber, found an option");
Expand All @@ -193,7 +230,7 @@ pub fn expand_asm<'cx>(cx: &'cx mut ExtCtxt,
}
}
Options => {
let (option, _str_style) = panictry!(p.parse_str());
let (option, _) = p.parse_str()?;

if option == "volatile" {
// Indicates that the inline assembly has side effects
Expand Down Expand Up @@ -234,26 +271,15 @@ pub fn expand_asm<'cx>(cx: &'cx mut ExtCtxt,
}
}

// If there are no outputs, the inline assembly is executed just for its side effects,
// so ensure that it is volatile
if outputs.is_empty() {
volatile = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

where did this logic (about volatile = true) go to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in the main expansion function now.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.


MacEager::expr(P(ast::Expr {
id: ast::DUMMY_NODE_ID,
node: ast::ExprKind::InlineAsm(P(ast::InlineAsm {
asm,
asm_str_style: asm_str_style.unwrap(),
outputs,
inputs,
clobbers: clobs,
volatile,
alignstack,
dialect,
ctxt: cx.backtrace(),
})),
span: sp,
attrs: ThinVec::new(),
Ok(Some(ast::InlineAsm {
asm,
asm_str_style: asm_str_style.unwrap(),
outputs,
inputs,
clobbers: clobs,
volatile,
alignstack,
dialect,
ctxt: cx.backtrace(),
}))
}
68 changes: 44 additions & 24 deletions src/libsyntax_ext/assert.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
use syntax::ast::*;
use errors::DiagnosticBuilder;
use syntax::ast::{self, *};
use syntax::source_map::Spanned;
use syntax::ext::base::*;
use syntax::ext::build::AstBuilder;
use syntax::parse::token;
use syntax::print::pprust;
use syntax::ptr::P;
use syntax::symbol::Symbol;
use syntax::tokenstream::{TokenStream, TokenTree};
use syntax_pos::{Span, DUMMY_SP};
Expand All @@ -13,33 +15,18 @@ pub fn expand_assert<'cx>(
sp: Span,
tts: &[TokenTree],
) -> Box<dyn MacResult + 'cx> {
let mut parser = cx.new_parser_from_tts(tts);

if parser.token == token::Eof {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we have some similar code duplication here already, though.

cx.struct_span_err(sp, "macro requires a boolean expression as an argument")
.span_label(sp, "boolean expression required")
.emit();
return DummyResult::expr(sp);
}

let cond_expr = panictry!(parser.parse_expr());
let custom_msg_args = if parser.eat(&token::Comma) {
let ts = parser.parse_tokens();
if !ts.is_empty() {
Some(ts)
} else {
None
let Assert { cond_expr, custom_message } = match parse_assert(cx, sp, tts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should use try here instead of creating a new function?

This could be

let result = try { /* old code */ };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but doesn't this require migrating to the new edition?

Also, I personally prefer the separation of the parsing logic and the expansion logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine. We can leave it like this. I agree separating the parsing/expansion logic makes sense.

Ok(assert) => assert,
Err(mut err) => {
err.emit();
return DummyResult::expr(sp);
}
} else {
None
};

let sp = sp.apply_mark(cx.current_expansion.mark);
let panic_call = Mac_ {
path: Path::from_ident(Ident::new(Symbol::intern("panic"), sp)),
tts: if let Some(ts) = custom_msg_args {
ts.into()
} else {
tts: custom_message.unwrap_or_else(|| {
TokenStream::from(TokenTree::Token(
DUMMY_SP,
token::Literal(
Expand All @@ -49,8 +36,8 @@ pub fn expand_assert<'cx>(
))),
None,
),
)).into()
},
))
}).into(),
delim: MacDelimiter::Parenthesis,
};
let if_expr = cx.expr_if(
Expand All @@ -67,3 +54,36 @@ pub fn expand_assert<'cx>(
);
MacEager::expr(if_expr)
}

struct Assert {
cond_expr: P<ast::Expr>,
custom_message: Option<TokenStream>,
}

fn parse_assert<'a>(
cx: &mut ExtCtxt<'a>,
sp: Span,
tts: &[TokenTree]
) -> Result<Assert, DiagnosticBuilder<'a>> {
let mut parser = cx.new_parser_from_tts(tts);

if parser.token == token::Eof {
let mut err = cx.struct_span_err(sp, "macro requires a boolean expression as an argument");
err.span_label(sp, "boolean expression required");
return Err(err);
}

Ok(Assert {
cond_expr: parser.parse_expr()?,
custom_message: if parser.eat(&token::Comma) {
let ts = parser.parse_tokens();
if !ts.is_empty() {
Some(ts)
} else {
None
}
} else {
None
},
})
}
Loading