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

Privacy for enum variants and trait items #2028

Closed
wants to merge 1 commit into from

Conversation

@petrochenkov
Copy link
Contributor

petrochenkov commented Jun 11, 2017

Support pub/pub(restricted) on enum variants and trait items, it helps with controlling matching exhaustiveness and trait implementability.

cc rust-lang/rust#32770 #943 #2008

Rendered

@le-jzr

This comment has been minimized.

Copy link

le-jzr commented Jun 12, 2017

Marking non-exhaustiveness with a dummy variant seems like a step in the wrong direction to me. It feels weird.

One of the reason it feels weird to me is that I have trouble imagining a case where this is useful for things you actually want to match on. Is it possible to show extant code where matchable private variants would be helpful?

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jun 12, 2017

From the RFC:

pub trait Trait1 {
    fn method1();
    pub(crate) fn method_without_default();
}

This feels strange to me. Under this RFC, the "package private" method here is method_without_default, but users coming from Java etc. would probably expect method1 to be the more private method, since it isn't explicitly marked pub. Although I understand the historical reasoning behind it, it seems really odd that the way to restrict the visibility of an element is to explicitly mark it as pub(X).

@withoutboats withoutboats added the T-lang label Jun 12, 2017
@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Jun 13, 2017

it seems really odd that the way to restrict the visibility of an element is to explicitly mark it as pub(X).

You can read pub(X) as publicness X.

@nikomatsakis nikomatsakis self-assigned this Jun 13, 2017
@ssokolow

This comment has been minimized.

Copy link

ssokolow commented Jun 13, 2017

You can read pub(X) as publicness X.

I think the issue being raised is that pub(X) is a keyword developed for a "private by default" context where it's still an example of making things "more public" and it's counter-intuitive to repurpose it to a context where pub(x) would make things "less public".

Especially since it can easily muddy the waters and side-track someone into wondering whether pub is valid and what the equivalent to omitting pub from a function is, given that, in this context, an unqualified pub's behaviour is the default.

TL;DR: It's bad for teachability if you have to explain that to everyone who concluded that pub(X) means "more public... but constrain to scope X".

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Jun 13, 2017

Oh, I get your point now. The issue is that the defaults are swapped here. Struct members are private by default while enum variants and trait items are pub by default. That's a valid point.

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Jun 13, 2017

Maybe we could require explicit pub/pub(foo) annotations to be present on either every variant / associated item or none of them?

@ssokolow

This comment has been minimized.

Copy link

ssokolow commented Jun 13, 2017

@glaebhoerl To be honest, that really screams "we wanted this so badly that we're making a hack everyone will recognize as ugly in order to circumvent our backwards-compatibility requirement".

In fact, that "you've gotta retroactively annotate all your old variants/items with default behaviour in order to start annotating non-default behaviour" pattern which appears nowhere else in the language feels like enough of a hack that I worry people I'm teaching/mentoring might count it against my credibility as an experienced authority unless I explicitly throw the language under the bus on that point as part of teaching them.

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Jun 13, 2017

To be clear, I think inherited privacy for trait items and enum variants was a mistake, but we gotta live in the world we live in and not the one we wish we were in. Having to sometimes decide which is the least-bad option that preserves backwards compatibility is just a fact of life (and to be clear again, it's not necessarily the one I floated). Maybe we could transition to a cleaner solution with epochs.

@burdges

This comment has been minimized.

Copy link

burdges commented Jun 13, 2017

You could add a unpub contextual keyword.. or even accept !pub perhaps. lol

@ssokolow

This comment has been minimized.

Copy link

ssokolow commented Jun 13, 2017

!pub would probably more intuitive, given how impl handles trait specification.

@bjorn3

This comment has been minimized.

Copy link

bjorn3 commented Jun 13, 2017

priv: its already a keyword.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Jun 13, 2017

We can change the default in a Rust 2.0 future, and requiring it just in the types using the new features is fairly small churn.

I think not doing things simply because they are deemed to give impression that we're admitting mistakes is rather obnoxious. Better to come clean than sweep under the rug.

@ssokolow

This comment has been minimized.

Copy link

ssokolow commented Jun 13, 2017

I'm not by any means saying that admitting to a mistake is bad.

I'm saying that, from my perspective, the "pub/pub(X) everywhere or no keywords on any of them" fix is such an uncharacteristic bodge that it would be a stain of Rust's grammar.

If priv is already a keyword and it wouldn't be at odds with how it's intended to be used elsewhere, I see no problem with a solution based on it.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jun 13, 2017

@glaebhoerl

To be clear, I think inherited privacy for trait items and enum variants was a mistake, but we gotta live in the world we live in and not the one we wish we were in.

I don't think it's a mistake, so far this scheme served us with almost no complaints, and this is indeed what you want in most cases.
If pubs for variants/trait-items were explicit, someone would probably already wrote an RFC about removing them as a part of the ergonomics initiative roadmap 😄

@le-jzr

This comment has been minimized.

Copy link

le-jzr commented Jun 13, 2017

I'm still waiting for that example of a public enum in existing crate that would benefit from non-public variants. :)

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jun 13, 2017

@le-jzr

I'm still waiting for that example of a public enum in existing crate that would benefit from non-public variants.

Please, wait some more, I haven't even looked through all the comments yet!

Do you mean in addition to exhaustiveness checks?
For example, inside of the crate you can use unreachable!() instead of "do nothing" for the dummy variant :)
But I suspect this should be better done automatically during lowering to or from MIR.

