Permit duplicate imports by jswrenn · Pull Request #141043 · rust-lang/rust (original) (raw)

Thanks for the ping! This is a thorny one. As is often the case, making the point-in-time Rust experience better can make the SemVer story much more tricky.

TL;DR:

• I'm deeply sympathetic to the zerocopy macros problem. It sucks, and we should solve it.
• I'm cautiously optimistic that allowing duplicate macro imports might be viable. I think we have to be very careful, and invest much more time into considering the edge cases than we have so far, though.
• Duplicate imports in the general case feels like a non-starter, unless we at minimum completely overhaul type aliases in the process.

This is my thinking after dedicating ~2.5h to this question. I'm reasonably confident that more discussion and effort will produce more things to consider — if I were on lang-advisors I'd want to register a concern for the FCP process.

Exact resolution of duplicate items matters semantically

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.

Strongly agree that the specific reexport chain is important. Consider the following:

mod inner { mod defn { pub struct Example; }

#[doc(hidden)]
mod nested {
   pub use super::defn::Example;
}

// This PR would allow this:
#[deprecated]
pub use defn::Example;
pub use nested::Example;

}

// Which Example are we re-exporting? // One of them is non-public API, // the other is deprecated. // // User code that depends on non-public API (conceptually) should lint. // Using deprecated code already does trigger a lint. // Which lint should fire when users import Example from here? pub use inner::*;

It might be tempting to say something like "just combine the modifiers — it's both deprecated and doc(hidden)" but that's actually incorrect: deprecated doc(hidden) items are public API in Rust! That fact is the "least-bad but at least self-consistent" interpretation of how the two concepts interact. The alternative interpretations either allow major breakage without a SemVer major bump, or de facto rule that common real-world doc(hidden) use cases like "don't show deprecated items on docs.rs" are SemVer-violating.

I would recommend looking through my post Breaking semver in Rust by adding a private type, or by adding an import — some of the examples there are quite cursed, and you'll likely come up with more edge cases based on it.

Explicit vs glob resolution priority considerations (click to expand)

Currently, explicit imports and definitions take priority over glob imports: playground

pub mod inner { mod defn { pub struct Example; }

mod nested {
    #[allow(non_upper_case_globals)]
    pub const Example: i64 = 1;
}

pub use defn::Example;
pub use nested::*;

}

use inner::Example;

fn main() { // This works because Example in the values namespace // refers to the unit struct, not the const item. let _x: Example = Example; }

However we choose to handle duplicate imports of the same item, we have to make sure it's consistent with the existing priority order

Type aliases make everything difficult

Type aliases are also a nightmare — so much so that (for the time being) I gave up on doing any better than the bare minimum analysis on them in cargo-semver-checks.

Aside, to demonstrate how problematic and counter-intuitive type aliases are (click to expand)

Making a pub type alias of a private type is allowed (!) and merely produces a warn-by-default lint:

struct Private;

#[allow(private_interfaces)] pub type AliasOfPrivate = Private;

If we choose to allow duplicating more items than just macros, we have to grapple with "when is a type alias the same as the underlying type." This is a nightmare of a question.

For example:

pub mod a { pub struct Example(T); }

mod b { pub type Alias<T = i64> = super::a::Example;
}

pub use a::Example as Reexport;

// Should this be allowed? pub use b::Alias as Reexport;

This is a good example of "improving point-in-time ergonomics can make SemVer hazards worse."

It's tempting to allow the duplicate here: anything Example can do, Alias can do too! We can quite literally find-and-replace every use of Example with a valid path to Alias, and the code will still compile.

This is a nasty SemVer hazard though! Consider the nominally minor, non-breaking change of adding a default value to Example<T>:

• pub struct Example(T);

• pub struct Example<T = String>(T);

What generic type does my_crate::Reexport carry now?

• If we resolve Reexport -> a::Example then T = String.
• If we resolve Reexport -> b::Alias then T = i64.

None of these choices is obviously correct — especially in a cross-crate setting where not all these items might be defined in the same crate.

Since there's no good choice, we can also deny uses of Reexport without an explicit type argument. We can do so at the point of attempted use (like in some cases with ambiguous glob re-exports), or attempt to do so eagerly at the point where the duplicate import happens. But again, the cross-crate setting means these boil down to the same thing from a SemVer perspective — adding the default triggered a SemVer-major breaking change.