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

Rust 1.19 regression, 0.1.8, variant is private and cannot be reexported in pub glob #42460

Closed
brson opened this Issue Jun 6, 2017 · 17 comments

Comments

Projects
None yet
4 participants
@brson
Contributor

brson commented Jun 6, 2017

https://github.com/rust-locale/rust-locale

commit bd222a46a1ca2326220b6a071a6c712afe97f19f
Author: Ben S <ogham@bsago.me>
Date:   Thu Apr 23 07:59:28 2015 +0100

    Remove #[deprecated] attributes

brian@ip-10-145-43-250:/mnt2/dev⟫ rustc +nightly -Vv
rustc 1.19.0-nightly (0418fa9d3 2017-06-04)
binary: rustc
commit-hash: 0418fa9d382a47d782cc1e195c14573be9c32095
commit-date: 2017-06-04
host: x86_64-unknown-linux-gnu
release: 1.19.0-nightly
LLVM version: 4.0
error: variant `_NL_CTYPE_OUTDIGIT2_MB` is private, and cannot be reexported, consider declaring its enum as `pub`
   --> src/linux/langinfo.rs:135:9
    |
135 | pub use self::CTypeStringItems::*;
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
error: variant `_NL_TIME_WEEK_NDAYS` is private, and cannot be reexported, consider declaring its enum as `pub`
   --> src/linux/langinfo.rs:415:9
    |
415 | pub use self::ByteItems::*;
    |         ^^^^^^^^^^^^^^^^^^^
@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jun 6, 2017

Contributor

Probably from #42136

What exactly is going on here that is broken? It looks like they might be reexporting private variants, but I don't see a lint in the linked PR that looks exactly like that.

Contributor

brson commented Jun 6, 2017

Probably from #42136

What exactly is going on here that is broken? It looks like they might be reexporting private variants, but I don't see a lint in the linked PR that looks exactly like that.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jun 6, 2017

Contributor

cargobomb sees this in 5-6 crates revisions, some in wide use like mustache and nickel.

Contributor

brson commented Jun 6, 2017

cargobomb sees this in 5-6 crates revisions, some in wide use like mustache and nickel.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jun 6, 2017

Contributor

nickel inherits the breakage from mustache.

cc @erickt @cburgdorf

Contributor

brson commented Jun 6, 2017

nickel inherits the breakage from mustache.

cc @erickt @cburgdorf

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jun 6, 2017

Contributor

Quite a few crates infected by mustache here.

Contributor

brson commented Jun 6, 2017

Quite a few crates infected by mustache here.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Jun 6, 2017

Contributor

