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

ty macro arguments can be used as traits in impls #46438

Closed
petrochenkov opened this issue Dec 1, 2017 · 6 comments
Closed

ty macro arguments can be used as traits in impls #46438

petrochenkov opened this issue Dec 1, 2017 · 6 comments
Labels
A-grammar Area: The grammar of Rust A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. I-needs-decision Issue: In need of a decision. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@petrochenkov
Copy link
Contributor

petrochenkov commented Dec 1, 2017

macro_rules! m {
    ($my_type: ty) => {
        impl $my_type for u8 {} // Hmmm.

        // Not OK, this trick can't be used in any other position.
        // trait Z: $my_type {}
    }
}

trait Tr {}
m!(Tr); // OK

fn main() {}

This is a side effect of how impls are parsed - the trait in impl Trait for Type is first parsed as a type, and then reinterpreted as a type if it has appropriate syntactic form (trait syntax is a subset of type syntax).
(Same trick was used for pub(path) visibilities for some time, but then abandoned in favor of pub(in path).)

The reinterpretation trick in impl parsing can hardly be avoided, but this specific issue can be fixed because reinterpretation can be avoided if we see a ty fragment after impl.

The question is whether we should fix it or not - the issue seems benign even if it's probably unintended, an error is immediately reported if ty doesn't fit into the trait syntax.
This trick is also similar to other cases like limited use of expr fragment in range patterns (#42886), which are intended.
I would also be surprised if this was not exploited in practice.

@petrochenkov petrochenkov added A-grammar Area: The grammar of Rust A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) I-needs-decision Issue: In need of a decision. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Dec 1, 2017
@petrochenkov
Copy link
Contributor Author

cc @durka because macro tricks

@durka
Copy link
Contributor

durka commented Dec 2, 2017

I would also be surprised if this was not exploited in practice.

I agree :)

It seems kinda harmless? It would be nice to have a :trait matcher anyway... it's annoying that there's nothing you can use except $bound:ident/$($bound:tt)* if you want to output <T: $bound> or trait Tr: $bound {}.

@durka
Copy link
Contributor

durka commented Dec 2, 2017

In the compiler:

$ rg 'impl \$[^ ]+ for' -trust src
src/libcore/slice/mod.rs
2594:            impl $traitname for $ty { }

src/test/run-pass/hygiene/trait_items.rs
20:    impl $T for () {}

These are idents though.

@durka
Copy link
Contributor

durka commented Dec 2, 2017

In the wild: (just grepping my cargo cache for impl \$[a-zA-Z0-9_]+ for)

@ExpHP
Copy link
Contributor

ExpHP commented Dec 5, 2017

Anecdotally, I've done this in my own code:

(impl$par:tt [$($Trait:ty),+] for $Ty:ty as $Wrap:ident $bnd:tt)

(though I only found this one instance hiding in a couple years worth of code)

bors added a commit that referenced this issue Jan 14, 2018
syntax: Rewrite parsing of impls

Properly parse impls for the never type `!`
Recover from missing `for` in `impl Trait for Type`
Prohibit inherent default impls and default impls of auto traits (#37653 (comment), #37653 (comment))
Change wording in more diagnostics to use "auto traits"
Fix some spans in diagnostics
Some other minor code cleanups in the parser
Disambiguate generics and qualified paths in impls (parse `impl <Type as Trait>::AssocTy { ... }`)
Replace the future-compatibility hack from #38268 with actually parsing generic parameters
Add a test for #46438
@XAMPPRocky XAMPPRocky added the C-bug Category: This is a bug. label Feb 26, 2018
@petrochenkov
Copy link
Contributor Author

The decision in was to accept this code and maybe pursue substitution of macro arguments as "untyped" token trees in general.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-grammar Area: The grammar of Rust A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. I-needs-decision Issue: In need of a decision. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants