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

Reject a LHS formed of a single sequence TT during macro_rules! checking. #33841

Merged
merged 1 commit into from
Jun 1, 2016

Conversation

LeoTestard
Copy link
Contributor

@LeoTestard LeoTestard commented May 24, 2016

This was already rejected during expansion. Encountering malformed LHS or RHS during expansion is now considered a bug.

Follow up to #33689.

r? @pnkfelix

Note: this can break code that defines such macros but does not use them.

@pnkfelix
Copy link
Member

@LeoTestard does this need to wait for #33713 to land before it can land? Or are they independent?

@LeoTestard
Copy link
Contributor Author

Ah, good point, they are independent but I'm afraid it will break it and require rebase. Let's wait then.

@bors
Copy link
Contributor

bors commented May 25, 2016

☔ The latest upstream changes (presumably #33713) made this pull request unmergeable. Please resolve the merge conflicts.

@LeoTestard LeoTestard force-pushed the macro-sequence-lhs branch 7 times, most recently from b14e484 to c589685 Compare May 26, 2016 17:05
…cking.

This was already rejected during expansion. Encountering malformed LHS or RHS during expansion is now considered a bug.
@LeoTestard
Copy link
Contributor Author

LeoTestard commented May 26, 2016

I removed a test because the issue it was introduced for (#21350) involves code that seem to rely on the feature that this PR is removing. Thing is that the macro expander doesn't accept macros with this kind of LHS. I don't know why this issue was opened, maybe it was accepted by the expander at the time?

Another possibility is of course to fix the test to use parens, but I feel like that would be a completely different test (and actually, there are others test that do the same). Thoughts @cmr?

@pnkfelix
Copy link
Member

pnkfelix commented May 27, 2016

@LeoTestard my interpretation is that we wanted to ensure we didn't ICE as a first priority

So you might move the original text to compile-fail and add the new error message for it. Or maybe your unit tests already cover it any way.


this change might break crates in the wild (only ones that have macros with dead arms using the erroneous syntax I think) . So we might want to put it through a warning cycle?

@LeoTestard
Copy link
Contributor Author

@pnkfelix I'm pretty sure it's already covered.

@LeoTestard
Copy link
Contributor Author

@pnkfelix Up? r?

@pnkfelix
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 31, 2016

📌 Commit 864b3c8 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented May 31, 2016

⌛ Testing commit 864b3c8 with merge 4561415...

bors added a commit that referenced this pull request May 31, 2016
Reject a LHS formed of a single sequence TT during `macro_rules!` checking.

This was already rejected during expansion. Encountering malformed LHS or RHS during expansion is now considered a bug.

Follow up to #33689.

r? @pnkfelix

Note: this can break code that defines such macros but does not use them.
@bors
Copy link
Contributor

bors commented May 31, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@alexcrichton
Copy link
Member

@bors: retry

On Tue, May 31, 2016 at 9:17 AM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt-rustbuild
http://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt-rustbuild/builds/1305


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#33841 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAD95JehuTKEXQV684xo5vgKfg4RJdDoks5qHF8hgaJpZM4IlmmE
.

@bors
Copy link
Contributor

bors commented May 31, 2016

💔 Test failed - auto-linux-64-cross-freebsd

@alexcrichton
Copy link
Member

@bors: retry

On Tue, May 31, 2016 at 3:55 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-cross-freebsd
http://buildbot.rust-lang.org/builders/auto-linux-64-cross-freebsd/builds/480


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#33841 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAD95CemTOdKDGN_8hk0P4S38_8CBJZxks5qHLxJgaJpZM4IlmmE
.

Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 1, 2016
…kfelix

Reject a LHS formed of a single sequence TT during `macro_rules!` checking.

This was already rejected during expansion. Encountering malformed LHS or RHS during expansion is now considered a bug.

Follow up to rust-lang#33689.

r? @pnkfelix

Note: this can break code that defines such macros but does not use them.
bors added a commit that referenced this pull request Jun 1, 2016
Rollup of 11 pull requests

- Successful merges: #33385, #33606, #33841, #33892, #33896, #33915, #33921, #33967, #33970, #33973, #33977
- Failed merges:
@bors bors merged commit 864b3c8 into rust-lang:master Jun 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants