Skip to content

Conversation

SpriteOvO
Copy link
Contributor

@SpriteOvO SpriteOvO commented Dec 23, 2024

Tracking issues: #130977, #54722

This PR changed proc_macro::quote! to use ToTokens trait instead of TokenStream::from, and migrated test cases from quote crate.

r? @dtolnay
CC @tgross35

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 23, 2024
@rust-log-analyzer

This comment has been minimized.

@SpriteOvO SpriteOvO force-pushed the proc-macro-use-to-tokens-in-quote branch from 519aec5 to 0b32dec Compare December 23, 2024 18:02
@tgross35
Copy link
Contributor

Thanks for working on this!

Could you update the new test file with applicable tests from https://github.com/dtolnay/quote/blob/aafba72e10919ad47c05169271cb78e614fb2b9d/tests/test.rs? Because of token stream limitations we can't actually mark them #[test] or add the assert! (eventually we will, this should be a FIXME), but keeping the quote! invocations at least allows us to verify that the expected things do quote.

We can probably reuse the UI tests from https://github.com/dtolnay/quote/tree/aafba72e10919ad47c05169271cb78e614fb2b9d/tests/ui too. I think you can just make a new tests/ui/proc-macro/quote directory.

@tgross35 tgross35 added A-proc-macros Area: Procedural macros WG-macros Working group: Macros T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 23, 2024
@dtolnay dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 27, 2024
@SpriteOvO SpriteOvO force-pushed the proc-macro-use-to-tokens-in-quote branch from 0b32dec to 7e77f06 Compare January 4, 2025 01:54
@SpriteOvO
Copy link
Contributor Author

I have updated the PR for the macro implementation, and I will migrate the test cases from quote crate later.

@rust-log-analyzer

This comment has been minimized.

@SpriteOvO SpriteOvO force-pushed the proc-macro-use-to-tokens-in-quote branch from 7e77f06 to 5d0a6c5 Compare January 4, 2025 02:21
@rust-log-analyzer

This comment has been minimized.

@SpriteOvO SpriteOvO force-pushed the proc-macro-use-to-tokens-in-quote branch from 5d0a6c5 to 858ad51 Compare January 4, 2025 16:51
@rustbot
Copy link
Collaborator

rustbot commented Jan 4, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@SpriteOvO SpriteOvO force-pushed the proc-macro-use-to-tokens-in-quote branch from 858ad51 to 6404cf5 Compare January 5, 2025 22:50
@SpriteOvO
Copy link
Contributor Author

SpriteOvO commented Jan 8, 2025

I'm working on migrating the applicable tests from https://github.com/dtolnay/quote/blob/0245506323a3616daa2ee41c6ad0b871e4d78ae4/tests/test.rs. It seems like the current proc_macro::quote! is still incomplete? Some features (quote_spanned!, format_ident!, repetition) are still not implemented, because of that many test cases cannot be migrated.

// - quote_spanned:
//   - fn test_quote_spanned_impl
//   - fn test_type_inference_for_span
// - format_ident:
//   - fn test_format_ident
//   - fn test_format_ident_strip_raw
// - repetition:
//   - fn test_iter
//   - fn test_array
//   - fn test_fancy_repetition
//   - fn test_nested_fancy_repetition
//   - fn test_duplicate_name_repetition
//   - fn test_duplicate_name_repetition_no_copy
//   - fn test_btreeset_repetition
//   - fn test_variable_name_conflict
//   - fn test_nonrep_in_repetition
//   - fn test_closure
//   - fn test_star_after_repetition

And many tests cases failed with proc_macro::quote!, e.g.

/*
message: assertion `left == right` failed
             left: "impl < 'a , T : ToTokens > ToTokens for & 'a T { fn to_tokens (& self , tokens : & mut TokenStream) { (* * self) . to_tokens (tokens) } }"
            right: "impl < 'a, T : ToTokens > ToTokens for & 'a T\n{\n    fn to_tokens(& self, tokens : & mut TokenStream)\n    { (** self).to_tokens(tokens) }\n}"
*/
fn test_quote_impl() {
    let tokens = quote! {
        impl<'a, T: ToTokens> ToTokens for &'a T {
            fn to_tokens(&self, tokens: &mut TokenStream) {
                (**self).to_tokens(tokens)
            }
        }
    };

    let expected = concat!(
        "impl < 'a , T : ToTokens > ToTokens for & 'a T { ",
        "fn to_tokens (& self , tokens : & mut TokenStream) { ",
        "(* * self) . to_tokens (tokens) ",
        "} ",
        "}"
    );

    assert_eq!(expected, tokens.to_string());
}
/*
message: assertion `left == right` failed
             left: "X < X > (X) [X] { X }"
            right: "X <X > (X) [X] { X }"
*/
fn test_substitution() {
    let x = X;
    let tokens = quote!($x <$x> ($x) [$x] {$x});

    let expected = "X < X > (X) [X] { X }";

    assert_eq!(expected, tokens.to_string());
}
/*
message: assertion `left == right` failed
             left: "struct SerializeWith < 'a , T > where T : Serialize { value : & 'a String , phantom : :: std :: marker :: PhantomData < Cow < 'a , str > > , } impl < 'a , T > :: serde :: Serialize for SerializeWith < 'a , T > where T : Serialize { fn serialize < S > (& self , s : & mut S) -> Result < () , S :: Error > where S : :: serde :: Serializer { SomeTrait :: serialize_with (self . value , s) } } SerializeWith { value : self . x , phantom : :: std :: marker :: PhantomData :: < Cow < 'a , str > > , }"
            right: "struct SerializeWith < 'a, T > where T : Serialize\n{\n    value : & 'a String, phantom : :: std :: marker :: PhantomData <Cow < 'a,\n    str > >,\n} impl < 'a, T > :: serde :: Serialize for SerializeWith < 'a, T > where T :\nSerialize\n{\n    fn serialize < S > (& self, s : & mut S) -> Result < (), S :: Error >\n    where S : :: serde :: Serializer\n    { SomeTrait :: serialize_with(self.value, s) }\n} SerializeWith\n{\n    value : self.x, phantom : :: std :: marker :: PhantomData ::<Cow < 'a, str\n    > >,\n}"
*/
fn test_advanced() { /* ... */ }

and more.

I propose to open a new PR to add these test cases with fixes, and leave a FIXME for these unimplemented features, to keep this PR clean. What do you think? @tgross35

@tgross35
Copy link
Contributor

tgross35 commented Jan 8, 2025

I'm working on migrating the applicable tests from https://github.com/dtolnay/quote/blob/0245506323a3616daa2ee41c6ad0b871e4d78ae4/tests/test.rs. It seems like the current proc_macro::quote! is still incomplete? Some features (quote_spanned!, format_ident!, repetition) are still not implemented, because of that many test cases cannot be migrated.

At least at this time, there is no plan for quote_spanned or format_ident so just ignore any tests out that rely on these. We definitely want repetition at some point, so keeping a list of missing tests for repetition in a FIXME comment sounds reasonable.

And many tests cases failed with proc_macro::quote!, e.g.

The examples you listed look like they have the same meaning but some different spacing. Updating the whitespace to match should be fine, as long as there aren't other inconsistencies.

@SpriteOvO
Copy link
Contributor Author

SpriteOvO commented Jan 8, 2025

Something is wrong with raw string.

/*
message: assertion `left == right` failed
             left: "# [doc = r\" doc\"]"
            right: "#[doc = \" doc\"]"
*/
fn test_outer_line_comment() {
    let tokens = quote! {
        /// doc
    };
    let expected = "# [doc = r\" doc\"]";
    assert_eq!(expected, tokens.to_string());
}

/*
message: `"r#raw_id"` is not a valid identifier
*/
fn test_quote_raw_id() {
    let id = quote!(r#raw_id);
    assert_eq!(id.to_string(), "r#raw_id");
}

@tgross35
Copy link
Contributor

tgross35 commented Jan 8, 2025

We probably need to call to Ident::new_raw rather than Ident::new in some cases, like quote has https://github.com/dtolnay/quote/blob/0245506323a3616daa2ee41c6ad0b871e4d78ae4/src/runtime.rs#L486.

For doc comments, maybe proc_macro2 and proc_macro unquote strings differently? @dtolnay would know better. You could run a test like this with our quote and see if it unquotes the same https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=19cdf0381b312f9ad5fac14af49e55f7.

@tgross35
Copy link
Contributor

tgross35 commented Jan 8, 2025

We probably need to call to Ident::new_raw rather than Ident::new in some cases, like quote has https://github.com/dtolnay/quote/blob/0245506323a3616daa2ee41c6ad0b871e4d78ae4/src/runtime.rs#L486.

Also if you prefer not to fix this here, it is okay to just open an issue and leave the test as a FIXME. I'd be happy to merge this nearly as-is since it's a big improvement over what we have, as long as tests pass and dtolnay has no objection.

Same goes for the r# string discrepancy as long as we understand where it is coming from.

@SpriteOvO
Copy link
Contributor Author

Also if you prefer not to fix this here, it is okay to just open an issue and leave the test as a FIXME. I'd be happy to merge this nearly as-is since it's a big improvement over what we have, as long as tests pass and dtolnay has no objection.

I'm fine to fix it in this PR as long as it's not a huge diff :)

Also, compilefail tests are still being migrated.

@SpriteOvO SpriteOvO force-pushed the proc-macro-use-to-tokens-in-quote branch from 8859f8d to b13e0a6 Compare January 9, 2025 20:02
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Couple small suggestions but otherwise lgtm.

Fyi all CI is having some problems currently.

@SpriteOvO SpriteOvO force-pushed the proc-macro-use-to-tokens-in-quote branch from b13e0a6 to 3a2f3b9 Compare January 9, 2025 21:15
@SpriteOvO SpriteOvO force-pushed the proc-macro-use-to-tokens-in-quote branch from 3a2f3b9 to ef69c30 Compare January 9, 2025 21:16
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
curl: (22) The requested URL returned error: 404
curl: (22) The requested URL returned error: 404
ERROR: failed to download llvm from ci

    HELP: There could be two reasons behind this:
        1) The host triple is not supported for `download-ci-llvm`.
        2) Old builds get deleted after a certain time.
    HELP: In either case, disable `download-ci-llvm` in your config.toml:
    [llvm]
    download-ci-llvm = false
    
Build completed unsuccessfully in 0:00:23

@tgross35
Copy link
Contributor

tgross35 commented Jan 9, 2025

This is a significant improvement, thanks for the changes! Tree is closed for the CI fixes so this will have to wait a bit to merge.

@tgross35 tgross35 self-assigned this Jan 10, 2025
@tgross35
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 10, 2025

📌 Commit ef69c30 has been approved by tgross35

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 10, 2025
@bors bors merged commit 34fa27b into rust-lang:master Jan 10, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 10, 2025
@SpriteOvO SpriteOvO deleted the proc-macro-use-to-tokens-in-quote branch January 10, 2025 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-proc-macros Area: Procedural macros S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. WG-macros Working group: Macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants