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

Completing support for trailing commas in std macros #46241

Closed
6 tasks
ExpHP opened this issue Nov 24, 2017 · 1 comment · Fixed by #48056
Closed
6 tasks

Completing support for trailing commas in std macros #46241

ExpHP opened this issue Nov 24, 2017 · 1 comment · Fixed by #48056
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-enhancement Category: An issue proposing an enhancement or a PR with one. 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 Nov 24, 2017

I really want to see complete support for trailing commas in std macros.

As I described on Internals, all (stable) "variadic" macros correctly support trailing commas for sufficiently long argument lists. It is only a few, select, specific argument counts that trip them up.

Here is a check list of all test cases that currently fail to compile:

Failing cases that obviously need fixing

These macros already support trailing commas when given longer argument lists, they just fail for these specific lengths:

  • {debug_,}assert!(true, );
  • {debug_,}assert!(true, "hello",);
  • {debug_,}assert_{eq,ne}!(1, 1,);
    • Status: Fixed in Beta
  • panic!("hello",);
  • writeln!(&mut stdout,);
  • unreachable!("hello",);

Worth discussion

These macros technically have not yet committed to the "function-call-like" syntax and we still currently have the option to extend these with an entirely different syntax. Like, I don't know, compile_error!("a" => b) or something.

  • include{,_bytes,_str}!("dumdum.rs",);
  • compile_error!("lel",);
  • try!(Ok(()),);
  • option_env!("PATH",);
    • note: env! handles the trailing comma so we might want this just for consistency's sake
  • cfg!(unix,);

Oddball cases

  • the unstable mpsc select! does not support trailing commas in general...
    • ...but it also contains numerous other papercuts and probably deserves an issue all of its own.

I offer to help with at least those macros which are implemented via macro_rules!.

@ExpHP
Copy link
Contributor Author

ExpHP commented Nov 24, 2017

Concerns

Of the top of my head I can think of at least the following concerns which should be addressed during implementation.

  • Instastability. (I assume changes to macro_rules! macros are instastable...)
  • The recursion limit. Macro invocations which were previously valid should go through the same number of recursive expansions (or fewer) or else this technically becomes a breaking change.
  • Impact on compile times. Will adding cases to ubiquitous macros like assert! negatively impact compile times?
  • Testing for double commas. There should probably be compile-fail tests that give double trailing commas. (note that rust is currently missing such tests in general, see Test coverage for double trailing commas is poor #46238)
  • Appearance of documentation. Since macro patterns appear in docs.

@pietroalbini pietroalbini added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) labels Jan 23, 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-enhancement Category: An issue proposing an enhancement or a PR with one. 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