@brson
This is a subset of private_in_public lint (#34537), cc'd from #42136. This was a warning since 2015 and even then locale was the only regression for the "private variant reexport" case.
The problem is that the 0.2.x branch of locale is not developed and was last updated on Feb 15, 2016.
I asked @jan-hudec to publish a new version and locale-0.2.2 was published recently, it includes the fix.
Dependent crates have to be updated. I'm not sure why all these dependent regression didn't show up in #42136 crater run. I sent patches to everything that appeared there (I can send more, but I need to know what crates are affected).

Contributor

petrochenkov commented Jun 6, 2017

@brson
This is a subset of private_in_public lint (#34537), cc'd from #42136. This was a warning since 2015 and even then locale was the only regression for the "private variant reexport" case.
The problem is that the 0.2.x branch of locale is not developed and was last updated on Feb 15, 2016.
I asked @jan-hudec to publish a new version and locale-0.2.2 was published recently, it includes the fix.
Dependent crates have to be updated. I'm not sure why all these dependent regression didn't show up in #42136 crater run. I sent patches to everything that appeared there (I can send more, but I need to know what crates are affected).

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 15, 2017

Contributor

@petrochenkov did we consider exempting the private variants from the *? I guess that would be somewhat inconsistent.

Contributor

nikomatsakis commented Jun 15, 2017

@petrochenkov did we consider exempting the private variants from the *? I guess that would be somewhat inconsistent.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jun 15, 2017

Contributor

@petrochenkov I've assigned you.

Contributor

brson commented Jun 15, 2017

@petrochenkov I've assigned you.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Jun 15, 2017

Contributor

@nikomatsakis
If I understand correctly you are suggesting this

enum E {
    V,
    U,
}

pub use E::*; // OK, but exports nothing, should be reported as unused import

which is quite reasonable and consistent with module behavior

mod m {
    struct A;
    struct B;
}

pub use m::*; // OK, but exports nothing, should be reported as unused import but doesn't currently

There are two issues with this

  • This doesn't fix locale-0.2.1, because it relies on pub use E::*; actually reexporting something. Even if this is permitted in resolve it's still a type privacy violation that will be caught by #42125 (you can actually see locale-0.2.1 in the crater report there).
  • This may be more surprising than the current early error - "wait, I've just reexported these variants, where are they?", but probably not too surprising, because glob reexport from modules behave in the same way.

As a result, I'm mildly in favor of turning this error into "unused import" warning, but I't won't help with locale-0.2.1 which is legitimately broken.

Contributor

petrochenkov commented Jun 15, 2017

@nikomatsakis
If I understand correctly you are suggesting this

enum E {
    V,
    U,
}

pub use E::*; // OK, but exports nothing, should be reported as unused import

which is quite reasonable and consistent with module behavior

mod m {
    struct A;
    struct B;
}

pub use m::*; // OK, but exports nothing, should be reported as unused import but doesn't currently

There are two issues with this

  • This doesn't fix locale-0.2.1, because it relies on pub use E::*; actually reexporting something. Even if this is permitted in resolve it's still a type privacy violation that will be caught by #42125 (you can actually see locale-0.2.1 in the crater report there).
  • This may be more surprising than the current early error - "wait, I've just reexported these variants, where are they?", but probably not too surprising, because glob reexport from modules behave in the same way.

As a result, I'm mildly in favor of turning this error into "unused import" warning, but I't won't help with locale-0.2.1 which is legitimately broken.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 16, 2017

Contributor

@petrochenkov

Yes, I think you understood me.

This doesn't fix locale-0.2.1, because it relies on pub use E::*; actually reexporting something.

In other words, it really wants the enum to be public, but it's not?

because glob reexport from modules behave in the same way.

This was the precedent I was thinking of, yeah. It's sort of "as if" the enum were a mini-module -- but the analogy doesn't quite work, since if the items were truly acting like private members, then E::V wouldn't work.

Contributor

nikomatsakis commented Jun 16, 2017

@petrochenkov

Yes, I think you understood me.

This doesn't fix locale-0.2.1, because it relies on pub use E::*; actually reexporting something.

In other words, it really wants the enum to be public, but it's not?

because glob reexport from modules behave in the same way.

This was the precedent I was thinking of, yeah. It's sort of "as if" the enum were a mini-module -- but the analogy doesn't quite work, since if the items were truly acting like private members, then E::V wouldn't work.

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Jun 16, 2017

Contributor

@nikomatsakis

In other words, it really wants the enum to be public, but it's not?

Yes.

Contributor

petrochenkov commented Jun 16, 2017

@nikomatsakis

In other words, it really wants the enum to be public, but it's not?

Yes.

@erickt

This comment has been minimized.

Show comment
Hide comment
@erickt

erickt Jun 17, 2017

Contributor

@brson unfortunately I don't have commit access to the current mustache repository. I've asked for access in nickel-org/rust-mustache#47.

Contributor

erickt commented Jun 17, 2017

@brson unfortunately I don't have commit access to the current mustache repository. I've asked for access in nickel-org/rust-mustache#47.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 29, 2017

Contributor

What is the current status here? I'm having second thoughts about accepting this code, given that the analogy to a module doesn't really hold up -- can we patch up the crates in question?

Contributor

nikomatsakis commented Jun 29, 2017

What is the current status here? I'm having second thoughts about accepting this code, given that the analogy to a module doesn't really hold up -- can we patch up the crates in question?

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Jun 30, 2017

Contributor

@nikomatsakis
locale is already patched and I don't know what else needs to be fixed.
@brson mentioned mustache and nickel, but they both do not depend on locale, maybe only older versions are affected?
ping @erickt

Contributor

petrochenkov commented Jun 30, 2017

@nikomatsakis
locale is already patched and I don't know what else needs to be fixed.
@brson mentioned mustache and nickel, but they both do not depend on locale, maybe only older versions are affected?
ping @erickt

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jul 6, 2017

Contributor

@petrochenkov ok -- so -- is any further action needed here then? I guess we just want to see whether older versions of mustache and nickel need some sort of update?

Contributor

nikomatsakis commented Jul 6, 2017

@petrochenkov ok -- so -- is any further action needed here then? I guess we just want to see whether older versions of mustache and nickel need some sort of update?

@petrochenkov

This comment has been minimized.

Show comment
Hide comment
@petrochenkov

petrochenkov Jul 6, 2017

Contributor

@nikomatsakis
I believe no action is required from our side.

I guess we just want to see whether older versions of mustache and nickel need some sort of update?

Most of mustache versions are not tagged, there are also no version branches to send PRs to, and @erickt , who has publishing rights, is silent. What can we do? Wash our hands, I guess.

Contributor

petrochenkov commented Jul 6, 2017

@nikomatsakis
I believe no action is required from our side.

I guess we just want to see whether older versions of mustache and nickel need some sort of update?

Most of mustache versions are not tagged, there are also no version branches to send PRs to, and @erickt , who has publishing rights, is silent. What can we do? Wash our hands, I guess.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jul 6, 2017

Contributor

OK, I am going to close as "changed due to bug fix" then. I added the relnotes tag, because this change potentially affects end users. Specifically, code like this used to compile, even though it is re-exporting private variants (Bar, Baz) as public:

enum Foo { Bar, Baz }
pub use self::Foo::Bar;

We now correctly report a compilation error.

Contributor

nikomatsakis commented Jul 6, 2017

OK, I am going to close as "changed due to bug fix" then. I added the relnotes tag, because this change potentially affects end users. Specifically, code like this used to compile, even though it is re-exporting private variants (Bar, Baz) as public:

enum Foo { Bar, Baz }
pub use self::Foo::Bar;

We now correctly report a compilation error.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Jul 7, 2017

Contributor

@petrochenkov it is an older mustache but one that is a dependency of many crates.

Contributor

brson commented Jul 7, 2017

@petrochenkov it is an older mustache but one that is a dependency of many crates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment