Skip to content

with #![no_std] enabled, 'panic!("{{}}")' and 'panic!("{{}}",)' are different #48042

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
ExpHP opened this issue Feb 7, 2018 · 1 comment · Fixed by #48056
Closed

with #![no_std] enabled, 'panic!("{{}}")' and 'panic!("{{}}",)' are different #48042

ExpHP opened this issue Feb 7, 2018 · 1 comment · Fixed by #48056
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ExpHP
Copy link
Contributor

ExpHP commented Feb 7, 2018

Discovered while trying to implement fixes for #46241. Possible back-compat hazard for trailing comma support, but hopefully not.

libstd/macros.rs

macro_rules! panic {
    () => (/* default message */);
    ($msg:expr) => (/* use msg as literal */);
    ($fmt:expr, $($arg:tt)+) => (/* use format_args! */);
}

libcore/macros.rs

macro_rules! panic {
    () => (/* default message */);
    ($msg:expr) => (/* use msg as literal */);
    ($fmt:expr, $($arg:tt)*) => (/* use format_args! */);
}

Can you spot the difference?


The definition in std fails to support trailing commas after a single string literal:

fn main() {
    panic!("a");
    panic!("a",); //~ ERROR unexpected end of macro invocation
    
    panic!("{}");
    panic!("{}",); //~ ERROR unexpected end of macro invocation
}

The definition in core accepts a trailing comma, but follows a completely different code-path with potentially different semantics.

#![no_std]

fn main() {
    panic!("a");
    panic!("a",);
    
    panic!("{}");
    panic!("{}",); //~ ERROR 1 positional argument in format string, but no arguments were given
}

Fortuitously, it seems to me that it libcore's definition produces equivalent behavior for panic!(<literal>,) and panic!(<literal>) in all cases where both forms successfully compile, meaning it should hopefully be safe to change panic!(<literal>,) to behave like panic!(<literal>).

@ExpHP
Copy link
Contributor Author

ExpHP commented Feb 7, 2018

Crap. Found an example of the sort of back-compat hazard I was talking about.

In today's rust, the following two programs are different:

#![no_std]

fn main() {
    panic!("{{}}");  // panics with "{{}}"
}
#![no_std]

fn main() {
    panic!("{{}}",);  // panics with "{}"
}

@ExpHP ExpHP changed the title with #![no_std] enabled, 'panic!(<literal>,)' compiles and is not necessarily the same as 'panic!(<literal>)' with #![no_std] enabled, 'panic!(<literal>,)' compiles and is different from 'panic!(<literal>)' Feb 7, 2018
@ExpHP ExpHP changed the title with #![no_std] enabled, 'panic!(<literal>,)' compiles and is different from 'panic!(<literal>)' with #![no_std] enabled, 'panic!("{{}}")' and 'panic!("{{}}",)' are different Feb 7, 2018
@kennytm kennytm added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Feb 7, 2018
Manishearth added a commit to Manishearth/rust that referenced this issue Feb 24, 2018
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 rust-lang#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 rust-lang#48042
Closes rust-lang#46241
bors added a commit that referenced this issue Feb 28, 2018
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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants