Hygiene opt-out for idents in expansion of declarative macros by alexreg · Pull Request #47992 · rust-lang/rust

alexreg · 2018-02-04T03:33:12Z

This PR adds basic support for hygiene opt-out for idents during macro expansion. This has been previously discussed in #40847 and #39412.

The syntax involves prefixing idents with the # (i.e. pound/hash) character to indicate that the syntax context of the ident should be that of the call site rather than the definition site.

The following now compiles and runs as expected:

#![feature(decl_macro)]macro foo($mod_name:ident){pubmod $mod_name {pubconst #BAR:u32 = 123;}}foo!(foo_mod);fnmain(){println!("{}", foo_mod::BAR);}

as does:

#![feature(decl_macro)]macro foo(){pubmod #foo_mod {pubconst #BAR:u32 = 123;}}foo!();fnmain(){println!("{}", foo_mod::BAR);}

but the following does not:

#![feature(decl_macro)]macro foo($mod_name:ident){pubmod $mode_name {pubconstBAR:u32 = 123;}}foo!(foo_mod);fnmain(){println!("{}", foo_mod::BAR);}

nor does:

#![feature(decl_macro)]macro foo(){pubmod #foo_mod {pubconstBAR:u32 = 123;}}foo!();fnmain(){println!("{}", foo_mod::BAR);}

Questions:

Is the above the behaviour we want?
Should we implement syntax context "lifting" using functionality suggested by RFC? RFC: Eager Macro Expansion rfcs#2320 – this related to the 2nd question of @petrochenkov here
Hygiene opt-out doesn't make any sense for macro_rules macros, of course, but should we explicitly disallow the # syntax in such places? (Currently that is not done.)

CC @jseyfried

rust-highfive · 2018-02-04T03:33:19Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

petrochenkov · 2018-02-05T13:40:57Z

Could you add all the examples from the PR descriptions as tests?
The example from #39412 (comment) needs to be added as a test as well.
It would also be good to test that #foo is accepted in position expecting ident matcher (macro m(arg: ident) { ... }; m!(#foo)) and probably make sure that identifier prefixes $ and # are composable with each other ($#foo - use a macro argument foo while referring to it unhygienically).

alexreg · 2018-02-05T16:57:16Z

@petrochenkov Yep, sounds fair. run-pass and compile-fail tests, I presume? Are you okay with the examples 3 and 4 failing?

Also, thoughts on my above comment about use of the syntax in legacy macro_rules?

alexreg · 2018-02-05T17:21:16Z

Oh, and I think the syntax #$foo makes slightly more sense BTW.

alexreg · 2018-02-06T02:30:21Z

A further question:

Should we do hygiene opt-out for lifetimes too? (This should cover all ident-like tokens that have hygiene, if I'm not mistaken.)

alexreg · 2018-02-06T03:48:12Z

Okay, my present thoughts are that 3 and 4 should indeed fail. To make them work would mean having the macro parser understand items, which would mean binding it more tightly to the normal parser, and really make a mess of things.

pierzchalski · 2018-02-06T04:06:26Z

@alexreg I'm confused as to what it would mean for them to work, even if it would involve reworking the parser. If I understand correctly, in examples 3 and 4 foo!() expands into:

// `main` scope// vvvvvvvpubmod foo_mod {// `foo!` scope// vvvpubconstBAR:u32 = 123;}

In both cases BAR is out of scope in main, right? I'm curious what alternative semantics there are here.

alexreg · 2018-02-06T04:20:22Z

@pierzchalski Well, the alternative semantics would be BAR inheriting its syntax context from foo_mod (since its part of the item whose name is foo_mod). They are complicated semantics though, I admit.

i.e. We have

// `main` scope
// vvvvvvv
pub mod foo_mod {
// `main` scope
// vvv
pub const BAR: u32 = 123;
}

(The same end result in terms of hygiene as for macro_rules! now, but requiring the # ident prefix in example 4.)

pierzchalski · 2018-02-06T04:32:24Z

@alexreg Ah, I see! Yeah, making scopes semantics-aware sounds like a lot of work. It also means you'd need a way to 'un-lift' any identifiers you expected to use privately:

// `main` scope// vvvvvvvpubmod foo_mod {// `main` scope// vvvpubfnbar(){// I want this to be in `foo_mod!` scopebaz();}// Perhaps I want this private to `foo_mod!` but// `pub` because it's used by... other modules generated by `foo_mod!`,// or something else silly.pubfnbaz(){ ...}}

petrochenkov · 2018-02-06T11:48:42Z

@alexreg

Also, thoughts on my above comment about use of the syntax in legacy macro_rules?

If it can be easily prohibited, then let's prohibit it for a start, otherwise any use of the # syntax can be feature gated so it's not available in macro_rules on stable.
(Also, what if a macro_rules is defined inside of macro and uses # inside it? That's an interesting question!)

petrochenkov · 2018-02-06T11:56:31Z

Oh, and I think the syntax #$foo makes slightly more sense BTW.

Why?
I see # is an innate property of the identifier (Ident = {Symbol, Hygiene} ), while $ is "just some syntax" to use that identifier in a certain way (MacroParam = {Ident}).

petrochenkov · 2018-02-06T11:56:59Z

Should we do hygiene opt-out for lifetimes too?

Yes, ideally we should.

petrochenkov · 2018-02-06T11:58:55Z

Okay, my present thoughts are that 3 and 4 should indeed fail.

What is "3 and 4"?

pierzchalski · 2018-02-06T12:01:56Z

@petrochenkov The 3rd and 4th code snippets in the starting post (I was also briefly lost on that).

petrochenkov · 2018-02-06T12:04:56Z

The 3rd and 4th code snippets in the starting post

They should fail then, BAR being inaccessible is kinda the point of macro 2.0 hygiene.

petrochenkov · 2018-02-06T12:43:07Z

Should we do hygiene opt-out for lifetimes too?

cc https://internals.rust-lang.org/t/pre-rfc-splitting-liftime-into-two-tokens/6716

alexreg · 2018-02-06T15:55:59Z

@pierzchalski Exactly. The difficulties outweigh the small benefits and added complexity I think.

alexreg · 2018-02-06T16:01:25Z

@petrochenkov Sounds good to me. If we can solve the spaces problem mentioned there, it will definitely simplify a lot of code. For now I may hack around it.

alexreg · 2018-02-06T17:00:30Z

If it can be easily prohibited, then let's prohibit it for a start, otherwise any use of the # syntax can be feature gated so it's not available in macro_rules on stable.
(Also, what if a macro_rules is defined inside of macro and uses # inside it? That's an interesting question!)

Sure. I'll have a go at this – I don't think it's too hard.

alexreg · 2018-02-06T17:04:06Z

Why?
I see # is an innate property of the identifier (Ident = {Symbol, Hygiene}), while $ is "just some syntax" to use that identifier in a certain way (MacroParam = {Ident}).

I think of it in terms of composability. For a metavar foo, #foo doesn't make sense in general, but taking $foo then prefixing it with # to yield #$foodoes make sense. i.e. The metavar ident needs to be substituted for the actual used ident (macro argument, if you will), before we can talk about applying hygiene.

alexreg · 2018-02-07T01:49:35Z

Okay, tests added (both run-pass & compile-fail) – everything passing locally.

r? @jseyfried

CC @nikomatsakis too, in case @jseyfried is too busy still (I've noticed a bit of activity from him though).

nikomatsakis · 2018-02-08T16:28:08Z

r? @nikomatsakis

jseyfried · 2018-02-09T01:08:21Z

The context of this span needs to match the context of the identifier or lifetime token for proc-macros to be coherent (a proc-macro identifer token is just an interned string and a span; when we reconstruct the libsyntax::TokenTree the interned string turns into an identifier with the span's context).
You might have chosen to use cx.current_expansion.mark for better diagnostics (even if the ident resolves at call site, it should show up in diagnostics at the call site). To achieve this, the plan is for spans to have separate SyntaxContexts for hygiene and for diagnostics. I'm working on a PR for this now.

Yeah, I was in a bit of a conundrum over this... I'm glad you're working on separating out hygiene and diagnostic contexts. To be clear, how should the span be correctly constructed under this new model?
I guess we can wait for your PR to land, before merging this, presuming it won't be too much more work? Do post a link here when you open it.

alexreg · 2018-05-09T01:32:36Z

@petrochenkov @jseyfried Just to confirm, we only want this feature gate to apply to the macro! (macros 2.0) syntax, right?

We want this to fail for the time being, per <rust-lang#47992 (comment)>.

rust-highfive · 2018-05-09T02:53:54Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:56:42] ...........test [run-pass] run-pass/mir_heavy_promoted.rs has been running for over 60 seconds
[00:57:11] .........................................................................................
[00:57:30] ..................................................................................ii................
[00:58:27] ..............................................i....................................................i
[00:58:32] .ii............test [run-pass] run-pass/saturating-float-casts.rs has been running for over 60 seconds
[00:59:31] .......iiiiiii......................................................................................
[00:59:53] ....................................................................................................
[01:00:13] ....................................................................................................
[01:00:32] .................................................................................
---
[01:03:35] ---- [compile-fail] compile-fail/feature-gate-macro-hygiene-optout.rs stdout ----
[01:03:35] [01:03:35] error: compile-fail test compiled successfully!
[01:03:35] status: exit code: 0
[01:03:35] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/compile-fail/feature-gate-macro-hygiene-optout.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/feature-gate-macro-hygiene-optout.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/feature-gate-macro-hygiene-optout.stage2-x86_64-unknown-linux-gnu.aux" "-A" "unused"
[01:03:35] ------------------------------------------
[01:03:35] [01:03:35] ------------------------------------------
[01:03:35] stderr:
[01:03:35] stderr:
[01:03:35] ------------------------------------------
[01:03:35] [01:03:35] ------------------------------------------
[01:03:35] [01:03:35] thread '[compile-fail] compile-fail/feature-gate-macro-hygiene-optout.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3033:9

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

rust-highfive · 2018-05-09T04:08:37Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:51:23] .....................test [run-pass] run-pass/mir_heavy_promoted.rs has been running for over 60 seconds
[00:51:44] ...............................................................................
[00:52:03] ..................................................................................ii................
[00:52:52] ..............................................i....................................................i
[00:53:05] .ii...............................test [run-pass] run-pass/saturating-float-casts.rs has been running for over 60 seconds
[00:53:53] .......iiiiiii......................................................................................
[00:54:14] ....................................................................................................
[00:54:32] ....................................................................................................
[00:54:50] .................................................................................
---
[00:55:01] ....................................................................................................
[00:55:07] ....................................................................................................
[00:55:14] ....................................................................................................
[00:55:20] ....................i............................................................ii.iii.............
[00:55:26] ..............................................................................................F.....
[00:55:39] ....................................................................................................
[00:55:44] ..................i.................................................................................
[00:55:51] ....................................................................................................
[00:55:59] ....................................................................................................

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

alexreg · 2018-05-10T00:56:07Z

How are we looking now, @petrochenkov / @jseyfried? :-)

joshtriplett · 2018-05-10T19:46:38Z

We discussed this in the language team meeting, and we agree that this needs an actual RFC to specify this for review. This is not the kind of thing we should just do without an RFC.

alexreg · 2018-05-10T19:57:09Z

@joshtriplett Okay. What I'll do then is get @petrochenkov & @jseyfried's feedback now, and then start writing up an RFC including what's been done (and learnt) in this PR, plus future plans. Maybe we can leave this open in the meanwhile, then once that's accepted, we can hopefully get this merged with few changes?

emilyalbini · 2018-05-10T20:19:48Z

@alexreg I'd prefer for this PR to be closed in the meantime, otherwise triage would have to check this PR every week. Then when you're ready you can open it again.

alexreg · 2018-05-10T20:44:37Z

@pietroalbini As long as it can be reopened easily, sure. Hopefully @petrochenkov & @jseyfried will see my above comments anyway, and leave some feedback before I get to writing the RFC.

petrochenkov · 2018-05-13T16:48:53Z

+pub struct Escaper(pub SyntaxContext);
+
+impl Folder for Escaper {
+ fn fold_ident(&mut self, mut ident: Ident) -> Ident {


Overriding fold_ident is not necessary, overridden new_span applies to identifiers as well.

Ah, I see. Why is it necessary in the Folder implementation for Marker though?

It isn't necessary there either, it's just something I forgot to remove in #49154.

petrochenkov · 2018-05-13T16:56:37Z

@alexreg

Just to confirm, we only want this feature gate to apply to the macro! (macros 2.0) syntax, right?

I'm not even sure there's a need in a separate feature gate beyond feature(decl_macro), but I guess it won't hurt.
In #47992 (comment) I suggested how to treat # in the most forward-compatible way.

petrochenkov · 2018-05-13T17:00:08Z

@alexreg

Hopefully @petrochenkov ... leave some feedback before I get to writing the RFC.

Feedback: the implementation looks good to me, but if lang team thinks it needs an RFC, then it needs an RFC.

emilyalbini · 2018-05-13T21:25:23Z

Ok, closing this until an RFC is approved. @alexreg when you're ready to work on this again just click the reopen button ;)

petrochenkov · 2018-05-13T21:30:38Z

@pietroalbini
FYI, only organization members can click the reopen button (unless PR is closed by author).

alexreg · 2018-05-13T21:47:53Z

@pietroalbini It turns out I don't have the rights to reopen my PRs (as I thought), but I'll ping you or someone else when it's time, sure!

alexreg · 2018-05-13T22:24:39Z

@petrochenkov There's no easy way to get whether we're in legacy mode from within the quoted::parse fn, is there? I have to add a new parameter, right? (Just reworking a few things according to your latest feedback.)

alexreg · 2018-05-13T22:37:14Z

@petrochenkov Also, should the errors be emitted when doing the actual parsing of the macro or the expansion?

petrochenkov · 2018-05-13T23:24:51Z

@alexreg

There's no easy way to get whether we're in legacy mode from within the quoted::parse fn, is there?

Don't know, looks like no.

Also, should the errors be emitted when doing the actual parsing of the macro or the expansion?

Ideally, if some error will be reported at any expansion, then it should be reported at macro definition.
But I'm not even sure we are looking at macro's contents on definition beyond building a basic token tree, so if it's not reasonably doable, then everything can be reported during expansion.

alexreg · 2018-05-13T23:33:58Z

@petrochenkov Makes sense, thanks. I think we can bail within the quoted::parse method for the errors mentioned in your comment here.

alexreg · 2018-05-13T23:48:43Z

@petrochenkov Incidentally, do you want to disallow $#ident with an explicit error message about that syntax being unsupported, or just the error message for "unexpected # symbol" or such...?

I'm not sure what the error message should be when an interpolated token follows # either. Any ideas?

petrochenkov · 2018-05-13T23:55:44Z

Something like "Hygiene opt-out is not supported for macro parameters" or "... on the left side of the macro".
"Hygiene opt-out is not supported for interpolated tokens".

alexreg · 2018-05-14T00:07:18Z

Hmm, what are interpolated tokens? Something to do with proc macros I seem to recall, but I don’t really know. The term might confuse users, no?

alexreg · 2018-05-14T00:49:10Z

"Hygiene opt-out is not supported for macro parameters"

The problem with this is, we're allowing #$ident but not $#ident.

alexreg · 2018-05-14T02:46:37Z

@petrochenkov Pushed new commits anyway, in case you want to have a look. May make another, depending on your answers to the above two queries, but otherwise I'll focus on the RFC now.

rust-highfive assigned petrochenkov Feb 4, 2018

emilyalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 4, 2018

rust-highfive assigned jseyfried and unassigned petrochenkov Feb 7, 2018

rust-highfive assigned nikomatsakis and unassigned jseyfried Feb 8, 2018

petrochenkov self-assigned this Feb 8, 2018

jseyfried reviewed Feb 9, 2018
View reviewed changes

This comment has been minimized.
Sign in to view

Alexander Regueiro added 2 commits May 9, 2018 02:47

Fixed bug with modification of SyntaxContext for escaped idents.
e9991a0

Added compile-fail test for $#metavar hygiene opt-out syntax.
61c2a4d
We want this to fail for the time being, per <rust-lang#47992 (comment)>.

Added feature gate (macro_hygiene_optout).
c7f158b

petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2018

petrochenkov reviewed May 13, 2018
View reviewed changes

emilyalbini closed this May 13, 2018

petrochenkov mentioned this pull request May 20, 2018
Tracking issue for "macro naming and modularisation" (RFC #1561) #35896
Closed

Uh oh!

Uh oh!

Conversation

alexreg commented Feb 4, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Feb 4, 2018

Uh oh!

petrochenkov commented Feb 5, 2018

Uh oh!

alexreg commented Feb 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexreg commented Feb 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexreg commented Feb 6, 2018

Uh oh!

alexreg commented Feb 6, 2018

Uh oh!

pierzchalski commented Feb 6, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexreg commented Feb 6, 2018

Uh oh!

pierzchalski commented Feb 6, 2018

Uh oh!

petrochenkov commented Feb 6, 2018

Uh oh!

petrochenkov commented Feb 6, 2018

Uh oh!

petrochenkov commented Feb 6, 2018

Uh oh!

petrochenkov commented Feb 6, 2018

Uh oh!

pierzchalski commented Feb 6, 2018

Uh oh!

petrochenkov commented Feb 6, 2018

Uh oh!

petrochenkov commented Feb 6, 2018

Uh oh!

alexreg commented Feb 6, 2018

Uh oh!

alexreg commented Feb 6, 2018

Uh oh!

alexreg commented Feb 6, 2018

Uh oh!

alexreg commented Feb 6, 2018

Uh oh!

alexreg commented Feb 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikomatsakis commented Feb 8, 2018

Uh oh!

jseyfriedFeb 9, 2018

Choose a reason for hiding this comment

Uh oh!

alexregFeb 9, 2018

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

alexreg commented May 9, 2018

Uh oh!

rust-highfive commented May 9, 2018

Uh oh!

rust-highfive commented May 9, 2018

Uh oh!

alexreg commented May 10, 2018

Uh oh!

joshtriplett commented May 10, 2018

Uh oh!

alexreg commented May 10, 2018

Uh oh!

emilyalbini commented May 10, 2018

Uh oh!

alexreg commented May 10, 2018

Uh oh!

petrochenkovMay 13, 2018

Choose a reason for hiding this comment

Uh oh!

alexreg commented Feb 4, 2018•
edited
Loading

alexreg commented Feb 5, 2018•
edited
Loading

alexreg commented Feb 5, 2018•
edited
Loading

pierzchalski commented Feb 6, 2018•
edited
Loading

alexreg commented Feb 7, 2018•
edited
Loading