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

Fix the span information created by TokenStream's FromStr impl #48949

Closed
wants to merge 2 commits into from

Conversation

mystor
Copy link
Contributor

@mystor mystor commented Mar 12, 2018

This patch makes 3 changes:

  1. StringReader would not update its peek_span or span fields when
    override_span was set, so this was moved into a method which
    updates the internal state.
  2. StringReader was changed to hold a DUMMY_SP instead of a None
    for override_span, this decreases the size of StringReader and
    set's us up for (3).
  3. StringReader::mk_sp was changed to copy the override_span's
    SyntaxContext, even if override_span was DUMMY_SP.
  4. proc_macro::TokenStream::FromStr was updated to set the
    override_span to be based on DUMMY_SP instead of call_site.

This means that the line/column information for all tokens in the parsed
string will be correct, while also using the call site's hygiene
information.

Fixes #48944

This patch makes 3 changes:

1. `StringReader` would not update its `peek_span` or `span` fields when
   `override_span` was set, so this was moved into a method which
   updates the internal state.
2. `StringReader` was changed to hold a `DUMMY_SP` instead of a `None`
   for `override_span`, this decreases the size of `StringReader` and
   set's us up for (3).
3. `StringReader::mk_sp` was changed to copy the `override_span`'s
   `SyntaxContext`, even if `override_span` was `DUMMY_SP`.
4. `proc_macro::TokenStream::FromStr` was updated to set the
   `override_span` to be based on `DUMMY_SP` instead of `call_site`.

This means that the line/column information for all tokens in the parsed
string will be correct, while also using the call site's hygiene
information.
@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(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 Mar 12, 2018
@mystor
Copy link
Contributor Author

mystor commented Mar 12, 2018

r? @jseyfried

(I think you're probably the person to review this?)

@rust-highfive rust-highfive assigned jseyfried and unassigned pnkfelix Mar 12, 2018
@shepmaster
Copy link
Member

Ping from triage, @jseyfried !

@mystor
Copy link
Contributor Author

mystor commented Mar 18, 2018

There are still some tests which are failing which I haven't had the time to figure out, so it's probably ok if you don't get around to reviewing it for a bit.

@shepmaster shepmaster 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 Mar 18, 2018
@pietroalbini
Copy link
Member

Ping from the triage team @mystor! Will you have time to finish this PR in the near future?

@bors
Copy link
Contributor

bors commented Apr 6, 2018

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

@pietroalbini
Copy link
Member

Thank you for this PR @mystor! Unfortunately we haven't heard from you in a while, so I'm going to close this to keep things tidy.

Don't worry though, we would be happy to get this merged! If you'll have time in the future to work on this again please reopen the PR, and we'll review it!

@pietroalbini pietroalbini added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants