Permit duplicate macro imports

[jswrenn]

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.

[oxalica]

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.

[jswrenn]

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.

[traviscross]

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.

I agree. And working to specify our language has me in the mood to prefer being consistent where possible rather than adding little carve-outs here and there. So I propose that we allow duplicate imports of the same item for all namespaces.

@rfcbot fcp merge

[rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[petrochenkov]

So long as two bindings resolve to the same item, it's semantically unproblematic for them to have the same name.

Not exactly, the specific reexport chain through which we arrive to the final definition is important.

• For privacy checking, first of all.
• For stability/deprecation checking if we finally migrate it to resolve (Stability checker doesn't recurse down reexports properly #21905).
• Also for some rustdoc logic like doc(hidden), and for diagnostics, but that's less important.

We currently have some cases in which ambiguities between bindings ultimately having the same Res are ignored, but we are not really doing it correctly in general case.
E.g. ambiguities

// between two globs
pub(in a) use tralala::*; // imports `struct Foo;`
pub(in b) use trulala::*; // also imports `struct Foo;`

// or between outer and inner names in scope under restricted shadowing

are not reported (*), but which visibility (and potentially stability) - a or b will actually be used for privacy checking depends on random internal ordering details of the resolution-expansion algorithm, which is bad.

I'm not sure what the proper rules here should be, and what it takes to implement them, because it has always been a low priority issue. Perhaps support multiple declaration bindings for name uses, and check all of them for privacy/stability/etc.

If we support these ambiguities for single imports as well, we'll expose more of these under-specified/under-implemented parts of language to users.

(*) See the logic in the same fn try_define function as the code added in this PR, a bit above it.

[traviscross]

If we support these ambiguities for single imports as well, we'll expose more of these under-specified/under-implemented parts of language to users.

Do you perhaps have an example of code demonstrating an ambiguity that would be exposed by this that isn't already possible to demonstrate with glob imports, or is the argument here that by exposing more ways to reach these same already-exposed ambiguities, more code in the wild will lean on them?

[petrochenkov]

the argument here that by exposing more ways to reach these same already-exposed ambiguities, more code in the wild will lean on them?

Probably this, I don't think we enable qualitatively more issues than just with globs.