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

Expose span locations on stable #166

Merged
merged 1 commit into from
Jan 31, 2019
Merged

Expose span locations on stable #166

merged 1 commit into from
Jan 31, 2019

Conversation

dtolnay
Copy link
Owner

@dtolnay dtolnay commented Jan 28, 2019

In the way suggested by #165 (comment).

src/fallback.rs Outdated Show resolved Hide resolved
@mitsuhiko

This comment has been minimized.

@mitsuhiko
Copy link

Can confirm this works!

@dtolnay dtolnay added the wip label Jan 28, 2019
@dtolnay
Copy link
Owner Author

dtolnay commented Jan 28, 2019

Tagging wip because this needs tests. Does not work when I tried it:

use proc_macro2::TokenTree;

fn main() {
    let sp = syn::parse_str::<TokenTree>("testing").unwrap().span();
    println!("start line={} column={}", sp.start().line, sp.start().column);
    println!("end line={} column={}", sp.end().line, sp.end().column);
}
start line=1 column=0
end line=1 column=0

I am not going to be able to get to this today. @mitsuhiko could you investigate?

@mitsuhiko
Copy link

Yes, will investigate. Information seems absent indeed.

@alexcrichton
Copy link
Contributor

If we're going to offer this in a published stable version then I think we can probably go ahead and do this without a feature. The proc-macro2 crate can't have many major version bumps due to the degree of reliance on it through syn in the proc-macro ecosystem, so being stable is a big commitment. If we're willing to commit to this as a stable feature then I think it's worth providing by default.

I think that may also simplify the cfg story here (which I am always confused by every time I look at this!). That way outside of a proc-macro context you always have accurate information, but in a proc-macro context you'll only have accurate information if a nightly feature toggle is enabled.

@dtolnay
Copy link
Owner Author

dtolnay commented Jan 28, 2019

I kept a feature because last time I checked there was a big performance hit for Span going from 0 bytes to 8 bytes. There are a lot of spans in the syntax tree...

@alexcrichton
Copy link
Contributor

Ah ok that makes sense to me, and in that case seems fine to document as such as to why it's a feature

@mystor
Copy link
Contributor

mystor commented Jan 28, 2019

Hmm, I wonder if a span interning system could compress span sizes down even more, and if that would be helpful here.

@dtolnay
Copy link
Owner Author

dtolnay commented Jan 28, 2019

I added a note in build.rs about the performance hit.

@dtolnay dtolnay removed the wip label Jan 28, 2019
@dtolnay
Copy link
Owner Author

dtolnay commented Jan 28, 2019

I think this works now.

I filed #167 to follow up on mitigating the performance impact.

@mitsuhiko
Copy link

Is this blocked on #167 or is it mergeable as such?

@dtolnay
Copy link
Owner Author

dtolnay commented Jan 31, 2019

I don't think this needs to block on the performance work, because the increase in size of Span only happens if you opt in to the new feature. Thanks for the reminder!

@dtolnay dtolnay merged commit 2b01a3f into dtolnay:master Jan 31, 2019
@dtolnay dtolnay deleted the location branch January 31, 2019 23:29
@dtolnay
Copy link
Owner Author

dtolnay commented Jan 31, 2019

Published in 0.4.27.

@chinedufn
Copy link

@dtolnay noob question here - is this performance hit increased memory, noticeably slower speed for working with a TokenStream.. or both.. or something else?

#167 suggests that it's increased memory but I'm not sure if that's also having a big impact on other things?

Trying to better understand whether or not to start taking advantage of this.

Thanks!!

@dtolnay
Copy link
Owner Author

dtolnay commented Feb 20, 2019

@chinedufn it was noticeably longer compile time in debug mode for Serde's test suite the last time I tried it. You could enable the feature and check the compile time for some crate that uses your macro heavily.

@chinedufn
Copy link

Awesome advice, perfect, thanks!!

@chinedufn
Copy link

chinedufn commented Mar 5, 2019

Hmm so I'm seeing some weird behavior for span locations

Take the following proc macro

#[test]
fn text_space_before_end_tag() {
    assert_eq!(
        &html! { <div>Before End Tag </div> }.to_string(),
        "<div>Before End Tag </div>"
    )
}

For the token Tag, the span is perfectly correct. Its span's end column is 36, which .. after counting characters it's definitely the 36th column.

However, the < in </div has a start column of 37.

I would expect it to be 38 here since there is a space between the end of the word Tag and the <.

Does that sound familiar at all? Held off on pushing up something easily reproducible in case there is an obvious reason here that you might already know but if not I can totally do that if that would help!

@alexcrichton
Copy link
Contributor

@chinedufn that's likely actually a bug in rustc itself, want to see if you can reduce it to a standalone proc-macro and report it upstream?

@chinedufn
Copy link

Will do, thanks!

@chinedufn
Copy link

For posterity! -> rust-lang/rust#58958

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.

5 participants