Do not accept interpolated types as traits in trait impls

[petrochenkov]

Fixes #46438

Needs crater run before proceeding.

[rust-highfive]

r? @nikomatsakis

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

[nikomatsakis]

@bors try

[bors]

⌛ Trying commit ba61b745c7aa0f6d5c608b73e25e03aa8c061f95 with merge 0c6cdef8a4781d48cd2df0423ca893bf143bd5e2...

[bors]

☀️ Test successful - status-travis
State: approved= try=True

[nikomatsakis]

cc @rust-lang/release -- needs crater

[Mark-Simulacrum]

Crater run started.

[Mark-Simulacrum]

Crater results: http://cargobomb-reports.s3.amazonaws.com/pr-48052/index.html. 'Blacklisted' crates (spurious failures etc) can be found
here.
If you see any spurious failures not on the list, please make a PR against that file.

[petrochenkov]

6 legit regressions:

• downcast-rs-1.0.0
• faster-0.4.3
• glm-0.2.3
• glm_color-0.1.2
• guilt-by-association-0.4.1
• sql-0.4.2

I think we can deprecate this.
I'll change the error into a warning (no lints in the parser unfortunately), add a help message suggesting to replace ty with path and send PRs to affected crates.

[Mark-Simulacrum]

Reverse deps for breakage: https://gist.github.com/Mark-Simulacrum/e06765d8cd4a324da9fd47e4acf3fce7. I agree that it seems reasonable to deprecate this -- very few crates in the ecosystem are broken by this.

[nikomatsakis]

Hmm, I'm of two minds here. To be clear, we are specifically talking about deprecating this behavior, right?

macro_rules! foo {
  ($t:ty) => {
    impl $t for ... { }
  }
}

This would be deprecated because $t is a "type", not a trait. I agree with that distinction and I do think this is roughly consistent with how we do things in macros. I just don't like how macros do things.

In particular, I don't like that it matters what kind of fragment you use -- and hence that $t is (internally) replaced with a "special type token" that can only appear in type position and so forth. I find this kind of makes macros a pain in the neck and breaks my mental model pretty regularly (which is basically one of token tree interpolation).

OTOH, I'm not sure if we have any room to change this until a hypothetical Macros 2.0, and I do feel like -- given that we have type tokens -- it is sort of "wrong" to use them in trait position. And not that many crates are affected.

OTOOH, it does work today, and it seems like the kind of kludge we could keep working. @petrochenkov can you elaborate a bit more on the motivation here?

[petrochenkov]

@nikomatsakis

given that we have type tokens -- it is sort of "wrong" to use them in trait position.

That's basically the motivation.
ty is never accepted in trait position except for this specific case caused by unintentional leakage of implementation details.
The less weird stuff is in the language the better.

Since this does cause some breakage I don't think it's reasonable to make this an error, but we can warn to prevent this being used in new code.

[petrochenkov]

This PR is updated (error -> warning), PRs to affected crates are sent.

[nikomatsakis]

Is there a reason we can't use the standard lint infrastructure?

[petrochenkov]

It's too early in the parser, we don't even have NodeIds yet to attach the lint.

[nikomatsakis]

OK. Before we merge, let's try to follow the other steps though, i.e., create a tracking issue and so forth.

[sgrif]

Just out of curiosity, why is path considered more appropriate here than ty? path also allows all sorts of things that aren't traits (I guess path is a strict subset of ty, but still...)

[nikomatsakis]

@sgrif

Just out of curiosity, why is path considered more appropriate here than ty? path also allows all sorts of things that aren't traits (I guess path is a strict subset of ty, but still...)

A fair question. Using "type" here feels sort of like a "category error" -- that is, the idea is roughly that when you parse a trait as a type, you convert it into dyn Trait, so this is "as wrong" as writing impl dyn Debug for Foo.

(That said, I still feel like macros should behave as if they were more-or-less copying tokens to the extent possible, in which case the actual fragment you use wouldn't matter.)

[nikomatsakis]

Nominating for discussing in @rust-lang/lang meeting.

To clean up, or not to clean up?

[eddyb]

I prefer the fragment distinction to be only used for matching and erased when expanding.
cc @jseyfried

[petrochenkov]

That would be a pretty serious shift from the current scheme though. We should probably track it as a separate issue as part of macros 2.0 experimentation.
(You'd have to either re-tokenize everything parsed by macro matchers like ty back from AST, or somehow throw away the AST produced during parsing/matching and reuse the orinal tokens.)
(Anyway, this PR operates under assumptions of the current rules.)

[eddyb]

@petrochenkov We already keep the original tokens for at least items, I believe.
Although I supposed we haven't fully moved in the direction of caching parse results.

[nikomatsakis]

We discussed this in the @rust-lang/lang meeting today. I believe the consensus (correct me if I am mistaken) was that it generally makes sense to move towards a future where the precise fragment specifier that you used doesn't matter so much: so e.g. writing $t:ty doesn't mean you can't use those same tokens later as something else. We probably can't actually make macro rules behave that way entirely -- that would have to wait until Macros 2.0 -- but we don't have to close off cases where it does work today.

(Note though: we should not accept dyn Trait in these positions. =)

(As a side argument, it strikes me that -- for the same reason that we accept this in the parser -- it's perhaps convenient for macro authors to be able to use ty as a "cover grammar" for types and traits.)

Moreover, on the other side of the scale, it doesn't seem like this particularly simplifies implementation (more the opposite: we need an extra check), nor enables any future uses.

Therefore I move to close. (But feel free @petrochenkov or others to raise counterarguments.)

@rfcbot fcp close

[rfcbot]

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

• @aturon
• @cramertj
• @eddyb
• @joshtriplett
• @nikomatsakis
• @nrc
• @pnkfelix
• @scottmcm
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

[nikomatsakis]

ping @rust-lang/lang -- FCP ticky marks await you!

[scottmcm]

Given the crazy things that are possible with macro_rules, I have no problem with close here.

That said, I'd love some kind of a "what do capture kinds even mean" intent document for macros 2.0, because it still feels really weird to me that you can use a capture as something totally different from what you said it was in the capture kind. (If it was just token based that would make sense, but iirc it's not in general...)

[rfcbot]

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

[bors]

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

[nikomatsakis]

@scottmcm

That said, I'd love some kind of a "what do capture kinds even mean" intent document for macros 2.0

Agreed -- macros 2.0 definitely needs more definition, it's in a bit of "see what you want to see" situation.

(If it was just token based that would make sense, but iirc it's not in general...)

As I said in the meeting, I definitely feel like we should try to ensure you can think of them as token trees to the extent possible (modulo expression precedence, as discussed in ... some RFC or other). That is, $lhs * $rhs should be equivalent to ($lhs) * ($rhs), even when e.g. lhs = x + y. =)

[scottmcm]

@nikomatsakis Well, I just don't understand where the "token tree interpretation" starts and ends. Like I know I can use $my_ident Foo {}; to declare a struct, which is very token interpretation, but then I try to do the following and all of a sudden I can't use my expressions as tokens? (But the macro itself is legal?)

macro_rules! wha {
    ($lhs:expr ; $rhs:expr) => ($lhs $rhs)
    //~^ ERROR macro expansion ignores token `*3` and any following
}
fn main() {
    let z = wha!( 4 ; * 3 );
    println!("{}", z);
}

(And wait, "*3" is a token?)

This thread certainly isn't the place to resolve or document that; I just hope macros 2.0 will be clearer 🙂

[eddyb]

@scottmcm I think that's consistent, in that you don't have tokens but one token ("tree") grouping together several tokens. That is, you can't take apart a fragment or "insert" anything into it.

An interesting example that I would hope to work in the future, is taking (A, B) as an expression, and putting it into a type position, having it reparsed as a type.
OTOH, I don't want a captured X<Y> (as a type) allowed in expression position followed by another expression, because that would reinterpret the second >.

I'm not sure how to formalize this, but one way to put it is that "syntactical elements used in the same level by the AST cannot come from different 'captured bags' of tokens".

This would imply that e.g. an expression followed by a "captured bag" of more than one token can never parse (although maybe some people would want foo $tuple to be a function call?).

[rfcbot]

The final comment period is now complete.