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

Do not accept interpolated types as traits in trait impls #48502

Closed
wants to merge 2 commits into from

Conversation

petrochenkov
Copy link
Contributor

Fixes #46438

Needs crater run before proceeding.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 24, 2018
@petrochenkov petrochenkov added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Feb 24, 2018
@pietroalbini pietroalbini removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2018
@nikomatsakis
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Feb 27, 2018

⌛ Trying commit ba61b745c7aa0f6d5c608b73e25e03aa8c061f95 with merge 0c6cdef8a4781d48cd2df0423ca893bf143bd5e2...

@bors
Copy link
Contributor

bors commented Feb 27, 2018

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

@nikomatsakis
Copy link
Contributor

cc @rust-lang/release -- needs crater

@Mark-Simulacrum
Copy link
Member

Crater run started.

@Mark-Simulacrum
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor Author

petrochenkov commented Mar 8, 2018

@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
Copy link
Contributor Author

petrochenkov commented Mar 8, 2018

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

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 8, 2018
if let Some(span) = ty_first_interpolated {
self.diagnostic()
.struct_span_warn(span, "expected a trait, found type")
.note("this warning will become a hard error in a future release")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sgrif
Copy link
Contributor

sgrif commented Mar 9, 2018

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
Copy link
Contributor

@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
Copy link
Contributor

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

To clean up, or not to clean up?

@nikomatsakis nikomatsakis added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2018
@eddyb
Copy link
Member

eddyb commented Mar 14, 2018

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

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Mar 15, 2018

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
Copy link
Member

eddyb commented Mar 15, 2018

@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
Copy link
Contributor

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
Copy link

rfcbot commented Mar 22, 2018

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

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.

@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 Mar 22, 2018
@nikomatsakis
Copy link
Contributor

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

@scottmcm
Copy link
Member

scottmcm commented Apr 5, 2018

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
Copy link

rfcbot commented Apr 5, 2018

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

@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 Apr 5, 2018
@bors
Copy link
Contributor

bors commented Apr 6, 2018

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

@nikomatsakis
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Member

eddyb commented Apr 11, 2018

@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
Copy link

rfcbot commented Apr 15, 2018

The final comment period is now complete.

@petrochenkov petrochenkov deleted the notytrait branch June 5, 2019 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. 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.

10 participants