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

Make span location information available through a feature #165

Closed

Conversation

mitsuhiko
Copy link

This makes more of the Span API available by turning on the span-location-info feature. Not entirely sure what happens if this stabilizes in different ways but I figured putting it behind a feature is not the worst thing in the world.

Refs #89 and dtolnay/syn#406

@mystor
Copy link
Contributor

mystor commented Jan 24, 2019

When I implemented this feature in #36, I originally had it behind a feature, however the decision was made that proc_macro2 should strictly limit itself to the stable API exposed by proc_macro.

This feature as implemented will currently force off the native APIs implementation if it is used, which is not what I'm expecting people would expect.

I'm guessing this probably will not be merged to avoid having feature flags which force use of the fallback on the latest stable rustc, no matter how useful it may be :-/

That being said, perhaps it would be useful to have a feature which force-enables the fallback, and is intended for use in non-proc-macro contexts, though I think it should probably have a clear name.

@mitsuhiko
Copy link
Author

I started this PR because of this conversation I had with @dtolnay on IRC:

23:51:06 <dtolnay> mitsuhiko: yes i would like to
23:51:46 <dtolnay> didn't realize it would take so long in the compiler
23:52:17 <dtolnay> i would be okay exposing the current API as stable and dealing with any necessary changes in a future major version of proc-macro2
23:52:24 <mitsuhiko> that would be neat
23:52:35 <mitsuhiko> i would really like to use it for my snapshotting lib
23:53:03 <dtolnay> could you send a proc-macro2 PR?
23:53:26 <dtolnay> there may be some cases to think through to avoid non-additive behavior of features
23:54:13 <dtolnay> or i can schedule time this weekend to look into it

@mystor
Copy link
Contributor

mystor commented Jan 24, 2019

Ok, cool :-) - good to know. If David and Alex are on board then I'd say go for it, though I might still choose a different feature name which explicitly mentions enabling the fallback.

@dtolnay
Copy link
Owner

dtolnay commented Jan 24, 2019

Haven't thought this through but here's a quick idea: is it possible to expose these functions without forcing fallback mode? We can expose the signature but have it so that in wrapper mode the implementations look like:

match self {
    Span::Compiler(span) => {
        // if nightly, return the compiler's right value
        // otherwise return a placeholder like -1 (or whatever)
    }
    Span::Fallback(span) => {
        // return the fallback's right value
    }
}

When running in main.rs or build.rs context (which I assume is the case for your snapshotting lib) you'll always get the fallback impl's right value. But if your snapshotting lib implementation uses some proc macros, you will continue getting correctly spanned error messages from those macros as you develop.

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 think the suggestion in #165 (comment) should work and we shouldn't need to force fallback mode here.

@mitsuhiko
Copy link
Author

@dtolnay I will try to do this though I'm not 100% sure yet which this means :)

@mitsuhiko
Copy link
Author

I fixed up the patch above now to pass. About the proposed logic in #165 (comment) I'm not sure how to proceed. I understood it that I should define types like this in wrapper.rs:

#[derive(Clone, PartialEq, Eq)]
#[cfg(span_location_info)]
pub enum SourceFile {
    #[cfg(super_unstable)]
    Compiler(proc_macro::SourceFile),
    #[cfg(not(super_unstable))]
    Compiler,
    Fallback(fallback::SourceFile),
}

Did I understand this correctly?

@dtolnay
Copy link
Owner

dtolnay commented Jan 28, 2019

See #166.

@mitsuhiko
Copy link
Author

#166 is clearly the better approach. Closing this PR for it.

@mitsuhiko mitsuhiko closed this Jan 28, 2019
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.

3 participants