Add deprecation lint for duplicated `macro_export`s by petrochenkov · Pull Request #50143 · rust-lang/rust


+ let report_already_exported = |this: &mut Self, ident, span, prev_span| {
+ let msg = format!("a macro named `{}` has already been exported", ident);
+ this.session.struct_span_err(span, &msg)


I'm ok with a hard error, but it seems like there is a defined semantics, right? (Shadowing) We could always issue warnings, in other words.

Mark-Simulacrum · 2018-04-25T16:08:19Z

Crater results: http://cargobomb-reports.s3.amazonaws.com/pr-50143/index.html

alexcrichton · 2018-04-25T17:57:02Z

Legitimate regressions

The lsio and fixed-hash regressions seemed to transitively affect a lot of other dependencies.

Sounds like this may want to start out as a warning?

durka · 2018-04-25T18:28:52Z

What would be the motivation for a break here? The whole #[macro_export] system is going to be deprecated anyway, right?

petrochenkov · 2018-04-26T22:08:54Z

@nikomatsakis

it seems like there is a defined semantics, right? (Shadowing)

The exact problem is that there's no defined semantics.
This "shadowing" works over the whole crate, not a single module, and macros are "shadowed" in whatever unspecified order in which the resolution/expansion algorithm processes the crate (#35896 (comment)).

@durka
While the whole Macro 1.0 system is a large widely used part of the language that's going to be supported forever even if deprecated, the macro_export duplication is a corner case that's more on the "kill it with fire" territory due to undefined semantics.

Since there's breakage, I'd prefer to turn this into a preferably deny-by-default lint so dependent crates are not affected and send PRs to crates with root regressions.

durka · 2018-04-26T22:14:13Z

I realize that it got accidentally "defined" to work the way it does, but since it does work and it's not unsound or anything, it seems unfortunate to do the breaking change.

nikomatsakis · 2018-04-27T20:32:22Z

@petrochenkov I would have expected the order to be DFS, since that is significant to other parts of macro expansion. I'm surprised that is complex to achieve, but I'm probably not thinking especially clearly about it. Do you have an example where a problem might arise, or just a bad feeling? I see you wrote this:

The order is generally determined by depth-first search, but I suspect things can become more complex with macro expansion.

Can you point at the relevant code?

(This walk is occurring at the end of expansion.)

nikomatsakis · 2018-04-27T20:34:27Z

(At minimum, some kind of active deprecation definitely makes sense -- and perhaps a hard error in Edition 2018.)

petrochenkov · 2018-04-29T16:30:20Z

@nikomatsakis

Do you have an example where a problem might arise, or just a bad feeling?

Here, for example, foo (1) will "shadow" foo (2) for other crates due to expansion algorithm order.
At the same time, foo (2) will shadow foo (1) locally because it's defined later in the module.

macro_rules! generate_foo {() => {
#[macro_export]
macro_rules! foo {(1) => {}}}}generate_foo!();#[macro_export]macro_rules! foo {(2) => {}}foo!(2);// Local use, OK

petrochenkov · 2018-04-29T18:59:45Z

I've replaced the error with a deprecation lint.

bors · 2018-05-19T17:08:50Z

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

rfcbot · 2018-05-21T18:27:18Z

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

petrochenkov · 2018-05-30T13:43:14Z

Rebased.

rfcbot · 2018-05-31T18:41:08Z

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

nikomatsakis · 2018-06-08T15:02:44Z

@bors r+

bors · 2018-06-08T15:02:44Z

📌 Commit 11c283c has been approved by nikomatsakis

bors · 2018-06-08T15:02:45Z

🌲 The tree is currently closed for pull requests below priority 1, this pull request will be tested once the tree is reopened

Add deprecation lint for duplicated `macro_export`s cc rust-lang#35896 (comment)

bors · 2018-06-08T23:21:22Z

🔒 Merge conflict

Rollup of 13 pull requests Successful merges: - #50143 (Add deprecation lint for duplicated `macro_export`s) - #51099 (Fix Issue 38777) - #51276 (Dedup auto traits in trait objects.) - #51298 (Stabilize unit tests with non-`()` return type) - #51360 (Suggest parentheses when a struct literal needs them) - #51391 (Use spans pointing at the inside of a rustdoc attribute) - #51394 (Use scope tree depths to speed up `nearest_common_ancestor`.) - #51396 (Make the size of Option<NonZero*> a documented guarantee.) - #51401 (Warn on `repr` without hints) - #51412 (Avoid useless Vec clones in pending_obligations().) - #51427 (compiletest: autoremove duplicate .nll.* files (#51204)) - #51436 (Do not require stage 2 compiler for rustdoc) - #51437 (rustbuild: generate full list of dependencies for metadata) Failed merges:

rust-highfive assigned nikomatsakis Apr 21, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2018

petrochenkov added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Apr 21, 2018

bors added a commit that referenced this pull request Apr 21, 2018
Auto merge of#50143- petrochenkov:mexuniq, r=<try>
a314612
Prohibit duplicated `macro_export`s cc #35896 (comment)

emilyalbini removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2018

nikomatsakis approved these changes Apr 25, 2018
View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 26, 2018

petrochenkov force-pushed the mexuniq branch from eaebec0 to 171055cCompare April 29, 2018 18:59

petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 30, 2018

nikomatsakis added A-decl-macros-2-0 Area: Declarative macros 2.0 (#39412) I-nominated T-lang Relevant to the language team and removed A-decl-macros-2-0 Area: Declarative macros 2.0 (#39412) labels Apr 30, 2018

This was referenced May 18, 2018
Remove deprecated duplicate macro exports lambdastackio/lsio#5
Closed
Fix deprecated duplicate macro exports Rahix/eagre-asn1#2
Merged
Remove deprecated duplicate macro exports ErichDonGubler/adhesion-rs#36
Merged

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels May 21, 2018

Prohibit duplicate macro_exports
11c283c

petrochenkov force-pushed the mexuniq branch from 171055c to 11c283cCompare May 30, 2018 13:42

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 31, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-team labels Jun 8, 2018

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 8, 2018
Rollup merge ofrust-lang#50143- petrochenkov:mexuniq, r=nikomatsakis
91b6842
Add deprecation lint for duplicated `macro_export`s cc rust-lang#35896 (comment)

Mark-Simulacrum mentioned this pull request Jun 8, 2018
Rollup of 13 pull requests #51448
Merged

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 8, 2018

bors merged commit 11c283c into rust-lang:masterJun 9, 2018

petrochenkov mentioned this pull request Jun 11, 2018
Implement #[macro_export(local_inner_macros)] (a solution for the macro helper import problem) #51496
Merged

petrochenkov mentioned this pull request Jul 11, 2018
resolve: Modularize crate-local #[macro_export] macro_rules#52234
Merged

ehuss mentioned this pull request May 26, 2019
duplicate_macro_exports lint says it will be a hard error in 2018 but it is still a lint #61221
Closed

petrochenkov deleted the mexuniq branch June 5, 2019 16:10

Uh oh!

Uh oh!

Conversation

petrochenkov commented Apr 21, 2018

Uh oh!

rust-highfive commented Apr 21, 2018

Uh oh!

petrochenkov commented Apr 21, 2018

Uh oh!

kennytm commented Apr 21, 2018

Uh oh!

bors commented Apr 21, 2018

Uh oh!

bors commented Apr 21, 2018

Uh oh!

Mark-Simulacrum commented Apr 21, 2018

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

nikomatsakisApr 25, 2018

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum commented Apr 25, 2018

Uh oh!

alexcrichton commented Apr 25, 2018

Uh oh!

durka commented Apr 25, 2018

Uh oh!

petrochenkov commented Apr 26, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

durka commented Apr 26, 2018

Uh oh!

nikomatsakis commented Apr 27, 2018

Uh oh!

nikomatsakis commented Apr 27, 2018

Uh oh!

petrochenkov commented Apr 29, 2018

Uh oh!

petrochenkov commented Apr 29, 2018

Uh oh!

bors commented May 19, 2018

Uh oh!

rfcbot commented May 21, 2018

Uh oh!

petrochenkov commented May 30, 2018

Uh oh!

rfcbot commented May 31, 2018

Uh oh!

nikomatsakis commented Jun 8, 2018

Uh oh!

bors commented Jun 8, 2018

Uh oh!

bors commented Jun 8, 2018

Uh oh!

bors commented Jun 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

petrochenkov commented Apr 26, 2018•
edited
Loading