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

Comprehensively support trailing commas in std/core macros #48056

Merged
merged 6 commits into from Feb 28, 2018

Conversation

Projects
None yet
9 participants
@ExpHP
Copy link
Contributor

ExpHP commented Feb 7, 2018

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
  • 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 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

ExpHP added some commits Feb 7, 2018

libcore/libstd: fix commas in macro_rules! macros
BREAKING CHANGE: (or perhaps, *bugfix*)

In #![no_std] applications, the following calls to `panic!` used
to behave differently; they now behave the same.

Old behavior:

    panic!("{{");   // panics with "{{"
    panic!("{{",);  // panics with "{"

New behavior:

    panic!("{{");   // panics with "{{"
    panic!("{{",);  // panics with "{{"

This only affects calls to `panic!` (and by proxy `assert`
and `debug_assert`) with a single string literal followed by
a trailing comma, and only in `#![no_std]` applications.
libsyntax/ext: trailing commas in builtin macros
Most notably 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 understanding
that this crate is private rustc implementation detail (I hope this is
correct!). After reviewing all call sites, I believe the revised
semantics are closer to the intended spirit of the function.
@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Feb 7, 2018

Highfive's missing, randomly assigning a reviewer.

r? @dtolnay

// make sure we don't accidentally forward to `write!("text")`
#[cfg(std)]
#[test]
fn writeln_2arg() {

This comment has been minimized.

@ExpHP

ExpHP Feb 8, 2018

Author Contributor

whoops, I had intended to test the single-argument invocation instead, because it is currently handled by its own separate pattern and it is unlikely that there are any other tests in test/ that use it. Like:

#[cfg(std)]
#[test]
fn writeln_1arg() {
    use fmt::Write;

    let mut s = String::new();
    writeln!(&mut s,).unwrap();
    assert_eq!(&s, "\n");
}
@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Feb 8, 2018

Do we want to wait for #47752 to use $(,)? here?

@ExpHP

This comment has been minimized.

Copy link
Contributor Author

ExpHP commented Feb 8, 2018

I don't think so.

I think that by far the most onerous part is writing the tests. The actual fixes to macro_rules! macros in this commit amount to only 18 lines of code. The macros in std and core are relatively simple (with the exception of select!) and only derive tiny, incremental benefits from $()?.

After my changes, libcore has 7 places in its macro_rules! macros where it could use $()? to delete a single rule. I also count 9 places where the builtin stubs could use $()?, simply so that the docs reflect best practices. These nine include format_args!, which I feel should be changed as:

// current form
macro_rules! format_args {
    ($fmt:expr) => ({ /* compiler built-in */ });
    ($fmt:expr, $($args:tt)*) => ({ /* compiler built-in */ });
}

// "best practices"
macro_rules! format_args {
    ($fmt:expr $(,)?) => ({ /* compiler built-in */ });
    ($fmt:expr, $($args:tt)+) => ({ /* compiler built-in */ });
}
@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Feb 12, 2018

@dtolnay this needs a review!

@dtolnay
Copy link
Member

dtolnay left a comment

Thanks so much for working through this and for the clear explanations.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 13, 2018

#[cfg(unix,)] already works so I believe cfg!(unix,) also needs to work. I am on board with supporting trailing comma in all of include!, compile_error!, option_env!, try!.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 13, 2018

Let's get a few more eyes on the breaking change here -- fixing #48042 by treating panic!("{}",) the same as panic!("{}").

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 13, 2018

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Feb 13, 2018

Do we want to wait for #47752 to use $(,)? here?

FWIW, #47752 has merged now...

@ExpHP

This comment has been minimized.

Copy link
Contributor Author

ExpHP commented Feb 14, 2018

@mark-i-m: So fast!

But in any case, since these definitions show up in documentation, the std and core macros probably shouldn't be changed to use it until the feature is stabilized.

@ExpHP
Copy link
Contributor Author

ExpHP left a comment

Note there are some FIXMEs I left in the tests since I didn't figure that it would get approved in this revision. 🙂

// The expectation is for this to be updated as new macros are added,
// or as functionality is added to existing macros.
//
// (FIXME: (please discuss in PR) is the above expectation reasonable?)

This comment has been minimized.

@ExpHP

ExpHP Feb 14, 2018

Author Contributor

Leftover FIXME

(I think I might just consider it my own responsibility to update this every now and then. Additions to stdlib macros seem pretty infrequent and hard to miss)

println!("hello {}", "world",);
}

// FIXME: select! (please discuss in PR)

This comment has been minimized.

@ExpHP

ExpHP Feb 14, 2018

Author Contributor

Leftover FIXME

I might just change this to TODO: select! and move my concerns about select! to its tracking issue (#27800)

}

#[cfg(std)] {
// FIXME: compile-fail says "expected error not found" even though

This comment has been minimized.

@ExpHP

ExpHP Feb 14, 2018

Author Contributor

Leftover FIXME

Unless someone knows what's up, I'm tempted to just leave this one here, to document that it would be tested here if it could.

