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

Tracking issue for "macro naming and modularisation" (RFC #1561) #35896

Closed
nikomatsakis opened this Issue Aug 22, 2016 · 164 comments

Comments

@nikomatsakis
Contributor

nikomatsakis commented Aug 22, 2016

Tracking issue for rust-lang/rfcs#1561.

Roadmap: #35896 (comment).

cc @nrc @jseyfried

@camlorn

This comment has been minimized.

Contributor

camlorn commented Aug 23, 2016

One question I don't think was ever answered on the RFC thread is if providing a way for current macros to opt in is feasible.

Just throwing this out there as to make sure it doesn't get forgotten. My vote is obviously for yes we can, but I'm also obviously not on a team, or even an active contributor beyond these discussions.

@nrc

This comment has been minimized.

Member

nrc commented Aug 23, 2016

@camlorn this was actually address in my final edit to the RFC before merging. To summarise, it might be possible but we need implementation experience to be sure and to work out exactly how it might work. So, basically punting on the issue for now. It is certainly an area where I think we need to tread extremely carefully.

bors added a commit that referenced this issue Oct 21, 2016

Auto merge of #37247 - jseyfried:future_proof_no_link, r=nrc
macros: Future proof `#[no_link]`

This PR future proofs `#[no_link]` for macro modularization (cc #35896).

First, we resolve all `#[no_link] extern crate`s. `#[no_link]` crates without `#[macro_use]` or `#[macro_reexport]` are not resolved today, this is a [breaking-change]. For example,
```rust
```
Any breakage can be fixed by simply removing the `#[no_link] extern crate`.

Second, `#[no_link] extern crate`s will define an empty module in type namespace to eventually allow importing the crate's macros with `use`. This is a [breaking-change], for example:
```rust
mod syntax {} //< This becomes a duplicate error.
```

r? @nrc

bors added a commit that referenced this issue Oct 25, 2016

Auto merge of #37292 - jseyfried:import_macros_in_resolve, r=nrc
Process `#[macro_use]` imports in `resolve` and clean up macro loading

Groundwork macro modularization (cc #35896).
r? @nrc
@jseyfried

This comment has been minimized.

Contributor

jseyfried commented Jan 28, 2017

@jseyfried

This comment has been minimized.

Contributor

jseyfried commented Feb 7, 2017

Tasks

  • Stackless macro expansion (PR #36214).
  • Fix $crate with inter-crate re-exports (PR #37463).
  • Future-proof #[no_link] crates (PR #37247).
  • Parse paths in bang macro invocations (e.g. path::to::mac!();) (PR #36662).
  • Back-port shadowing restrictions from rust-lang/rfcs#1560 where appropriate (PR #36767).
  • Forbid user-defined macros named "macro_rules" (PR #36730).
  • Miscellaneous groundwork in syntax, resolve, and metadata (PRs #36438, #36525, #36573, #36601, #36616, #37036, #37213, #37292, and #37542).
  • Land #![feature(use_extern_macro)] (PRs #37732 and #38082).
  • Allow use to shadow macros from the prelude, currently is an ambiguity error (PR #40501).
  • Improve interaction with macros 1.0 exports in the same crate, i.e. emit more duplicate errors (PR #40509).
  • Integrate with rustdoc, issue #39436 (PR #40814).
@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 25, 2018

You have three options as a macro author right now:

  • Do nothing: Your crate will still work on the new edition and on new compiler versions. Consumers will be shown a warning, which if they attempt to fix will cause errors
  • Use $crate::inner: No longer works on old compilers, works on the old edition. Fixing the warning does not cause errors
  • Use local_inner_macros: Works on old compilers, works on the old edition. Fixing the warning does not cause errors.
@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Jul 25, 2018

It sort of depends what you mean by compatible. It is possible to make a crate "not compatible with 2015" in the sense that removing edition = "2018" in its Cargo.toml would make it not compile anymore. But it is still compatible in the sense that other crates in the dependency graph can be on a different edition, so there is not Python-3-like ecosystem split.

@lnicola

This comment has been minimized.

Contributor

lnicola commented Jul 25, 2018

Use $crate::inner: No longer works on old compilers, works on the old edition. Fixing the warning does not cause errors

Use local_inner_macros: Works on old compilers, works on the old edition. Fixing the warning does not cause errors.

Does "old compilers" here mean 1.30 while "new" is 1.31? Why is local_inner_macros more portable than $crate::inner?

EDIT: I didn't realize that #[macro_export(local_inner_macros)] (or #[macro_export(foo)]) already works on stable, even if it has no effect.

@dtolnay

This comment has been minimized.

Member

dtolnay commented Jul 25, 2018

  • A1: unqualified helper macro calls imported through macro_use (2015 style)
  • A2: unqualified helper macro calls imported through module system
  • B1: attribute local_inner_macros imported through macro_use
  • B2: attribute local_inner_macros imported through module system
  • C1: $crate:: qualified macro calls imported through macro_use
  • C2: $crate:: qualified macro calls imported through module system (2018 vision)

Note that your dependencies select A/B/C and your crate selects 1/2.


2015 edition 2018 edition
rustc <1.30 • Supports A1, B1
• Error on A2, B2, C1, C2
(not supported)
rustc >=1.30 • Supports A1, B1, B2, C1, C2
• Error on A2
• Supports B2, C2
• Warning on A1, B1, C1
• Error on A2

A few things you can see from this table:

  • Upgrading from rustc <1.30 to rustc >=1.30 is not a breaking change because A1 and B1 continue to be supported.
  • Enabling the 2018 edition in your crate requires rewriting B1 to B2, C1 to C2, and suffering the A1 warning for any dependencies still on A.
  • Crates that are stuck on A are problematic and will keep #[macro_use] extern crate alive.
  • Foundational crates must use B to support a range of compiler versions and editions.
  • Changing a crate from A to B is not a breaking change.
  • Changing a crate from A to C or B to C drops support for old1 compiler versions.
  • Using B2, C1, or C2 in a crate makes it incompatible with old1 compiler versions.
  • Unfortunately, tooling and lints and documentation all tell you to change A1 to A2 which never works.
@durka

This comment has been minimized.

Contributor

durka commented Jul 26, 2018

Very helpful table! I still feel that we should be recommending B (for all macro crates -- not just "foundational", which seems like a too-fuzzy line) until "old" compilers are sufficiently old. And changing cargo fix to suggest A1 -> B2, I guess.

@dtolnay

This comment has been minimized.

Member

dtolnay commented Jul 26, 2018

Local_inner_macros can be more complicated to use than $crate:: in the case that one macro needs to invoke both helper macros and also macros from the standard library, because it declares that every macro invocation be resolved as a local helper macro. This can require a substantial amount of refactoring that is not needed when using $crate::.

It would be unusual for cargo fix to suggest A1 -> B2 because this requires code changes outside of the crate being fix'd.

@durka

This comment has been minimized.

Contributor

durka commented Jul 26, 2018

Another problem with local_inner_macros: #52726

I guess since cargo-fix can't assume anything beyond the current crate, the only things it could do that would actually work are (a) nothing, or (b) somehow crawl the macro expansion and see which macros you really need to import.

When cargo-fixing a macro-exporting crate, there's a question of whether to suggest A->B or A->C.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Jul 26, 2018

It sort of depends what you mean by compatible. It is possible to make a crate "not compatible with 2015" in the sense that removing edition = "2018" in its Cargo.toml would make it not compile anymore. But it is still compatible in the sense that other crates in the dependency graph can be on a different edition.

Yes, I would say it this way: a library itself uses only one edition, but is always compatible with all editions - that is, you can depend on it whatever edition you are in. When we say that multiple editions can be compiled together, I think we are not expressing this compatibility forcefully enough: it is not possible to write a library that can only be depended on by crates on one edition or another. Library authors don't even have to worry about compatibility with multiple editions; they simply are compatible always.

(Also unless somethings changed recently (and I don't think so, based on @dtolnay's post), the only thing editions change related to this issue are turning on some lints. Macro imports don't behave differently between 2015 and 2018 edition.)

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Jul 26, 2018

@dtolnay I want to clarify one thing based on your chart: you suggest that the "1" column options (using #[macro_use]) will be linted against in 2018. However, based on @aturon's most recent comment, these lints will be allow by default until the ecosystem transitions more.

@Manishearth makes this comment that I don't understand:

But as it stands even if they're not on by default, we are asking people to use them post-transition, so we should either stop asking people to do so or ensure they're polished by the transition.

I don't know what it means to "ask people to use them;" I haven't heard about this plan and it seems dubious. If we think people should use these warnings, we should have them turned on. As a rule, I do not believe we should ever have allow by default lints we recommend that you turn on; this is use strict; and to me its a sign of something gone quite wrong.

From my perspective, it seems like we should evaluate each 2018 idiom lint for disruptiveness and turn it on as soon as it seems like a net benefit. This will probably leave the macro related ones off for some time until the ecosystem has moved off the "A" system onto the "B" and "C" systems. Its unfortunate, but its the long term cost we're paying for having stabilized the "A" system for 1.0.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 26, 2018

As a rule, I do not believe we should ever have allow by default lints we recommend that you turn on;

It's not quite this, it's not "use strict";. We recommend you temporarily turn them on, like we do for the migration lints.

The original plan was that we recommend a two step edition migration process. In the first step you turn on the migration lints and fix those, then you upgrade the edition in Cargo, and do the same again with the idiom lints. Ideally, this would be managed by cargo fix. The idiom lints aren't ones you keep enabled in perpetuity; you flip them on when you use cargo fix (or cargo fix flips them on for you), and when you're done with the upgrade you flip them off. We can make them on by default on 2018 a couple months into the edition if we wish, with the hope that everyone has cargo fixed them already.

The reason they're not just on by default is because these lints are super noisy and really need to be run with cargo fix.

This is all being discussed in #52679

Given that many of the idiom lints are rather broken the current plan may just be to not recommend this for a while, and have a gradual rollout.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Jul 26, 2018

@Manishearth thanks for the clarification! I think really when you say we recommend that you turn them on, the issue is what behavior cargo fix demonstrates in light of this: the lint is really just a part of the API between rustc and cargo here, mostly an implementation detail (ideally, cargo fix flips the lint on and off for you, so that cargo fix can be a single atomic step).

I'll reply more about the general problem on the issue you linked.

So I think the open issue here can be scoped down to this: what will cargo fix do about macro imports, given that the upstream crate needs to be compatible first? And how?

For macro authors

If cargo fix can convert macro authors to the C scheme ($crate::) for their macros, that seems ideal. There's no reason I see to convert them to B since by depending on 2018, they inherently the require the 1.30+ compiler.

For macro users

Assuming cargo fix can upgrade a user from 1 (#[macro_use]) to 2 (normal imports), I think there are three reasonable options:

  1. Be pessimistic: we don't try to move users from #[macro_use] to normal imports, because we assume too many of the upstream crates won't have upgraded yet.
  2. Be optimistic: we do try to make the transition for all crates, breaking the user's code if the upstream crate has not transitioned.
  3. Be discerning: we use some method (a whitelist, a heuristic?) to upgrade the user to 2018.

Its possible that our behavior should change over time.

It also occurs to me that if we do either 2 or 3, the user is likely to have a lock file that locks them to a version of the package that is incompatible. If we do any of these fixes, we should probably cargo update the macro-exporting package as a part of cargo fix so that users will be more likely to get the fixed code.

@Nemo157

This comment has been minimized.

Contributor

Nemo157 commented Jul 26, 2018

It also occurs to me that if we do either 2 or 3, the user is likely to have a lock file that locks them to a version of the package that is incompatible. If we do any of these fixes, we should probably cargo update the macro-exporting package as a part of cargo fix so that users will be more likely to get the fixed code.

Since I was just reading about --minimal-versions, this should maybe be a cargo upgrade as otherwise the users Cargo.toml will be claiming to work with a version that it might not actually work with.

@durka

This comment has been minimized.

Contributor

durka commented Jul 26, 2018

Probably a bad idea, but we could add another hack: #[macro_export(trust_me_its_2018_compatible)], for macro authors to use once they've updated to B or C. Seeing this attribute, cargo fix could recommend C2.

What I still don't get is the business about "foundational crates". Sure, crates that are managed by the core team are special and we trust y'all, but how is Joe the Macro Author supposed to know whether their crate is special enough to ignore the cargo fix advice? Should there be an official guideline, like, "you can apply this fix when you are ready to drop compatibility with rustc v1.XY"?

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Jul 26, 2018

@durka Since the version needed to compile a project with rust = "2018" in the Cargo.toml is greater than or equal to the version needed to support option C, anyone running cargo fix --prepare-for 2018 has opted into dropping compatibility with rustc versions that don't support option C. For now, each crate author decides if they want to go onto 2018 or if they want to continue to support rustc versions from before the 2018 release.

@durka

This comment has been minimized.

Contributor

durka commented Jul 26, 2018

But lazy_static will be threading the needle by upgrading to B.

@Nemo157

This comment has been minimized.

Contributor

Nemo157 commented Jul 26, 2018

Be discerning: we use some method (a whitelist, a heuristic?) to upgrade the user to 2018.

Since options B2 and C2 work the only thing you have to watch out for is a dependency still using A. I assume detecting that a dependency is using option B is possible since that presumably is reflected in the metadata, so that should be easy to upgrade. Option C is likely undecidable, but the heuristic could just be "if dependency is Rust 2018 then they should be using option C" and upgrade the user (will break if a crate has updated to Rust 2018 but not transitioned to option C)?

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 26, 2018

@withoutboats this plan seems pretty good! I think changing behavior over time is definitely something we should also try for; be pessimistic at first for macro users , and over time start suggesting fixes. There's also a lot of interesting stuff here that can be done by teaching cargo fix about good and bad macro crate versions.

@withoutboats

This comment has been minimized.

Contributor

withoutboats commented Jul 26, 2018

Building on @Nemo157's comment, if we can get this metadata for each of your dependencies:

  1. Are any of their macros tagged local_inner_macros?
  2. Are they on the 2018 edition?

That could be a good heuristic for whether or not we should upgrade their macro invokations. But I'm not sure how well the current set up allows rustfix to operate differently depending on the dependency metadata like this.

We could consider disallowing option C on 2015, so that anyone who wants to stay on 2015 will switch to B, which we can detect, and anyone who wants to switch to C will move to 2018, which we can also detect.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Jul 26, 2018

Well, we have crater, and we can use that to obtain this metadata ourselves and hardcode it (and hardcode what versions fix the problem)

I don't expect there to be that many macro crates affected, especially if we only consider popular ones. The problem is that people depend on these.

@glmdgrielson

This comment has been minimized.

glmdgrielson commented Aug 3, 2018

Wait, why is this still open? Everything on the roadmap is checked off. What's blocking this other than working out the kinks of a potential edition lint?

@Manishearth

This comment has been minimized.

Member

Manishearth commented Aug 3, 2018

Tracking issues close when stabilized.

@stjepang

This comment has been minimized.

Contributor

stjepang commented Sep 17, 2018

I have a question. Why do we need to import macros to use them? For example:

extern crate crossbeam;
use crossbeam::channel;

// Doesn't compile unless we uncomment this line:
// use self::channel::select;

fn main() {
    channel::select! {
        default => {}
    }
}

It is surprising to me that this code doesn't compile unless we import the macro. Is this intentional behavior or a bug?

@dtolnay

This comment has been minimized.

Member

dtolnay commented Sep 17, 2018

@stjepang maybe channel::select! expands to an invocation of plain select!? That select! invocation would be the one that is not finding a resolution in scope.

The expansion should use $crate::channel::select! instead (or macro_export(local_inner_macros) if you need to support compilers older than 1.30.0).

@stjepang

This comment has been minimized.

Contributor

stjepang commented Sep 17, 2018

@dtolnay Oh right, that was indeed the issue. Thank you! :)

@dhardy

This comment has been minimized.

Contributor

dhardy commented Oct 6, 2018

Could I please request some transparency on what exactly is implemented by this "tracking issue" and what continued plans there are related to RFC 1561?

My understanding of the process is that a tracking issue is not a place for discussion of how new features work (other than internal details), however I see a lot of discussion here of what exactly the new macro modularisation rules are. Perhaps part of the problem is that RFC 1561 is vague and far too broad.

For example, 1561 declares the following which does not appear to be covered here (and does not appear to be possible on the latest nightly under either edition):

If a macro baz (by example or procedural) is defined in a module bar which is nested in foo, then it may be used anywhere in the crate using an absolute path: ::foo::bar::baz!(...). It can be used via relative paths in the usual way, e.g., inside foo as bar::baz!().

All documentation I can find is either hopelessly out of date or refers back to this issue.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Oct 8, 2018

@dhardy unfortunately the transparency here is all written down, but it takes some effort to sift through it. What's stabilized here is described online and further tracking issues track remaining work items for known unstable items in the compiler. Work that hasn't ever been implemented from the original RFC doesn't currently have tracking issues, but they can definitely be created!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment