Add deprecation lint for duplicated `macro_export`s #50143

petrochenkov · 2018-04-21T14:23:26Z

cc #35896 (comment)

rust-highfive · 2018-04-21T14:23:36Z

r? @nikomatsakis

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

petrochenkov · 2018-04-21T14:24:40Z

cc @rust-lang/lang

Needs check-only crater run before proceeding.
ping @rust-lang/infra

kennytm · 2018-04-21T14:27:59Z

@bors try

bors · 2018-04-21T14:28:10Z

⌛ Trying commit eaebec0 with merge a314612...

Prohibit duplicated `macro_export`s cc #35896 (comment)

bors · 2018-04-21T16:27:10Z

☀️ Test successful - status-travis
State: approved= try=True

Mark-Simulacrum · 2018-04-21T17:09:11Z

Crater run commenced, check-only, ETA ~3 days.

nikomatsakis

r=me from the technical perspective. I'm not 100% sure about the policy decision, but I guess let's see what the crater run has to say.

nikomatsakis · 2018-04-25T15:07:42Z

src/librustc_resolve/resolve_imports.rs

@@ -887,14 +887,24 @@ impl<'a, 'b:'a> ImportResolver<'a, 'b> {
        // Since import resolution is finished, globs will not define any more names.
        *module.globs.borrow_mut() = Vec::new();

+        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)

pietroalbini 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 171055c Compare 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-macros-2.0 Area: Declarative macros 2.0 (#39412) I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed A-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 11c283c Compare 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 Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 8, 2018

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 8, 2018

Rollup merge of rust-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:master Jun 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deprecation lint for duplicated `macro_export`s #50143

Add deprecation lint for duplicated `macro_export`s #50143

petrochenkov commented Apr 21, 2018

rust-highfive commented Apr 21, 2018

petrochenkov commented Apr 21, 2018

kennytm commented Apr 21, 2018

bors commented Apr 21, 2018

bors commented Apr 21, 2018

Mark-Simulacrum commented Apr 21, 2018

nikomatsakis left a comment

nikomatsakis Apr 25, 2018

Mark-Simulacrum commented Apr 25, 2018

alexcrichton commented Apr 25, 2018

durka commented Apr 25, 2018

petrochenkov commented Apr 26, 2018 •

edited

Loading

durka commented Apr 26, 2018

nikomatsakis commented Apr 27, 2018

nikomatsakis commented Apr 27, 2018

petrochenkov commented Apr 29, 2018

petrochenkov commented Apr 29, 2018

bors commented May 19, 2018

rfcbot commented May 21, 2018

petrochenkov commented May 30, 2018

rfcbot commented May 31, 2018

nikomatsakis commented Jun 8, 2018

bors commented Jun 8, 2018

bors commented Jun 8, 2018

bors commented Jun 8, 2018

Add deprecation lint for duplicated macro_exports #50143

Add deprecation lint for duplicated macro_exports #50143

Conversation

petrochenkov commented Apr 21, 2018

rust-highfive commented Apr 21, 2018

petrochenkov commented Apr 21, 2018

kennytm commented Apr 21, 2018

bors commented Apr 21, 2018

bors commented Apr 21, 2018

Mark-Simulacrum commented Apr 21, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Apr 25, 2018

Choose a reason for hiding this comment

Mark-Simulacrum commented Apr 25, 2018

alexcrichton commented Apr 25, 2018

durka commented Apr 25, 2018

petrochenkov commented Apr 26, 2018 • edited Loading

durka commented Apr 26, 2018

nikomatsakis commented Apr 27, 2018

nikomatsakis commented Apr 27, 2018

petrochenkov commented Apr 29, 2018

petrochenkov commented Apr 29, 2018

bors commented May 19, 2018

rfcbot commented May 21, 2018

petrochenkov commented May 30, 2018

rfcbot commented May 31, 2018

nikomatsakis commented Jun 8, 2018

bors commented Jun 8, 2018

bors commented Jun 8, 2018

bors commented Jun 8, 2018

Add deprecation lint for duplicated `macro_export`s #50143

Add deprecation lint for duplicated `macro_export`s #50143

petrochenkov commented Apr 26, 2018 •

edited

Loading