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

Let qpath contain NtTy: <$:ty as $:ty>::… #91150

Merged
merged 4 commits into from
Jan 19, 2022
Merged

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Nov 23, 2021

Example:

macro_rules! m {
    (<$type:ty as $trait:ty>::$name:ident) => {
        <$type as $trait>::$name
    };
}

fn main() {
    let _: m!(<str as ToOwned>::Owned);
}

Previous behavior:

error: expected identifier, found `ToOwned`
 --> src/main.rs:3:19
  |
3 |         <$type as $trait>::$name
  |                   ^^^^^^ expected identifier
...
8 |     let _: m!(<str as ToOwned>::Owned);
  |            ---------------------------
  |            |
  |            this macro call doesn't expand to a type
  |            in this macro invocation

The expected identifier, found `ToOwned` error is particularly silly. I think it should be fine to accept this code as long as $trait is of the form TyKind::Path(None, path); if it is any other kind of NtTy, we'll keep the same behavior as before.

@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(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 Nov 23, 2021
@camelid camelid added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 25, 2021
@bors
Copy link
Contributor

bors commented Nov 25, 2021

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

Currently fails:

    error: expected identifier, found `ToOwned`
      --> src/test/ui/macros/macro-interpolation.rs:23:19
       |
    LL |         <$type as $trait>::$name
       |                   ^^^^^^ expected identifier
    ...
    LL |     let _: qpath!(ty, <str as ToOwned>::Owned);
       |            -----------------------------------
       |            |
       |            this macro call doesn't expand to a type
       |            in this macro invocation
@dtolnay
Copy link
Member Author

dtolnay commented Nov 25, 2021

@petrochenkov
Copy link
Contributor

This is very similar to #91166 (comment) (see it for some more details).

There's a definitive method to determine whether something interpolated should parse or not - checking how it would work in token-based expansion model (aka "how it would work in proc macro output"), as opposed to AST-based model that is currently used for implementing $interpolated fragments internally.

<$type as $trait>::$name in the example above will expand to <( str ) as ( ToOwned )>::Owned.
Should it parse? No, parentheses are not allowed inside a path.
So this change cannot go alone without some accompanying changes to $ty wrapping or path parsing.

Also see #67062 for the current issues with parsing Delimiter::Nones.

@dtolnay
Copy link
Member Author

dtolnay commented Dec 4, 2021

@petrochenkov:

<$type as $trait>::$name in the example above will expand to <( str ) as ( ToOwned )>::Owned.
Should it parse? No, parentheses are not allowed inside a path.

The following (where the rhs of as is $:path rather than $:ty) has worked all the way since Rust 1.0.0:

macro_rules! m {
    (<$t:ty as $p:path>::$name:ident) => {
        type $name = <$t as $p>::$name;
    };
}

m!(<str as ToOwned>::Owned);

so the path parsing already supports the "parentheses" in that position.

It's also supported in proc macro output already, for as long as Delimiter::None has existed (since Rust 1.29.0):

use proc_macro::{Delimiter, Group, Ident, Punct, Spacing, Span, TokenStream, TokenTree};

#[proc_macro]
pub fn repro(_input: TokenStream) -> TokenStream {
    // type T = <str as ⟪ToOwned⟫>::Owned;
    TokenStream::from_iter([
        TokenTree::Ident(Ident::new("type", Span::call_site())),
        TokenTree::Ident(Ident::new("T", Span::call_site())),
        TokenTree::Punct(Punct::new('=', Spacing::Alone)),
        TokenTree::Punct(Punct::new('<', Spacing::Alone)),
        TokenTree::Ident(Ident::new("str", Span::call_site())),
        TokenTree::Ident(Ident::new("as", Span::call_site())),
        TokenTree::Group(Group::new(Delimiter::None, TokenStream::from_iter([
            TokenTree::Ident(Ident::new("ToOwned", Span::call_site()))
        ]))),
        TokenTree::Punct(Punct::new('>', Spacing::Alone)),
        TokenTree::Punct(Punct::new(':', Spacing::Joint)),
        TokenTree::Punct(Punct::new(':', Spacing::Alone)),
        TokenTree::Ident(Ident::new("Owned", Span::call_site())),
        TokenTree::Punct(Punct::new(';', Spacing::Alone)),
    ])
}

So this change cannot go alone without some accompanying changes to $ty wrapping or path parsing.

Given the above, I can't tell what other change you have in mind by this.

@petrochenkov
Copy link
Contributor

Ah, ok, if $path is already supported then no additional changes are required.

It's also supported in proc macro output already, for as long as Delimiter::None has existed

The current parser currently eliminates all Delimiter::Nones before parsing (and it's a source of bugs, #67062), so it doesn't mean much.

@wesleywiser
Copy link
Member

r? rust-lang/compiler-team

@wesleywiser
Copy link
Member

r? compiler-team

@Mark-Simulacrum
Copy link
Member

r? compiler-team

(trying to trigger some logs for this case)

@davidtwco
Copy link
Member

Apologies for the time this has taken to be reviewed.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 16, 2022

📌 Commit 87a7def has been approved by davidtwco

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2022
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#90782 (Implement raw-dylib support for windows-gnu)
 - rust-lang#91150 (Let qpath contain NtTy: `<$:ty as $:ty>::…`)
 - rust-lang#92425 (Improve SIMD casts)
 - rust-lang#92692 (Simplify and unify rustdoc sidebar styles)
 - rust-lang#92780 (Directly use ConstValue for single literals in blocks)
 - rust-lang#92924 (Delete pretty printer tracing)
 - rust-lang#93018 (Remove some unused `Ord` derives based on `Span`)
 - rust-lang#93026 (fix typo in `max` description for f32/f64)
 - rust-lang#93035 (Fix stdarch submodule pointing to commit outside tree)

Failed merges:

 - rust-lang#92861 (Rustdoc mobile: put out-of-band info on its own line)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f372476 into rust-lang:master Jan 19, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 19, 2022
@dtolnay dtolnay deleted the qpath branch January 19, 2022 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants