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 macros in some more positions #63931

Open
wants to merge 2 commits into
base: master
from

Conversation

@petrochenkov
Copy link
Contributor

commented Aug 27, 2019

  • Fn-like macros and attribute macros in extern blocks
  • Fn-like procedural macros in type positions
  • Attribute macros on inline modules (moved to #64273)

Stabilization report: #63931 (comment).

Closes #49476
cc #54727

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

commented Aug 27, 2019

r? @eddyb

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

src/libsyntax/ext/expand.rs Outdated Show resolved Hide resolved
src/libsyntax/ext/expand.rs Outdated Show resolved Hide resolved
@Centril

This comment was marked as resolved.

Copy link
Member

commented Aug 27, 2019

  • Fn-like macros and attribute macros in extern blocks
  • Fn-like procedural macros in type positions
  • Attribute macros on inline modules

From a language POV these all seem orthogonal and it seems like that's true from the POV of the compiler code as well. As such, I would like us to consider these separately in 3 different stabilization PRs with associated reports & FCPs.

  • Fn-like procedural macros in type positions

In the associated report, it would be good to elaborate re. hygiene, how this may e.g. interact with expressions in type contexts, and give a recap (because issues re. hygiene isn't the best known subject...) of the issues wit proc macros expanding to expressions and why it isn't an issue here.

@bors

This comment was marked as resolved.

Copy link
Contributor

commented Aug 27, 2019

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

@petrochenkov petrochenkov force-pushed the petrochenkov:stabmac branch from da2eadb to 910ba02 Aug 28, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

Stabilization report

All the features below target Rust 1.39 (November 7th 2019).
Tests can be found in test files touched by this PR.

Fn-like macros and attribute macros in extern blocks

Macro attributes are now supported on items in extern blocks, the behavior is the same as for attributes on free/trait/impl items available on stable.

The feature name is #![feature(macros_in_extern)].
The feature was implemented in April 2018 in #49350 to fix issue #48747.
It didn't go through an RFC because the feature seemed to be a trivial extension consistent with already supported macros in item and item-like positions.

Fn-like procedural macros in type positions

Proc macros can now be used in type positions (type A = foo!();), the behavior is the same as for macro_rules macros in the same position, which are available on stable.
The difference is that proc macros on stable use call-site hygiene, while macro_rules use mixed call-site/def-site hygiene, but that difference only affects local variables, labels and $crate so it doesn't apply to types (or at least applies to types no more than to item-position macros, which are available on stable).

The feature is also a part of the polyphyletic #![feature(proc_macro_hygiene)].
The feature was implemented together with all other proc macros somewhere in 2017-2018.
It wasn't stabilized yet because it somehow got lost during the Macro 1.2 stabilization wave, and nobody actively pushed it after that.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

@Centril

it would be good to elaborate re. hygiene, how this may e.g. interact with expressions in type contexts, and give a recap (because issues re. hygiene isn't the best known subject...) of the issues wit proc macros expanding to expressions and why it isn't an issue here.

Proc macros in expressions and patterns have mostly philosophical issues - they can 1) consume local variables from their environment code and 2) can produce local variables consumable from their environment code.
Given that proc macros on stable can only use "unhygienic" Span::call_site hygiene, and unhygienic local variables are bad, proc macros in expression and patterns are prohibited in general.
Those philosophical issues do not apply to types, they can neither produce nor consume local variables.

The technical issue with expressions and patterns is that local variables stress the hygiene algorithm more and are more likely to hit some corner cases that may certainly exist because the hygiene algorithm does a few questionable things and is not fully understood.
That said, you still can work that feature gate around with stable item macros (https://crates.io/crates/proc-macro-hack), but some hope exists that proc-macro-hack is still a more limited exposure than arbitrary stable expression/pattern macros would be.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

As such, I would like us to consider these separately in 3 different stabilization PRs with associated reports & FCPs.

The features are pretty trivial (that's why they were selected for immediate stabilization in the first place), I can split the PR if the lang team wants, but that would mostly create extra work for everyone involved, IMO.

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

The features are pretty trivial (that's why they were selected for immediate stabilization in the first place), I can split the PR if the lang team wants, but that would mostly create extra work for everyone involved, IMO.

Fair enough; the types bit is the only bit I currently have worries about (see below).

Those philosophical issues do not apply to types, they can neither produce nor consume local variables.

How does this affect interactions with VLAs in the style of #48055 or run-time-value-dependent typing more generally? For example, let x: type_mac!() = ...; looks like it could "consume" a local let n if unhygienic and producing [u8; dyn n] or some such?

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

r? @Centril (r=me on the code itself once FCP completes)

@rust-highfive rust-highfive assigned Centril and unassigned eddyb Aug 31, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

@Centril

How does this affect interactions with VLAs in the style of #48055 or run-time-value-dependent typing more generally? For example, let x: type_mac!() = ...; looks like it could "consume" a local let n if unhygienic and producing [u8; dyn n] or some such?

To clarify, Span::call_site hygiene generally works for local variables (I haven't seen bug reports for it at least), and already available on stable after jumping through a couple of hoops, e.g.

#[proc_macro]
fn item_macro(input) {
    "fn item() {
        ${input}
        let y = x; // OK    
    }"
}

item_macro!(let x = 10);

As I understand, the nrc's desire was to limit it (#52121 (comment)) rather than prohibit completely.
So further minor leaks like [u8; dyn n] should be ok.

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

First, sorry about the delay here but I think I need to understand better...

As I understand, the nrc's desire was to limit it (#52121 (comment)) rather than prohibit completely.

cc @nrc, @pnkfelix (maybe back from vacation soon?), @samth, and @rust-lang/lang

So further minor leaks like [u8; dyn n] should be ok.

So I feel a distinct lack of understanding re. what minor constitutes here. Would e.g. [u8; dyn { n = 3; n }] change anything?

@Centril Centril added the I-nominated label Sep 3, 2019

@nrc

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

As I understand, the nrc's desire was to limit it (#52121 (comment)) rather than prohibit completely.
So further minor leaks like [u8; dyn n] should be ok.

To clarify, I don't have a specific objection, just a general level of uncertainty about our macro hygiene in general. I'd prefer to not stabilise anything until we resolve that uncertainty, but that might have happened since I was last involved (or might be unrealistic to expect).

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

So I feel a distinct lack of understanding re. what minor constitutes here. Would e.g. [u8; dyn { n = 3; n }] change anything?

Minor as in amount of code it's used in, so it less likely to hit corner cases statistically.
(n = 3 doesn't change anything.)
Anyway, we can feature gate dyn n in expansions specifically, if it's ever implemented.

@eddyb

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

[T; dyn expr] in types isn't at all a thing that I've ever seen proposed.

The VLA(-like) proposal was for [expr; dyn expr] evaluating to a [T] value.

Runtime expressions in types amount to dependent typing, which we can all agree is far away, if ever, and for the time being these proc macros don't really have issues like that.


That said, the interesting case IMO is this:

macro_rules! foo {
    ($x:ident, $e:expr) => ([(); {
        let $x = 0;
        let x = 1;
        $e
    }])
}

macro_rules! bar {
    ($x:ident, $e:expr) => ([(); {
        let $x = 0;
        let $x = 1;
        $e
    }])
}

const _: foo!(x, x + 1) = [()];
const _: bar!(x, x + 1) = [(), ()];

If you were to implement either foo or bar as proc macros, would they both behave like bar?

However, even if that is the case, since the expression using x must be passed into the macro for the distinction to matter, the proc macro could very well "scavenge" for the right identifier in the input to produce the unhygienic behavior anyway.

So it does help that types are "closed expressions", wrt local bindings in scope.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

I've just noticed that this PR stabilizes the next code as well:

#[my_attr]
mod m {
    mod n; // Out-of-line module, how should `my_attr` see it in its input?
}

So, we additionally need to feature gate out-of-line modules in attr/derive input.

This may affect stable in corner cases:

#[my_attr]
fn m() {
    #[path = "zzz.rs"]
    mod n;
}

UPDATE: Done in #64273.

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

I've just noticed that this PR stabilizes the next code as well:

Hmm; can we move the inline module bits to a different PR then?

@Robbepop

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

I currently see problems stabilizing custom attributes on inline modules due to #43081.
The effect of this Rust compiler bug is that instead of receiving proper errors in your custom attributes that point to concrete spans of the source code you receive errors that completely lost all span information which is kind of bad for user experience.

I would seriously not stabilize this as long as #43081 is not fixed.

Btw.: Does anybody know what the current state of #43081 is? The tracking issue wasn't updated in a while.

Hmm; can we move the inline module bits to a different PR then?

I think this is the way to go.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2019

@Robbepop

I currently see problems stabilizing custom attributes on inline modules due to #43081.

#43081 affects all proc macros, why should it block stabilization of module macros specifically?

@Centril

can we move the inline module bits to a different PR then?

Done in #64273.

Stabilize macros in `extern` blocks
Add some tests for macros in extern blocks, remove duplicate tests

@petrochenkov petrochenkov force-pushed the petrochenkov:stabmac branch from 910ba02 to b9dcb72 Sep 8, 2019

@Robbepop

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

I currently see problems stabilizing custom attributes on inline modules due to #43081.

I might be confused about this and maybe I am missing details but I do not have any span information problems with any kind of proc. macros that are not applied to inline-modules. But for the inline-module macro all span information is missing. So if you are right and my examples do generally not apply I am fine with stabilization.

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 8, 2019

We discussed this proposal on this week's language team meeting.

@nrc attended and elaborated on their concerns in #63931 (comment). To summarize, they felt uncertain about our hygiene story and many in the language team concurred that we don't have the best understanding of hygiene. In the best of worlds, we would perhaps want a PhD thesis around hygiene under the supervision of @samth to gain deep understanding. Realistically however, that might not materialize for a long time and would take even more time to complete.

We did however note that proc-macro-hack exists and so we don't expect that we would make the situation worse by stabilizing. On the upside, we were also able to transition to Rust 2018 and hygien-ize things. We noted that where problems might arise, it would be in the edge cases. I also noted on the meeting that some work has also been done by @petrochenkov and @matthewjasper since then so the situation should have improved a bit at least.

In summary, despite the aforementioned risks, we believe we are ready to propose a final comment period at this point.

@rfcbot merge

@rfcbot

This comment has been minimized.

Copy link

commented Sep 8, 2019

Team member @Centril 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.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

@Robbepop

I might be confused about this and maybe I am missing details but I do not have any span information problems with any kind of proc. macros that are not applied to inline-modules.

Could you give some minimized reproduction of the span issues you are encountering?
Perhaps they are related to e.g. inner attributes or cfgs, that are just more likely to be used with modules, or something like that.

@Robbepop

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

@Robbepop

I might be confused about this and maybe I am missing details but I do not have any span information problems with any kind of proc. macros that are not applied to inline-modules.

Could you give some minimized reproduction of the span issues you are encountering?
Perhaps they are related to e.g. inner attributes or cfgs, that are just more likely to be used with modules, or something like that.

This is what I could come up with quickly:
https://github.com/Robbepop/missing_spans
Go to the tests/ui where you can find tests of this minimal proc macro and the expected output.
Note that for the test that uses a random inner attribute in the module body the span information is completely missing.

So, yes you are right that the span information is only lost as soon as you use an inner attribute within the proc-macro attribute. However, given how long this issue is already existing and unfixed (last comment in thread from 9th May 2019) I do not think that this is going to be fixed soon (maybe I am a bit too pessimistic about this 🙃 ) and it really badly hurts UX, especially (as you said) for modules since they are more prone to use inner attributes than other more local scopes. Don't get me wrong, I would actually love to see this being stabilized and maybe stabilization would motivate fixing #43081.

@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.