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

Stabilize `Span::mixed_site` #68716

Open
wants to merge 1 commit into
base: master
from

Conversation

@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 31, 2020

Closes #65049.
cc #54727 (comment)

Pre-requisite for #68717 ("Stabilize fn-like proc macros in expression, pattern and statement positions").

Stabilization report: #68716 (comment).

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 31, 2020

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@jhpratt

This comment was marked as resolved.

Copy link

jhpratt commented Jan 31, 2020

Is the discrepancy between the 1.43 milestone and the #[stable(since = "1.44.0")] intentional?

@petrochenkov

This comment was marked as resolved.

Copy link
Contributor Author

petrochenkov commented Feb 1, 2020

No, it should be since = "1.44.0" (optimistically), I've just miscalculated the version.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Feb 1, 2020

Stabilization report

This PR stabilizes a proc macro API Span::mixed_site() exposing macro_rules hygiene to proc macro authors.

Tracking issue: #65049
Version target: 1.43

Technical specification

Procedural macro producing tokens with Span::mixed_site() spans should be equivalent to a macro_rules macro producing the same tokens.
Identifiers with this span will resolve at the macro's definition site for local variables, labels and $crate, and resolve at the call site for everything else.

Motivation

Proc macros do not have direct access to our oldest and most stable hygiene kind - macro_rules hygiene.

To emulate it macro authors have to go through two steps - first generate a temporary macro_rules item, then generate a macro call to that item. Popular crates like proc_macro_hack use this trick to generate hygienic identifiers from proc macros.

I'd say that these workarounds with nested macro definitions have more chances to hit some corner cases in our hygiene system, in which we don't have full confidence.
So, let's provide a direct access to macro_rules hygiene instead.
This access is provided with a method Span::mixed_site, which is similar to existing Span::call_site (stable, fully unhygienic) and Span::def_site (unstable, fully hygienic).