I can imagine cases when private variants could be useful. For example the enum is a state machine with some states that can be observed externally, and some states that are purely internal/intermediate.
I don't think I've seen in practice, but I don't think I've seen many state machines written in Rust as well.
EDIT: The @seanmonstar's example looks very similar to this.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Jun 13, 2017

My primary argument in support of the private variant idiom is that making non-exhaustive enums is not a dire everyday need and doesn't worth extending the language, there's just need to be some way to do it.
As for private variants/trait items, I don't feel like they really extend the language, they only support syntax for writing what is already there + give some extra bonuses like trait implementability control.

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Jun 13, 2017

I'm still waiting for that example of a public enum in existing crate that would benefit from non-public variants. :)

Not going to bother finding a crate with such a situation, but here's a very plausible example:

You could make use of an enum for configuring input arguments, such as some enum carrying a Vec<u8> or String, and you may have some other internal methods that make some guarantees for you, but you don't want to expose those publicly, as they could be unsafe, or leak implementation details.

pub enum Source {
    Utf8(String),
    Binary(Vec<u8>),
    pub(crate) PartiallyUtf8(Vec<u8>, usize),
}

pub fn encode(src: Source) -> EncodedString {

}

fn grow_and_encode(s: String, more: &[u8]) -> EncodedString {
    let index = s.len();
    let mut vec = s.into_bytes();
    vec.extend_from_slice(more);
    encode(Source::PartiallyUtf8(vec, index))
}
@burdges

This comment has been minimized.

Copy link

burdges commented Jun 13, 2017

I've kinda wanted non-public variant several times. I even learned the private-in-public when trying to create them. I run into them when trying to use a polymorphic enum for several purposes, including object internal state as well as object configuration.

That said, I think non-public variants look weaker than being able to circumvent the private-in-public rules, so maybe one should work towards that instead. If there was a pub(containable) that left the item private but allowed violating the private-in-public rules, and if we got ..Struct for struct inclusion, then I'd write something like :

pub(containable) struct InstantiateHereOnly;
...
pub enum Orientation<BCs: BodyCiphers,SKs: SURBKeys> {
    Send { bodies: BCs },
    SURB { surb_keys: SKs },
    SendAndSURB { surb_keys: SKs, bodies: BCs, ..InstantiateHereOnly },
}
pub const SEND: Orientation<(),()> = Orientation::Send { bodies: () };
pub const SURB: Orientation<(),()> = Orientation::SURB { surb_keys: () };

I could use this enum in several ways : I can use it for internal state. I can pass it in as a configuration option, usually using the two consts, but regardless nobody can instantiate SendAndSURB outside this module. Yet, I can return this enum too and destructure SendAndSURB outside this module.

You might argue against abusing polymorphic enum this way of course, as I could always write two or three separate enums. Arguably, my code would be more flexible if done that way, yet doing it this way may better connects the exhaustiveness checks between the three usages.

tl;dr I think the case against non-public variants is ultimately stylistic and subtle, but I also think mechanisms for breaking the private-in-public rules look more powerful.

@crumblingstatue

This comment has been minimized.

Copy link

crumblingstatue commented Jun 17, 2017

If this RFC establishes the pub(restricted) syntax for trait items, supporting #275 could be done through pub(impl).

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Aug 26, 2017

#2008 was merged, so I'm closing this as not having sufficient motivation.

@petrochenkov petrochenkov deleted the petrochenkov:morevis branch Sep 10, 2017
@jonhoo

This comment has been minimized.

Copy link
Contributor

jonhoo commented Sep 18, 2018

@petrochenkov I don't understand how #2008 solves the issues raised by this issue, in particular as it relates to enums? Marking an enum as non_exhaustive is not sufficient if you don't want users to peek inside of the variants. You currently have to work around this by introducing a newtype around the enum, but that makes it much more clumsy to work with the type internally...

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Oct 31, 2019

@petrochenkov I'm running into a situation where #[non_exhaustive] doesn't help in the slightest. I have an enum variant that should only ever be used internally, as it represents an intermediate state that can never occur in user code. I could certainly work around it, but right now the easiest way by far is to add an __internal variant, which is the same problem as the __nonexhaustive variant that led to #2008.

@KrishnaSannasi

This comment has been minimized.

Copy link

KrishnaSannasi commented Oct 31, 2019

@jhpratt could you do something like this?

pub enum Foo {
    Bar(...),
    Private(Private),
}

pub struct Private {
    fueld: Quax
}

This way users can't construct the variant if they never see the variant.

@jhpratt

This comment has been minimized.

Copy link

jhpratt commented Nov 1, 2019

@KrishnaSannasi I could, but as I mentioned in my previous comment, it's easier to add an __internal variant. As it turns out, there was a clever workaround in the way my data was structured, but that's not always the case.

@kennytm kennytm added the A-privacy label Feb 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.