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

Change visibility scoping rules for macro_rules macros #3067

Closed
wants to merge 3 commits into from

Conversation

rylev
Copy link
Member

@rylev rylev commented Jan 25, 2021

This RFC proposes to change the current visibility scoping rules for macro_rules! macros to the same rules as all other items, namely private by default and exported through pub or pub($PATH). The use of #[macro_export] and #[macro_use] become hard errors.

In addition to laying out the end goal, this RFC also proposes a transition plan as well as alternatives to that plan.

Rendered

@Julian-Wollersberger
Copy link

Julian-Wollersberger commented Jan 25, 2021

An alternative migration plan would be to allow pub macro_rules in all editions, emit deprecation warnings for #[macro_use], #[macro_export] and textual scoping in Edition 2018 and make them hard errors in Edition 2021. Thus the criterias from the Edition RFC should be satisfied:

  • As with today, each new version of the compiler may gain stabilizations and deprecations.
  • When opting in to a new edition, existing deprecations may turn into hard errors, and the compiler may take advantage of that fact to repurpose existing usage

This would mean that the mixed scoping behavior is only present in Edition 2018 (and probably Edition 2015), but Edition 2021 is fully migrated. I'm not sure if this still counts as having a "less than ideal intermediate state", but at least the new edition would have the consistent behaviour.

The concern about "too exciting edition" still applies though. Any macro that is defined in one file and used in another file would get a warning. Which probably are most macro usages.

How were the lints handled when Edition 2018 came out? (This could be described in the Prior Arts section)
For bare_trait_objects, it was allow-by-default in Rust 1.31 and changed to warn-by-default in Rust 1.37. I haven't looked at other lints.

EDIT: After thinking more about it, this feels like a bad idea. Emitting a warning on most existing macro usages, without an opt-in or transition period, would probably be to much churn.

@jonas-schievink jonas-schievink added A-macros Macro related proposals and issues A-privacy Privacy related proposals & ideas T-lang Relevant to the language team, which will review and decide on the RFC. labels Jan 26, 2021
@nalply
Copy link

nalply commented Feb 2, 2021

I don't know. Changing scoping rules for all macros by example at the same time? This might cause a lot of churn.

Perhaps we should just add another way to define macros by example. The old way should continue to work and be deprecated later. The downside is, of course, that the tools all need to know about the multiple ways a macro can be defined. The upside is that a gradual migration to the new way is possible.

I currently see two or three bikesheds: require an attribute #[enable_textual_scoping] or similar to opt into textual scoping. The RFC mentions this as an alternative. Or use a different identifier than macro_rules or a keyword like macro.

Example (just a bikeshed):

// `find_min!` will calculate the minimum of any number of arguments.
pub macro find_min {
    // Base case:
    ($x:expr) => ($x);
    // `$x` followed by at least one `$y,`
    ($x:expr, $($y:expr),+) => (
        // Call `find_min!` on the tail `$y`
        std::cmp::min($x, find_min!($($y),+))
    )
}

@m-ou-se
Copy link
Member

m-ou-se commented Feb 2, 2021

Or use a different identifier than macro_rules or a keyword like macro.

pub macro already exists for 'macros 2.0' (example). Those have new hygiene rules and also behave like regular items w.r.t. visibility and scoping, and are already in use in std. This RFC proposes to make macro_rules! start follow these regular visibility and scoping rules, without having to wait for macros 2.0 to be finished.

@nalply
Copy link

nalply commented Feb 2, 2021

pub macro already exists for 'macros 2.0' (example).

Perhaps the RFC should mention 'macros 2.0' and how the proposal relates to it.

By the way: https://github.com/rust-lang/rfcs/blob/master/text/1584-macros.md, is it about this?

@rylev
Copy link
Member Author

rylev commented Feb 4, 2021

Changing scoping rules for all macros by example at the same time? This might cause a lot of churn.

I'm certainly sympathetic to the churn argument though I'm pretty hesitant to introduce an intermediary state, and I'm very hesitant to propose new syntax.

Perhaps the RFC should mention 'macros 2.0' and how the proposal relates to it

I can add a small thing about macros 2.0. Essentially this proposal exists completely independently of macros 2.0. Macros 2.0 is not necessarily the natural end point for the future evolution of this proposal, though there is a the upside that the scoping rules of macros 2.0 aligns with this proposal perfectly which would make an eventual move to macros 2.0 potentially smoother.

@danielhenrymantilla
Copy link

danielhenrymantilla commented Feb 5, 2021

By the way, regarding shadowing, I recently came up with a "more realistic" example of the callback-based pattern that requires that shadowing be allowed in order for the macro to be called multiple times in the same scope: URLO post

@porglezomp
Copy link

porglezomp commented Feb 8, 2021

Removing #[macro_use] immediately is probably too much—there are still crates that don't work properly with use-based macro imports (or they require a bunch of undocumented extra work to use?) because they rely on other internal macros. Diesel is a prominent example of this (although in that particular case it will be fixed whenever their next release is).

@nrc
Copy link
Member

nrc commented Feb 9, 2021

I would much prefer focussing energy on pub macro with the goal of eventually (soft) deprecating macro_rules rather than changing the behaviour of macro_rules. I think any significant changes will be very hard to make backwards compatible and will be confusing for users.

Also, macro_rules! is, to some extent, just a regular macro, so I suspect that implementing the transition plan will be complex and possible not possible with full back compat (I do not recall the details of the implementation any more, so I might be wrong).

@nikomatsakis
Copy link
Contributor

This arguably goes directly against the edition system as laid out in RFC 2052:

Actually, it doesn't. The language of the RFC was very clear that you should get warnings in the latest compiler release. This basically means that so long as we have the migration lints, we're ok. It's not require that the warnings are there for the entire edition or anything.

I think any significant changes will be very hard to make backwards compatible and will be confusing for users.

I'm not sure if I agree with either part of this. @petrochenkov opened a PR which adds the ability to "opt-in" macro-rules to path-based resolution, and it's very little code (actually negative line count). What's not accounted for in that PR is adding in the migration code to convert foo! to super::foo! when in a child module. (It's true that recursively defined macros cannot be trivially migrated; I think that's not a problem, we don't guarantee 100% migration, though it'd be nice if we could describe the replacement pattern.)

Regarding confusion, can you say more about what would be confusing? It seems to me that "macro-rules behaves like any other item" is not confusing at all.

I would love to have macro come out, but I don't think it's on a "short path to success". It seems silly to wait and block relatively easy progress on that basis. I guess the counter-argument would be that we go actively recruit some hygiene and macro expertise and put design energy into macro, and that's plausible, but it's not happened for several years. (I think there are a number of things we would want to address when we use that keyword -- improved hygiene, some of the details of how fragments work, etc).

@nrc
Copy link
Member

nrc commented Feb 11, 2021

Regarding confusion, can you say more about what would be confusing? It seems to me that "macro-rules behaves like any other item" is not confusing at all.

I was thinking of the transition rather than for new code, I agree that the ultimate behaviour is not confusing. I was thinking that something like having a macro call inside another macro might result in a confusing error when migrating. Or perhaps calling macros between crates with different editions?

It is encouraging if implementation was easy. Perhaps we can also provide enough tooling and nice enough error messages that it could be not confusing.

@nikomatsakis
Copy link
Contributor

We discussed this RFC during our @rust-lang/lang meeting today (minutes). The TL;DR was that we are almost happy to move this RFC to merge, but before doing so we would like to see the question of recursive macro definitions examined a bit more deeply.

What pattern can we use to replace the recursive pattern? It is not, we believe, required for this to be automatically migrated, but it would be nice to have a recommendation for what pattern to use instead. Is it, for example, possible to use nested modules to achieve the same effect?

The other migration questions we believe can be left as open questions:

  • Would we try to transition for Rust 2021 to the new system, or a later edition?
  • What are the precise migrations we offer and how effective?

@danielhenrymantilla
Copy link

danielhenrymantilla commented Feb 17, 2021

EDIT: Hiding the stale part of the comment

Regarding the migration fixes, the "algorithm" the RFC suggests will also fail in the following case:

#[macro_use]
mod parent {
    #[macro_use]
    mod private { macro_rules! m {() => ()} }
}
m!();

since it would become:

mod parent {
    mod private { pub(in crate) macro_rules! m {() => ()} }
}
parent::private::m; // Error, module `private` is private

I think that the better solution, that would also cover recursive macros, is to try and use the macros when moving across namespaces.

The naïve approach is to do