This API addition opens the way to stabilizing proc macros in expression, pattern and statement positions (#68717), for which use of call-site hygiene or workarounds with temporary items would be quite unfortunate.
macro_rules expanded in all these positions, on the other hand, are stable since 1.0 and widely used.
So, we are suggesting to stabilize such use of procedural macros and give a way to proc macro authors to make these macros reasonably hygienic with Span::mixed_site.

Tests

ui/proc-macro/mixed-site-span.rs makes sure that the API works.
Otherwise, all tests for macro_rules test the underlying mechanism.

History

The implementation (#64690) was merged about 4 months ago.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Feb 1, 2020

@rust-highfive rust-highfive assigned dtolnay and unassigned nikomatsakis Feb 1, 2020
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Feb 2, 2020

This API addition opens the way to stabilizing proc macros in expression, pattern and statement positions (#68717), for which use of call-site hygiene or workarounds with temporary items would be quite unfortunate.

One thing I'm wondering about here is the documentation ("how do we teach this") aspect and making it easy to "do the right thing" wrt. hygiene in expr/pat/stmt contexts. Hygiene is, in my experience, one of the less well known parts of the language and so it seems like this could be easy to get wrong without good teaching material. In the implementation PR, @dtolnay noted (#64690 (comment)) that:

I will try to test whether changing the default span of quote! from call_site to mixed_site on sufficiently new compilers can possibly break anything that's currently stable. The alternative would be adding a macro like quote_expr! that is shorthand for quote_spanned!(Span::mixed_site()=>...).

I'm wondering if more thought has been put into this. @CAD97 did note in #64690 (comment) that changing to mixed_site could trivially break some crates using quote!, which is now a 1.0.X crate, so it seems like the most practical path would be quote_expr! { ... } (although that name is sub-optimal for patterns & statements).

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Feb 2, 2020

@Centril
I'm not sure this is our problem.
AFAIK, most user-facing APIs are provided by third-party crates, proc_macro only provides primitives.

The recommendation on using Span::mixed_site is #64690 (comment) - use it everywhere unless you specifically want unhygienic local variables, labels or $crate, which is a rare case.

I don't understand why the @CAD97's example would be broken, it doesn't seem to involve local variables.

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Feb 2, 2020

The teams should, in my view, still be interested in the wider ecosystem effects with respect to #68717 and not just the narrow technical aspect of mixed_site. We should make sure we're not inadvertently laying traps for users.

Specifically, I'm somewhat concerned that the recommendation won't be heeded by many due to the relative ergonomics of quote!(...) vs quote_spanned!(Span::mixed_site()=> ...). So having some sort of plan for how to address that would be good. If quote! can change to mixed_site then the issue is resolved, but if not, it might involve adding a second macro to quote (e.g. q!(...)) and deprecating the other macro. (This is primarily directed at @dtolnay.)

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 2, 2020

I would be interested in a crater run that changes proc_macro::Span::call_site() to call mixed_site(), as a way to mimic what would happen from replacing quote's default. I agree with #68716 (comment) that I don't understand why the example given in the other PR would break.

  • If nothing breaks, that's great.
  • If the only things that break are things using unstable proc_macro_hygiene, we can evaluate but I am open to breaking those.
  • If more complicated, this may be a good time to introduce impl Default for Span which produces call_site or mixed_site depending on whether we are in the expansion of an attribute/item macro vs expr/pat/stmt macro.
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Feb 2, 2020

A crater run sounds like a great idea. 👍

(I'm skeptical of Default for Span but we can have that discussion when necessary.)

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Feb 2, 2020

I would be interested in a crater run that changes proc_macro::Span::call_site() to call mixed_site(), as a way to mimic what would happen from replacing quote's default.

#68766 is the PR doing that.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 7, 2020

#68766 (comment) has the experiment results.

I haven't looked into the regressions too closely, but it seems a large variety of derive macros and attribute macros no longer work. That suggests to me mixed_site may not be a reasonable default and that it should be reserved as a power tool. But I intend to take a closer look at what particular patterns exhibit different behavior (and I could use all of your help with this). It is interesting that serde_derive seems completely fine using mixed_site everywhere.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Feb 15, 2020

@dtolnay

This would require some kind of function in proc_macro that returns an enum revealing whether the proc macro currently being expanded is in item position, statement, expression, pattern, type, etc (nonexhaustive?).

I would prefer not to block stabilization on this, too many unresolved questions.
Imaging the macro call context like this:

#[non_exhaustive] // !
enum MacroCallContext {
    Expression, // Statement?
    Pattern,
    Type,
    Statement, // Item? Expression?
    Item, // Statement?
    TraitItem, // Item?
    ImplItem, // Item?
    ForeignItem, // Item?
    Unknown, // !
}

First, an explicitly Unknown context should be supported, because macros should be expandable in situations like eager expansion where we just paste tokens into tokens without any AST-like context.

Second, the AST-like context itself is pretty vague.

fn main() {
    // Item statement - item context or statement context?
    #[attr]
    struct S;

    // Trailing position in a block - statement or expression?
    mac!()
}

Should we report trait/impl/foreign items as just items, given the ongoing syntax unification effort?

The TokenStream model was chosen to avoid answering AST-related questions like this, and with the macro call context support they will kinda creep in again.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Feb 15, 2020

@dtolnay
Would it be practically possible to make a new major quote release with the new hygiene default?
Most proc macro crates will be able to migrate trivially.
Macros with hygiene mismatches like those found in #68716 (comment) will be able to migrate at their own pace and use the old version in the meantime without any breakage.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 15, 2020

the AST-like context itself is pretty vague

Regarding this point, my understanding is it's not vague at all if we define syntax positions precisely in terms of parsable output. Something like:

  • If returning let (); from the macro would not be a parse error, then statement.
  • Otherwise if returning return from the macro would not be a parse error, then expression.
  • Otherwise if returning ! from the macro would not be a parse error, then type.
  • Otherwise if returning _ from the macro would not be a parse error, then pattern.
  • Otherwise if returning struct S; from the macro would not be a parse error, then item.
@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 15, 2020

Would it be practically possible to make a new major quote release with the new hygiene default?

It's definitely possible but I am not planning to do this in the near future. =/

@Aaron1011

This comment has been minimized.

Copy link
Member

Aaron1011 commented Feb 15, 2020

If returning let (); from the macro would not be a parse error, then statement.

This might have weird interactions with let-chains, specifically PR #68577

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 15, 2020

That would be extremely surprising to me @Aaron1011, considering the semicolon. The let-chains work involves let-expression which do not have a semicolon.

@jhpratt jhpratt mentioned this pull request Feb 16, 2020
9 of 10 tasks complete
@pksunkara

This comment has been minimized.

Copy link

pksunkara commented Mar 4, 2020

It's definitely possible but I am not planning to do this in the near future. =/

If we are not doing that, what is stopping us from stabilizing this for 1.43?

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Mar 4, 2020

I still don't want to block this stabilization on the "get the current macro invocation context" interface, it needs design and an RFC.

Authors of macros that are expected to be called in expression context can use quote_spanned!(Span::mixed_site(), ...) in the near future even everything here is stabilized as is.

@macpp

This comment has been minimized.

Copy link

macpp commented Mar 4, 2020

Identifiers with this span will resolve at the macro's definition site for local variables, labels and $crate

Small question : how do you actualy use $crate and Span::mixed_site() in proc macro? I'm puzzled with this, because with macros like this :

#[proc_macro]
pub fn call_foo(_i:TokenStream) -> TokenStream {
    let span : proc_macro2::Span = proc_macro::Span::mixed_site().into();
    quote_spanned!(span=> $crate::foo!();).into()
}

#[proc_macro]
pub fn foo(i: TokenStream) -> TokenStream { i }

using call_foo!() results in compiler error: error: macro expansion ignores token $ and any following

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Mar 4, 2020

@macpp
$crate as an identifier can only be produced by macro_rules right now, so a proc macro has to be called from macro_rules to obtain it.
The test has an example.

In principle, we can add a method for creating it in proc macros (Ident::dollar_crate), but nobody proposed and motivated it so far.

@jplatte

This comment has been minimized.

Copy link
Contributor

jplatte commented Mar 4, 2020

@petrochenkov having Ident::dollar_crate would obviate the need for this hack in diesel_derives, right?

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Mar 4, 2020

@jplatte

having Ident::dollar_crate would obviate the need for this hack in diesel_derives, right?

No, $crate would help to refer to diesel_derives from macros, but not to diesel.

@jplatte

This comment has been minimized.

Copy link
Contributor

jplatte commented Mar 4, 2020

Ah, right.

@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.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 10, 2020

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

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Mar 15, 2020

@rust-lang/libs @rust-lang/lang: This PR stabilizes proc_macro::Span::mixed_site.

impl Span {
    pub fn mixed_site() -> Self;
}

A procedural macro that emits mixed site spans behaves equivalently to a macro_rules macro producing the same tokens. In particular, using a mixed site span for the span of a local variable or 'label restricts that name to the hygiene context of the expanded code, which implies both of the following:

  • code outside what is produced by the current macro expansion can't refer to it;
  • it can't refer to something defined outside the current macro expansion.

For all things other than local variables and 'labels, a mixed site span is indistinguishable from a call site span.

This feature is a prerequisite for stabilizing bang proc macros in expression, pattern and statement positions; without it, proc macros in those positions would be as bad as what you would have if macro_rules didn't have hygiene for variable names.

You can find @petrochenkov's more detailed stabilization report in #68716 (comment).

@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 17, 2020

The teams should, in my view, still be interested in the wider ecosystem effects with respect to #68717 and not just the narrow technical aspect of mixed_site. We should make sure we're not inadvertently laying traps for users.

Specifically, I'm somewhat concerned that the recommendation won't be heeded by many due to the relative ergonomics of quote!(...) vs quote_spanned!(Span::mixed_site()=> ...). So having some sort of plan for how to address that would be good. If quote! can change to mixed_site then the issue is resolved, but if not, it might involve adding a second macro to quote (e.g. q!(...)) and deprecating the other macro. (This is primarily directed at @dtolnay.)

Based on the crater results it seems unlikely that changing the quote! span to mixed_site would fly.

Would it be practically possible to make a new major quote release with the new hygiene default?

It's definitely possible but I am not planning to do this in the near future. =/

@dtolnay So given the discussion thus far, do you have any plans for e.g., mitigating the footguns around mixed_site not being the default (which don't involve language extensions like Default for Span)? For example involving deprecation, some timeline for a new major release in the medium term, incentivizing the right hygiene behavior via a new & shorter macro, and/or explicit and better docs. Basically I'd like to be disabused of my concerns that #68717 combined with mixed_site not being "the default" will result in footguns for proc macro authors.

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Mar 17, 2020

@Centril Out of the possibilities you listed, docs are the only one I would do. I don't think the tradeoff of the other ones is justified even if there is a footgun for expression position macros; I am not intending to do deprecations, major release in the medium term, or adding another macro with a different name. A major release would probably be early 2021 at the soonest but likely later.

If the language exposes a way to do it, I would be comfortable changing the default span in expression + pattern + statement positions only, but I don't think that got traction above. If we're doing it, it would need to be available in the same release as stabilizing macros in those positions (not necessarily in the same release as stabilizes mixed_site).

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Mar 17, 2020

@dtolnay I see. Although I think we should do more, I think I would be happy with some nice docs in quote and syn showing users how to avoid the pitfalls. 2021/2022 isn't all that far away anyways. On the upside, this will help nice crates like rocket.

As for exposing a context-aware mechanism in the language, I feel as @petrochenkov outlined in #68716 (comment) that it wouldn't be a good idea, and particularly not worth the implications / complexity for the language long-term given Span::mixed_site.

@Centril Centril added I-nominated and removed needs-fcp labels Mar 21, 2020
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.

You can’t perform that action at this time.