-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
Support meaningful spans in the stable version of proc-macro2 #36
Conversation
r? @alexcrichton (apparently I can't set you as the reviewer on this repo?) |
Looks great to me! I forget what came out of our discussion though, did we figure it was best to hold off on this until there's a "more solid" use case? Or did we decide to go ahead and merge? |
OOooh, oooh, you definitely decided to go ahead and merge!! ;) |
@alexcrichton Blandy apparently wants it, so I figure we might as well merge it. |
@jimblandy heh now I'm curious, do you have something that'd benefit from this? If so, could you explain it a bit as well? (to know if this is sufficient) |
I'd like to implement a crate for Rust source-to-source transformations that preserve formatting and comments, along the lines of the recast library for JavaScript:
I just learned about the rerast tool for writing Rust; it seems very similar to what I was hoping to write. It simply links with the compiler directly, and requires nightly. |
Cargo.toml
Outdated
@@ -19,6 +19,7 @@ doctest = false | |||
|
|||
[dependencies] | |||
unicode-xid = "0.1" | |||
memchr = "2.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With @Manishearth's work on improving the performance of the find method in rust-lang/rust#46735, it probably won't be useful to have this dependency on memchr anymore, so we might want to drop it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's why you wanted this.
Sneaky, sneaky 😺
src/lib.rs
Outdated
} | ||
|
||
pub fn end(&self) -> LineColumn { | ||
// XXX(nika): We can't easily wrap LineColumn right now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of rust-lang/rust#46690, the fields have been made public in the proc-macro implementation, so we can probably do a wrap of this type now.
Hm ok so I was about to merge this but it ended up hitting a snag and at this point I think I'd prefer to not merge it. I originally was going to drop the Overall I feel like these APIs are much more of a moving target on nightly than the initial set of APIs. The original purpose of this crate was to allow authors today to use this crate today on stable and then once procedural macros are stable basically instantly switch over to get better spans. From a stabilization point of view it's hard for me to see how spans and such will make the cut initially as opposed to the apis already here. In that sense I fear that a feature like this will promote creation of libraries that aren't actually compatible with the first pass of stabilizing procedural macros... |
Perhaps we want to have a tool to build There are uses like I'm not sure what that would look like in syn? perhaps we could move We could also make |
@alexcrichton How would you feel about landing this behind an off-by-default feature flag like "experimental-apis" to discourage consumers? |
I think one thing that worked well with rayon, for example, is an env var rather than a Cargo feature. That way you have to always explicitly opt into "unstable APIs" which I'd personally be fine doing. |
I ran into rust-lang/rust#47077 while updating the APIs, so I had to do some unfortunate hacky stuff to work around it. Hopefully we can remove that in the future. |
@alexcrichton Any chance you could look at this & see if we can merge it? |
Looks great, thanks! |
I came across this PR when trying to preserve white-space when converting a |
This doesn't add support for the diagnostic methods on
Span
in stable rust. Perhaps we should add support for that, but I'm not sure what it would look like & I don't think we want a diagnostics engine in proc-macro2.It does add support for things like fetching the line and column numbers of the start and end of a span.