Lint on incorrect unseparated repetition

[estebank]

Macro pattern ($a:expr $b:expr) is invalid, for good reason.
Correctly reject pattern ($($a:expr)*), as it is functionally the
same.

This PR adds a warn by default lint that checks for any repeatable
pattern that would be rejected by the parser if it had been written
as independent arguments:

• expr and stmt variables may only be followed by one of:
  => , ;
• ty and path variables may only be followed by one of:
  => , = | ; : > [ { as where
• pat variables may only be followed by one of:
  => , = | if in
• Other variables may be followed by any token.

Fix #44975, fix #48220.

[rust-highfive]

r? @sfackler

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

[estebank]

@bors try

[bors]

⌛ Trying commit 5c5fca01e7afa4060e1d5910dc259db470afb4ed with merge a7b077cffd907eec053b7d79dbe723dd94548c08...

[estebank]

cc @nikomatsakis @durka @joshtriplett

---

I'm afraid fixing this is going to be problematic, because even we are using this "feature" in rustc. I'll post a PR with the fix (including changing our mis-uses), but will expect there to be breakage. If that is the case after a crater run, should we still include the fix as a warning?

This will need a crater run to see how much this would break.

[estebank]

@nikomatsakis I fully expect this change to wreak havoc. I wouldn't want to make it a lint because the logic to accomplish that would be much more involved than this change. Is it ok for us to start generating an unsilenceable warning all of a sudden?

[estebank]

cc @kennytm

[estebank]

@dtolnay added deprecation message, changed to allow by default and rebased.

[petrochenkov]

Sigh, the following token restrictions exist to prevent breakage from extending Rust syntax, but this change alone breaks more code than syntax extensions ever did.

[petrochenkov]

It looks like the warning may be reported both at definition crate (when the macro is defined) and at use crate when the macro is loaded?
Could you make sure this doesn't happen and add a test?
(The warning needs to be reported only when macro_rules::compile is called from define_macro.)

[petrochenkov]

Also a test making sure that #[allow(incorrect_macro_fragment_repetition)] on the macro item silences the warning for both local crate and clients.

[petrochenkov]

Otherwise LGTM.

[withoutboats]

We discussed this in the lang team meeting today. Overall, we're uncertain that this is worth the churn it will cause. The point of these restrictions is to avoid a lot of breakage when we do extend the expression syntax: are we just frontloading that breakage by adopting this change, defeating the point of the restriction in the first place?

Contrarily, one could argue that if we do ever extend expressions in the future, after Rust's adoption has continued to grow, we might end up breaking a lot more people than this lint will cause problems for.

But overall we were uncertain, so we're throwing it back to the people invested in the PR: this change is definitely correct, but is it worth the breakage to become more correct? Why or why not?

[estebank]

Contrarily, one could argue that if we do ever extend expressions in the future, after Rust's adoption has continued to grow, we might end up breaking a lot more people than this lint will cause problems for.

This is exactly the camp where I would fall on. Getting a lint in at the earliest possible moment, even if it is disabled by default for several releases and warn only until the next edition makes it much more likely that breaking this in 2021 will be possible.

this change is definitely correct, but is it worth the breakage to become more correct? Why or why not?

I feel that merging as an allow by default lint is the clear right choice. That way projects that care about it can enable it selectively or opt out of it completely (indefinitely). After that we can work with crates to move away from exploiting this loophole. I don't think this lint should be deny-by-default unless it is part of a new edition.

Alternatively, we could explicitly allow some particular use cases, like repeated bare types and literals in expr arguments (enabling the common use in rustc), while disallowing/warning of plainly incorrect usage on use, not definition like we do here...

[bors]

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

[XAMPPRocky]

Triage; @estebank Hello, have you been able to get back to this PR?

[estebank]

@Aaronepower I'll clean this up this weekend.

[TimNN]

Ping from triage @estebank: What is the status of this PR?

[estebank]

@TimNN I have bene quite busy and unable to get back to this, but will try to do so soonish. If you wish to close in the meantime, go ahead and I'll reopen once I'm ready.

Alternatively, we could explicitly allow some particular use cases, like repeated bare types and literals in expr arguments (enabling the common use in rustc), while disallowing/warning of plainly incorrect usage on use, not definition like we do here...

Does anyone have thoughts on this point?

[TimNN]

If you wish to close in the meantime, go ahead and I'll reopen once I'm ready.

Thank you for your understanding and contributions!