Skip to content
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

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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