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

Require public trait items to be marked pub #227

Closed
wants to merge 2 commits into from
Closed

Require public trait items to be marked pub #227

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 4, 2014

@ghost ghost mentioned this pull request Sep 4, 2014
@ghost
Copy link
Author

ghost commented Sep 4, 2014

Maybe I should have also mentioned that in an implementation of a trait for a concrete type, any definition for a public trait item must be marked pub also (obviously).

@huonw
Copy link
Member

huonw commented Sep 4, 2014

(This would be improved with some code examples.)

@Ericson2314
Copy link
Contributor

Sounds good to me. I'd like to see unification of traits and modules in Rusts 2.0 or something like that, and this pushes Rust ever so slightly in that direction.

@Kimundi
Copy link
Member

Kimundi commented Sep 20, 2014

Implementing this change also allows backwards compatible removing it again in the future in case private trait items are not wanted after all: Just make the pub optional and emit a deprecated warning if its present.

@pnkfelix
Copy link
Member

pnkfelix commented Nov 4, 2014

There was some discussion of this RFC at tonight's team meeting: discussion

I think the team is generally positive about going in this direction, but there were some questions about whether putting the pub annotations within the impl Trait for Type { ... } items actually has the right mental connotations. i believe the parties involved in that discussion should present their views in some public forum (preferably this one) in the near future.

cc @pcwalton @aturon @nikomatsakis

@nikomatsakis
Copy link
Contributor

As @pnkfelix said, the biggest sticking point was whether or not one ought to repeat the pub in the impl, as well as the fact that there isn't much agreement about what a priv method on a trait ought to mean.

On the pro side of repeating pub:

  • We repeat lots of things from the trait, it's confusing to repeat only somethings.
  • Easier to cut-and-paste.
  • Use cases for private trait methods have arisen.

On the con side of repeating pub:

  • Privacy (when we implement it) in an impl will not work like privacy elsewhere. Private items in an impl are not going to be limited to the trait where the impl appears; in fact they are quite the opposite, probably limited to being used from some other trait. Maybe we want to use a keyword or something to indicate this.
  • Lots of churn (tons of pubs!) only weakly justified ("maybe we'll do privacy in the future"). I think a clearer picture of what privacy might look like and what use cases it would fill would help to motivate.

Some things to consider:

  • It seems likely we will grow a more nuanced form of privacy in the future, to accommodate at minimum the idea of crate-local (though I personally favor the idea of being able to say pub(some::path), which means "public to submodules of some::path). Should we repeat this full specification in the impl as well?
  • What are the use cases for private methods in traits? I believe one example of a use case are the routines in Any. I'm not sure what other use cases exist, though I feel like I've encountered the desire from time to time.

Personally, I think the most likely semantics for private items in a trait would be that they can be defined anywhere but referenced only from within the module that defined the trait. I think I would prefer to do this by just denoting private things via the absence of pub, despite the possible confusion that arises, because having multiple ways to designate privacy feels like overkill -- instead people will just have to learn what privacy in an impl means. But I'm not sure about this yet.

@ghost
Copy link
Author

ghost commented Nov 5, 2014

For motivation, read The D Programming Language chapter 6.9.1 The Non-Virtual Interface (NVI) Idiom (by the way, we also need final default/provided trait methods for NVI's). Then read the whole book if you want to understand some of D's language design choices (invariably compromises).

@pnkfelix
Copy link
Member

@nikomatsakis when you write "I think I would prefer to do this by just denoting private things via the absence of pub", I interpret it as a statement in support of this RFC. Is that correct, or do I misunderstand you?

I am a little wary of letting the discussion here get bogged down in discussion of the details of the semantics of privacy. . . but I do agree that a small example is warranted. And by that I mean something that is both 1. not stuck behind a paywall (hint hint), and 2. written in a hypothetical extension of Rust.

@pnkfelix
Copy link
Member

Up front caveat: I normally would not use an in-progress RFC to provide motivation for an unrelated RFC. However, @nikomatsakis and i have been talking much about this other topic recently, and so I thought he might be able to provide much insight into whether this actually could be a use case in the long term (like Rust 2.0 ... or maybe Rust 1.1 ...).


@nikomatsakis also, in case you are interested: I thought that private trait items might possibly be motivated by cases I encountered while working on the placement box RFC #470. In particular, it might be possible (and desirable) to make things like the particular impl of PlacementAgent a private detail of the libraries that implement Placer.

I am thinking of something like this:

pub trait Placer {
    pub type Data;
    pub type Owner;
    type Interim: PlacementAgent<Self::Data, Self::Owner>;

    fn make_place(self) -> Interim;
}

This illustrates that the input Data and the ouput Owner for a Placer are both public parts of the interface used by end clients of the placement form. But the Interim type that implements PlacementAgent is not meant to be exposed to the people writing in (<place-expr>) <value-expr> ; it is only meant to be something that the implementor of Placer itself cares about.

Like so:

/// This the singleton type used solely for `boxed::HEAP`.
pub struct ExchangeHeapSingleton { _force_singleton: () }

pub const HEAP: ExchangeHeapSingleton =
    ExchangeHeapSingleton { _force_singleton: () };

pub struct Box<Sized? T>(*mut T);

// (This used to be forced to be `pub`; now it can be hidden within `heap::alloc::boxed`)
struct IntermediateBox<Sized? T>{
    ptr: *mut u8,
    size: uint,
    align: uint,
}

impl<T> Placer<T, Box<T>, IntermediateBox<T>> for ExchangeHeapSingleton {
    fn make_place(self) -> IntermediateBox<T> {
        ...
    }
}

impl<Sized? T> PlacementAgent<T, Box<T>> for IntermediateBox<T> {
    fn pointer(&mut self) -> *mut T {
        ...
    }
    unsafe fn finalize(self) -> Box<T> {
        ...
    }
}

(Of course this probably does not work in any way with today's expansion based prototype rust-lang/rust#18233 since the privacy pass is going to be looking at the expanded code.)

@ghost
Copy link
Author

ghost commented Nov 18, 2014

I presented this use case (for more efficient for-loops) a while ago but it turned out to be void. The general idea of it might be useful in other contexts though. The example is written assuming public by default and marking privates by priv. Also, I don't agree with my then notion of the semantics of private trait items anymore.

@Manishearth
Copy link
Member

I'd personally prefer if we simply reserved priv now, and later started using it for private trait items. Putting pub everywhere seems rather unnecessary, we should be tryign to make the common case easier to type.

(Also we may want priv for something else later)

@glaebhoerl
Copy link
Contributor

Kind of frustrating that I have a better solution (syntactically and semantically), but am mostly incapable of writing RFCs. Also that there's zero chance it would be adopted, because 1.0. (Which are not unconnected.)

@asb
Copy link

asb commented Nov 21, 2014

@glaebhoerl could you elaborate a little on your idea?

@aturon
Copy link
Member

aturon commented Nov 21, 2014

Kind of frustrating that I have a better solution (syntactically and semantically), but am mostly incapable of writing RFCs. Also that there's zero chance it would be adopted, because 1.0. (Which are not unconnected.)

Sorry to hear that. Do you mean a solution to traits with private items? What would the backwards-incompatible changes be?

@glaebhoerl
Copy link
Contributor

@aturon

There's a partially written RFC here. The part specifically about closed traits is nearer the end (but builds on ideas from the earlier parts). See also the private supertraits section here, which was the genesis of this here RFC, and this earlier rejected RFC which is essentially the same idea as in the first link. The idea of closed traits just falls out as the intersection of these two ideas.

The basic idea is that structs, enums, as well as traits would each come in two flavors: "data structs" and "data enums" vs. "abstract structs" and "abstract enums", resp. "open traits" vs. "closed traits". Open traits are analogous to a data struct of fns, while closed traits to an abstract struct of same, and impls to instances of the structs.

In each case the difference is that with the abstract/closed flavors, the type author is "hiding something" - it is an abstract type, while in the data/open case, nothing is hidden - it is "just data". In one case, fields (methods) are private by default, in the other, everything is public. Only the owner of the type may construct (via a literal) and pattern match on an abstract type, anyone may do so for data types. Built-in traits are derived automatically for data types, and must be implemented explicitly for abstract types. With an abstract type, it should not be possible for clients to observe anything that is not intentionally exposed by the author. And so on. With the analogy of traits to structs, it falls out naturally that only the owner may "construct instances" i.e. impls of the abstract/closed flavor, and you get a closed trait.

See the linked RFCs for a more thorough explanation.

TL;DR Existing traits would stay the same (no added pubs), and we would also grow closed traits (or whatever we call them) whose associated items are private by default and only the owning module may impl them. (I guess this much is actually backwards compatible, but the corresponding changes to structs and enums - and it would be rather incoherent to do it only for traits - would not be.)

@aturon
Copy link
Member

aturon commented Mar 5, 2015

ping @pnkfelix, this seems to've fallen through the cracks?

@nikomatsakis
Copy link
Contributor

I think we should close this. There doesn't seem to be momentum and it's something we can address later without breaking backwards compatibility if we so choose.

@Kimundi
Copy link
Member

Kimundi commented Mar 5, 2015

How could this be addressed later? By re-adding the priv keyword?

@glaebhoerl
Copy link
Contributor

Or we could add sealed trait or whatever other modifier on trait. (Not sure if there's an existing keyword that would be appropriate?)

(cc also @freebroccolo)

@nikomatsakis
Copy link
Contributor

That is one possibility, yes. Or perhaps a modifier on the trait that makes the items private by default. 

-------- Original message --------
From: Marvin Löbel notifications@github.com
Date:03/05/2015 09:43 (GMT-05:00)
To: rust-lang/rfcs rfcs@noreply.github.com
Cc: Niko Matsakis niko@alum.mit.edu
Subject: Re: [rfcs] Require public trait items to be marked pub (#227)

How could this be addressed later? By re-adding the priv keyword?


Reply to this email directly or view it on GitHub.

@pnkfelix
Copy link
Member

pnkfelix commented Mar 5, 2015

I agree we should close this.

@aturon
Copy link
Member

aturon commented Mar 5, 2015

OK, it seems pretty clear that nothing on this front is going to happen before 1.0, and we'll just have to use one of the suggested "workarounds" if we want to add private trait items later on.

In the interest of cleaning up the RFC PRs, I'm going to go ahead and close this.

Thanks for the PR!

@aturon aturon closed this Mar 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants