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

macros: preserve none-delimiters in function body #3583

Closed
wants to merge 4 commits into from

Conversation

Kestrer
Copy link
Contributor

@Kestrer Kestrer commented Mar 5, 2021

Motivation

When user-generated token streams are returned from proc macros, all none-delimited groups are flattened. This can cause compilation errors as shown in the issue below.

Solution

By instead passing the function body token stream straight through from the input of the proc macro to its output, it does not get deconstructed and reconstructed, and so the none-delimited groups are not lost.

Fixes: #3579

When user-generated token streams are returned from proc macros, all
none-delimited groups are flattened. This can cause compilation errors
as shown in the issue below. By instead passing the function body token
stream straight through from the input of the proc macro to its output,
it does not get deconstructed and reconstructed, and so the
none-delimited groups are not lost.

Fixes: tokio-rs#3579
@Kestrer
Copy link
Contributor Author

Kestrer commented Mar 5, 2021

Well that's weird. It fails on 1.45.2 but not on 1.47.0, and only when $e() is inside a macro - in this case assert_eq!. I'll just wait until 2021-04-08 when the MSRV can be bumped.

@Kestrer Kestrer marked this pull request as draft March 5, 2021 14:37
@Darksonn Darksonn added the A-tokio-macros Area: The tokio-macros crate label Mar 5, 2021
tokio-macros/src/entry.rs Outdated Show resolved Hide resolved
@taiki-e taiki-e self-assigned this Mar 9, 2021
@taiki-e
Copy link
Member

taiki-e commented Mar 16, 2021

Well that's weird. It fails on 1.45.2 but not on 1.47.0, and only when $e() is inside a macro - in this case assert_eq!. I'll just wait until 2021-04-08 when the MSRV can be bumped.

If this fix only works with the new compiler, I think we probably don't need to block this PR (because it doesn't work with older versions in either way). Or does this mean this PR introducing some new regression with the old compiler? (If so, we need to postpone until MSRV increases.)

@Kestrer
Copy link
Contributor Author

Kestrer commented Mar 16, 2021

Oh yes, that's true. Although I'd have to figure out how to gate a test's compilation based on Rustc version so CI doesn't break. If that's not trivial it might be easier to just wait until the bump, since it's not urgent.

@Kestrer Kestrer marked this pull request as ready for review April 8, 2021 15:17
@Darksonn
Copy link
Contributor

Darksonn commented May 5, 2021

Rust 1.47 is now more than six months old.

@taiki-e
Copy link
Member

taiki-e commented May 5, 2021

I think the "right" fix here is to fix the syn parser to preserve the none-delimited group (like dtolnay/syn#854). I will look into whether that is possible.

@Kestrer
Copy link
Contributor Author

Kestrer commented May 5, 2021

@taiki-e The problem is not syn, it's proc_macro itself. Even something as simple as tokens.into_iter().collect::<TokenStream>() will destroy None-delimited token spans.

@taiki-e
Copy link
Member

taiki-e commented May 6, 2021

Seems this is not a bug of syn as @Kestrer said, but None-delimited token spans are actually preserved.
Also, #3579 works with all versions of 1.45 to 1.48, without this patch.
So looks like this is the regression introduced in 1.49 (probably by rust-lang/rust#77135). See rust-lang/rust#82784 (comment) for more.

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me. That's a bit hacky, but there's no other workaround that looks good, and the problem is unlikely to be fixed on the rustc side soon.

README.md Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@Kestrer
Copy link
Contributor Author

Kestrer commented May 8, 2021

Also, #3579 works with all versions of 1.45 to 1.48, without this patch.

I don't think that's true; the bug was reported before 1.49 existed. They wouldn't've reported it if it worked without the patch.

But either way the behaviour I'm observing is that this workaround only worked in 1.47.0 and 1.48.0. Given that @Aaron1011 mentioned that the workaround working at all was only due to a hack, I think it's unlikely this will ever work again, so I'm going to close this PR.

@Kestrer Kestrer closed this May 8, 2021
@Kestrer Kestrer deleted the preserve-none-delimiters branch May 8, 2021 13:59
@taiki-e taiki-e removed their assignment May 8, 2021
// groups inside the function body; see <https://github.com/tokio-rs/tokio/issues/3579>.
let body =
proc_macro2::TokenStream::from(TokenStream::from(input_tokens.into_iter().last().unwrap()));

input.block = syn::parse_quote! {
Copy link
Member

Choose a reason for hiding this comment

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

The reason test failed after the master was merged is that body was converted to syn ast again here (confirmed locally: 38326a5)

@taiki-e
Copy link
Member

taiki-e commented May 8, 2021

the bug was reported before 1.49 existed. They wouldn't've reported it if it worked without the patch.

rust-lang/rust#77135 was merged in 2020-10-14, 1.49 was released in 2020-12-31, #3579 was reported in 2021-03-05.
The underlying problem was indeed reported before 1.49, but was hidden by another problem so that #3579 only occurred after 1.49.

But either way the behaviour I'm observing is that this workaround only worked in 1.47.0 and 1.48.0. Given that @Aaron1011 mentioned that the workaround working at all was only due to a hack, I think it's unlikely this will ever work again, so I'm going to close this PR.

I think this PR's hack is still usable at this point (#3583 (comment)), but I think it's likely that it won't work in the future.
So, I support your decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-macros Area: The tokio-macros crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compile error mixing declarative macros with #[tokio::test]
3 participants