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

Allow lifetimes to be passed to macros #33135

Closed
wants to merge 2 commits into from

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Apr 21, 2016

Partially fixes #10413. This only handles lifetimes, as they're much
more "obvious" than the type parameter list would be, as that implies
that we probably want to mirror ast::Generics as well as all of the
individual pieces. That should probably have an RFC.

These are essentially just treated the same as idents. They're a single
token, and are quite pervasive, so we interpolate them back into a real
token rather than handling NtLifetime all over the parser.

This change is specifically needed in Diesel in order for us to provide
a "normal macro" alternative to some of our syntax extensions. With this
change, we'd be able to have a decent stable story without syntex.

Partially fixes rust-lang#10413. This only handles lifetimes, as they're much
more "obvious" than the type parameter list would be, as that implies
that we probably want to mirror `ast::Generics` as well as all of the
individual pieces. That should probably have an RFC.

These are essentially just treated the same as idents. They're a single
token, and are quite pervasive, so we interpolate them back into a real
token rather than handling `NtLifetime` all over the parser.

This change is specifically needed in Diesel in order for us to provide
a "normal macro" alternative to some of our syntax extensions. With this
change, we'd be able to have a decent stable story without syntex.
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@DanielKeep
Copy link
Contributor

Only change I'd personally make would be to abbreviate the matcher name, since we have expr, ident, stmt, pat, ty. Maybe ltime or lt.

Really, there's no compelling reason not to support this. This would also play into rust-lang/rfcs#1583, allowing the expansion to use lifetime NTs.

@nikomatsakis
Copy link
Contributor

I'm strongly in favor of this change. According to our policies, though, I believe an RFC would be required (though not a complex one). Something like rust-lang/rfcs#1576 or rust-lang/rfcs#1575. I imagine we could "fast-track" such an RFC. (As we ought to consider fast-tracking those, which are changes of a similar magnitude.)

cc @rust-lang/lang

@nikomatsakis
Copy link
Contributor

also cc @pnkfelix and @LeoTestard specifically

@sgrif
Copy link
Contributor Author

sgrif commented Apr 22, 2016

According to our policies, though, I believe an RFC would be required

I assumed that wasn't required since there was an open issue on the main Rust repo that had been around for a while. Certainly happy to write up an RFC though.

@sgrif
Copy link
Contributor Author

sgrif commented Apr 22, 2016

The RFC is pretty brief as there's not a ton to say about the change. rust-lang/rfcs#1590

All of the other non-terminals have this implementation. If we allow
them in macros, we should allow them in quoting
@nikomatsakis
Copy link
Contributor

@sgrif I'm going to close this PR until the RFC is settled, just to keep queue under control.

@sgrif
Copy link
Contributor Author

sgrif commented May 4, 2016

No problem

On Wed, May 4, 2016, 9:37 AM Niko Matsakis notifications@github.com wrote:

Closed #33135 #33135.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#33135 (comment)

@mikeyhew
Copy link
Contributor

mikeyhew commented May 4, 2017

@sgrif I'm going to try to finish this, can you tell me what still needs to be done?

@sgrif
Copy link
Contributor Author

sgrif commented May 9, 2017

This PR should actually be good to go as-is (modulo rebasing). The thing I wanted to fix was an issue with $($x: lifetime)* followed by any other repeater, but that seems like a more general problem with the macro_rules parser that is out of scope for this feature.

@mikeyhew
Copy link
Contributor

@sgrif OK, sounds good. Rebasing has been pretty straightforward for the most part, with a couple of exceptions that I'll need some help with. I'll post a new PR and ping you when it's ready for your input.

mikeyhew added a commit to mikeyhew/rust that referenced this pull request May 11, 2017
Started rebasing @sgrif's PR rust-lang#33135 off of current master. (Well, actually merging it into a new branch based off current master.)

The following files still need to be fixed or at least reviewed:

- `src/libsyntax/ext/tt/macro_parser.rs`: calls `Parser::parse_lifetime`, which doesn't exist anymore
- `src/libsyntax/parse/parser.rs`: @sgrif added an error message to `Parser::parse_lifetime`. Code has since been refactored, so I just took it out for now.
- `src/libsyntax/ext/tt/transcribe.rs`: This code has been refactored bigtime. Not sure whether @sgrif's changes here are still necessary. Took it out for this commit.
@mikeyhew mikeyhew mentioned this pull request May 11, 2017
3 tasks
@ricochet1k ricochet1k mentioned this pull request Dec 20, 2017
1 task
ricochet1k pushed a commit to ricochet1k/rust that referenced this pull request Dec 28, 2017
Started rebasing @sgrif's PR rust-lang#33135 off of current master. (Well, actually merging it into a new branch based off current master.)

The following files still need to be fixed or at least reviewed:

- `src/libsyntax/ext/tt/macro_parser.rs`: calls `Parser::parse_lifetime`, which doesn't exist anymore
- `src/libsyntax/parse/parser.rs`: @sgrif added an error message to `Parser::parse_lifetime`. Code has since been refactored, so I just took it out for now.
- `src/libsyntax/ext/tt/transcribe.rs`: This code has been refactored bigtime. Not sure whether @sgrif's changes here are still necessary. Took it out for this commit.
bors added a commit that referenced this pull request Jan 1, 2018
Allow lifetimes in macros

This is a resurrection of PR #41927 which was a resurrection of #33135, which is intended to fix #34303.

In short, this allows macros_rules! to use :lifetime as a matcher to match 'lifetimes.

Still to do:
- [x]  Feature gate
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.

Add 'lifetime' and 'type parameter list' fragment specifiers to macro parser.
5 participants