Skip to content

Commit

Permalink
Auto merge of #48056 - ExpHP:macro-commas, r=dtolnay
Browse files Browse the repository at this point in the history
Comprehensively support trailing commas in std/core macros

I carefully organized the changes into four commits:

* Test cases
* Fixes for `macro_rules!` macros
* Fixes for builtin macros
* Docs for builtins

**I can easily scale this back to just the first two commits for now if such is desired.**

### Breaking (?) changes

* This fixes #48042, which is a breaking change that I hope people can agree is just a bugfix for an extremely dark corner case.

* To fix five of the builtins, this changes `syntax::ext::base::get_single_str_from_tts` to accept a trailing comma, and revises the documentation so that this aspect is not surprising. **I made this change under the (hopefully correct) understanding that `libsyntax` is private rustc implementation detail.** After reviewing all call sites (which were, you guessed it, *precisely those five macros*), I believe the revised semantics are closer to the intended spirit of the function.

### Changes which may require concensus

Up until now, it could be argued that some or all the following macros did not conceptually take a comma-separated list, because they only took one argument:

  * **`cfg(unix,)`** (most notable since cfg! is unique in taking a meta tag)
  * **`include{,_bytes,_str}("file.rs",)`**  (in item form this might be written as "`include!{"file.rs",}`" which is even slightly more odd)
  * **`compile_error("message",);`**
  * **`option_env!("PATH",)`**
  * **`try!(Ok(()),)`**

So I think these particular changes may require some sort of consensus.  **All of the fixes for builtins are included this list, so if we want to defer these decisions to later then I can scale this PR back to just the first two commits.**

### Other notes/general requests for comment

* Do we have a big checklist somewhere of "things to do when adding macros?" My hope is for `run-pass/macro-comma-support.rs` to remain comprehensive.
* Originally I wanted the tests to also comprehensively forbid double trailing commas.  However, this didn't work out too well: [see this gist and the giant FIXME in it](https://gist.github.com/ExpHP/6fc40e82f3d73267c4e590a9a94966f1#file-compile-fail_macro-comma-support-rs-L33-L50)
* I did not touch `select!`. It appears to me to be a complete mess, and its trailing comma mishaps are only the tip of the iceberg.
* There are [some compile-fail test cases](https://github.com/ExpHP/rust/blob/5fa97c35da2f0ee/src/test/compile-fail/macro-comma-behavior.rs#L49-L52) that didn't seem to work (rustc emits errors, but compile-fail doesn't acknowledge them), so they are disabled. Any clues? (Possibly related: These happen to be precisely the set of errors which are tagged by rustc as "this error originates in a macro outside of the current crate".)

---

Fixes #48042
Closes #46241
  • Loading branch information
bors committed Feb 28, 2018
2 parents 89e5a07 + af503be commit ddab91a
Show file tree
Hide file tree
Showing 9 changed files with 653 additions and 14 deletions.
42 changes: 35 additions & 7 deletions src/libcore/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ macro_rules! panic {
($msg:expr) => ({
$crate::panicking::panic(&($msg, file!(), line!(), __rust_unstable_column!()))
});
($fmt:expr, $($arg:tt)*) => ({
($msg:expr,) => (
panic!($msg)
);
($fmt:expr, $($arg:tt)+) => ({
$crate::panicking::panic_fmt(format_args!($fmt, $($arg)*),
&(file!(), line!(), __rust_unstable_column!()))
});
Expand Down Expand Up @@ -79,6 +82,9 @@ macro_rules! assert {
panic!(concat!("assertion failed: ", stringify!($cond)))
}
);
($cond:expr,) => (
assert!($cond)
);
($cond:expr, $($arg:tt)+) => (
if !$cond {
panic!($($arg)+)
Expand Down Expand Up @@ -359,7 +365,8 @@ macro_rules! try {
$crate::result::Result::Err(err) => {
return $crate::result::Result::Err($crate::convert::From::from(err))
}
})
});
($expr:expr,) => (try!($expr));
}

/// Write formatted data into a buffer.
Expand Down Expand Up @@ -456,6 +463,9 @@ macro_rules! writeln {
($dst:expr) => (
write!($dst, "\n")
);
($dst:expr,) => (
writeln!($dst)
);
($dst:expr, $fmt:expr) => (
write!($dst, concat!($fmt, "\n"))
);
Expand Down Expand Up @@ -524,6 +534,9 @@ macro_rules! unreachable {
($msg:expr) => ({
unreachable!("{}", $msg)
});
($msg:expr,) => ({
unreachable!($msg)
});
($fmt:expr, $($arg:tt)*) => ({
panic!(concat!("internal error: entered unreachable code: ", $fmt), $($arg)*)
});
Expand Down Expand Up @@ -603,7 +616,10 @@ mod builtin {
#[stable(feature = "compile_error_macro", since = "1.20.0")]
#[macro_export]
#[cfg(dox)]
macro_rules! compile_error { ($msg:expr) => ({ /* compiler built-in */ }) }
macro_rules! compile_error {
($msg:expr) => ({ /* compiler built-in */ });
($msg:expr,) => ({ /* compiler built-in */ });
}

/// The core macro for formatted string creation & output.
///
Expand Down Expand Up @@ -639,7 +655,10 @@ mod builtin {
#[stable(feature = "rust1", since = "1.0.0")]
#[macro_export]
#[cfg(dox)]
macro_rules! option_env { ($name:expr) => ({ /* compiler built-in */ }) }
macro_rules! option_env {
($name:expr) => ({ /* compiler built-in */ });
($name:expr,) => ({ /* compiler built-in */ });
}

/// Concatenate identifiers into one identifier.
///
Expand Down Expand Up @@ -715,7 +734,10 @@ mod builtin {
#[stable(feature = "rust1", since = "1.0.0")]
#[macro_export]
#[cfg(dox)]
macro_rules! include_str { ($file:expr) => ({ /* compiler built-in */ }) }
macro_rules! include_str {
($file:expr) => ({ /* compiler built-in */ });
($file:expr,) => ({ /* compiler built-in */ });
}

/// Includes a file as a reference to a byte array.
///
Expand All @@ -725,7 +747,10 @@ mod builtin {
#[stable(feature = "rust1", since = "1.0.0")]
#[macro_export]
#[cfg(dox)]
macro_rules! include_bytes { ($file:expr) => ({ /* compiler built-in */ }) }
macro_rules! include_bytes {
($file:expr) => ({ /* compiler built-in */ });
($file:expr,) => ({ /* compiler built-in */ });
}

/// Expands to a string that represents the current module path.
///
Expand Down Expand Up @@ -755,5 +780,8 @@ mod builtin {
#[stable(feature = "rust1", since = "1.0.0")]
#[macro_export]
#[cfg(dox)]
macro_rules! include { ($file:expr) => ({ /* compiler built-in */ }) }
macro_rules! include {
($file:expr) => ({ /* compiler built-in */ });
($file:expr,) => ({ /* compiler built-in */ });
}
}
28 changes: 23 additions & 5 deletions src/libstd/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ macro_rules! panic {
($msg:expr) => ({
$crate::rt::begin_panic($msg, &(file!(), line!(), __rust_unstable_column!()))
});
($msg:expr,) => ({
panic!($msg)
});
($fmt:expr, $($arg:tt)+) => ({
$crate::rt::begin_panic_fmt(&format_args!($fmt, $($arg)+),
&(file!(), line!(), __rust_unstable_column!()))
Expand Down Expand Up @@ -312,7 +315,10 @@ pub mod builtin {
/// ```
#[stable(feature = "compile_error_macro", since = "1.20.0")]
#[macro_export]
macro_rules! compile_error { ($msg:expr) => ({ /* compiler built-in */ }) }
macro_rules! compile_error {
($msg:expr) => ({ /* compiler built-in */ });
($msg:expr,) => ({ /* compiler built-in */ });
}

/// The core macro for formatted string creation & output.
///
Expand Down Expand Up @@ -400,7 +406,10 @@ pub mod builtin {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[macro_export]
macro_rules! option_env { ($name:expr) => ({ /* compiler built-in */ }) }
macro_rules! option_env {
($name:expr) => ({ /* compiler built-in */ });
($name:expr,) => ({ /* compiler built-in */ });
}

/// Concatenate identifiers into one identifier.
///
Expand Down Expand Up @@ -580,7 +589,10 @@ pub mod builtin {
/// Compiling 'main.rs' and running the resulting binary will print "adiós".
#[stable(feature = "rust1", since = "1.0.0")]
#[macro_export]
macro_rules! include_str { ($file:expr) => ({ /* compiler built-in */ }) }
macro_rules! include_str {
($file:expr) => ({ /* compiler built-in */ });
($file:expr,) => ({ /* compiler built-in */ });
}

/// Includes a file as a reference to a byte array.
///
Expand Down Expand Up @@ -614,7 +626,10 @@ pub mod builtin {
/// Compiling 'main.rs' and running the resulting binary will print "adiós".
#[stable(feature = "rust1", since = "1.0.0")]
#[macro_export]
macro_rules! include_bytes { ($file:expr) => ({ /* compiler built-in */ }) }
macro_rules! include_bytes {
($file:expr) => ({ /* compiler built-in */ });
($file:expr,) => ({ /* compiler built-in */ });
}

/// Expands to a string that represents the current module path.
///
Expand Down Expand Up @@ -700,7 +715,10 @@ pub mod builtin {
/// "🙈🙊🙉🙈🙊🙉".
#[stable(feature = "rust1", since = "1.0.0")]
#[macro_export]
macro_rules! include { ($file:expr) => ({ /* compiler built-in */ }) }
macro_rules! include {
($file:expr) => ({ /* compiler built-in */ });
($file:expr,) => ({ /* compiler built-in */ });
}
}

/// A macro for defining #[cfg] if-else statements.
Expand Down
6 changes: 4 additions & 2 deletions src/libsyntax/ext/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -890,8 +890,8 @@ pub fn check_zero_tts(cx: &ExtCtxt,
}
}

/// Extract the string literal from the first token of `tts`. If this
/// is not a string literal, emit an error and return None.
/// Interpreting `tts` as a comma-separated sequence of expressions,
/// expect exactly one string literal, or emit an error and return None.
pub fn get_single_str_from_tts(cx: &mut ExtCtxt,
sp: Span,
tts: &[tokenstream::TokenTree],
Expand All @@ -903,6 +903,8 @@ pub fn get_single_str_from_tts(cx: &mut ExtCtxt,
return None
}
let ret = panictry!(p.parse_expr());
let _ = p.eat(&token::Comma);

if p.token != token::Eof {
cx.span_err(sp, &format!("{} takes 1 argument", name));
}
Expand Down
2 changes: 2 additions & 0 deletions src/libsyntax_ext/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ pub fn expand_cfg<'cx>(cx: &mut ExtCtxt,
let mut p = cx.new_parser_from_tts(tts);
let cfg = panictry!(p.parse_meta_item());

let _ = p.eat(&token::Comma);

if !p.eat(&token::Eof) {
cx.span_err(sp, "expected 1 cfg-pattern");
return DummyResult::expr(sp);
Expand Down
101 changes: 101 additions & 0 deletions src/test/compile-fail/macro-comma-behavior.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
// Copyright 2018 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.

// Companion test to the similarly-named file in run-pass.

// compile-flags: -C debug_assertions=yes
// revisions: std core

#![cfg_attr(core, no_std)]

#[cfg(std)] use std::fmt;
#[cfg(core)] use core::fmt;

// (see documentation of the similarly-named test in run-pass)
fn to_format_or_not_to_format() {
let falsum = || false;

// assert!(true, "{}",); // see run-pass

assert_eq!(1, 1, "{}",);
//[core]~^ ERROR no arguments
//[std]~^^ ERROR no arguments
assert_ne!(1, 2, "{}",);
//[core]~^ ERROR no arguments
//[std]~^^ ERROR no arguments

// debug_assert!(true, "{}",); // see run-pass

debug_assert_eq!(1, 1, "{}",);
//[core]~^ ERROR no arguments
//[std]~^^ ERROR no arguments
debug_assert_ne!(1, 2, "{}",);
//[core]~^ ERROR no arguments
//[std]~^^ ERROR no arguments

#[cfg(std)] {
eprint!("{}",);
//[std]~^ ERROR no arguments
}

#[cfg(std)] {
// FIXME: compile-fail says "expected error not found" even though
// rustc does emit an error
// eprintln!("{}",);
// <DISABLED> [std]~^ ERROR no arguments
}

#[cfg(std)] {
format!("{}",);
//[std]~^ ERROR no arguments
}

format_args!("{}",);
//[core]~^ ERROR no arguments
//[std]~^^ ERROR no arguments

// if falsum() { panic!("{}",); } // see run-pass

#[cfg(std)] {
print!("{}",);
//[std]~^ ERROR no arguments
}

#[cfg(std)] {
// FIXME: compile-fail says "expected error not found" even though
// rustc does emit an error
// println!("{}",);
// <DISABLED> [std]~^ ERROR no arguments
}

unimplemented!("{}",);
//[core]~^ ERROR no arguments
//[std]~^^ ERROR no arguments

// if falsum() { unreachable!("{}",); } // see run-pass

struct S;
impl fmt::Display for S {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}",)?;
//[core]~^ ERROR no arguments
//[std]~^^ ERROR no arguments

// FIXME: compile-fail says "expected error not found" even though
// rustc does emit an error
// writeln!(f, "{}",)?;
// <DISABLED> [core]~^ ERROR no arguments
// <DISABLED> [std]~^^ ERROR no arguments
Ok(())
}
}
}

fn main() {}
20 changes: 20 additions & 0 deletions src/test/compile-fail/macro-comma-support.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright 2018 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.

// This is a companion to the similarly-named test in run-pass.
//
// It tests macros that unavoidably produce compile errors.

fn compile_error() {
compile_error!("lel"); //~ ERROR lel
compile_error!("lel",); //~ ERROR lel
}

fn main() {}
11 changes: 11 additions & 0 deletions src/test/run-pass/auxiliary/macro-comma-support.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright 2018 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.

()
Loading

0 comments on commit ddab91a

Please sign in to comment.