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

Don't accidentally parse MatchNt #22814

Closed
kmcallister opened this issue Feb 25, 2015 · 8 comments
Closed

Don't accidentally parse MatchNt #22814

kmcallister opened this issue Feb 25, 2015 · 8 comments
Assignees
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. P-high High priority

Comments

@kmcallister
Copy link
Contributor

This bit of macro from libcore:

( $($name:ident)+) => (
    #[stable(feature = "rust1", since = "1.0.0")]
    impl<$($name: Hash),*> Hash for ($($name,)*) {

was apparently parsing as a MatchNt $name:Hash, which breaks with my fix to #21370.

There's no need to parse MatchNt on a macro RHS, but we don't know we're on a macro RHS when we're parsing a token tree. Probably we should get rid of MatchNt. It's a weird "composite token" and the tokenizer phase is clearly too early to spot it. Rather we can look for the colon and fragment specifier when we parse a LHS TT into matchers.

Nominating 1.0-beta, P-backcompat-lang.

@kmcallister kmcallister added I-nominated A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) labels Feb 25, 2015
@kmcallister kmcallister self-assigned this Feb 25, 2015
@kmcallister
Copy link
Contributor Author

I guess it used to turn into a SubstNt in TokenTree::get_tt but that's too late on my branch.

@pnkfelix
Copy link
Member

1.0-beta, P-backcompat-lang.

@pnkfelix pnkfelix added this to the 1.0 beta milestone Feb 26, 2015
Manishearth added a commit to Manishearth/rust that referenced this issue Feb 27, 2015
 Fixes rust-lang#21370.

`unused-macro-with-follow-violation.rs` was already handled correctly. That test is just for good measure. :)

I have a more involved plan to clean this up, but it ran into difficulties such as rust-lang#22814.
@kmcallister
Copy link
Contributor Author

My best plan at the moment is to change the parser so it never parses MatchNts, and then do a separate pass to create them from SubstNt Colon Ident just before parsing a macro_rules! LHS. It's a hack for sure. Really, we should have a separate matcher-side token tree type, or some higher level AST for matchers. But I don't want to make huge changes all over macro parsing and transcription.

(There is already some other code that decomposes a MatchNt into SubstNt Colon Ident under some circumstances.)

@kmcallister
Copy link
Contributor Author

The main parser won't even parse SubstNt at the moment, which is a known weirdness. Maybe that should be fixed along with this issue.

@kmcallister kmcallister assigned bstrie and unassigned kmcallister Mar 16, 2015
@kmcallister
Copy link
Contributor Author

@bstrie is going to look into this.

@pnkfelix
Copy link
Member

Provisionally reclassifying as 1.0.

@kmcallister is there a strong reason to put this back on the 1.0 beta milestone?

@bstrie is there a status update?

@pnkfelix pnkfelix modified the milestones: 1.0, 1.0 beta Mar 26, 2015
@kmcallister
Copy link
Contributor Author

I'm not sure if this is actually a back-compat hazard on master, or just an obstacle to the refactoring that I was pursuing. I'm going to look into this more in the near future.

I agree that we can put this off until 1.0-final.

@brson brson added the P-high High priority label Apr 29, 2015
@brson brson removed this from the 1.0 milestone Apr 29, 2015
@Stebalien
Copy link
Contributor

Triage: Can't reproduce. @kmcallister?

trait Test {}

macro_rules! test {
( $($name:ident)+) => (
    impl<$($name: Test),*> Test for ($($name,)*) {
    }
)
}

test!(A B C);

fn main() {}

@alexcrichton alexcrichton added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Sep 30, 2015
frewsxcv added a commit to frewsxcv/rust that referenced this issue Oct 12, 2015
Manishearth added a commit to Manishearth/rust that referenced this issue Oct 14, 2015
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, ..) E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. P-high High priority
Projects
None yet
Development

No branches or pull requests

6 participants