From e6c84e14b578f0f3c283b5fde0d0b41ee21c82a9 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Sun, 24 Jun 2018 09:34:37 +0200 Subject: [PATCH 01/21] rfc, hidden-impls: initial draft. --- text/0000-hidden-impls.md | 669 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 669 insertions(+) create mode 100644 text/0000-hidden-impls.md diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md new file mode 100644 index 00000000000..62d42ac2d70 --- /dev/null +++ b/text/0000-hidden-impls.md @@ -0,0 +1,669 @@ +# Draft RFC: Hidden implementations + +- Feature Name: `hidden_impl` +- Start Date: 2018-06-24 +- RFC PR: +- Rust Issue: + +# Summary +[summary]: #summary + +Allow a visibility modifier on a `trait` implementation such that: + ++ it is public wrt. coherence and overlap, ++ it is not usable in contexts it which it is *hidden* in. + +An example: + +```rust +#[derive(Clone)] +struct Foo(..); + +crate impl Copy for Foo {} // Can only be used in the current crate! +``` + +# Motivation +[motivation]: #motivation + +First, let's assume that we have a trait `Property` defined somewhere. +This trait, which doesn't have to be inside your crate, is also visible +outside of it. It also happens that your crate has defined and exported +a type called `Thing`. The specifics of `Property` and `Thing` are not +interesting for our purposes, but they exist. + +You now want to, define an implementation of `Property` for `Thing`. +But for some reason, you are not willing, or able, to expose this implementation +outside your crate and make it part of your semantic versioning guarantees. + +An example of such a scenario is defining a new-type wrapper `TestRng` around +`XorShiftRng` of the `rand` crate. We want to define an implementation of +`SeedableRng` for `TestRng` to take advantage of the logic of `FromEntropy` +which is automatically provided for us when `Rng: SeedableRng`. +But we also want to avoid exposing `SeedableRng` as a guarantee to the +users of our library (`proptest`) right now. The allure of getting the logic +provided when defining `SeedableRng` could also have caused us to prematurely +and mistakenly provide a guarantee we were not ready to provide. + +However, we have no way eat our cake and keep it. We can't segregate who +has access to what implementations by visibility. + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +## The basic idea + +The problem set out by the [motivation] is solved by allowing you to +hide away implementations which you, for whatever reason, are not willing or +able to expose to users of your library. This RFC solves that with a simple +solution: *visibility modifiers on implementations.* + +Instead of writing: + +```rust +// crate A: + +impl Property for Thing { .. } +``` + +we can now write: + +```rust +// crate A: + +crate impl Property for Thing { .. } +``` + +where `crate`, or previously `pub(crate)`, is the familiar visibility +modifier for the current crate. + +## The effect of hiding + +### Error E0277 + +The effect `crate` has on `Property for Thing` is that when you `use Property;` +outside of the current crate, and you are working with a `Thing`, +then the compiler will pretend that `Property for Thing` does not exist. + +Assuming we have a function that creates a `Thing`: + +```rust +// some crate: + +fn make_a_thing() -> Thing { .. } +``` + +and functions that accept things that implement `Property`: + +```rust +// some crate: + +fn use_property_1(..) { .. } +fn use_property_2(arg: impl Property) { .. } +``` + +Concretely, this means that all of these usages will be rejected by the compiler: + +```rust +// crate B: + +fn main() { + use_property_1::(..); // ← error[E0277] + use_property_2(make_a_thing()); // ← error[E0277] + + let x: ::Projection; // ← error[E0277] + let y = ::MY_CONST; // ← error[E0277] + let z = ::function(); // ← error[E0277] + ... +} + +fn return_property() -> impl Property { + make_a_thing() // ← error[E0277] +} +``` + +[`E0277`]: https://doc.rust-lang.org/stable/error-index.html#E0277 + +in all of these cases, the error [`E0277`] will be emitted. A possible variant +on `E0277` that highlights the particulars of the problem could be: + +```rust +error[E0277]: the trait bound `Thing: Property` is not satisfied + --> src/main.rs:: + | +L | use_property_1::(); + | ^^^^^^^^^^^^^^^^^^^^^^^ the trait `Property` is implemented for `Thing`, + | but the implementation is hidden. + | +note: required by `use_property_1` +``` + +### Method calls and E0599 + +Let's assume that `Property` is in scope, and that we've written: + +```rust +// crate B: + +fn main() { + let thing = make_a_thing(); + thing.method(); +} +``` + +Let's also assume that there's no other `method` defined in some inherent +or trait method for `Thing`. In that case, error `E0599` will be emitted, +with a twist: + +```rust +error[E0599]: no method named `method` found for type `Thing` in the current scope + --> src/main.rs:15:7 + | +L | struct Thing; + | ------------- method `method` not found for this +... +L | thing.method(); + | ^^^^^^ + | + = help: items from traits can only be used if the trait is implemented and in scope + = note: an implementation of `Property`, which defines an item `method`, + for `Thing` exists, but is hidden +``` + +If instead there does exist some trait implemented for `Thing` which does +provide the item `method`, then the `hidden_function` lint, adapted with +language appropriate for methods, will be triggered. + +## Lint: `hidden_fn` ⇒ Use UFCS + +Consider for a moment that we've written: + +```rust +// crate B: + +trait Foo { + fn function(); +} + +fn main() { + use crate_a::Property; // ← assume that `Property::function` exists. + + let x = Thing::function(); +} +``` + +Here we want to avoid the future possible breakage of `Property for Thing` +eventually becoming public. Since the compiler is aware of `Property for Thing` +and that we've brought `Property` into scope, a warning will be emitted +in addition to the `unused import` lint, suggesting that you should use +*uniform function call syntax (UFCS)* instead: + +```rust +warning: the trait bound `Thing: Property` is not satisfied + --> src/main.rs:: + | +L | let x = Thing::function(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ a hidden implementation of trait `Property`, + | which has a function `function`, exists for `Thing`. + | + | help: rewrite using UFCS: + | + | let x = ::function(); + | + = note: #[warn(hidden_fn)] on by default +``` + +## Hidden, but not parallel + +It is crucial to note that `Property for Thing` acts, with respect to coherence, +as if it were public. That means that crate B, which is a reverse dependency of +A, may not write: + +```rust +// crate B: + +impl Property for Thing { .. } +``` + +This implementation is rejected, by the compiler, as overlapping with the one +specified in crate A. The following is one *possible* error message, +which is a variant of `E0119`, tailored for this specific case: + +```rust +error[E0119]: conflicting implementations of trait `path::to::Property` for type `Thing`: + --> src/main.rs:: + | +L | impl Property for Thing { + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: conflicting implementation in crate `A`: + - crate impl Property for Thing + note: the implementation exists, but is hidden. +``` + +[unsound-named]: https://internals.rust-lang.org/t/looking-for-rfc-coauthors-on-named-impls/6275/2 + +If the compiler did not reject this implementation, it would amount to the same +problem as with named implementations, which would be decidedly unsound in Rust +since [*"coherence is an invariant that unsafe code is allowed to assume"*][unsound-named]. + +## Hiding things from yourself + +Thus far, we've only used `crate impl ..` to hide implementations. +While we think that will be the most likely usage, we also believe +that more fine grained control is beneficial for larger projects +with many people involved in the development of those projects. +Therefore, this RFC proposes that visibility modifiers (such as +`pub(super)`) be permitted on `impl` to hide implementations from +different parts of the crate. + +## `pub` by default + +Given that you are now allowed to put any visibility modifier on an `impl`, +you might wonder if not specifying one is the same as private visibility +(module visibility). This RFC specifies that it is not. Instead, default +visibility is contextual. This means that `impl Foo for Bar { .. }` is +equivalent to `pub impl Foo for Bar { .. }`. This is right for two reasons: + ++ Changing the default visibility to module private would constitute a major + breaking change which would be too large to countenance even with an edition. + ++ `pub` as a default for implementations is a *good* default, since it is what + you want most of the time. + +Having `pub` be permitted on `impl` also means that you can use the `vis` +macro fragment specifier. + +To promote uniformity in the ecosystem, directly writing `pub impl ..` will +be linted against with a lint named `redundant_vis` (or a naming in that spirit). +A sample warning is: + +```rust +warning: `pub` is redundant on `impl` + --> src/main.rs:: + | +L | pub impl Foo for Bar { .. } + | ^^^ help: remove `pub` + | + = note: #[warn(redundant_vis)] on by default +``` + +## `priv` visibility + +To be able to make implementations module private, this RFC introduces a new +visibility modifier called `priv` (which is currently a reserved keyword). +This new visibility modifier is made the default for all contexts where +visibility modifiers are permitted *but* in the context of implementations +(where `pub` is the default). Directly writing: + +```rust +struct Foo { + priv x: Bar, +} +``` + +will be linted against with `redundant_vis` just like `pub impl ..` +will be linted against. + +## `#[derive(crate Trait)]` + +To make it simpler for you to provide implementation of common traits +which can be derived, you are permitted to place a visibility modifier, +such as `crate`, before `Trait` in the `#[derive(..)]` attribute on a +type definition. An example: + +```rust +#[derive(Default, crate Copy, crate Clone)] +struct Foo { + ... +} +``` + +In the case of derivable standard library traits, the compiler will insert the +visibility modifier before `impl` in the implementations generated. +Custom derive macros are not obligated to respect this modifier. +However, it is nonetheless recommended that such macros, +which are available for public use, handle these modifiers, +or emit an error notifying the user that modifiers are not supported. + +## In relation to trait objects + +One place where crate B is allowed to use a hidden implementation of crate A +is when it comes to trait objects. If A provides access to a trait object +`Box` (which implies that `Property` is object safe) by returning +a value of that type, the compiler has no means in the general case to perform +static analysis on that object to see where the implementation came from. +This is fine. In this case, it is crate A that expressly gave access to an +implementation of `Property` for `Thing`. Crate A is also under no obligation +under the semver rules to provide an erased `Thing` value. As long as the value +returned behaves observably in an equivalent manner, for some semantic +definition of "equivalent", no breakage has occurred. + +## Recommendation: When you should use hidden implementations + +Simply put, it is recommended that your implementation be hidden if you are +not yet ready to guarantee that the implementation exists to users of your +API, be they internal APIs within a project, or the public APIs of a crate. +In most cases, you implementations will not need to be hidden. + +## Teaching beginners about hidden implementations + +The contents of this RFC are mostly considered to be *advanced* features of Rust. +However, it should mostly be sufficient to mention to beginners in sections about +trait implementations that they can hidden, with one or two simple examples. +The still more advanced details in this RFC can be deferred to the reference. + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +## Grammar and parsing + +### Visibility modifier `priv` + ++ The `PRIV` token will lex the terminal `"priv"`. + ++ To the production `visibility` we add: + + ``` + visibility : PRIV | .. ; + ``` + + where `..` is the old definition of `visibility`. + ++ The macro fragment specifier `vis` will accept `PRIV`. + ++ `priv` is considered the default visibility for all contexts but `impl`. + +### Visibility modifiers on `impl` + ++ The grammar of `item_impl` is amended such that `visibility` is optionally +permitted on *trait implementations*. `visibility` is not allowed before +inherent implementations. + ++ `pub` is considered the default visibility for `impl`. + +#### `unsafe` and visibility + ++ When specifying `unsafe` on an `impl`, if a visibility modifier is specified, + it must come first. That is, you must write: + + ```rust + crate unsafe impl TrustedLen for Bar { .. } + ``` + + but never: + + ```rust + unsafe crate impl TrustedLen for Bar { .. } + ``` + +#### Linting on default visibility + ++ If the *default visibility* for some *context* is specified in that context + *directly* in a source file, then a warn-by-default lint `redundant_vis` + is raised. + +## `#[derive(..)]`ing + ++ The grammar for the `derive` attribute will permit `visibility` before +each trait. + ++ When the compiler is generating code for each derivable standard library trait, +if a visibility modifier is specified before the trait in `#[derive(..)]`, +then it will also be specified on the generated `impl`. + +### On `#[structural_match]` + ++ The `#[structural_match]` attribute will be changed to optionally permit +a visibility modifier with the syntax `#[structural_match($vis)]`. + ++ Not specifying one is desugared to: `#[structural_match(pub)]`. + ++ When generating code for `#[derive($vis_a PartialEq, $vis_b Eq)]` on a type, +a `#[structural_match($vis)]` will be generated where `$vis` is the least +visible of `$vis_{a, b}`. + ++ When pattern matching, if `$vis` in `#[structural_match($vis)]` is visible + in the given context, then the type on which `#[structural_match($vis)]` is + specified on is considered structurally matchable. Otherwise, it is not. + +## Type checking + +1. The visibility modifier `priv` entails that an item is only visible in the + module in which the item is defined in. + +2. The default visibility modifier for a context is first desugared to a form + where it is explicitly specified. That is, `impl<..> Trait<..> for Type<..>` + is desugared to `pub impl<..> Trait<..> for Type<..>` and + `struct X { field: Y }` is desugared to `struct X { priv field: Y }`. + +3. When type checking an implementation with respect to checking its coherence, + all implementations are considered irrespective of their specified visibility. + When overlap is detected, the compiler should specify whether the + other implementation, which caused the overlap, was hidden or not. + +4. When otherwise type checking if an implementation exists, the visibility + modifier - which exists in the crate metadata, specified on the `impl` will + be taken into account. If the specified visibility is not considered to be + visible in the context where the existence of an implementation on the type + of the trait is checked, the implementation will be considered to not exist. + Such an implementation that doesn't exist solely due to visibility is + classified as *hidden*. + +5. If in 4. the implementation did not exist due to being hidden, + error messages will take that into account when informing users + that no implementation exists. + +6. Move checking will consider the visibility of `Copy` for a type when checking + if `Copy` is implemented for that type or not. + +7. If: + + A trait `$A`, with some `fn` item named `$candidate`, is in scope in a + scope `S`. + + A trait `$B`, with some `fn` item named `$candidate`, is in scope in + scope `S`. + + There exists a visible implementation of `$A` for a type `$T`. + + There exists a hidden implementation of `$B` for type `$T`. + + In the scope `S` fn `$candidate` is called either as a free function + (without specifying the trait with UFCS) or method call syntax. + + Then: + + A warn-by-default lint `hidden_fn` will be emitted at the call site + suggesting that the user use UFCS `<$T as $A>::$candidate(args..)` + instead. + +## Code generation + ++ Code generation will ignore visibility specified on any implementations. + ++ However, the visibility of implementations will be included in the crate + metadata. + +## Documentation + ++ Only `pub` (whether explicitly specified or inferred) implementations are + visible in the documentation generated by `rustdoc`. + +# Drawbacks +[drawbacks]: #drawbacks + +While this RFC argues that the changes proposed in this RFC are intuitive, +and uniformly specified, and will therefore not have a severe impact on +learning and the complexity budget, it should be recognized that the additions +to the language in the RFC are non-trivial. These should be judged in comparison +to the benefits provided by the changes. + +# Rationale and alternatives +[alternatives]: #rationale-and-alternatives + +In this section, we discuss some of the design choices made in this RFC +and what some alternatives to those are. + +## Lexical order: `vis? unsafe? impl` or `unsafe? vis? impl` + +Let's first deal with a softball. We have to decide whether one should write +`unsafe crate impl`, or `crate unsafe impl`, or whether both should be legal. +Since it is only legal to write: + +```rust +pub unsafe trait Foo { .. } +``` + +but illegal to write: + +```rust +unsafe pub trait Foo { .. } +``` + +the only consistent alternative is to only permit the user to +write `crate unsafe impl`. + +## Should you be able to implement a trait parallel to a hidden `impl`? + +Irrespective of one's view of whether this would be beneficial, +it is unworkable to permit this. As previously discussed, doing so would +[violate][unsound-named] the invariant that `unsafe` code may assume coherence. + +## Should `crate` be the only visibility modifier on `impl`? + +This RFC argues no. While it does make the extent of changes smaller and while +`crate impl` is the most likely scenario, there can be cases of large projects +where more fine tuned privacy is beneficial, as discussed in the [motivation]. +Furthermore, we argue that introducing an artificial limit on the usefulness of +this feature does not aid learning of Rust. In fact, we consider a less of +uniformity to have a negative impact on learning the language. + +## Should a `priv` visibility modifier be introduced? + +We argue yes. If there is no way to specify private visibility textually, +then the least private you can make an implementation is `pub(super)`. +For larger modules, which do occur, module level privacy may also be useful +for the same reasons as other visibility modifiers than `crate` can be useful. +Other than that, being able to specify visibility for the `vis` macro fragment +in a way that is uniform irrespective on what it is applied to is an added bonus. + +However, there are drawbacks to this approach. For one, unless we make it more +interesting for other places, such as specifying `priv` on the items of a trait +`impl`, `priv` will only be useful for tweaking the visibility of `impl`. +Furthermore, if someone writes something like: + +```rust +struct Foo { + baz: Wibble + priv bar: Wobble, +} +``` + +they may mistakenly infer, if they are beginners, that `bar` is module private +but `baz` is not (so it is public). To make this mistaken inference extremely +unlikely, we lint in favour of removing `priv` and `pub` whenever they are the +default for that context. This also provides a uniform style. + +### Which syntax? + +The reason for using the keyword `priv` specifically is that it is a reserved +keyword, and that it is fitting for the proposed usage. We also argue that +`pub` and `priv` match well together. However, there are other alternatives to +consider: + ++ `pub(self)` + + The meaning is less self-documenting and we have recently decided to move + away from `pub(crate)` in favour of just `crate`. + ++ `mod` + + This one is interesting, but ultimately we consider `priv` to be more + intuitive. + +## On permitting visibility in `#[derive(..)]` + +Sometimes, you might want to make a type `Copy`, but only for internal usage. +Other times, you could want to compare types for equality, but not expose that +ability as public API. Rust makes it possible to derive implementations for +some of these standard library traits. If we want to extend that ability to +hidden implementations, which we argue is useful, then some way to specify +the visibility desired when deriving will be necessary. + +Furthermore, if `#[structural_match]` should be controllable wrt. visibility, +then there is no choice but to provide a way to control visibility in +`#[derive(..)]` since you need to `#[derive(PartialEq, Eq)]` for the compiler +to specify `#[structural_match]` for you. + +As syntax goes, `#[derive($vis Trait)]` is chosen as a simple but intuitive +syntax. One could consider applying visibility to a group of crates, +but it does not seem worth it to make this much terser. + +When it comes to custom deriving, it might be possible in the general case to +ensure or automate that the implementation provided is of the visibility +specified. However, in that case, it has to be added or checked on every +implementation one invocation of a custom derive macro emits. +Some custom derive macros don't emit trait implementations in the first place. +However, automating this may be a good mechanism to consider. + +# Prior art +[prior-art]: #prior-art + +[guarantees coherence]: http://blog.ezyang.com/2014/07/type-classes-confluence-coherence-global-uniqueness/ + +As Rust's trait system (and thus implementations on types thereof) has its +direct descendant in Haskell, we consider how that language deals with +the relation between modules and type class instances (same as `impl` in Rust). +While Haskell - without certain extensions enabled, like Rust, +[guarantees coherence] of type class instances, it does not guarantee +global uniqueness of instances. Haskell will allow you to define overlapping +instances but won't allow you to actually use them - producing an error once +you try to do that. + +[optional_doi]: https://link.springer.com/chapter/10.1007/978-3-319-45279-1_9 +[optional_pdf]: http://homepages.dcc.ufmg.br/~camarao/CT/optional-type-classes.pdf + +In a paper [Optional Type Classes for Haskell][optional_pdf] due to +Ribero et. al [(DOI: 10.1007/978-3-319-45279-1_9)][optional_doi] +modularization of instances is considered. This is however radically different +than what is proposed in this RFC, which does *not* propose modularization of +implementations. Fundamentally, that paper seems permit different instances +of the same type class to coexist as long as the instance to use for a given +expression is uniquely determinable. As previously discussed, such a mechanism +would not be sound in Rust. The main difference between these approaches is +that in our proposal, all implementations are considered when coherence checking +an implementation. The effect of that is to prevent different crates and modules +from defining implementations for a trait and a type when a hidden one exists. + +# Unresolved questions +[unresolved]: #unresolved-questions + +Exact wordings of error messages discussed in this RFC and their error codes, +as well as lint names and their phrasings are out of the scope of this RFC. + +The following questions are in the scope of the RFC and should be resolved +*before* the RFC is merged: + +1. Are there any strange interactions with possible plans for negative bounds? + +2. Should `crate` (and `pub(crate)`) be the only visibility modifier permitted + on `impl`? + +3. Should `priv` be introduced to hide implementations from other modules? + +4. Should you be able to specify a visibility modifier in `#[derive(..)]`? + + 1. Should `#[structural_match]` take visibility of `impl`s into account? + + 2. Should the visibility modifier be automatically added to all + trait implementations emitted by a custom derive macro? + +The following questions should be resolved at least before stabilization +(but possibly sooner): + +5. What is an appropriate default lint level for `hidden_fn`? + 1. Should `hidden_fn` be promoted to `deny`-by-default? + 2. should it be `allow`-by-default? + 3. Should it perhaps be a hard error instead? + +6. Should `pub impl ..` be linted against suggesting `impl ..` instead? + +7. Should `priv field: Type` and the `priv` visibility modifier in general + be linted against when it is the contextual default? + +If 2. is answered in the positive, questions 5-7 become moot. +Similarly, if 3. is answered in the negative, 7. is moot. From a581ee73292853bce953d8f6e82531407f792291 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Sun, 24 Jun 2018 21:01:02 +0200 Subject: [PATCH 02/21] rfc, hidden-impls: reword -> hidden, but coherent. --- text/0000-hidden-impls.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 62d42ac2d70..bd62180342c 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -212,7 +212,7 @@ L | let x = Thing::function(); = note: #[warn(hidden_fn)] on by default ``` -## Hidden, but not parallel +## Hidden, but coherent It is crucial to note that `Property for Thing` acts, with respect to coherence, as if it were public. That means that crate B, which is a reverse dependency of From 602248e93ea986c3ed34bf306ccb04822843dc0f Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Sat, 14 Jul 2018 04:05:32 +0200 Subject: [PATCH 03/21] rfc, hidden-impls: reword summary for clarity a bit. --- text/0000-hidden-impls.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index bd62180342c..9a7d49f5a1d 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -10,8 +10,8 @@ Allow a visibility modifier on a `trait` implementation such that: -+ it is public wrt. coherence and overlap, -+ it is not usable in contexts it which it is *hidden* in. ++ it is public wrt. coherence and overlap ++ it is not usable in contexts where it is *hidden* (not visible) An example: From d16d1ba98900114b219b178e4b82f839f825c580 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Sat, 14 Jul 2018 05:28:15 +0200 Subject: [PATCH 04/21] rfc, hidden-impls: unpropose priv and make it future work. --- text/0000-hidden-impls.md | 187 +++++++++++++++++++++----------------- 1 file changed, 105 insertions(+), 82 deletions(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 9a7d49f5a1d..9f1054f2171 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -73,7 +73,7 @@ we can now write: crate impl Property for Thing { .. } ``` -where `crate`, or previously `pub(crate)`, is the familiar visibility +where `crate`, or `pub(crate)`, is the familiar visibility modifier for the current crate. ## The effect of hiding @@ -252,7 +252,7 @@ Thus far, we've only used `crate impl ..` to hide implementations. While we think that will be the most likely usage, we also believe that more fine grained control is beneficial for larger projects with many people involved in the development of those projects. -Therefore, this RFC proposes that visibility modifiers (such as +Therefore, this RFC proposes that all visibility modifiers (such as `pub(super)`) be permitted on `impl` to hide implementations from different parts of the crate. @@ -287,22 +287,27 @@ L | pub impl Foo for Bar { .. } = note: #[warn(redundant_vis)] on by default ``` -## `priv` visibility +## Private visibility, `pub(self)` + +[RFC 1442]: https://github.com/nox/rust-rfcs/blob/master/text/1422-pub-restricted.md + +As we previously discussed, writing just `impl Foo for Bar { .. }` +does not entail that `Foo for Bar` is only visible in the current module. +So how do we achieve that effect? It just so happens that the visibility +`pub(self)`, which makes something (only) visible in the current module, +already exists in Rust (since 1.18, introduced by [RFC 1422]). -To be able to make implementations module private, this RFC introduces a new -visibility modifier called `priv` (which is currently a reserved keyword). -This new visibility modifier is made the default for all contexts where -visibility modifiers are permitted *but* in the context of implementations -(where `pub` is the default). Directly writing: +The visibility modifier `pub(self)` is the default for all contexts but `impl`. +As with writing `pub(self)`, if you directly write out the default visibility +such as in: ```rust struct Foo { - priv x: Bar, + pub(self) x: Bar, } ``` -will be linted against with `redundant_vis` just like `pub impl ..` -will be linted against. +then the lint `redundant_vis` will be raised. ## `#[derive(crate Trait)]` @@ -357,21 +362,9 @@ The still more advanced details in this RFC can be deferred to the reference. ## Grammar and parsing -### Visibility modifier `priv` +### Visibility modifier `pub(self)` -+ The `PRIV` token will lex the terminal `"priv"`. - -+ To the production `visibility` we add: - - ``` - visibility : PRIV | .. ; - ``` - - where `..` is the old definition of `visibility`. - -+ The macro fragment specifier `vis` will accept `PRIV`. - -+ `priv` is considered the default visibility for all contexts but `impl`. ++ `pub(self)` is considered the default visibility for all contexts but `impl`. ### Visibility modifiers on `impl` @@ -428,13 +421,13 @@ visible of `$vis_{a, b}`. ## Type checking -1. The visibility modifier `priv` entails that an item is only visible in the - module in which the item is defined in. +1. The visibility modifier `pub(self)` entails that an item is only visible in the + module in which the item is defined in. *(this is already the case in Rust)* 2. The default visibility modifier for a context is first desugared to a form where it is explicitly specified. That is, `impl<..> Trait<..> for Type<..>` is desugared to `pub impl<..> Trait<..> for Type<..>` and - `struct X { field: Y }` is desugared to `struct X { priv field: Y }`. + `struct X { field: Y }` is desugared to `struct X { pub(self) field: Y }`. 3. When type checking an implementation with respect to checking its coherence, all implementations are considered irrespective of their specified visibility. @@ -453,8 +446,8 @@ visible of `$vis_{a, b}`. error messages will take that into account when informing users that no implementation exists. -6. Move checking will consider the visibility of `Copy` for a type when checking - if `Copy` is implemented for that type or not. +6. Move checking will consider the visibility of `Copy` for a type when + checking if `Copy` is implemented for that type or not. 7. If: + A trait `$A`, with some `fn` item named `$candidate`, is in scope in a @@ -532,48 +525,9 @@ Furthermore, we argue that introducing an artificial limit on the usefulness of this feature does not aid learning of Rust. In fact, we consider a less of uniformity to have a negative impact on learning the language. -## Should a `priv` visibility modifier be introduced? - -We argue yes. If there is no way to specify private visibility textually, -then the least private you can make an implementation is `pub(super)`. -For larger modules, which do occur, module level privacy may also be useful -for the same reasons as other visibility modifiers than `crate` can be useful. -Other than that, being able to specify visibility for the `vis` macro fragment -in a way that is uniform irrespective on what it is applied to is an added bonus. - -However, there are drawbacks to this approach. For one, unless we make it more -interesting for other places, such as specifying `priv` on the items of a trait -`impl`, `priv` will only be useful for tweaking the visibility of `impl`. -Furthermore, if someone writes something like: - -```rust -struct Foo { - baz: Wibble - priv bar: Wobble, -} -``` - -they may mistakenly infer, if they are beginners, that `bar` is module private -but `baz` is not (so it is public). To make this mistaken inference extremely -unlikely, we lint in favour of removing `priv` and `pub` whenever they are the -default for that context. This also provides a uniform style. - -### Which syntax? - -The reason for using the keyword `priv` specifically is that it is a reserved -keyword, and that it is fitting for the proposed usage. We also argue that -`pub` and `priv` match well together. However, there are other alternatives to -consider: - -+ `pub(self)` - - The meaning is less self-documenting and we have recently decided to move - away from `pub(crate)` in favour of just `crate`. - -+ `mod` - - This one is interesting, but ultimately we consider `priv` to be more - intuitive. +Other than that, being able use the `vis` macro fragment on `impl` is a boon +for macro authors who can now provide a macro invoked as `mac!(pub(super))` +which will then use `pub(super)` on everything you can specify visibility on. ## On permitting visibility in `#[derive(..)]` @@ -629,6 +583,80 @@ that in our proposal, all implementations are considered when coherence checking an implementation. The effect of that is to prevent different crates and modules from defining implementations for a trait and a type when a hidden one exists. +# Future work + +This section outlines some *possible* future work that we might want to consider +but which this RFC does not propose. + +## `priv` visibility + +As we've previously noted, the `pub(self)` visibility already exists. +That visibility modifier suffices to make trait implementations private. +However; `pub(self)` is perhaps not the most ergonomic syntax if we wish +to make it *easy* to make an implementation private. + +To do that, we *could* introduce the visibility modifier `priv`. +As it happens, `priv` is a reserved keyword, but it is unused. + +As we've previously specified, `pub(self)` is the default visibility +for all contexts except `impl`. If we were to introduce `priv`, +it would merely be a syntactic alias for `pub(self)` and would act, +in all respects, as `pub(self)` does. + +### Is `priv` better than `pub(self)`? + +As always: It depends! + ++ On the one hand, `pub(self)` is more consistent with the `pub(path)` syntax. + Since `pub(self)` already exists, it could also be argued that introducing + another way to do the same thing is a bad choice due to the common saying that + *"there should only be one way to do it"*. + ++ On the other hand, we have diverged slightly from both the consistency angle + and the above cited "principle" (since `pub(crate)` already exists) by + introducing `crate` as a visibility modifier to make things more easy and + ergonomic. It is therefore clear that Rust often does not adhere to that + principle. If we want to *encourage* users *more* to not overpromise on facts + about their type, then introducing `priv` helps. We also note that larger + modules *frequently* do occur, and as such, `priv impl Foo for Bar` may + not be too rare. + +* We also argue that `priv` is fitting for the proposed usage since it matches + well with `pub`. In our view, the keyword `priv` is also more self documenting + and intuitive than `pub(self)` is. + +### Other possible keywords + ++ `mod` + + This one is interesting, ergonomic, and short, + but ultimately we consider `priv` to be more intuitive. + +### A specification for `priv` + +Here we lay out a feel technical details for `priv` for future reference. + +#### Grammar and parsing + ++ The `PRIV` token will lex the terminal `"priv"`. + ++ To the production `visibility` we add: + + ``` + visibility : PRIV | .. ; + ``` + + where `..` is the old definition of `visibility`. + ++ The macro fragment specifier `vis` will accept `PRIV`. + ++ `priv` is considered the default visibility for all contexts but `impl`. + +#### Semantics + +1. The visibility modifier `priv` will be treated as `pub(self)` in all respects + except for error reporting, where `priv` is used instead. + # Unresolved questions [unresolved]: #unresolved-questions @@ -643,9 +671,7 @@ The following questions are in the scope of the RFC and should be resolved 2. Should `crate` (and `pub(crate)`) be the only visibility modifier permitted on `impl`? -3. Should `priv` be introduced to hide implementations from other modules? - -4. Should you be able to specify a visibility modifier in `#[derive(..)]`? +3. Should you be able to specify a visibility modifier in `#[derive(..)]`? 1. Should `#[structural_match]` take visibility of `impl`s into account? @@ -655,15 +681,12 @@ The following questions are in the scope of the RFC and should be resolved The following questions should be resolved at least before stabilization (but possibly sooner): -5. What is an appropriate default lint level for `hidden_fn`? +4. What is an appropriate default lint level for `hidden_fn`? 1. Should `hidden_fn` be promoted to `deny`-by-default? 2. should it be `allow`-by-default? 3. Should it perhaps be a hard error instead? -6. Should `pub impl ..` be linted against suggesting `impl ..` instead? - -7. Should `priv field: Type` and the `priv` visibility modifier in general - be linted against when it is the contextual default? +5. Should `pub impl ..` be linted against, suggesting `impl ..` instead? -If 2. is answered in the positive, questions 5-7 become moot. -Similarly, if 3. is answered in the negative, 7. is moot. +6. Should `pub(self) field: Type` and the `pub(self)` visibility modifier in + general be linted against when it is the contextual default? From d2f5db6a59aaabdcca311547447901b06a941ffe Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Sat, 14 Jul 2018 05:53:51 +0200 Subject: [PATCH 05/21] rfc, hidden-impls: correct mistakes in 'Hidden, but coherent'. --- text/0000-hidden-impls.md | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 9f1054f2171..5fd04a8d07a 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -215,13 +215,25 @@ L | let x = Thing::function(); ## Hidden, but coherent It is crucial to note that `Property for Thing` acts, with respect to coherence, -as if it were public. That means that crate B, which is a reverse dependency of -A, may not write: +as if it were public. Consider that a public trait `Property` is defined in +crate A and that a blanket implementation exists in crate A: + +```rust +// crate A: + +pub trait Property { ... } + +crate impl Property for T { ... } // `From` provides a blanket impl like this. +``` + +That means that crate B, which is a reverse dependency of crate A, may not write: ```rust // crate B: -impl Property for Thing { .. } +struct Thing { ... } // or `union` / `enum` + +impl Property for Thing { ... } ``` This implementation is rejected, by the compiler, as overlapping with the one @@ -229,14 +241,14 @@ specified in crate A. The following is one *possible* error message, which is a variant of `E0119`, tailored for this specific case: ```rust -error[E0119]: conflicting implementations of trait `path::to::Property` for type `Thing`: +error[E0119]: conflicting implementations of trait `crate_A::Property` for type `Thing`: --> src/main.rs:: | -L | impl Property for Thing { - | ^^^^^^^^^^^^^^^^^^^^^^^ +L | impl Property for Thing { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: conflicting implementation in crate `A`: - - crate impl Property for Thing + - crate impl crate_A::Property for T { ... } note: the implementation exists, but is hidden. ``` From 9b9944f9cf67a0c2c3b891362cf03df5d4e0b19c Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Sat, 14 Jul 2018 07:01:26 +0200 Subject: [PATCH 06/21] rfc, hidden-impls: reword section on trait objects. --- text/0000-hidden-impls.md | 62 +++++++++++++++++++++++++++++++++------ 1 file changed, 53 insertions(+), 9 deletions(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 5fd04a8d07a..b34787829dd 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -345,15 +345,59 @@ or emit an error notifying the user that modifiers are not supported. ## In relation to trait objects One place where crate B is allowed to use a hidden implementation of crate A -is when it comes to trait objects. If A provides access to a trait object -`Box` (which implies that `Property` is object safe) by returning -a value of that type, the compiler has no means in the general case to perform -static analysis on that object to see where the implementation came from. -This is fine. In this case, it is crate A that expressly gave access to an -implementation of `Property` for `Thing`. Crate A is also under no obligation -under the semver rules to provide an erased `Thing` value. As long as the value -returned behaves observably in an equivalent manner, for some semantic -definition of "equivalent", no breakage has occurred. +is when it comes to trait objects. This is fine for a few reasons: + ++ It is useful to pass around trait objects of `Property` inside crate A + internally even such objects arise from hidden implementations + so we don't want to ban making trait object of hidden implementations + outright. + ++ It is also useful to pass trait objects from crate A to crate B + because crate A is under no obligation, under semver rules, + to pass an object of type `Thing` in a new minor / patch version. + As long as operations afforded by `Property` for the trait object, + that crate B gets, behaves observably in an equivalent manner, + for some semantic definition of "equivalent", no breakage has occurred. + ++ If we consider passing hidden implementations to crate B to be "bad" + and passing public implementations to crate B to be "good", + then it is impossible to reject all bad programs (*soundness*) + but allow all good programs (*completeness*). + To see why, let's first assume that `Property` is object safe. + Let's also assume that the following definitions exist in crate A: + + ```rust + struct Thing; + struct OtherThing; + + crate impl Property for Thing { ... } + pub impl Property for OtherThing { ... } + + fn halting_problem() -> bool { ... } + + pub fn make_with_property() -> Box { + if halting_problem() { + Box::new(Thing) + } else { + Box::new(OtherThing) + } + } + ``` + + [undecidable]: https://en.wikipedia.org/wiki/Undecidable_problem + + Since we know that [the halting problem is undecidable][undecidable], + and whether we return `Box::new(Thing)` or `Box::new(OtherThing)` is + dependent on the halting problem, then it is also undecidable whether + a hidden implementation is leaked or not via a trait object. + Thus, static analysis can't be used unless we accept that some good programs + be rejected by the compiler. + + If we want to devise such an analysis, we could do so by starting at + the root of each `pub` function and transitively check whether it, + or any of its dependencies, will possibly construct a pointer to the + vtable of any hidden implementations. This could be done as a lint + of some form, but is currently left as possible future work. ## Recommendation: When you should use hidden implementations From 45a660581d10f7c91af5ec6ae3a7216436599318 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Sat, 14 Jul 2018 07:39:46 +0200 Subject: [PATCH 07/21] rfc, hidden-impls: reject negative impls less visible than the trait. --- text/0000-hidden-impls.md | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index b34787829dd..0b4be7f1978 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -438,9 +438,9 @@ inherent implementations. ```rust crate unsafe impl TrustedLen for Bar { .. } ``` - + but never: - + ```rust unsafe crate impl TrustedLen for Bar { .. } ``` @@ -520,6 +520,23 @@ visible of `$vis_{a, b}`. suggesting that the user use UFCS `<$T as $A>::$candidate(args..)` instead. +8. When type checking a *negative implementation* of an `auto`-trait, i.e: + + ```rust + pub(self) impl !Send for MyUnsafeType {} + ``` + + if the visibility of the negative implementation is less than that of the + `auto`-trait, then the implementation is rejected. + Conversely, if the visibility of the impl is ≥ that of the trait, + then that will not cause the implementation to be rejected. + + The visibility specified on a negative implementation has no impact on + type checking if an implementation exists (4.). This is sound because + we've ruled out a situation where a negative implementation is less + visible than the trait it negatively implements. This is an intentionally + conservative restriction which may or may not be lifted in the future. + ## Code generation + Code generation will ignore visibility specified on any implementations. From 44d97d56b709666d6b87bc6625252dd74569f602 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Sat, 14 Jul 2018 09:14:41 +0200 Subject: [PATCH 08/21] rfc, hidden-impls: discuss newtypes, improve deriving, super traits, teaching, motivation. --- text/0000-hidden-impls.md | 129 +++++++++++++++++++++++++++++++++++--- 1 file changed, 119 insertions(+), 10 deletions(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 0b4be7f1978..614b2fa5c81 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -44,6 +44,10 @@ users of our library (`proptest`) right now. The allure of getting the logic provided when defining `SeedableRng` could also have caused us to prematurely and mistakenly provide a guarantee we were not ready to provide. +Another example use-case which [@nikomatsakis](https://github.com/nikomatsakis) +has provided is wanting to have the convenience of `#[derive(Default)]`, +but not wanting to commit to there being a default value. + However, we have no way eat our cake and keep it. We can't segregate who has access to what implementations by visibility. @@ -337,10 +341,11 @@ struct Foo { In the case of derivable standard library traits, the compiler will insert the visibility modifier before `impl` in the implementations generated. -Custom derive macros are not obligated to respect this modifier. -However, it is nonetheless recommended that such macros, -which are available for public use, handle these modifiers, -or emit an error notifying the user that modifiers are not supported. +This *also* applies to custom derive macros. After a custom derive macro +has generated the `TokenStream` for the `#[derive(crate CustomTrait)]` invocation, +then the compiler will ensure that nothing generated in the `TokenStream` +is more visible than `crate`. It will do so by clamping a `pub` visibility +to `crate`. This applies generally for any visibilities. ## In relation to trait objects @@ -413,6 +418,13 @@ However, it should mostly be sufficient to mention to beginners in sections abou trait implementations that they can hidden, with one or two simple examples. The still more advanced details in this RFC can be deferred to the reference. +One particular aspect of this RFC is important to teach: The coherent nature +of hidden implementations. This should be stressed, and in particular, +it should be noted that these do *not* act as the modular implementations +discussed in the [prior art][prior-art]. The naming of these implementations +as *hidden*, as opposed to *private* is intentional. The word *private* +should therefore be *avoided* in learning material. + # Reference-level explanation [reference-level-explanation]: #reference-level-explanation @@ -460,6 +472,17 @@ each trait. if a visibility modifier is specified before the trait in `#[derive(..)]`, then it will also be specified on the generated `impl`. ++ The generated code by a custom derive macro `#[derive(vis? Trait)]`, + including any other macro invocations in that macro as well as + the transitive closure of macro invocations in those is here referred + to as "the expansion". + + When the compiler executes a custom derive macro `#[derive(vis? Trait)]`, + if then a visibility modifier was specified on `vis? Trait`, + then the visibility of everything (which can have a visibility modifier) + in "the expansion" will be clamped to `vis` such that there is nothing + which is more visible than `vis` in "the expansion". + ### On `#[structural_match]` + The `#[structural_match]` attribute will be changed to optionally permit @@ -498,6 +521,29 @@ visible of `$vis_{a, b}`. Such an implementation that doesn't exist solely due to visibility is classified as *hidden*. + Note that when type checking the super trait obligations placed on + an implementation of a trait such as `Copy`, the visibility modifier + on the `impl` will be considered the context where the existence of + the super trait implementation is checked. Thus, if a user writes: + + ```rust + #[derive(crate Clone, pub Copy)] + struct Foo; + ``` + + or equivalently: + + ```rust + struct Foo; + + crate impl Clone for Foo { .. } + + pub impl Copy for Foo {} + ``` + + then the implementation of `Copy` is rejected, because it is not considered + as visible as `pub`. + 5. If in 4. the implementation did not exist due to being hidden, error messages will take that into account when informing users that no implementation exists. @@ -558,6 +604,14 @@ learning and the complexity budget, it should be recognized that the additions to the language in the RFC are non-trivial. These should be judged in comparison to the benefits provided by the changes. +There is also the *possibility* that visibility modifiers on implementations +will be mistaken for modular instances as laid out in the [prior art][prior-art]. +However, such misconceptions should be eventually cleared up by the error +messages which are generated when incoherent setups using hidden implementations +are attempted. For those who understand the coherent nature of traits and +their implementations in Rust, there will be no change, thus, there is no +misconception to clear up. + # Rationale and alternatives [alternatives]: #rationale-and-alternatives @@ -620,12 +674,67 @@ As syntax goes, `#[derive($vis Trait)]` is chosen as a simple but intuitive syntax. One could consider applying visibility to a group of crates, but it does not seem worth it to make this much terser. -When it comes to custom deriving, it might be possible in the general case to -ensure or automate that the implementation provided is of the visibility -specified. However, in that case, it has to be added or checked on every -implementation one invocation of a custom derive macro emits. -Some custom derive macros don't emit trait implementations in the first place. -However, automating this may be a good mechanism to consider. +When it comes to custom deriving, this RFC specifies that nothing generated +by the custom derive macro (including by invocations of macros inside +that macro, and transitively) will be more visible than the `vis` specified +in `#[derive($vis Trait)]`. We argue that this is a good thing to do because +it removes a large implementation burden on custom derive authors as well as +allowing users a uniform experience in the ecosystem. By clamping the visibility, +we may rule out some unhygenic things that users may want to do, but the ability +to write the macro generated code always remains. We could potentially have +some mechanism to opt-out of the hygiene at the macro definition site, +but this should await users actually needing that feature and be left as +future work. + +## Alternative: Newtypes + +The principal alternative to this RFC is to continue using the +manual workaround mechanism of newtypes around our types and providing +implementations only for the former. An example of this is: + +```rust +pub struct MyOriginalType { ... } + +pub trait Property { ... } + +struct MyNewType(MyOriginalType); + +impl Property for MyNewType { ... } +``` + +[RFC 2393]: https://github.com/rust-lang/rfcs/pull/2393 + +We then use `MyNewType` inside of our crate but don't expose the type, and thus +the implementation, to anyone who shouldn't get access to the implementation +of `Property` for `MyNewType`. + +This mechanism usually works, but incurs +significant boilerplate. This is especially true if `Property` has any +super-trait bounds or has a lot of required items in need of specification. +It doesn't take a trait more complex than `Clone` or `Iterator` for there +to be, comparatively speaking, a lot of overhead. + +One way of reducing the boilerplate is with `#[derive(..)]` on the newtype or +with some sort of generalized newtype deriving or "delegation" (as proposed in +[RFC 2393]). However, delegation does not extend well to all forms of traits. +And even if delegation or deriving does work, there is still overhead. + +This syntactic overhead, whether large (as with manual implementations), +or medium (as with delegation or deriving) exists both for the writer *and* +the reader. In comparison, allowing the user to specify `crate impl ...` +is both a more direct approach, which indicates the semantic intent clearly, +while also being superior for reading, writing, and maintaining. + +Assuming that you already know about this newtype pattern, +which you may very well not do as a beginner, +the only comparative advantage of newtypes is that it does not +require learning any new language features at all. +However, as we've previously argued, permitting visibility modifiers +on `impl` only takes existing concepts from other parts of the language +and applies them to a new place. Thus, the increased learning burden +should not be large. For anyone who already understands visibility +modifiers on anything else in Rust, seeing them for the first time +should also be relatively intuitive. # Prior art [prior-art]: #prior-art From d2f6e26cb9ff3f990faf779955e8948abe566ec8 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Tue, 17 Jul 2018 02:03:04 +0200 Subject: [PATCH 09/21] rfc, hidden-impls: note about E0449. --- text/0000-hidden-impls.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 614b2fa5c81..2c80aa6f590 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -583,6 +583,10 @@ visible of `$vis_{a, b}`. visible than the trait it negatively implements. This is an intentionally conservative restriction which may or may not be lifted in the future. +9. Error [`E0449`](https://doc.rust-lang.org/error-index.html#E0449) will + no longer be emitted when a visibility modifier is applied on a `trait` + implementation. + ## Code generation + Code generation will ignore visibility specified on any implementations. From 04f18455c5b0fe8d2a2fc760705fa1d8fd3bd1a6 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 1 Aug 2018 01:10:00 +0200 Subject: [PATCH 10/21] rfc, hidden-impls: more discussion on pub impl.. vs. impl.. and drawbacks. --- text/0000-hidden-impls.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 2c80aa6f590..1a24c70bc69 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -616,6 +616,19 @@ are attempted. For those who understand the coherent nature of traits and their implementations in Rust, there will be no change, thus, there is no misconception to clear up. +Another possible drawback is the now contextual nature of visibility +when it is implicit rather than specified in the source code. +With this proposal, the default for implementations is `pub`, +but for everywhere else it is `pub(self)`. What we could do is make `pub(self)` +the implicit default on implementations as well, but this would break every +crate in existence because all public implementations would become private. +It is allowed to do this with an edition and so we could do it in 2021. +However, just because we can make edition breaking changes does not mean that +we should. For now, this RFC specifies a lint away from `pub impl..`. +If it turns out to be a problem in practice that `impl..` is thought of as +private, we can lint in the other direction favoring `pub impl..` over `impl..`. +This step should however be taken when we have some practical experience. + # Rationale and alternatives [alternatives]: #rationale-and-alternatives From fa91ed984f78b2437e9609b15d9c5dc80f7160ea Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 1 Aug 2018 01:55:33 +0200 Subject: [PATCH 11/21] rfc, hidden-impls: note crate-local inherent impls of extern types as out of scope. --- text/0000-hidden-impls.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 1a24c70bc69..8da0a1218e3 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -860,7 +860,9 @@ Here we lay out a feel technical details for `priv` for future reference. [unresolved]: #unresolved-questions Exact wordings of error messages discussed in this RFC and their error codes, -as well as lint names and their phrasings are out of the scope of this RFC. +as well as lint names and their phrasings are out of the scope of this RFC. +Designs on crate-local inherent implementations of crate-external types are, +while related, also out of scope for this RFC. The following questions are in the scope of the RFC and should be resolved *before* the RFC is merged: From 05a2c554235bc92c38936f4cd6cb2cfbea4de8c6 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Fri, 24 Aug 2018 19:24:11 +0200 Subject: [PATCH 12/21] rfc, hidden-impls: discuss privacy hygiene. --- text/0000-hidden-impls.md | 57 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 3 deletions(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 8da0a1218e3..23431a642b5 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -1,6 +1,4 @@ -# Draft RFC: Hidden implementations - -- Feature Name: `hidden_impl` +- Feature Name: `hidden_impls` - Start Date: 2018-06-24 - RFC PR: - Rust Issue: @@ -888,6 +886,59 @@ The following questions should be resolved at least before stabilization 3. Should it perhaps be a hard error instead? 5. Should `pub impl ..` be linted against, suggesting `impl ..` instead? + Or perhaps the reverse situation instead? 6. Should `pub(self) field: Type` and the `pub(self)` visibility modifier in general be linted against when it is the contextual default? + +# Future work +[future work]: #future-work + +## Privacy hygiene + +Consider a situation with a declarative macro and method dispatch such as: + +```rust +// Crate A: +// ----------------------------------------------------------------------------- + +pub trait Foo { + fn method(&self) -> u8; +} + +pub struct Bar {} + +crate impl Foo for Bar { + fn mehtod(&self) -> u8 { 42u8 } +} + +pub macro mac() { + println!("result: {}", Bar {}.method()) + // Or instead: ::method(&Bar {}) +} + +// Crate B: (depends on crate A) +// ----------------------------------------------------------------------------- + +fn main() { + mac!(); +// ^^^^^^^ +} +``` + +Would the call to `mac!()` inside `main` be accepted by the compiler? +The answer to this question depends on whether we have *privacy hygiene* or not. +If we do, then calling `mac!()` will use the definition site of `mac`'s +privacy context to check whether the implementation is visible or not. +In this case, `::method` *is* visible in crate A and +so the call would be accepted. However, if we do not have privacy hygiene, +then `mac!()` would result in an error because `::method` +is hidden in crate B. + +Procedural macros and `macro_rules!` macros are unhygienic, +so we would expect them to produce an error in this case. +However, for declarative macros 2.0, there is a design choice to be made. +Fortunately, this question remains open to us as declarative macros are unstable. +As we move towards finalizing macros 2.0, we will need to consider this question. +However, this RFC does not consider the question of whether or not we should +have privacy hygiene but leaves it instead as future work. From a94870618dd29c7cb00590abed0e4a9115e95404 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Fri, 24 Aug 2018 23:06:21 +0200 Subject: [PATCH 13/21] rfc, hidden-impls: add link. --- text/0000-hidden-impls.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 23431a642b5..a48b2049722 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -922,7 +922,7 @@ pub macro mac() { fn main() { mac!(); -// ^^^^^^^ +// ^^^^^^^ Error? This depends on whether we have privacy hygiene or not. } ``` @@ -935,9 +935,12 @@ so the call would be accepted. However, if we do not have privacy hygiene, then `mac!()` would result in an error because `::method` is hidden in crate B. +[decl_mac_2]: https://github.com/rust-lang/rust/issues/39412 + Procedural macros and `macro_rules!` macros are unhygienic, so we would expect them to produce an error in this case. -However, for declarative macros 2.0, there is a design choice to be made. +However, for [declarative macros 2.0][decl_mac_2], +there is a design choice to be made. Fortunately, this question remains open to us as declarative macros are unstable. As we move towards finalizing macros 2.0, we will need to consider this question. However, this RFC does not consider the question of whether or not we should From 85d9fc7daf867397adc534d16e021db1421a4ef6 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Fri, 24 Aug 2018 23:26:14 +0200 Subject: [PATCH 14/21] rfc, hidden-impls: more motivation and other fixes. --- text/0000-hidden-impls.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index a48b2049722..56bf08dedba 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -42,6 +42,16 @@ users of our library (`proptest`) right now. The allure of getting the logic provided when defining `SeedableRng` could also have caused us to prematurely and mistakenly provide a guarantee we were not ready to provide. +[proptest#80]: https://github.com/AltSysrq/proptest/issues/80 +[regex#457]: https://github.com/rust-lang/regex/issues/457 + +A further example from `proptest` is when the author of this RFC +[mistakenly][proptest#80] exposed `impl From for SizeRange` +publicly while the API was only needed internally. +Another occasion where a similar scenario occurred was when +[regex@0.2][regex#457] accidentally had a public dependency +on `regex_syntax` due to yet another a `From` implementation. + Another example use-case which [@nikomatsakis](https://github.com/nikomatsakis) has provided is wanting to have the convenience of `#[derive(Default)]`, but not wanting to commit to there being a default value. @@ -572,7 +582,7 @@ visible of `$vis_{a, b}`. if the visibility of the negative implementation is less than that of the `auto`-trait, then the implementation is rejected. - Conversely, if the visibility of the impl is ≥ that of the trait, + Conversely, if the visibility of the `impl` is `>=` that of the trait, then that will not cause the implementation to be rejected. The visibility specified on a negative implementation has no impact on From 9b9d190885d464671968c7dd6ddfadd7697b0052 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Fri, 24 Aug 2018 23:40:52 +0200 Subject: [PATCH 15/21] rfc, hidden-impls: refresh start date. --- text/0000-hidden-impls.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 56bf08dedba..b5f0bd832cd 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -1,5 +1,5 @@ - Feature Name: `hidden_impls` -- Start Date: 2018-06-24 +- Start Date: 2018-08-24 - RFC PR: - Rust Issue: From f0bad086bcd2974d7125d00ee16dac75661b2a3b Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Sat, 25 Aug 2018 01:40:56 +0200 Subject: [PATCH 16/21] rfc, hidden-impls: fix typo. --- text/0000-hidden-impls.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index b5f0bd832cd..810fef66936 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -46,7 +46,7 @@ and mistakenly provide a guarantee we were not ready to provide. [regex#457]: https://github.com/rust-lang/regex/issues/457 A further example from `proptest` is when the author of this RFC -[mistakenly][proptest#80] exposed `impl From for SizeRange` +[mistakenly][proptest#80] exposed `impl From for Range` publicly while the API was only needed internally. Another occasion where a similar scenario occurred was when [regex@0.2][regex#457] accidentally had a public dependency From f41f034626f3fddccba0baff641b62e5328d17ce Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Tue, 28 Aug 2018 16:02:43 +0200 Subject: [PATCH 17/21] rfc, hidden-impls: specify behavior wrt. Drop and specialization. --- text/0000-hidden-impls.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 810fef66936..e6f372f0d04 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -595,6 +595,15 @@ visible of `$vis_{a, b}`. no longer be emitted when a visibility modifier is applied on a `trait` implementation. +10. When type checking an implementation `$I`, with visibility `$vis_I`, + of a trait with `#[lang = "drop"]`, + for a given type `$A` with visibility `$vis_A`, + if `$vis_I < $vis_A` then the implementation `$I` shall be rejected. + +11. When type checking a specialized implementation `$S` of a base implementation + `$B` where the visibility of `$S` is `$vis_S` and that of `$B` is `$vis_B`, + if `$vis_S < $vis_B` then `$S` shall be rejected. + ## Code generation + Code generation will ignore visibility specified on any implementations. @@ -782,7 +791,7 @@ In a paper [Optional Type Classes for Haskell][optional_pdf] due to Ribero et. al [(DOI: 10.1007/978-3-319-45279-1_9)][optional_doi] modularization of instances is considered. This is however radically different than what is proposed in this RFC, which does *not* propose modularization of -implementations. Fundamentally, that paper seems permit different instances +implementations. Fundamentally, that paper seems to permit different instances of the same type class to coexist as long as the instance to use for a given expression is uniquely determinable. As previously discussed, such a mechanism would not be sound in Rust. The main difference between these approaches is From 0f898cd3645b310cea21dc38110c85a78bf6144a Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Tue, 28 Aug 2018 16:31:32 +0200 Subject: [PATCH 18/21] rfc, hidden-impls: discuss behavior wrt. specialization & Drop. --- text/0000-hidden-impls.md | 80 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index e6f372f0d04..145afc66eb7 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -714,12 +714,90 @@ that macro, and transitively) will be more visible than the `vis` specified in `#[derive($vis Trait)]`. We argue that this is a good thing to do because it removes a large implementation burden on custom derive authors as well as allowing users a uniform experience in the ecosystem. By clamping the visibility, -we may rule out some unhygenic things that users may want to do, but the ability +we may rule out some unhygienic things that users may want to do, but the ability to write the macro generated code always remains. We could potentially have some mechanism to opt-out of the hygiene at the macro definition site, but this should await users actually needing that feature and be left as future work. +## Behaviour with respect to specialization + +Consider the following example: + +```rust +// crate A + +pub trait Property { + fn get(&self) -> u32; +} + +pub impl Property for T { + default fn get(&self) -> u32 { 42 } +} + +pub struct Thing; + +crate impl Property for Thing { + fn get(&self) -> u32 { 4242 } +} + +pub fn take_has_property(prop: impl Property) { .. } + +// crate B + +fn main() { + crate_A::take_has_property(Thing) +} +``` + +What will happen when `take_has_property(Thing)` is called? +Here are some possibilities: + +1. Because `crate impl Property for Thing` is hidden from crate B, + the type checker will resolve this implementation to + `pub impl Property for T` instead. + + This possibility would be unsound because we have allowed trait resolution + to result in different dynamic semantics (run-time behaviour) depending + on where an implementation was resolved. This behaviour is exactly what + coherence prevents. Because this option would be unsound, + alternative 1) must be ruled out. + +2. Because we want to preserve coherence, we allow `take_has_property(Thing)` + to resolve to `crate impl Property for Thing` instead, but we would not + permit calling `Thing.get()` or `::get(&Thing)` directly. + + This would make the hidden implementation indirectly observable in crate B. + A possible justification for this approach would be that an implementation + is already provided, so the specialization is not expected to behave in + a way that is radically different. Furthermore, if the specialized but hidden + implementation were removed, a base implementation would be there wherefore + no compile time error would occur. + + Under a situation where parametricity is culturally assumed by users, + this might work; However, given that we are likely to permit associated types + to be specialized and that we we expect that a non-trivial amount of users + will not preserve parametricity, we can not assume the culture to be universal. + Since we can't assume parametricity, this approach is likely to: + + lead to surprises that are hard to debug. + + be harder to understand for users. + +3. Require that any specialized implementation be at least as visible as its + base implementation which it is specializing. + Employing this rule would cause the snippet above to be ill typed at the + definition of `crate impl Property for Thing`. + The advantage of such a rule is that: + + it is more conservative, allowing us to relax it in the future in a way + that is forward compatible with 2). + + it is likely simpler to understand because the error message gives us an + opportunity to explain the situation to users. + + there will be no possibility of surprises. + +Because 1) is unsound and 2) could be surprising, we have chosen 3) as the +conservative and simpler solution to this dilemma. + +With respect to implementations of `Drop`; we employ a similar reasoning. + ## Alternative: Newtypes The principal alternative to this RFC is to continue using the From 7d05274ebe7a6318f8da25e1b5ebbc47a2af5544 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Tue, 28 Aug 2018 16:32:00 +0200 Subject: [PATCH 19/21] rfc, hidden-impls: discuss impl Trait. --- text/0000-hidden-impls.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 145afc66eb7..ccea9127884 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -412,6 +412,10 @@ is when it comes to trait objects. This is fine for a few reasons: vtable of any hidden implementations. This could be done as a lint of some form, but is currently left as possible future work. +Finally, because `impl Trait` in return position is in many ways used for the +same purposes but without the overhead of dynamic dispatch, we allow the same +behaviour for a function such as `pub fn make_with_property() -> impl Property`. + ## Recommendation: When you should use hidden implementations Simply put, it is recommended that your implementation be hidden if you are From f31dd9033c1e1ee104e170e68d891ceeebe0c011 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Tue, 28 Aug 2018 17:26:07 +0200 Subject: [PATCH 20/21] rfc, hidden-impls: discuss newtypes more and #[hide(..)]. --- text/0000-hidden-impls.md | 74 +++++++++++++++++++++++++++++++++++---- 1 file changed, 68 insertions(+), 6 deletions(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index ccea9127884..848bf373046 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -824,16 +824,30 @@ We then use `MyNewType` inside of our crate but don't expose the type, and thus the implementation, to anyone who shouldn't get access to the implementation of `Property` for `MyNewType`. -This mechanism usually works, but incurs -significant boilerplate. This is especially true if `Property` has any -super-trait bounds or has a lot of required items in need of specification. -It doesn't take a trait more complex than `Clone` or `Iterator` for there -to be, comparatively speaking, a lot of overhead. +This mechanism usually works, but incurs significant boilerplate due to: +1. the need to define the newtype, +2. to delegate or derive needed traits, +3. to wrap and unwrap values of the base type in the newtype where needed. + +This is especially true if `Property` has any super-trait bounds or has a lot of +required items in need of specification. It doesn't take a trait more complex than +`Clone` or `Iterator` for there to be a lot of overhead comparatively speaking. One way of reducing the boilerplate is with `#[derive(..)]` on the newtype or with some sort of generalized newtype deriving or "delegation" (as proposed in [RFC 2393]). However, delegation does not extend well to all forms of traits. -And even if delegation or deriving does work, there is still overhead. +For example, in [RFC 2393], the following restrictions apply at the time of writing: ++ no associated types (so no `Iterator`) ++ no associated `const`s. ++ functions without a receiver (`(&mut?)? self`) ++ functions where any argument type or the return type is `Self` (no `PartialEq`). + +And even if delegation or deriving does work, there is still overhead because +only the burden to define delegations (2) is reduced. + +Finally, delegation or deriving does sometimes not work at all because you do +not wish to give ownership away of your wrapped value but wrapping a reference +to it does not work because the trait requires `Self: 'static`. This syntactic overhead, whether large (as with manual implementations), or medium (as with delegation or deriving) exists both for the writer *and* @@ -852,6 +866,54 @@ should not be large. For anyone who already understands visibility modifiers on anything else in Rust, seeing them for the first time should also be relatively intuitive. +## Alternative `#[hide(..)]` + +An alternative due to @burdges is that implementations may be hidden using +a sort of linting scheme. For example: + +```rust +#[hide(crate, deny, + "X is not small, but we audit our internal Copy usage, + and fix benchmark regressions.")] +impl Copy for X {} + +#[hide(crate, allow, + "We use a slow conversion in X: Hash so you should use a + precursor to X or batch convert to X::Affine instead.")] +impl Hash for X { ... } +``` + +This would expose said implementations, but raise a warning for the user that +depends on the implementations. + +This linting mechanism has the advantage that the person trying to use the +"hidden" implementation receives a warning explaining why it is hidden. +However; such a mechanism can also be provided for visibility modifiers in +general by allowing you to specify some custom on-error handler: + +```rust +#[on_visibility_error("Sorry we are not exposing this because...")] +``` + +This mechanism is orthogonal to `crate impl` because it could be used at +any place where a visibility modifier is permitted today. An example would +be placing `#[on_visibility_error(..)]` on a `struct` definition. + +With respect to using `#[hide(..)]`, it also cannot be used as a means to +retain invariants because the attribute is only advisory and not enforced by +the type system. This entails that `unsafe { .. }` may not rely on `#[hide(..)]` +to ensure that what was intended to be hidden stays hidden. +Meanwhile `crate impl` is a true mechanism for encapsulation because it is +enforced by the type checker. + +Finally, we argue that `crate impl..` is the more consistent choice with Rust +as is because it is merely reusing the mechanism we already have for controlling +privacy on items to another item. In contrast, `#[hide(..)]` is not a familiar +syntax and as such it uses up too much of the complexity budget. +Furthermore, it is not clear that forcing the user to always give a reasoning +for why something is hidden is good. Our aim is to make it easy to not +overpromise things and having to give a reason runs counter to that aim. + # Prior art [prior-art]: #prior-art From 77c009802c804ffa9ffaacdb701d0f0c2736c90b Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Tue, 28 Aug 2018 17:40:46 +0200 Subject: [PATCH 21/21] rfc, hidden-impls: try to make GH update the PR. --- text/0000-hidden-impls.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-hidden-impls.md b/text/0000-hidden-impls.md index 848bf373046..136040ca678 100644 --- a/text/0000-hidden-impls.md +++ b/text/0000-hidden-impls.md @@ -912,7 +912,7 @@ privacy on items to another item. In contrast, `#[hide(..)]` is not a familiar syntax and as such it uses up too much of the complexity budget. Furthermore, it is not clear that forcing the user to always give a reasoning for why something is hidden is good. Our aim is to make it easy to not -overpromise things and having to give a reason runs counter to that aim. +overpromise things and having to give a reason runs counter to that goal. # Prior art [prior-art]: #prior-art