Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign up[Draft] RFC: Hidden implementations #7
Conversation
Centril
added some commits
Jun 24, 2018
Centril
added
rfc
draft
labels
Jun 25, 2018
Mark-Simulacrum
reviewed
Jun 25, 2018
| 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. |
This comment has been minimized.
This comment has been minimized.
| ```rust | ||
| // crate B: | ||
| impl Property for Thing { .. } |
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
Jun 25, 2018
I'm confused how this would be allowed anyway as (AIUI) neither Property nor Thing are defined in crate B, so this impl would be invalid, overlap or no overlap.
|
|
||
| ## `priv` visibility | ||
|
|
||
| To be able to make implementations module private, this RFC introduces a new |
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
Jun 25, 2018
Couldn't people who need this do pub(in self)? Seems like that would be less surprising than "priv"
This comment has been minimized.
This comment has been minimized.
kennytm
Jun 26, 2018
Or priv could be just made an alias of pub(self) (similar to crate being an alisa of pub(crate)).
This comment has been minimized.
This comment has been minimized.
Centril
Jul 14, 2018
Author
Owner
Apparently I was oblivious to the fact that pub(self) is already stable - so I'll make this section "possible future work".
| 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. |
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
Jun 25, 2018
So essentially, return Box::new(Thing): Box<dyn Property> is permitted with no compiler warnings? I'm confused why the compiler cannot lint against this -- it knows that it is creating a vtable for the impl of Property for Thing; as such that place can lint against this (though in many cases only after/during monomorphization).
However, I'm not opposed to this (just feels like the wording could be changed).
|
|
||
| + 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. |
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
Jun 25, 2018
Hm, so we hard-lint currently on pub in the the below impl; in a way, I could see this being a way to define the "extensible traits" talked about on Discord -- we could say that you could make certain methods of a trait not world-visible. I guess you can also do that with two (or more) traits, but this seems like a reasonable extension of this RFC.
trait Foo {
pub fn bar() {}
}
This comment has been minimized.
This comment has been minimized.
Centril
Jul 14, 2018
Author
Owner
Sure; you could have something like:
// crate A
pub trait Foo {
crate fn bar();
pub fn baz();
}and since only crate A can provide a definition for bar, then only crate A can provide implementations for Foo.
I'll leave this out for now tho :)
| 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? |
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
Jun 25, 2018
I'd like to say that the priv vs pub(self) etc debate is also something to solve after merging the RFC.
This comment has been minimized.
This comment has been minimized.
Centril
Jul 14, 2018
Author
Owner
Absolutely; Now that I know pub(self) is stable, we should punt on priv.
kennytm
reviewed
Jun 26, 2018
| 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? |
This comment has been minimized.
This comment has been minimized.
kennytm
Jun 26, 2018
Not negative bounds, but you do need to consider effect of negative impls of auto-traits:
priv impl !Send for MyUnsafeType {}I suggest that the visibility of a negative impl must be ≥ that of the trait, otherwise it would be an error.
This comment has been minimized.
This comment has been minimized.
Centril
Jul 14, 2018
Author
Owner
Hmm... this one is tricky... The impl above should be well-behaved (and not unsound)? and be interpreted as un-implementing Send for MyUnsafeType inside the module, but not elsewhere. Thus, the fact that MyUnsafeType: Send may be observed outside but not inside the module.
However; I can't see any use cases for this, and it will likely just lead to bad places; so your proposed rule sounds good, at least for now.
This comment has been minimized.
This comment has been minimized.
kennytm
commented
Jun 26, 2018
|
One of the biggest unresolved question is that, given rust-lang/rust#39935 is still not fixed (which forced every impl in libstd to be insta-stable), how would I know this RFC would be implementable |
Centril
added some commits
Jul 14, 2018
This comment has been minimized.
This comment has been minimized.
|
Addressed all review comments thus far and made other improvements. :) |
aturon
reviewed
Jul 26, 2018
aturon left a comment
|
I had time to read up through the start of the reference section; will read the rest later. Overall, this seems very solid and thoroughly thought out, and I think would fit nicely with some other proposals in the air (like being able to write inherent impls for nonlocal types as long as those impls are marked with sufficient privacy). |
| ``` | ||
|
|
||
| # Motivation | ||
| [motivation]: #motivation |
This comment has been minimized.
This comment has been minimized.
|
|
||
| ## The effect of hiding | ||
|
|
||
| ### Error E0277 |
This comment has been minimized.
This comment has been minimized.
aturon
Jul 26, 2018
Note that there's a non-trivial interaction with macros here; currently, "privacy hygiene" is not a thing.
This comment has been minimized.
This comment has been minimized.
Centril
Jul 31, 2018
Author
Owner
Could you elaborate a bit here? The way I'm thinking is that if we have a macro with say:
macro_rules! mac {
() => {
fn return_property() -> impl Property {
make_a_thing() // ← error[E0277]
}
}
}which is then called at some site, then the visibility context used is that of the call site of mac!().
The same should be the case with a macro defining an implementation; it too is visible according to the call-site and not the definition site of the macro.
| 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: |
This comment has been minimized.
This comment has been minimized.
| 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 |
This comment has been minimized.
This comment has been minimized.
| `pub(super)`) be permitted on `impl` to hide implementations from | ||
| different parts of the crate. | ||
|
|
||
| ## `pub` by default |
This comment has been minimized.
This comment has been minimized.
aturon
Jul 26, 2018
This is, to my mind, the biggest drawback of the proposal. I don't see a good way around it, though.
This comment has been minimized.
This comment has been minimized.
Centril
Jul 31, 2018
Author
Owner
Yup, I agree on this being a drawback :) I added a paragraph discussing it in the section on Drawbacks.
| = note: #[warn(redundant_vis)] on by default | ||
| ``` | ||
|
|
||
| ## Private visibility, `pub(self)` |
This comment has been minimized.
This comment has been minimized.
|
|
||
| then the lint `redundant_vis` will be raised. | ||
|
|
||
| ## `#[derive(crate Trait)]` |
This comment has been minimized.
This comment has been minimized.
| 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 |
This comment has been minimized.
This comment has been minimized.
| 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 |
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
reviewed
Jul 26, 2018
| 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. |
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
Jul 26, 2018
One thing to potentially add/explore perhaps is to take a look at the compiler's TyCtxt and similar types that are used throughout, but can't currently be implemented directly on outside of librustc; it feels loosely like that's another significant motivation -- currently there's lots of functions that should/could be inherent on tcx but can't be.
Centril
added some commits
Jul 31, 2018
This comment has been minimized.
This comment has been minimized.
|
Time to publish. |
Centril commentedJun 25, 2018
•
edited
This is a draft version of an RFC for you to review, before a formal proposal is made for consideration.
The RFC introduces a concept I am calling "hidden implementations".
Note that this idea is radically different than modularization of implementations as permitted in Idris, Scala and such languages in that it preserves coherence keeps the language free of orphan implementations.
I have left a bunch of unresolved questions which I'd like special attention given to. If you, the reader, have some rationale, motivation, drawbacks that you feel are missed, please do include them in your review. I would also love it if you find better ways to explain concepts than in this draft.