- #[macro_use]
+ pub(crate) use self::foo::{each_macro_in_foo_s_scope};
  mod foo { … }

The issue then would be to accidentally bring items from the other namespaces into scope.

In that regard, a possible solution –and I think it would even be useful for more general purposes– would be to have an attribute to restrict an import to a specific namespace:

use some::path::{#[from_namespace(macro)] a_macro};

But, awaiting such feature, if it ever happens, I could imagine the following transformation:

+ pub(crate) mod __macro_some_name__ {
+   pub(crate)
    macro_rules! some_name { … }
+ }
+ pub(crate) use __macro_some_name__::*;

Then, a #[macro_use]-ing module would become:

- #[macro_use]
+ pub(crate) mod __macrouse__ {
+     pub(crate) use foo::{
+         __macro_some_name__::*,
+         __macrouse__::*, // if nested
+     };
+ }
+ pub(crate) use __macro_use__::*;
  mod foo …

Although it may be annoying, transitively re-exporting the macros as is done here (ideally, using the approach with the #[from_namespace] attribute) is the one that better matches the current semantics of macro scoping, and thus, the more resilient to the more complex edge cases.

For instance, as a nice side-effect, it leads to recursive macros Just Working.


I still think, however, that there should be a way to opt out of the new namespacing behavior / to opt into the legacy macro-scoping behavior, in order to be able to use the shadowing trick to be able to write CPS macros "in item position".


FWIW, for the specific case of recursive macros, provided they be defined in a reachable module (which is already relied on the previous status of the RFC; truth be told, this may already be the case of most macro usages and definitions out there), would be for the std to provide some intrinsic magic macro that would mimic the behavior of the elsewhere-suggested $self magic module:

match_module_path! { $cur_module => (
    macro_rules! list {
        () => ( () );
        (
            $hd:expr $(, $remaining:expr)* $(,)?
        ) => (
            ($hd, $cur_module::list![ $($remaining),* ])
        );
    }
)}
  • (Or match! module_path!() { $cur_module => ( if we used the "eager evaluation of arbitrary macros" generalization idea, but I'm going off topic now…)

@rylev
Copy link
Member Author

rylev commented Feb 18, 2021

I've add a new section on common patterns for macros along with how they'd be translated in the new system. Unfortunately, for some of these patterns we don't yet have a good answer on how they'd be translated. I'm going to work on a prototype implementation which can help us spec these things out.

Regarding the migration fixes, the "algorithm" the RFC suggests will also fail in the following case

I've also added a quick "fix" for this to the algorithm. We can simply make the submodule pub(crate) if it's not pub(crate) or pub already. This might lead to breakage otherwhere in certain cases where items that were once private are no longer (leading to name resolution clashes), but it does work in the general case.


The `#[macro_use]` annotation has two meanings.

First, when applied to a module it makes all all the macros in that module useable below where that module is defined. Fixing this use requires marking all the macros in that module as `pub` and importing them into the parent module.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
First, when applied to a module it makes all all the macros in that module useable below where that module is defined. Fixing this use requires marking all the macros in that module as `pub` and importing them into the parent module.
First, when applied to a module it makes all the macros in that module useable below where that module is defined. Fixing this use requires marking all the macros in that module as `pub` and importing them into the parent module.

Copy link

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RFC is getting to a very comprehensive point, good job!

```rust
pub mod m {
mod n { // n is private
macro_rules! my_macro {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
macro_rules! my_macro {
pub macro_rules! my_macro {


Naively changing this to path based scope would not work as it is not guaranteed that the unqualified `print_expr` name is in scope. In the example above, `print_expr!` is used recursively inside the macro, but in a path scoped system the recursive call would not be in scope if the macro was called with a qualified path (e.g., user calls `m::print_expr!` which references unqualified `print_expr!` which is not in scope).

A possible way to handle this is to introduce a new macro specific keyword `$self` which is directly analogous to `$crate` except that it refers to the module where the macro is defined. This would work in the simple case but quickly breaks down in more complicated module paths. For example:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also the question of macros that would use $self as a capture; but I don't think rustfix would encounter a recursive and $self-binding macro very often 🙃

Here `__private` is defined twice which leads to an error.

How this should be overcome is not yet known. Possible ideas are:
* allowing macros defined inside of other macros to shadow one another. This may lead to ambiguities.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* allowing macros defined inside of other macros to shadow one another. This may lead to ambiguities.
* allowing macros defined inside of other macros to shadow one another (possibly through some opt-in attribute annotation; see _Opting into textual scoping_ below)). This may lead to ambiguities.


How this should be overcome is not yet known. Possible ideas are:
* allowing macros defined inside of other macros to shadow one another. This may lead to ambiguities.
* Disallow this use case

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this use case is required to express some more advanced patterns, as I've already voiced before. That being said, I have in mind having a with_unique_id helper (proc-)macro that would help dodge the issue, so it would be possible to have a library-defined workaround around that potential language limitation (especially when ::core could be offering an officially-blessed version of it; a unique-id-lending macro would be useful for many macro authors).

Comment on lines +267 to +272
### Disadvantages

There are several downsides to this proposal:
* The existence of two scoping systems at once can be confusing especially if users mix the two usages.
* Currently, it is possible to "convert" a macro to use path based scoping [by reexporting a macro from a module](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=275c52e038b5ab3db526a605874fbd17), but this use case is rare. The proposal above would likely see much deeper mixing of the two systems.
* Even if the user never uses the deprecated annotations, macros defined and used locally to a module still follow textual scoping rules unlike every other item in the language.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach is definitely very interesting. One hiccup to keep in mind, though, is the following:

fn foo () {}
mod scoped {
    use super::foo;

    /* textual */ macro_rules! foo {() => ()}
}
/* textual */ macro_rules! foo {() => ()}

This is code, that, IIUC, would not trigger warnings on this hypothetical 2021 edition, but then the next edition that removes textual scoping would have to deal with it, where it features the shadowing problem (which can be fixed as mentioned above in the RFC, by renaming the macro defined inside scoped to, say, foo2, and so on).

@Mark-Simulacrum
Copy link
Member

We discussed this RFC in the language team meeting, though not everyone on the team was in attendance for this discussion.

Exploring solutions to the problems noted so far (recursive macros, shadowing) that allow all Rust code to migrate to the new macro system without opt-outs is proving to be quite challenging, and it doesn't seem like there are obvious solutions that we can agree upon quickly. It felt like the team did not currently have the bandwidth to participate actively in that design work, though we would be interested in seeing that exploration done elsewhere (e.g., on internals) and then brought back to the team as a more fleshed out proposal, perhaps later this year or early next.

One of the particular fears was that accepting a proposal - even for experimentation on nightly - that doesn't contain a cohesive path towards integrating solutions to the recursive and shadowing macro problems (which are known now; others may arise over time) seems likely to introduce a 2nd variant of macros which co-exists at the same time, and since it may be incompatible with a more complete proposal later introduced, we would have the debt of documentation, usage, etc. on three different systems for a long time, which seems unfortunate.

In particular, we felt that the team wants to dedicate focus to the 2021 edition right now, and it seems clear that the design here is unlikely to get finalized with the resources we have in time for the edition; we have weight design questions left and no clear solution, but we'd like to get designs finalized in the next month or so for major features at least, so that we have sufficient time for the edition to go smoothly.

@joshtriplett
Copy link
Member

joshtriplett commented Mar 3, 2021

Marking as postponed. Please feel free to either reopen or repost once proposed solutions for recursion and shadowing are included. (Note that the proposed solution to recursion may simply be a transition strategy, or may be a new language mechanism making it more convenient. The proposed solution to shadowing, though, will need a language change.)

@rfcbot postpone

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 3, 2021

Team member @joshtriplett has proposed to postpone 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.

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Mar 3, 2021
@rfcbot rfcbot added disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Mar 3, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 9, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this RFC. label Mar 19, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 19, 2021

The final comment period, with a disposition to postpone, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC is now postponed.

@rfcbot rfcbot added to-announce postponed RFCs that have been postponed and may be revisited at a later time. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. disposition-postpone This RFC is in PFCP or FCP with a disposition to postpone it. labels Mar 19, 2021
@rfcbot rfcbot closed this Mar 19, 2021
@rylev rylev deleted the pub-macro-rules branch March 24, 2021 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Macro related proposals and issues A-privacy Privacy related proposals & ideas finished-final-comment-period The final comment period is finished for this RFC. postponed RFCs that have been postponed and may be revisited at a later time. T-lang Relevant to the language team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.