(IMO the regressions that would be caught by this test are far, far less likely to occur than the ones that would be caught by the corresponding tests in run-pass.)

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 16, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

1 similar comment
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 16, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 19, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 19, 2018

📌 Commit 9205f3d has been approved by dtolnay

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 24, 2018

Rollup merge of rust-lang#48056 - ExpHP:macro-commas, r=dtolnay
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 pull request Feb 24, 2018

Auto merge of #48510 - Manishearth:rollup, r=Manishearth
Rollup of 15 pull requests

- Successful merges: #47987, #48056, #48061, #48084, #48143, #48185, #48206, #48208, #48232, #48246, #48258, #48317, #48353, #48356, #48402
- Failed merges:

bors added a commit that referenced this pull request Feb 24, 2018

Auto merge of #48510 - Manishearth:rollup, r=Manishearth
Rollup of 15 pull requests

- Successful merges: #47987, #48056, #48061, #48084, #48143, #48185, #48206, #48208, #48232, #48246, #48258, #48317, #48353, #48356, #48402
- Failed merges:

bors added a commit that referenced this pull request Feb 24, 2018

Auto merge of #48510 - Manishearth:rollup, r=Manishearth
Rollup of 15 pull requests

- Successful merges: #47987, #48056, #48061, #48084, #48143, #48185, #48206, #48208, #48232, #48246, #48258, #48317, #48353, #48356, #48402
- Failed merges:
@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Feb 24, 2018

[01:52:21] 
[01:52:21] ---- [pretty] run-pass/macro-comma-support.rs stdout ----
[01:52:21] 	
[01:52:21] error in revision `std`: pretty-printed source does not typecheck
[01:52:21] status: exit code: 101
[01:52:21] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "-" "-Zno-trans" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/macro-comma-support.pretty-out" "--target=x86_64-unknown-linux-gnu" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/macro-comma-support.stage2-x86_64-unknown-linux-gnu.pretty.aux" "--cfg" "std" "-Crpath" "-O" "-Zmiri" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--test" "-C" "debug_assertions=yes"
[01:52:21] stdout:
[01:52:21] ------------------------------------------
[01:52:21] 
[01:52:21] ------------------------------------------
[01:52:21] stderr:
[01:52:21] ------------------------------------------
[01:52:21] thread 'rustc' panicked at 'cannot resolve relative path in non-file source `<anon>`', libsyntax/ext/source_util.rs:202:22
[01:52:21] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:52:21] 
[01:52:21] error: internal compiler error: unexpected panic
[01:52:21] 
[01:52:21] note: the compiler unexpectedly panicked. this is a bug.
[01:52:21] 
[01:52:21] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[01:52:21] 
[01:52:21] note: rustc 1.26.0-dev running on x86_64-unknown-linux-gnu
[01:52:21] 
[01:52:21] 
[01:52:21] ------------------------------------------
[01:52:21] 
[01:52:21] thread '[pretty] run-pass/macro-comma-support.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:2892:9
[01:52:21] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:52:21] 
[01:52:21] 
[01:52:21] failures:
[01:52:21]     [pretty] run-pass/macro-comma-support.rs
[01:52:21] 
[01:52:21] test result: �[31mFAILED�(B�[m. 2876 passed; 1 failed; 45 ignored; 0 measured; 0 filtered out
[01:52:21] 
[01:52:21] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:476:22
[01:52:21] 

bors added a commit that referenced this pull request Feb 24, 2018

Auto merge of #48510 - Manishearth:rollup, r=Manishearth
Rollup of 15 pull requests

- Successful merges: #47987, #48056, #48061, #48084, #48143, #48185, #48206, #48208, #48232, #48246, #48258, #48317, #48353, #48356, #48402
- Failed merges:
@ExpHP

This comment has been minimized.

Copy link
Contributor Author

ExpHP commented Feb 24, 2018

odd, will try to reproduce


Update:

  • I think the test is one that's not always run, which is why the lights were green. (see the check-aux make rule, which is only built by the failing CI Config.)

  • Not only that, but on the surface at least, the test seems to be opt-in? (based on all the pretty-expanded FIXME #23616 notes) But then why is it running on my input? I am confused...

  • Watching LLVM build...

  • Ahah, there is another annotation, ignore-pretty that other tests using include! in run-pass use.

  • "Fix" implemented, running the full check-aux suite...

ignore-pretty for the macro-comma-support test
include! and the pretty test do not mix
@ExpHP

This comment has been minimized.

Copy link
Contributor Author

ExpHP commented Feb 25, 2018

Should be fixed now. make check-aux passed locally.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 26, 2018

@bors: r=dtolnay

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 26, 2018

📌 Commit af503be has been approved by dtolnay

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 26, 2018

The final comment period is now complete.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 28, 2018

⌛️ Testing commit af503be with merge ddab91a...

bors added a commit that referenced this pull request Feb 28, 2018

Auto merge of #48056 - ExpHP:macro-commas, r=dtolnay
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
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: dtolnay
Pushing ddab91a to master...

@bors bors merged commit af503be into rust-lang:master Feb 28, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.