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

Accept a LHS formed of a single sequence TT in macro_rules!. #33689

Closed
wants to merge 1 commit into from

Conversation

LeoTestard
Copy link
Contributor

This is already accepted when checking that a macro definition is well-formed but not handled during expansion.

There is no reason to reject that since a sequence TT is a single TT.

This does not apply to macro RHSes, though, which must still be formed of a delimited TT. It would be cool to have it too, for consistency, but the purpose of this PR was to fix the inconsistency between macro definition checking and macro expansion, which was obviously a bug. Since a single sequence TT RHS is rejected in both places, I think it would be better to do a separate PR.

@rust-highfive
Copy link
Collaborator

r? @sfackler

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

@sfackler
Copy link
Member

I always thought the lhs of a macro had to be wrapped in (), [] etc - thoughts @nrc?

@LeoTestard
Copy link
Contributor Author

That's not what check_lhs_nt_follow says:

    match lhs {
        &TokenTree::Delimited(_, ref tts) => {
            check_matcher(cx, &tts.tts);
        },
        tt @ &TokenTree::Sequence(..) => {
            check_matcher(cx, ref_slice(tt));
        },
        _ => cx.span_err(sp, "invalid macro matcher; matchers must be contained \
                              in balanced delimiters or a repetition indicator")
    };

This was already accepted when checking that a macro definition was well-formed but not handled during expansion.
@LeoTestard
Copy link
Contributor Author

Made it a bit clearer (and closer to the old code and to check_lhs_nt_follow) by using ref_slice.

@nrc nrc added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-nominated labels May 18, 2016
@nrc
Copy link
Member

nrc commented May 18, 2016

Nominating for lang team discussion. This does effectively make the macro syntax more permissive. I am somewhat against this idea since I would like to link the kind of bracket in the lhs with the bracket used in the macro use (in macros 2.0) and this change drifts away from that, rather than closer to it.

Is there an advantage to allowing this syntax? It doesn't read any better to my eyes, although I guess it is closer to match arms and if statements where we don't require delimiters. I think I'd be happier if we could only do this, rather than making them optional.

@LeoTestard
Copy link
Contributor Author

@nrc I'm not sure what you mean by just don't requiring delimiters. Because it would be unambiguous only when the matcher consists of a single TT. Otherwise it's not possible to match the => token outside delimiters, for example. We could ask to provide delimiters if needed (i.e. if the matcher consists of several TT) but it would be counter-intuitive, since explicitly putting delimiters inside a matcher normally make the macro actually match the delimiters and it would not be the case when puttin delimiters around the macro for the sole purpose of disambiguating, I think.

I guess we can also just forbid that and just fix the macro rules compilation to report it when a macro is declared rather than on expansion...

@sanxiyn
Copy link
Member

sanxiyn commented May 18, 2016

Travis failed compile-fail/malformed_macro_lhs.rs, which should be just deleted if this PR is to be accepted.

@LeoTestard
Copy link
Contributor Author

Yep, I saw. Waiting for the lang team decision to fix it.

@nikomatsakis
Copy link
Contributor

Leaving this nominated since @nrc wasn't present at the last @rust-lang/lang meeting, and we wanted to get his take before we decided on anything.

@LeoTestard
Copy link
Contributor Author

Talked about it with @pnkfelix today. We agreed that the best solution was probably to close this PR and to instead fix the macro definition checking phase to reject such macros.

@LeoTestard
Copy link
Contributor Author

Closing since this is not what we want. I'll open another PR.

@LeoTestard LeoTestard closed this May 23, 2016
@LeoTestard LeoTestard deleted the macro-sequence-lhs branch May 23, 2016 10:12
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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants