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

Use proper span when generating matches token #398

Closed
wants to merge 1 commit into from

Conversation

Aaron1011
Copy link
Contributor

In struct-opt-derive, a function is generated with a parameter named
matches. Since quote! is used to generate the function, the
matches token will be resolved using Span::call_site.

However, the literal identifier matches is also used inside several
quote_spanned! expressions. Such a matches identifier will be
resolved using the Span passed to quote_spanned!, which may not be
the same as Span::call_site.

Currently, this is difficult to observe in practice, due to
rust-lang/rust#43081 . However, once PR rust-lang/rust#73084
is merged, proc macros will see properly spanned tokens in more cases,
which will cause these incorrect uses of quote_spanned! to break.

This PR uses quote! { matches } to generate a correctly spanned
matches token, which is then include in the quote_spanned!
expressions using #matches.

In `struct-opt-derive`, a function is generated with a parameter named
`matches`. Since `quote!` is used to generate the function, the
`matches` token will be resolved using `Span::call_site`.

However, the literal identifier `matches` is also used inside several
`quote_spanned!` expressions. Such a `matches` identifier will be
resolved using the `Span` passed to `quote_spanned!`, which may not be
the same as `Span::call_site`.

Currently, this is difficult to observe in practice, due to
rust-lang/rust#43081 . However, once PR rust-lang/rust#73084
is merged, proc macros will see properly spanned tokens in more cases,
which will cause these incorrect uses of `quote_spanned!` to break.

This PR uses `quote! { matches }` to generate a correctly spanned
`matches` token, which is then include in the `quote_spanned!`
expressions using `#matches`.
@Aaron1011
Copy link
Contributor Author

The test failures seem unrelated to this PR.

@CreepySkeleton
Copy link
Collaborator

This issue is pretty interesting.

I recall I had noticed this problem and been thinking about for a while it but eventually came to conclusion that since quote and quote_spanned invocations are located in distinct match branches, which means only one would be emitted, this was not a problem.

What I didn't notice is that the match is located inside a cycle (.iter().map() to be precise). So it might just happen that on N'th iteration a quote branch would be taken, and on N + M'th iteration quote_spanned bill be used, so spans might end up to be different.

@Aaron1011 Could you please factor the quote!{ matches } out of the loop and leave a comment explaining why this is done this way?

@CreepySkeleton CreepySkeleton mentioned this pull request Jun 12, 2020
@CreepySkeleton
Copy link
Collaborator

Merged in #400

@Aaron1011
Copy link
Contributor Author

@CreepySkeleton: Thanks! Would it be possible for you to make a new point release? This will prevent rust-lang/rust#73084 from causing regressiosn when it lands?

@CreepySkeleton
Copy link
Collaborator

Sure. I'm working on another bugfix ATM and I will either finish in a day or two and make one release for the both (just so we have something meaningful to put in changelog) or give up and make a release all the same. Is that fine or you need it ASAP?

@Aaron1011
Copy link
Contributor Author

@CreepySkeleton: That's fine by me - the Rust PR will be merged in a couple days at the absolute earliest, since it's waiting on a Crater run.

@CreepySkeleton
Copy link
Collaborator

Released in 0.3.15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants