Skip to content
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

make pub type foo = bar and pub use bar as foo interchangable in next edition #73191

Open
4 tasks
yaahc opened this issue Jun 9, 2020 · 11 comments
Open
4 tasks
Labels
A-maybe-future-edition Something we may consider for a future edition. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@yaahc
Copy link
Member

yaahc commented Jun 9, 2020

Right now renaming a type and re-exporting it via a type alias can be a breaking change whereas re exporting it under the new name is not a breaking change. As far as I know the only difference between the two is how they interact with tuple-struct constructors:

pub struct MyStruct(u32);

pub use self::MyStruct as PubUse;
pub type PubType = MyStruct;

fn main() {
    let _ = PubUse(5); // OK
    // let _ = PubType(5); // Doesn't work
}

This happens because tuple struct constructors are just free functions with the same ident as the type they're constructing. use statements bring in items from all 3 namespaces that match the given ident but type items only alias the type itself.

This can be fixed by making it so that the compiler generates a constructor for the alias when it sees that the RHS of the type alias is a tuple-struct. This would be a breaking change because users can already define a free function with the same name as the type alias, which people do currently do to add the necessary constructor to work around this very issue.

To resolve this @joshtriplett has recommended the following steps:

  • implement the alias constructor fix as a feature gated PR
  • add an allow by default lint to detect free functions that would conflict with the newly generated constructors
    • do a crater run with the above lint set to deny to measure the impact of this change
    • add this lint to the future-compatibility lint group
@joshtriplett joshtriplett added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. A-maybe-future-edition Something we may consider for a future edition. labels Jun 9, 2020
@scottmcm
Copy link
Member

scottmcm commented Jun 10, 2020

It it possible there's anyone relying on this for the opposite, exporting a type while intentionally not exporting the constructor? (And not providing a function of the same name.)

I guess it doesn't hide PubType { 0: 5 }, though...

@the8472
Copy link
Member

the8472 commented Jun 10, 2020

Maybe a clippy rule suggesting to use pub use instead of pub type for trivial tuple struct reexports would help people avoid these pitfalls in the current edition?

@joshtriplett
Copy link
Member

@scottmcm If someone is relying on that, they could avoid exporting the tuple struct's fields, which makes the constructor private.

@estebank
Copy link
Contributor

Doesn't pub type PubType = MyStruct; differ in semantics to the re-export if there are obligations introduced (or not included) in the type? We don't currently perform many checks on PubType or MyStruct until usage (which is why adding WF-check to types is a breaking change because if PubType isn't used but not WF we would start breaking builds).

@joshtriplett
Copy link
Member

Based on discussion in the lang team meeting today:

We'd like to take this through the new lang team process. (That should just involve moving this issue to the rust-lang/lang-team repository using the major change process template.)

This may potentially interact with name resolution. One challenge is that the name resolution code doesn’t (today) have to look at the RHS of a type alias. cc @petrochenkov

One common use case for this involves aliases that fill in specific type parameters for generic types:

struct Foo<T>(T);
type Bar = Foo<u32>;

The result doesn't currently work as a constructor (Bar(42)), or as a pattern.

@petrochenkov
Copy link
Contributor

This won't just interact with name resolution, this is almost entirely a name resolution feature.
It is possible to do this without heroic efforts, but with significant limitations (same limitations as imports of type-relative associated items).

type Alias = Something; will have to always reserve (possibly unusable) slots in both type and value namespaces regardless of the constructor actually existing or not.
So

type A = u8;
fn A() {}

will become ill-formed.

@eddyb
Copy link
Member

eddyb commented Feb 9, 2021

My opinion is that the value side of MyStruct (in the original example) should be explicitly reexported (to avoid introducing even more complexity into the name resolution system, and to clearly label it in the source), and my favorite way to do so is:

pub pat PubType(x: u32) = MyStruct(x);

That is, pattern aliases, which are always usable as patterns (expanding to the RHS pattern), but also could be allowed to be called as (constructor) functions, if there are no _ or .. on the RHS that make it impossible to materialize a value.

I can't fin a RFCs repo issue/PR about "pattern aliases", but I remember discussing them with several people, and it felt like all of them thought the feature would be useful but it never ended up going through the RFC pipeline.

It's probably (far) too late to try and cram this into the 2021 edition, but long-term this is what I would prefer.

@Aloso
Copy link
Contributor

Aloso commented Mar 5, 2021

What's the motivation for this? use and type have different use cases, and I don't see a reason why they should have the same semantics. This proposal adds a new corner case to the language, causes churn, possibly confusion for beginners, increases the number of things that differ between editions, makes some code more difficult or even impossible to write, has the possibility to become a maintenance burden in the compiler, and for what?

This proposal doesn't make pub use and pub type interchangeable, and I don't see why they should be. use can import all items (not just types). type is limited to types, BUT it can have generic parameters and lifetimes.

@yaahc
Copy link
Member Author

yaahc commented Mar 5, 2021

What's the motivation for this? use and type have different use cases, and I don't see a reason why they should have the same semantics. This proposal adds a new corner case to the language, causes churn, possibly confusion for beginners, increases the number of things that differ between editions, makes some code more difficult or even impossible to write, has the possibility to become a maintenance burden in the compiler, and for what?

The motivation was to reduce edge cases in the language and make the language more intuitive, at least based on my mental model, which is that type NewIdent = OldIdent; and pub use OldIdent as NewIdent; are not clearly or obviously different. I felt / feel that this is a potential footgun where you can unknowingly introduce breaking changes.

I'm not going to argue that they aren't different in practice, but any complexity there wasn't apparent to me when I wrote this issue. I've already updated the reference to try to clarify this so maybe this isn't an issue in practice anymore, but I'm guessing we could still put more effort into clarifying the semantic differences between these items.

@obi1kenobi
Copy link
Member

obi1kenobi commented Mar 5, 2023

Another edge case: glob imports of enum variants work through pub use but don't work through pub type on the enum.

Playground link

fn main() {
    // Works
    // enum Foo {A, B}

    enum SecretFoo {
        A,
        B,
    }
    type Foo = SecretFoo;
    
    // Works fine
    let x = Foo::A;

    // Doesn't work
    use Foo::*;
    let y = A;
}

Replacing the glob import with a direct import of the variant (use Foo::A) doesn't work either.

To be honest, I found this quite surprising. It seems worth documenting at least.

Related: obi1kenobi/cargo-semver-checks#413

@obi1kenobi
Copy link
Member

This exact issue just happened in time v0.3.35 which broke some downstream crates: time-rs/time#675

I'm hoping this can still make it into the 2024 edition. If not (and assuming I can find more funding) I'm hoping to add a check for this into cargo-semver-checks, since the breakage here is clearly not just a theoretical concern.

andriyDev added a commit to andriyDev/landmass that referenced this issue Jul 8, 2024
This is just because of rust-lang/rust#73191. Otherwise, creating instances of the type-aliased versions doesn't work.
andriyDev added a commit to andriyDev/landmass that referenced this issue Jul 11, 2024
This is just because of rust-lang/rust#73191. Otherwise, creating instances of the type-aliased versions doesn't work.
andriyDev added a commit to andriyDev/landmass that referenced this issue Jul 11, 2024
This is just because of rust-lang/rust#73191. Otherwise, creating instances of the type-aliased versions doesn't work.
andriyDev added a commit to andriyDev/landmass that referenced this issue Jul 11, 2024
This is just because of rust-lang/rust#73191. Otherwise, creating instances of the type-aliased versions doesn't work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-maybe-future-edition Something we may consider for a future edition. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
Status: Idea
Development

No branches or pull requests

9 participants