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

proc_macro: Stabilize `Span::resolved_at` and `Span::located_at` #69041

Open
wants to merge 1 commit into
base: master
from

Conversation

@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 10, 2020

Introduced in #47149.
Part of #54725.

Motivation: #68716 (comment).
Identifiers in proc macros may want to inherit span locations for diagnostics from one tokens (e.g. some tokens from the macro input), but resolve those identifiers from some different location (e.g. from the macro's definition site).
This becomes especially important when multiple resolution locations become available with stabilization of Span::mixed_site.

Why I think this is the right API for setting span's location and hygiene - #69041 (comment).

r? @dtolnay

@jonas-schievink jonas-schievink added this to the 1.43 milestone Feb 10, 2020
@dtolnay dtolnay added the needs-fcp label Feb 10, 2020
@Centril Centril added the T-lang label Feb 11, 2020
@petrochenkov petrochenkov changed the title proc_macro: Stabilize `Span::resolve_at` and `Span::located_at` proc_macro: Stabilize `Span::resolved_at` and `Span::located_at` Feb 11, 2020
@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Feb 12, 2020

@SergioBenitez & @jebrosen may be interested in this as well.

@jebrosen

This comment has been minimized.

Copy link
Contributor

jebrosen commented Feb 12, 2020

Yes, I suspect rocket could leverage these APIs for the same reason given in #68716 (comment) (mainly, diagnostic reporting).

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Feb 15, 2020

There are a few alternatives in terms of naming and the function signatures here.
I think the status quo hits a sweet spot between them.

The primary alternative is some API looking like,

impl Span {
	fn set_location(&mut self, location: ???);
	fn set_hygiene_context(&mut self, hygiene_context: ???);
}

, which has some issues.

  • We don't have stable types representing the location alone and the hygienic context alone.
    All our API is in terms of spans and it seems reasonable to keep the API surface minimal as it is now for both simplicity (less types less issues) and faster stabilization.
    This means the API will combine locations and hygiene from spans.

     impl Span {
     	fn set_location_from(&mut self, location: Span);
     	fn set_hygiene_context_from(&mut self, hygiene_context: Span);
     }
    
  • The setter interface is just inconvenient in practice. Built-in macros in the compiler use similar location and hygiene combining a lot, and they eventually evolved to a functional-style API returning a new span rather than modifying the old one for ergonomics.

     impl Span {
     	fn with_location_from(&self, location: Span) -> Span;
     	fn with_hygiene_context_from(&mut self, hygiene_context: Span) -> Span;
     }
    

This kind of promises that Span is cheaply copyable, but we already kind of promise it because Span is already Copy.

  • "Hygiene context" is too abstract, nobody knows what it means, the name should preferably tell what the user-visible consequences the choice of context has, and the primary consequence is where the identifier with that context is resolved from.

So, fn located_at and fn resolved_at are basically better worded alternatives of the previous variant.

let span = input_subspan.resolved_at(Span::def_site()); // Nice
@Centril Centril modified the milestones: 1.43, 1.44 Mar 10, 2020
@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Mar 10, 2020

ping @dtolnay, this is waiting on you.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Mar 15, 2020

@rust-lang/libs @rust-lang/lang: This PR stabilizes these two methods of proc_macro::Span:

impl Span {
    /// Creates a new span with the same line/column information as `self` but
    /// that resolves symbols as though it were at `other`.
    pub fn resolved_at(&self, other: Span) -> Span;

    /// Creates a new span with the same name resolution behavior as `self` but
    /// with the line/column information of `other`.
    pub fn located_at(&self, other: Span) -> Span;
}

These are for mixing and matching the two pieces of information held by Span, source location and hygiene (≈ name resolution) context.

We have experience with these methods in Serde, e.g. serde-rs/serde@6457331. To take a look at one of the diffs from that commit:

- quote!(try!(_serde::de::SeqAccess::next_element::<#field_ty>(&mut __seq)))
+ let span = Span::def_site().located_at(field.original.span());
+ let func = quote_spanned!(span=> _serde::de::SeqAccess::next_element::<#field_ty>);
+ quote!(try!(#func(&mut __seq)))

In this code:

  • Name resolution needs to happen at def_site, because the symbol _serde is defined as extern crate serde as _serde by the generated code and is not necessarily in scope at the call site, or if it is, it may refer to something different at the call site.
  • But if the relevant Deserialize trait impl is missing on #field_ty, we would really like for that error to appear pointing to the correct field inside the user's struct at the call site.

The green implementation uses Span::located_at to combine def_site's hygiene context with the caller's field's source location to get compilable code that produces good error messages.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Mar 15, 2020

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Mar 15, 2020

Reviewed, but do we have relevant tests for these methods? (i.e. as exposed, and not via tests exercising the deeper internal mechanism)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.