Skip to content

Ensure a sole string-literal passed to panic! is not a fmt string. #24370

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

Closed
wants to merge 11 commits into from
14 changes: 12 additions & 2 deletions src/libcore/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,23 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// SNAP 5520801
#[cfg(stage0)]
#[macro_export]
macro_rules! __unstable_rustc_ensure_not_fmt_string_literal {
($name:expr, $e:expr) => { $e }
}

/// Entry point of task panic, for details, see std::macros
#[macro_export]
macro_rules! panic {
() => (
panic!("explicit panic")
);
($msg:expr) => ({
static _MSG_FILE_LINE: (&'static str, &'static str, u32) = ($msg, file!(), line!());
static _MSG_FILE_LINE: (&'static str, &'static str, u32) =
(__unstable_rustc_ensure_not_fmt_string_literal!("unary `panic!`", $msg),
file!(), line!());
::core::panicking::panic(&_MSG_FILE_LINE)
});
($fmt:expr, $($arg:tt)*) => ({
Expand Down Expand Up @@ -56,7 +65,8 @@ macro_rules! panic {
macro_rules! assert {
($cond:expr) => (
if !$cond {
panic!(concat!("assertion failed: ", stringify!($cond)))
const MSG: &'static str = concat!("assertion failed: ", stringify!($cond));
panic!(MSG)
}
);
($cond:expr, $($arg:tt)+) => (
Expand Down
2 changes: 1 addition & 1 deletion src/libcoretest/nonzero.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,6 @@ fn test_match_option_string() {
let five = "Five".to_string();
match Some(five) {
Some(s) => assert_eq!(s, "Five"),
None => panic!("unexpected None while matching on Some(String { ... })")
None => panic!(("unexpected None while matching on Some(String { ... })"))
}
}
22 changes: 16 additions & 6 deletions src/libstd/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@

#![unstable(feature = "std_misc")]

// SNAP 5520801
#[cfg(stage0)]
#[macro_export]
#[allow_internal_unstable]
macro_rules! __unstable_rustc_ensure_not_fmt_string_literal {
($name:expr, $e:expr) => { $e }
}

/// The entry point for panic of Rust tasks.
///
/// This macro is used to inject panic into a Rust task, causing the task to
Expand Down Expand Up @@ -44,7 +52,8 @@ macro_rules! panic {
panic!("explicit panic")
});
($msg:expr) => ({
$crate::rt::begin_unwind($msg, {
$crate::rt::begin_unwind(
__unstable_rustc_ensure_not_fmt_string_literal!("unary `panic!`", $msg), {
// static requires less code at runtime, more constant data
static _FILE_LINE: (&'static str, usize) = (file!(), line!() as usize);
&_FILE_LINE
Expand Down Expand Up @@ -90,11 +99,12 @@ macro_rules! panic {
panic!("explicit panic")
});
($msg:expr) => ({
$crate::rt::begin_unwind($msg, {
// static requires less code at runtime, more constant data
static _FILE_LINE: (&'static str, u32) = (file!(), line!());
&_FILE_LINE
})
$crate::rt::begin_unwind(
__unstable_rustc_ensure_not_fmt_string_literal!("unary `panic!`", $msg), {
// static requires less code at runtime, more constant data
static _FILE_LINE: (&'static str, u32) = (file!(), line!());
&_FILE_LINE
})
});
($fmt:expr, $($arg:tt)+) => ({
$crate::rt::begin_unwind_fmt(format_args!($fmt, $($arg)+), {
Expand Down
43 changes: 35 additions & 8 deletions src/libsyntax/ext/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,14 @@ fn initial_syntax_expander_table<'feat>(ecfg: &expand::ExpansionConfig<'feat>)
syntax_expanders.insert(intern("format_args"),
// format_args uses `unstable` things internally.
NormalTT(Box::new(ext::format::expand_format_args), None, true));

// ensure_not_fmt_string_literal is passed `unstable` things to
// use in its expansion. (Of course, that should not actually
// force us to pass `true` to allow the use of unstable things;
// i.e. this is working around a bug elsewhere.)
let tt = NormalTT(Box::new(ext::format::ensure_not_fmt_string_literal), None, true);
syntax_expanders.insert(intern("__unstable_rustc_ensure_not_fmt_string_literal"), tt);

syntax_expanders.insert(intern("env"),
builtin_normal_expander(
ext::env::expand_env));
Expand Down Expand Up @@ -748,19 +756,38 @@ impl<'a> ExtCtxt<'a> {
}
}

pub type ExprStringLitResult =
Result<(P<ast::Expr>, InternedString, ast::StrStyle), P<ast::Expr>>;

/// Extract a string literal from macro expanded version of `expr`.
///
/// if `expr` is not string literal, then Err with span for expanded
/// input, but does not emit any messages nor stop compilation.
pub fn expr_string_lit(cx: &mut ExtCtxt, expr: P<ast::Expr>) -> ExprStringLitResult
{
// we want to be able to handle e.g. concat("foo", "bar")
let expr = cx.expander().fold_expr(expr);
let lit = match expr.node {
ast::ExprLit(ref l) => match l.node {
ast::LitStr(ref s, style) => Some(((*s).clone(), style)),
_ => None
},
_ => None
};
match lit {
Some(lit) => Ok((expr, lit.0, lit.1)),
None => Err(expr),
}
}

/// Extract a string literal from the macro expanded version of `expr`,
/// emitting `err_msg` if `expr` is not a string literal. This does not stop
/// compilation on error, merely emits a non-fatal error and returns None.
pub fn expr_to_string(cx: &mut ExtCtxt, expr: P<ast::Expr>, err_msg: &str)
-> Option<(InternedString, ast::StrStyle)> {
// we want to be able to handle e.g. concat("foo", "bar")
let expr = cx.expander().fold_expr(expr);
match expr.node {
ast::ExprLit(ref l) => match l.node {
ast::LitStr(ref s, style) => return Some(((*s).clone(), style)),
_ => cx.span_err(l.span, err_msg)
},
_ => cx.span_err(expr.span, err_msg)
match expr_string_lit(cx, expr) {
Ok((_, s, style)) => return Some((s, style)),
Err(e) => cx.span_err(e.span, err_msg),
}
None
}
Expand Down
72 changes: 72 additions & 0 deletions src/libsyntax/ext/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,78 @@ impl<'a, 'b> Context<'a, 'b> {
}
}

/// Expands `ensure_not_fmt_string_literal!(<where-literal>, <expr>)`
/// into `<expr>`, but ensures that if `<expr>` is a string-literal,
/// then it is not a potential format string literal.
pub fn ensure_not_fmt_string_literal<'cx>(cx: &'cx mut ExtCtxt,
sp: Span,
tts: &[ast::TokenTree])
-> Box<base::MacResult+'cx> {
use fold::Folder;
let takes_two_args = |cx: &ExtCtxt, rest| {
cx.span_err(sp, &format!("`ensure_not_fmt_string_literal!` \
takes 2 arguments, {}", rest));
DummyResult::expr(sp)
};
let mut p = cx.new_parser_from_tts(tts);
if p.token == token::Eof { return takes_two_args(cx, "given 0"); }
let arg1 = cx.expander().fold_expr(p.parse_expr());
if p.token == token::Eof { return takes_two_args(cx, "given 1"); }
if !panictry!(p.eat(&token::Comma)) { return takes_two_args(cx, "comma-separated"); }
if p.token == token::Eof { return takes_two_args(cx, "given 1"); }
let arg2 = cx.expander().fold_expr(p.parse_expr());
if p.token != token::Eof {
takes_two_args(cx, "given too many");
// (but do not return; handle two provided, nonetheless)
}

// First argument is name of where this was invoked (for error messages).
let lit_str_with_extended_lifetime;
let name: &str = match expr_string_lit(cx, arg1) {
Ok((_, lit_str, _)) => {
lit_str_with_extended_lifetime = lit_str;
&lit_str_with_extended_lifetime
}
Err(expr) => {
let msg = "first argument to `ensure_not_fmt_string_literal!` \
must be string literal";
cx.span_err(expr.span, msg);
// Compile proceeds using "ensure_not_fmt_string_literal"
// as the name.
"ensure_not_fmt_string_literal!"
}
};

// Second argument is the expression we are checking.
let warning = |cx: &ExtCtxt, c: char| {
cx.span_warn(sp, &format!("{} literal argument contains `{}`", name, c));
cx.span_note(sp, "Is it meant to be a `format!` string?");
cx.span_help(sp, "You can wrap the argument in parentheses \
to sidestep this warning");
};

let expr = match expr_string_lit(cx, arg2) {
Err(expr) => {
// input was not a string literal; just ignore it.
expr
}

Ok((expr, lit_str, _style)) => {
for c in lit_str.chars() {
if c == '{' || c == '}' {
warning(cx, c);
break;
}
}
// we still return the expr itself, to allow catching of
// further errors in the input.
expr
}
};

MacEager::expr(expr)
}

pub fn expand_format_args<'cx>(ecx: &'cx mut ExtCtxt, sp: Span,
tts: &[ast::TokenTree])
-> Box<base::MacResult+'cx> {
Expand Down
75 changes: 75 additions & 0 deletions src/test/compile-fail/issue-22932.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(rustc_attrs)]

// Issue 22932: `panic!("{}");` should not compile.

pub fn f1() { panic!("this does not work {}");
//~^ WARN unary `panic!` literal argument contains `{`
//~| NOTE Is it meant to be a `format!` string?
//~| HELP You can wrap the argument in parentheses to sidestep this warning
}

pub fn workaround_1() {
panic!(("This *does* works {}"));
}

pub fn workaround_2() {
const MSG: &'static str = "This *does* work {}";
panic!(MSG);
}

pub fn f2() { panic!("this does not work {");
//~^ WARN unary `panic!` literal argument contains `{`
//~| NOTE Is it meant to be a `format!` string?
//~| HELP You can wrap the argument in parentheses to sidestep this warning
}

pub fn f3() { panic!("nor this }");
//~^ WARN unary `panic!` literal argument contains `}`
//~| NOTE Is it meant to be a `format!` string?
//~| HELP You can wrap the argument in parentheses to sidestep this warning
}

pub fn f4() { panic!("nor this {{");
//~^ WARN unary `panic!` literal argument contains `{`
//~| NOTE Is it meant to be a `format!` string?
//~| HELP You can wrap the argument in parentheses to sidestep this warning
}

pub fn f5() { panic!("nor this }}");
//~^ WARN unary `panic!` literal argument contains `}`
//~| NOTE Is it meant to be a `format!` string?
//~| HELP You can wrap the argument in parentheses to sidestep this warning
}

pub fn f0_a() {
__unstable_rustc_ensure_not_fmt_string_literal!("`f0_a`", "this does not work {}");
//~^ WARN `f0_a` literal argument contains `{`
//~| NOTE Is it meant to be a `format!` string?
//~| HELP You can wrap the argument in parentheses to sidestep this warning
}

pub fn f0_b() {
__unstable_rustc_ensure_not_fmt_string_literal!("`f0_b`", "this does work");
}

pub fn f0_c() {
__unstable_rustc_ensure_not_fmt_string_literal!("`f0_c`", ("so does this {}"));
}

// This test is just checking that we get all the right warnings; none
// of them are outright errors, so use the special `rustc_error`
// attribute to force a compile error.
#[rustc_error]
pub fn main() {
//~^ ERROR compilation successful
}
3 changes: 2 additions & 1 deletion src/test/run-fail/explicit-panic-msg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,6 @@
fn main() {
let mut a = 1;
if 1 == 1 { a = 2; }
panic!(format!("woooo{}", "o"));
let msg = format!("woooo{}", "o");
panic!(msg);
Copy link
Member

Choose a reason for hiding this comment

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

Was this necessary to change?

Copy link
Member

Choose a reason for hiding this comment

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

Ah wait I see the commit now, how come this was necessary to change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because otherwise I get stability warnings saying that the internals of float are unstable.

I assume this is due to a bug in either my macro, the expander, or in the way we lint stability via the chain of spans associated with the expansion. I was/am not sure if this is a deal-breaker for this change; maybe I will have to track down that bug first (and presumably let this wait until 1.x to land.)

Copy link
Member Author

Choose a reason for hiding this comment

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

on reflection, the benefit of the warning (catching the programming mistake where one passes a format literal string to panic! but forgets the associated format arguments) does not warrant breaking even code like the example above.

I still think the fact that my macro did not work represents a bug somewhere, perhaps in the macro system or in the stability checker.

But for now I am closing this PR; we can put in this warning later (i.e. after 1.0).

}