Skip to content

Permit duplicate macro imports #141043

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jswrenn
Copy link
Member

@jswrenn jswrenn commented May 15, 2025

Consider a crate that depends on both serde (without the derive feature) and serde_derive, and imports serde::Serialize (a trait) and serde_derive::Serialize (a macro). Then, imagine some other crate in a build graph depends on serde with the derive feature; they import both the macro and trait simultaneously with use serde::Serialize. If duplicate imports of the same item are always forbidden, these crates cannot co-exist in the same build-graph; the former crate will fail to build, as its first import (which will now also import the Serialize macro) conflicts with its second import.

This build hazard is confusing — the author of the second crate had no idea that their dependence on the derive feature might be problematic for other crates. The author of the first crate can mitigate the hazard by only glob-importing from proc-macro crates, but glob imports run against many's personal preference and tooling affordances (e.g., rust-analyzer's auto-import feature).

We mitigate this hazard across the ecosystem by permitting duplicate imports of macros. We don't limit this exception to proc macros, as it should not be a breaking change to rewrite a proc macro into a by-example macro. Although it would be semantically unproblematic to permit all duplicate imports (not just those of macros), other kinds of imports have not, in practice, posed the same hazard, and there might be cases we'd like to warn-by-default against. For now, we only permit duplicate macro imports.

See https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/Allowing.20same-name.20imports.20of.20the.20same.20item/near/516777221

r? @compiler-errors

I'm lang-nominating this because I'm not sure if this carve-out rises to the point of requiring an RFC and for the below open questions.

Open Questions

Permitting All Duplicate Identifiers

So long as two bindings resolve to the same item, it's semantically unproblematic for them to have the same name. This PR currently takes the most conservative approach and only carves out macro imports as a case in which duplicate imports of the same item are accepted. However, the warn-by-default unusued_import lint already effectively nudges against duplicate imports. I think we could permit duplicate imports of the the same item — for all kinds of items — without issue.

@jswrenn jswrenn added the I-lang-nominated Nominated for discussion during a lang team meeting. label May 15, 2025
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 15, 2025
@rust-log-analyzer

This comment has been minimized.

@jswrenn jswrenn force-pushed the allow-duplicate-macro-imports branch from da5a69d to a978e7c Compare May 15, 2025 17:15
@rust-log-analyzer

This comment has been minimized.

@jswrenn jswrenn force-pushed the allow-duplicate-macro-imports branch from a978e7c to 3241967 Compare May 15, 2025 17:28
@oxalica
Copy link
Contributor

oxalica commented May 15, 2025

Can we have some tests about how does this incorporate with unused_imports? Eg. for use serde::Serialize; use serde_derive::Serialize;, will it warn on #[derive(Serialize)]? Does the use order matter? What about only using the trait fn f(_: &impl Serialize);?
The latter case, only the first use is unambiguously referenced so it should warn as expected. In the former case, the second use is technically unnecessary but there is a reason to keep it.

Consider a crate that depends on both `serde` (without the `derive`
feature) and `serde_derive`, and imports `serde::Serialize` (a trait)
and `serde_derive::Serialize` (a macro). Then, imagine some other crate
in a build graph depends on `serde` *with* the `derive` feature; they
import both the macro and trait simultaneously with `use
serde::Serialize`. If duplicate imports of the same item are always
forbidden, these crates cannot co-exist in the same build-graph; the
former crate will fail to build, as its first import (which will now
also import the `Serialize` macro) conflicts with its second import.

This build hazard is confusing — the author of the second crate had no
idea that their dependence on the `derive` feature might be problematic
for other crates. The author of the first crate can mitigate the hazard
by only glob-importing from proc-macro crates, but glob imports run
against many's personal preference and tooling affordances (e.g.,
`rust-analyzer`'s auto-import feature).

We mitigate this hazard across the ecosystem by permitting duplicate
imports of macros. We don't limit this exception to proc macros, as it
should not be a breaking change to rewrite a proc macro into a
by-example macro. Although it would be semantically unproblematic to
permit *all* duplicate imports (not just those of macros), other kinds
of imports have not, in practice, posed the same hazard, and there might
be cases we'd like to warn-by-default against. For now, we only permit
duplicate macro imports.

See https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/Allowing.20same-name.20imports.20of.20the.20same.20item/near/516777221
@jswrenn jswrenn closed this May 16, 2025
@jswrenn jswrenn force-pushed the allow-duplicate-macro-imports branch from 3241967 to 9d97e12 Compare May 16, 2025 13:39
@jswrenn
Copy link
Member Author

jswrenn commented May 16, 2025

Not sure how this got closed...

I've updated tests/ui/proc-macro/duplicate.rs to also test for the interaction with unused_imports. I'd forgotten about that lint, and it might resolve my concerns about allowing all duplicate imports (i.e., not just macro) of the same item. I'll add an 'Open Questions' section to the PR text and leave the question to t-lang.

@jswrenn jswrenn reopened this May 16, 2025
@traviscross traviscross added the P-lang-drag-2 Lang team prioritization drag level 2. label May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-lang-nominated Nominated for discussion during a lang team meeting. P-lang-drag-2 Lang team prioritization drag level 2. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants