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

Removing priv as a keyword #8122

Closed
alexcrichton opened this issue Jul 30, 2013 · 33 comments
Closed

Removing priv as a keyword #8122

alexcrichton opened this issue Jul 30, 2013 · 33 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Milestone

Comments

@alexcrichton
Copy link
Member

Now that you can't really put pub at the top-level of things like impl/extern/trait, you don't need the priv keyword to control method visibility of various items because they're all private by default.

pros:

  • I think that this would really simplify public/private because there's one and only one rule: private by default, public if you flag it
  • one less keyword!

difficulties:

  • Right now this is the only way to make struct fields private. I personally believe that private-by-default for struct fields isn't that bad of an idea, and if this were the only thing blocking removing priv I'd vote to remove it.

cons:

  • Enum variants can currently be tagged as priv, and if we removed priv there would be no way to make half the enum variants public and half private. In theory pub enum makes everything public and you could otherwise write pub on each variant, but the type name would still then have to be public and if it didn't have pub in front of it it wouldn't really make sense
  • Private-by-default for structs is kind of annoying sometimes.

What do others think about this?

@AndresOsinski
Copy link

Well, Haskell exposes members explicitly, as does Node. Java asks for
explicit private/public members, as does C++.

I think there's a good case, not only in language precedent but as a
general good practice, that public members should be explicitly tagged as
such. It also creates a slight amount of mental friction when deciding to
make a member public. IMO, this is good; you actually have to reason at
least a tiny bit when you expose a member, which generally leads to
more succinct and better-designed interfaces (much like immutability forces
you to reason about correctness in a way that mutable-everything does not).

On Tue, Jul 30, 2013 at 4:06 AM, Alex Crichton notifications@github.comwrote:

Now that you can't really put pub at the top-level of things like
impl/extern/trait, you don't need the priv keyword to control method
visibility of various items because they're all private by default.

pros:

  • I think that this would really simplify public/private because
    there's one and only one rule: private by default, public if you flag it
  • one less keyword!

difficulties:

  • Right now this is the only way to make struct fields private. I
    personally believe that private-by-default for struct fields isn't that bad
    of an idea, and if this were the only thing blocking removing priv I'd
    vote to remove it.

cons:

  • Enum variants can currently be tagged as priv, and if we removed
    priv there would be no way to make half the enum variants public and half
    private. In theory pub enum makes everything public and you could
    otherwise write pub on each variant, but the type name would still
    then have to be public and if it didn't have pub in front of it it
    wouldn't really make sense
  • Private-by-default for structs is kind of annoying sometimes.

What do others think about this?


Reply to this email directly or view it on GitHubhttps://github.com//issues/8122
.

Andrés Osinski
http://www.andresosinski.com.ar/

@Thiez
Copy link
Contributor

Thiez commented Jul 30, 2013

I don't think it's possible to pattern match on a struct that has one or more private members. Having to mark almost every struct field as pub just so I can use pattern matching seems much more inconvenient to me than having priv as a keyword.

@bstrie
Copy link
Contributor

bstrie commented Jul 30, 2013

I agree that "private by default, always" is a simpler rule, but I caution against overexuberance at the thought of removing a keyword. It very well might not be worth the loss of convenience.

One good data point would be to figure out how many of the currently-public fields in the compiler could actually be private. An off-by-default lint pass might help with this.

@bblum
Copy link
Contributor

bblum commented Jul 30, 2013

I was almost sold on this until I realized this meant you would have to write, for example, pub enum Option<T> { pub Some(T), pub None } in order for anybody else to construct or pattern-match against your enum at all.

@bluss
Copy link
Member

bluss commented Jul 30, 2013

Can we change static to priv by default as well?

@alexcrichton
Copy link
Member Author

@bblum: I would be ok with pub enum makes all variants public, while enum implies all variants are private. That's one of the sticky points of removing priv because then you can't have a mixture of public/private enum variants.

@blake2-ppc: I would certainly hope so!

@brson
Copy link
Contributor

brson commented Jul 31, 2013

I guess I'm in somewhat in favor since I think pub struct fields are the wrong default. I suspect that inheriting visibility from enums to their variants will not work out satisfactorily though - we tried that for impls and found it confusing.

@alexcrichton
Copy link
Member Author

@brson, although isn't that what enums do right now? (inheriting visibility from the type name to the variants).

@emberian
Copy link
Member

I don't think private-by-default struct fields are a good thing. You cannot construct them at all without constructor functions, which just makes it more work.

@thestinger
Copy link
Contributor

The fields would be private by-default only, we wouldn't require any more constructor functions than we have now.

@emberian
Copy link
Member

Well, yes, but it also requires an explosion of pub, unless we inherit
visibility, but I thought we were going away from that.

Then again, traits already require pub, so maybe it won't be as bad as
I'm imagining it. It would be nice to have a single rule.

On Wed, Jul 31, 2013 at 6:12 PM, Daniel Micay notifications@github.comwrote:

The fields would be private by-default only, we wouldn't require any
more constructor functions.


Reply to this email directly or view it on GitHubhttps://github.com//issues/8122#issuecomment-21899998
.

@thestinger
Copy link
Contributor

I think private-by-default is better because a wrongly private field is going to be quickly noticed. I wouldn't be surprised if we actually had more structs where the fields should be private, than those used as transparent blobs of data.

It's also odd for the fields in newtypes and tuple structs to be public by-default (or only public, as they are right now). You would expect half of their purpose to be concealing an implementation detail, otherwise you would just use a type alias (unless you needed to override a trait implementation).

@emberian
Copy link
Member

emberian commented Aug 7, 2013

This is true. After thinking about it some more I'm fine with this.

@alexcrichton
Copy link
Member Author

Nominating for the backwards-compatible milestone.

@catamorphism
Copy link
Contributor

accepted for backwards-compatible (note that we haven't decided for-sure to do this, but making the decision is on milestone 2)

@bstrie
Copy link
Contributor

bstrie commented Oct 30, 2013

Regardless of the fate of the priv keyword itself, I think I'm now in favor of making struct fields private-by-default after #4386. Note that this still allows the fields to be directly accessed by anything in the same module (possibly the same crate?), so it's not an especially onerous restriction. And it really does help keep the public API under control.

@emberian
Copy link
Member

+1

On Wed, Oct 30, 2013 at 9:03 AM, Ben Striegel notifications@github.comwrote:

Regardless of the fate of the priv keyword itself, I think I'm now in
favor of making struct fields private-by-default after #4386#4386.
Note that this still allows the fields to be directly accessed by
anything in the same module (possibly the same crate?), so it's not an
especially onerous restriction. And it really does help keep the public API
under control.


Reply to this email directly or view it on GitHubhttps://github.com//issues/8122#issuecomment-27386790
.

@alexcrichton
Copy link
Member Author

I believe that the main case holding back making fields private-by-default is the simple:

pub struct Point { x: int, y: int }

That's particularly easy to read, and if you compare it to

pub struct Point { pub x: int, pub y: int }

It's a little more verbose. This is an obvious consequence of private-by-default fields, but it's also unfortunate to regress in this simple example. We haven't decided yet whether this is an acceptable regression. Some attempts were made to make this syntax concise as well, and most of them focused on changing the declaration of the struct to say "I want my fields public by default". In this case, a normal struct would be private by default, and you would have to explicitly opt-in to having public-by-default fields.

We didn't really reach any good conclusions in this case, and as a result this sort of stagnated and stopped making progress. This is high up on my agenda of things to bring up at meetings, however, and I think that I'll get around to this at the next one.

@pnkfelix
Copy link
Member

Has anyone suggested adopting private-by-default fields but also adding extending the syntax for struct/enum with a leading pub: (note the trailing colon) inside the braces if you want to go back to pub-by-default? (This proposal might necessitate keeping priv as a keyword, but just not ever using it except in conjunction with pub:).

((Update: I guess a suggestion like this is what @alexcrichton was alluding to at the end of his second to last paragraph of the previous comment.))

So to express (under priv-by-default rules):

pub struct Point { pub x: int, pub y: int }
pub struct CP { pub x: int, pub y: int, col: Color }

you could instead do

pub struct Point { pub: x: int, y: int }
pub struct CP { pub: x: int, y: int, priv col: Color }

This way, you don't pollute each field with a pub annotation. Instead its a property of the whole definition. (In particular, I'm not suggesting we adopt C++ style public:/private: annotations; the pub: in this proposal would only be allowed before any fields/variants have been introduced.)

@emberian
Copy link
Member

That's a nice solution. 👍

On Fri, Nov 15, 2013 at 3:40 AM, Felix S Klock II
notifications@github.comwrote:

Has anyone suggested adopting private-by-default fields but also adding
extending the syntax for struct/enum with a leading pub: (note the
trailing colon) inside the braces if you want to go back to pub-by-default?
(This proposal might necessitate keeping priv as a keyword, but just
using it except in conjunction with pub:).

So to express (under priv-by-default rules):

pub struct Point { pub x: int, pub y: int }
pub struct CP { pub x: int, pub y: int, col: Color }

you could instead do

pub struct Point { pub: x: int, y: int }pub struct CPnt { pub: x: int, y: int, priv col: Color }

This way, you don't pollute each field with a pub annotation. Instead its
a property of the whole definition. (In particular, I'm not suggesting
we adopt C++ style public:/private: annotations; the pub: in this
proposal would only be allowed before any fields have been introduced.)


Reply to this email directly or view it on GitHubhttps://github.com//issues/8122#issuecomment-28554486
.

@pcwalton
Copy link
Contributor

I think I'm ok with @pnkfelix' solution. Removing a keyword is always nice.

@glaebhoerl
Copy link
Contributor

I think you could also use similar syntax to control the visibility of tuple structs innards (which is an annoying limitation right now). Then if you write

pub struct Handle(uint);

it would be an abstract type when viewed from outside the module (you couldn't access the uint).

You would have to write:

pub struct Point(pub: uint, uint);

to make a type whose tuple-ness is externally available.

@glaebhoerl
Copy link
Contributor

(alexcrichton) I would be ok with pub enum makes all variants public, while enum implies all variants are private. That's one of the sticky points of removing priv because then you can't have a mixture of public/private enum variants.

I checked with the folks in #haskell to make sure, and I don't think there's any use case for having a mixture of public and private variants. For constructing the type, which is usually what you care about, you can just use functions, so the question hinges on pattern matching. Does anyone else see a reason why you might want to let clients check for particular variants via pattern matching but not others (and therefore be required to use _ in every match for exhaustiveness)?

If that's so, then I think there's three inter-related questions left:

  1. Do enum variants inherit their visibility from the enum, or do they default to private and require @pnkfelix's pub: syntax to be made public? A public abstract datatype with many private variants does have use cases. If enum variants inherit visibility, then that can still be expressed by wrapping the enum in a newtype struct, it's just more annoying. While if variants default to private, then having to write pub: when you want them public is annoying.
  2. Do we require pub: for tuple-like structs as well? [See my previous comment.] Having to write pub: is annoying, but otherwise, there's no way to have a tuple-like struct with private fields, and a lightweight way to have a public type with private data is one of the primary motivations for newtypes.
  3. What about enum variants' fields? This interacts with both of the previous points. For struct-like enum variants, if the variants inherit their visibility, then do the fields also inherit it, or do they default to private? If the fields also inherit visibility, then it's inconsistent with structs, where they default to private; if they default to private, then it's yet another place you have to write pub: (and this would presumably also be the case if the variants default to private). If we say "yes" to 2., then the same thing applies to tuple-like variants as well.

@thestinger
Copy link
Contributor

IMO, variants shouldn't have privacy and should be imported into a scope when the type is imported. This would remove the need for the complexity of an enum mod type where each case would need to make a nearly arbitrary style decision.

@frgomes
Copy link

frgomes commented Mar 6, 2014

Explicit is better than implicit.

If you leave a variable without any keyword for visibility ... was that because you meant priv or was that because you left that code behind on your code review?

In Java, you have the case of "package protected", which is the default and does not have any keyword for it. I rarely use this visibility: in general I make everything private. But, in the rare circumstances i use "package protected"... I put an annotation I've created specially for this situation, meaning: Yes! I know exactly what I'm doing.

I even an additional compilation rule which detect the situation of "package protected" variables which are not annotated properly, which means: yes... I forgot to review this code.

@hatahet
Copy link
Contributor

hatahet commented Mar 6, 2014

Taking Scala as an example, the default is that classes and class members are all public by default, making it a lot less verbose than Java in this aspect.

@frgomes
Copy link

frgomes commented Mar 6, 2014

IMHO, reducing verbosity is a good thing in general.
I also think that everything priv by default is the right approach
for robust code.

I just don't think that priv should be removed.
I explain why below, borrowing experiences from Java.

In Java, "package protected" variables do not have an explicit keyword
for it.
So, when you don't see anything before a variable... what's the
interpretation?

  1. The author meant "package protected"?
  2. The author forgot to think about visibility in that case?

In my code, I usually put an annotation created specially for such sort
of situation, meaning "I know exactly what I'm doing, and this thing
must be @PackageProtected". I've also created additional compilation
checks which detect variables without any explicit visibility flag and
without my fancy annotation attached to them.

Thanks

Richard Gomes
(contact info removed by @pnkfelix; @frgomes feel free to put back if you intended for it to be preserved)

On 06/03/14 17:56, Ziad Hatahet wrote:

Taking Scala as an example, the default is that classes and class
members are all public by default, making it a lot less verbose than
Java in this aspect.


Reply to this email directly or view it on GitHub
#8122 (comment).

@liigo
Copy link
Contributor

liigo commented Mar 6, 2014

2014年3月7日 上午2:19于 "Richard Gomes" notifications@github.com写道:

IMHO, reducing verbosity is a good thing in general.
I also think that everything priv by default is the right approach
for robust code.

I just don't think that priv should be removed.
I explain why below, borrowing experiences from Java.
!

Agree.

In Java, "package protected" variables do not have an explicit keyword
for it.
So, when you don't see anything before a variable... what's the
interpretation?

  1. The author meant "package protected"?
  2. The author forgot to think about visibility in that case?

In my code, I usually put an annotation created specially for such sort
of situation, meaning "I know exactly what I'm doing, and this thing
must be @PackageProtected". I've also created additional compilation
checks which detect variables without any explicit visibility flag and
without my fancy annotation attached to them.

Thanks

Richard Gomes
http://rgomes.info
http://www.linkedin.com/in/rgomes
mobile: +44(77)9955-6813
inum http://www.inum.net/: +883(5100)0800-9804
sip:rgomes@ippi.fr

On 06/03/14 17:56, Ziad Hatahet wrote:

Taking Scala as an example, the default is that classes and class
members are all public by default, making it a lot less verbose than
Java in this aspect.


Reply to this email directly or view it on GitHub
#8122 (comment).


Reply to this email directly or view it on GitHub.

@lilyball
Copy link
Contributor

lilyball commented Mar 8, 2014

I just came up with a use-case for private enum variants. Specifically, I have a type that needs to be public, but its implementation needs to be private, and the only sane implementation is as an enum. If priv is removed, this could be emulated by using a public struct with a single private field that is a private enum. But that's awkward.

@sfackler
Copy link
Member

sfackler commented Mar 8, 2014

I've used enums with all private variants as well, but am not really opposed to wrapping in a struct.

@glaebhoerl
Copy link
Contributor

@kballard I would like to have that ability as well. (What I don't want to have, but we currently do, is the option to have some variants public, and others private. I don't think that makes any sense.) (See also some related thoughts from the ML.)

Edit: To summarize, I would like to support:

  • Structs with all fields public, or with all fields private
  • Tuple structs with all fields public, or with all fields private
  • Enums with all variants (and all fields thereof) public, or with all variants private

What I don't want to support:

  • Enums with some variants public, and others private
  • Enums with public variants with private fields

I'm mostly indifferent as to whether we support:

  • Structs with some fields public, and others private

This makes thinking about syntax a bit easier, but there's still no obvious winner.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 26, 2014
This is a necessary change in preparation for switching the defaults as part
of rust-lang#8122.

RFC: 0004-private-fields
alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 26, 2014
This change is in preparation for rust-lang#8122. Nothing is currently done with these
visibility qualifiers, they are just parsed and accepted by the compiler.

RFC: 0004-private-fields
alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 31, 2014
This commit switches privacy's checking of fields to have *all* fields be
private by default. This does not yet change tuple structs, this only affects
structs with named fields. The fallout of this change will follow shortly.

RFC: 0004-private-fields

cc rust-lang#8122
Closes rust-lang#11809
@alexcrichton
Copy link
Member Author

I'm going to close this in favor of discussion on the RFC. Note that RFC 4 has been implemented, so many parts of the original description have been implemented (just not removing private enum variants)

@pnkfelix
Copy link
Member

see also #13535

Manishearth added a commit to Manishearth/rust that referenced this issue May 1, 2015
…lexcrichton

Hi! While researching stuff for the reference and the grammar, I came across a few mentions of using the `priv` keyword that was removed in 0.11.0 (rust-lang#13547, rust-lang#8122, rust-lang/rfcs#26, [RFC 0026](https://github.com/rust-lang/rfcs/blob/master/text/0026-remove-priv.md)).

One occurrence is a mention in the reference, a few are in comments, and a few are marking test functions. I left the test that makes sure you can't name an ident `priv` since it's still a reserved keyword. I did a little grepping around for `priv `, priv in backticks, `Private` etc and I think the remaining instances are fine, but if anyone knows anywhere in particular I should check for any other lingering mentions of `priv`, please let me know and I would be happy to! 🍂 🌊
Manishearth added a commit to Manishearth/rust that referenced this issue May 1, 2015
…lexcrichton

Hi! While researching stuff for the reference and the grammar, I came across a few mentions of using the `priv` keyword that was removed in 0.11.0 (rust-lang#13547, rust-lang#8122, rust-lang/rfcs#26, [RFC 0026](https://github.com/rust-lang/rfcs/blob/master/text/0026-remove-priv.md)).

One occurrence is a mention in the reference, a few are in comments, and a few are marking test functions. I left the test that makes sure you can't name an ident `priv` since it's still a reserved keyword. I did a little grepping around for `priv `, priv in backticks, `Private` etc and I think the remaining instances are fine, but if anyone knows anywhere in particular I should check for any other lingering mentions of `priv`, please let me know and I would be happy to! 🍂 🌊
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

No branches or pull requests