[WIP] Warn when $name:matcher syntax is used on expansion side

[krdln]

This pull request is in obviously unfinished state (missing tests, etc.), but I wanted to publish it anyway to ask whether you think this kind of warning is a good idea in the first place.

I do often make mistake by unnecessarily adding :tt-like annotations on the expansion side, which may result in weird errors in unrelated places. That's why I wanted to take a stab at creating warning for such a case. Here's an example how the warning looks like.

Unresolved questions

1. Does this warning is worth creating at all?
2. Should we warn on any $foo:bar or only when bar is an actual tt, expr etc.?
3. Is the .clone() in the implementation lightweight enough?
4. Should the warning be registered, so one can allow it?
5. The exact warning/note message.

cc @estebank

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

[nikomatsakis]

I think I've made this mistake from time to time.

[estebank]

It's even specifically called out in the Programming Rust book. I'd be on favor of this lint, specially given the current error.

[estebank]

Does this warning is worth creating at all?

I think so.

Should we warn on any $foo:bar or only when bar is an actual tt, expr etc.?

I think we should be supplying the warning only when not only it is a valid macro argument matching type, but also the same matching type for the argument.

Is the .clone() in the implementation lightweight enough?

Given that you're cloning an Iterator it should be cheap enough to do liberally.

Should the warning be registered, so one can allow it?

Probably, although I am happy with gating the warning on adding a space.

The exact warning/note message.

Lets think about the wording. I think there's some improvement possible.

Also, add a test for this.

[durka]

Should the warning be registered, so one can allow it?

All warnings should, IMO. One situation where you might have to allow this warning, depending on how it's implemented, is for a macro-writing macro, e.g.

macro_rules! foo {
    ($dol:tt $a:ident) => {
        macro_rules! bar {
            ($dol$a:ident) => { $dol$a }
        }
    }
}

The exact warning/note message.

I think your current text is rather cryptic. As an aside, terminology for the parts of a macro is wildly inconsistent. The Reference seems to use "matcher" and "transcriber" to mean the entire left and right sides of a macro, respectively. It uses "designator" for the :tt part (what you called a "matcher"). Rust By Example also uses "designator". Meanwhile, the Book calls it a "fragment specifier". TLBoRM uses "pattern", "expansion", and "kind of capture".

How about this:

warning: macro expansion includes fragment specifier
 --> confusing-macro.rs:2:22
  |
2 |     ($tt:tt) => { $tt:tt }
  |                      ^^^
  |
  = help: did you mean `$tt`?
  = help: to suppress this warning, add a space: `$tt: tt`

This is still cryptic IMO. But it'd be better if we picked one of the terms "designator"/"fragment specifier" and went with that everywhere.

[nikomatsakis]

r? @estebank

[nikomatsakis]

(Just lightening my review load, @estebank if that doesn't work for you, feel free to reassign back.)

[estebank]

Nitpick: use span_suggestion instead of notes:

warning: macro expansion includes fragment specifier
 --> confusing-macro.rs:2:22
  |
2 |     ($tt:tt) => { $tt:tt }
  |                      ^^^
help: if you meant to use the macro argument, do not include the fragment specifier:
  |
2 |     ($tt:tt) => { $tt }
  |                   ^^^
help: to suppress this warning, add a space after the type argument:
  |
2 |     ($tt:tt) => { $tt: tt }
  |                   ^^^^^^^

[pietroalbini]

Hi @krdln, there are some failing tests on Travis and a comment from the reviewer in #47967 (comment). Could you address those so the PR can be merged? Thanks!

[krdln]

@pietroalbini I'd be more than happy to finish this PR – I'll do it as soon as I have some free time, hopefully it'll be sometime this week.

[pietroalbini]

Weekly ping for you @krdln!

[krdln]

@durka

Should the warning be registered, so one can allow it?

All warnings should, IMO.

I was trying to convert this warning to a lint, but I'm a little stuck here. The perfect method I could use would be struct_span_lint, but it's not available on Handler. What would be appropriate way to emit this warning as a lint? Should I approach this similarly to MISSING_FRAGMENT_SPECIFIER, which is somehow buffered and emitted in librustc_driver/driver.rs? (I feel like driver would be a weird place for such a warning)

[krdln]

@estebank I've reworded the error message according to your suggestion (also, thanks for pointing me to span_suggestion!) and added a test. Not sure what the appropriate place for a test would be, so for now, i've put it directly in ui.

I think we should be supplying the warning only when not only it is a valid macro argument matching type, but also the same matching type for the argument.

I've decided to just check whether it is a valid fragment specifier. My rationale for not checking whether it's the same type as argument (apart for implementation complexity) is that it wouldn't let us detect the mistake when the user changed the kind only on one side, or they tried to use the : syntax to eg. cast the expr back to tt.

I've tried to make this warning a proper lint, but I'm not sure how to approach it (see the comment above), so for now I've left it as it was.

[shepmaster]

Ping from triage, @estebank !

[estebank]

I've decided to just check whether it is a valid fragment specifier.

Could you add a test with @durka's macro usage to see the way this would work in that case?

@durka I'm ok with the solution presented in this PR. Do you have an objection with the current safeguards (checking for valid specifiers, allowing using a space after the colon to avoid the warning)?

[durka]

I'd prefer #[allow(fragment_specifier_in_expansion)], but adding a space is fine -- just seems slightly hacky.

[krdln]

@durka – as I said above I'd be happy to implement #[allow(fragment_specifier_in_expansion)], but got stuck, and I'll need a little guidance (or we can leave the hacky solution)

@estebank Regarding @durka's macro, I'm not sure what you mean – do you mean just to just copy this macro and test that it indeed incorrectly warns? Or do you mean to test the "silencing by space"?

[estebank]

@krdln I only just now noticed the reason that sample is problematic. I don't think we should merge before we have a way to enable this selectively or to have this not trigger in the nested macro case. The simplest will be to add #[allow(fragment_specifier_in_expansion)]. I'll try to make time to help you there this weekend.

[shepmaster]

Ping from triage, @krdln ! It looks like you got some feedback on your questions; will you be able to apply that feedback soon?

[bors]

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

[shepmaster]

Since we haven't heard from you in a few weeks, I'm going to go ahead and close this to keep things tidy. If you have time to resume work on this, please feel free to do so and reopen this pull request! Thank you for your hard work!