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

Add expr202x macro pattern #84364

Closed
wants to merge 1 commit into from
Closed

Add expr202x macro pattern #84364

wants to merge 1 commit into from

Conversation

lf-
Copy link
Contributor

@lf- lf- commented Apr 20, 2021

This makes it possible to use inline_const (#76001) and let_chains
(#53667) inside macros' expr patterns in a future edition by
bifurcating the expr nonterminal in a similar way to pat2021 to
remove some backwards compatibility exceptions that disallow
const/let at the beginning of an expr match.

Fixes #84155 and relaxes the backward compat restriction from #80135 for
a future edition. This is not necessarily intended to go into 2021 as it I don't
think it's simple to write an automatic fix, and I definitely won't have time very
soon.

Here is a pathological case of #79908 that forces this to be an edition
change:

macro_rules! evil {
    ($e:expr) => {
        // or something else
        const {$e-1}
    };
    (const $b:block) => {
        const {$b}
    }
}
fn main() {
    let x = 5;
    match x {
        evil!(const { 5 }) => panic!("oh no"),
        _ => (),
    };
}

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 20, 2021
@rust-log-analyzer

This comment has been minimized.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 20, 2021

cc @rust-lang/project-edition-2021

@nikomatsakis
Copy link
Contributor

Hmm, I'm in favor of the concept overall, but I'd like to have a bit of a "write-up" or something that brings together the required information. I don't quite remember, for example, what changes were required to the expr non-terminal and why.

@lf-
Copy link
Contributor Author

lf- commented Apr 20, 2021

Hmm, I'm in favor of the concept overall, but I'd like to have a bit of a "write-up" or something that brings together the required information. I don't quite remember, for example, what changes were required to the expr non-terminal and why.

Alright, that's entirely understandable! So it looks like the back compat thing for let chaining came from this comment discussion: #60861 (comment) and an associated fixme in the feature gate ui test introduced by that pr which passes here because this pr doesn't inflict the new behaviour on any edition yet. In other words, this one is here because of conservatism. But we can probably assume that someone out there has written a macro that depends on the exception in the current expression behaviour based on the next one's experience:

The other exception for inline const was because we got bitten by the issue that was proposed in the above discussion, but in the context of inline const. This exception got in because of #80135: it was needed to keep some macros on beta working, which expected the const keyword then some expression after, as I understand it based on my testing and reading examples.

The former exception was removed here since I was bifurcating anyway and could reserve it. The latter was because of inline const needing to have parens around it to be valid in macro invocations currently, which is odd and rather confusing (I didn't know about this trick to get it accepted by the parser until it was pointed out to me by a friend so I assumed it simply wasn't allowed. the error here is bad too). Macros taking :expr seem liable to hit an issue shaped like this whenever a syntax extension to expressions happens, as a form of inherent fragility, so it's probable some more exceptions like these might get introduced as extensions get added.

Does this need to happen now? Not strictly. It could happen later or never, at the expense of macros being a little more confusing: This PR was written while I was under the invalid impression you couldn't pass a const block to a macro as an expression, which I now know is possible with parens, but that fact was anything but discoverable. It would be good to resolve the syntax ambiguity to favour consistency in being able to put const { ...} anywhere an expression could go, including in macros, imo. I'm not super familiar with let chaining or how this applies in its case, although I'd be inclined to believe that a similar argument could be made.

@nikomatsakis
Copy link
Contributor

I see. So the expr macro nonterminal is starting to diverge from the full set of expressions, is the point, in some specific cases (the ones you identified). Hmm!

@nikomatsakis nikomatsakis added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Apr 21, 2021
@nikomatsakis
Copy link
Contributor

Nominated for @rust-lang/lang discussion.

@estebank estebank added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Apr 24, 2021
@bors
Copy link
Contributor

bors commented Apr 25, 2021

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

This makes it possible to use `inline_const` (rust-lang#76001) and `let_chains`
(rust-lang#53667) inside macros' `expr` patterns in a future edition by
bifurcating the `expr` nonterminal in a similar way to `pat2021` to
remove some backwards compatibility exceptions that disallow
`const`/`let` at the beginning of an `expr` match.

Fixes rust-lang#84155 and relaxes the backward compat restriction from rust-lang#80135 for
a future edition. This is not intended to go into 2021 as it I don't
think it's simple to write an automatic fix, and certainly not now that
it's past the soft deadline for inclusion in 2021 by a long shot.

Here is a pathological case of rust-lang#79908 that forces this to be an edition
change:

```rust
macro_rules! evil {
    ($e:expr) => {
        // or something else
        const {$e-1}
    };
    (const $b:block) => {
        const {$b}
    }
}
fn main() {
    let x = 5;
    match x {
        evil!(const { 5 }) => panic!("oh no"),
        _ => (),
    };
}
```
@lf-
Copy link
Contributor Author

lf- commented Apr 26, 2021

force push: Fixed the merge conflict and also the feature gate using the wrong suggestion

@bors
Copy link
Contributor

bors commented Apr 28, 2021

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

@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels May 14, 2021
@crlf0710 crlf0710 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 5, 2021
@Mark-Simulacrum Mark-Simulacrum removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 22, 2021
@pnkfelix
Copy link
Member

Opened issue #86730 for lang team to discuss how we want to resolve this, as discussed in the 2021-05-11 lang team meeting (minutes, youtube).

@pnkfelix
Copy link
Member

I believe the conclusion of the lang team is that we are not confident that the approach used in this PR is the exact right one we want to deploy, and so I propose we close this PR while we wait to resolve the questions raised in #86730.

@pnkfelix
Copy link
Member

@rfcbot close

@rfcbot
Copy link

rfcbot commented Jun 29, 2021

Team member @pnkfelix has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jun 29, 2021
@rfcbot rfcbot added the disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. label Jun 29, 2021
@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jul 6, 2021
@rfcbot
Copy link

rfcbot commented Jul 6, 2021

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

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 16, 2021
@rfcbot
Copy link

rfcbot commented Jul 16, 2021

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jul 16, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 5, 2021
@crlf0710
Copy link
Member

Closing according to the fcp decision above.

@crlf0710 crlf0710 closed this Sep 10, 2021
@pnkfelix
Copy link
Member

as noted by @traviscross elsewhere: Further action here is now gated on acceptance of rust-lang/rfcs#3531, "Macro matcher fragment specifiers edition policy".

@RalfJung
Copy link
Member

RalfJung commented Apr 17, 2024

That RFC has been accepted, so if someone wants to make progress here, you can help by picking up this PR, making sure it matches the RFC, and making it work with today's compiler. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). 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.

inline-const is not an expr in macros, but it should be