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

Ensure all quote! tokens have the default Span #51

Closed
wants to merge 1 commit into from

Conversation

alexcrichton
Copy link
Collaborator

Apparently parse can return some... "flavorful" spans! This resets all
spans coming out of parse back to that of Default::default to ensure
that we're spanned correctly. This also just so happens to fix an error
I was seeing locally, although I have no idea why.

Apparently `parse` can return some... "flavorful" spans! This resets all
spans coming out of `parse` back to that of `Default::default` to ensure
that we're spanned correctly. This also just so happens to fix an error
I was seeing locally, although I have no idea why.
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I would prefer to fix this somewhere other than quote if possible. It looks like these are the two impls involved: 1 2 both of which just call into libproc_macro. Is it possible to apply this fix in libproc_macro? If not, is it possible to fix in those impls in proc-macro2?

@alexcrichton
Copy link
Collaborator Author

Hm ok thinking on this more this may actually be the right fix I think. I believe the span information coming out is otherwise correct as we're basically stringifying a token at compile time and then parsing it at runtime. The span information points to that token itself (and sort of a source file that just has the one token in it). I think in general the tokens produced by quote! have the Span::default() value (created explicitly in a few locations for things like parentheses) and then this is just touching up other tokens like operators and identifiers.

In that sense I'm not entirely sure where else the fix would go? I don't think we'd want to change parse for that reason b/c you may not always want parse to return the default span

@alexcrichton
Copy link
Collaborator Author

Er wait, this fixed one thing locally and broke tons of others, now I have no idea what's going on... Should certainly hold off on merging!

@alexcrichton
Copy link
Collaborator Author

Ok so I made a much longer writeup with my findings here about the use case I was working with. This fix is not enough in that we also needed to use call_site instead of default to get things working. In the meantime I'm going to close this while I wait for more info on that issue.

Sorry for the noise!

@alexcrichton alexcrichton deleted the span-default branch November 11, 2017 15:47
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.

2 participants