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

Add Inner Trait pattern #355

Closed
wants to merge 14 commits into from
Closed

Add Inner Trait pattern #355

wants to merge 14 commits into from

Conversation

rbran
Copy link

@rbran rbran commented Apr 6, 2023

No description provided.

@simonsan simonsan added C-addition Category: Adding new content, something that didn't exist in the repository before A-pattern Area: Content about Patterns labels Apr 6, 2023
@simonsan
Copy link
Collaborator

simonsan commented Apr 7, 2023

I also like this one, I skimmed over it and it looks like a good addition to this repository. Maybe others, e.g. @neithernut and/or @pickfire can give it a read as well to leave some always valued feedback. So we can merge it next week. (Sorry for C&P, not much time right now.)

@burdges
Copy link

burdges commented Apr 7, 2023

You'd avoid re-implementing the exposed methods if you require InnerCar : Car btw.

It's a useful pattern if you've very large crates or maybe some dyn InnerCar patterns. It'll also become more useful whenever specialization arrives.

Yet, if your project has many small crates, then you'd typically achieve this using free fns instead.

fn set_speed<C: Car>(car: &mut C, new_speed: f64) { ... }
fn get_acceleration<C: Car>(car: &mut C) -> f64 { ... }

Copy link
Contributor

@neithernut neithernut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this is a specific manifestation of a more general pattern which may or may not warrant its own (sub)section including #310 and "type classes" (note the double quotes) as in

trait Foo {
    // ...
}

impl <T: ...> Foo for T {
    // ...
}

So maybe we want to broaden this into the "extension trait" pattern proposed in #310. If not we should draw a line bwtween those two at some point in the future (as in, if/when someone writes a section about "extension traits" or something similar).

TL;DR: LGTM, no objections against merging this as is.

implementation of a public trait without exposing that functionality as part of
the public API. By using a private trait, developers can also improve the
reusability of their code, since the private functionality can be reused across
multiple implementations of the public trait.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suppose I'm using a trait Car from another crate vehicles, but for a very specific purpose requiring some additional code. Do you feel that a new trait InnerCar private to my own crate would fall under this pattern? If yes, this would be an alternative motivation, wouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another layer on top of this is to prevent breaking changes, since we can now limit functionality without exposing public API.

@simonsan simonsan marked this pull request as ready for review April 7, 2023 13:59
Comment on lines 100 to 105
## See also

Wikipedia [OOP Interface](https://en.wikipedia.org/wiki/Interface_%28object-oriented_programming%29).

Blog post from [Predrag](https://web.archive.org/web/20230406211349/https://predr.ag/blog/definitive-guide-to-sealed-traits-in-rust/)
about sealed, private and other patterns for traits.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think once the other PR is merged, we need to link them to each others.

Comment on lines 10 to 14
## Example

This example demonstrate how a public trait `Car` can be implemented and include
extra private methods using a auxiliary private trait `InnerCar`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably want to show a real example in standard library or in popular crates for this.

Comment on lines 18 to 20
fn get_speed(&self) -> f64;
fn accelerate(&mut self, duration: f64);
fn brake(&mut self, force: f64);
Copy link
Contributor

@pickfire pickfire Apr 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend keeping this to at most 2 functions to make a minimal example. And also, car might not be a good example, since usually car isn't considered a trait, vehicle probably is, depending on the context.

@simonsan
Copy link
Collaborator

simonsan commented Apr 7, 2023

@rbran Could you please rebase on main, we just merged our refactoring efforts to support translations. Basically SUMMARY.md and the other markdown files are moved into ./src/**/{file}. Cheers.

@rbran
Copy link
Author

rbran commented Apr 7, 2023

The more that I think about this pattern, the less I like it. It fells too complex/edge-case for a pattern, or maybe is a simple adaptation/equivalent for an "abstract-class" made by someone that is too stuck with the OOP mentality.

@simonsan
Copy link
Collaborator

simonsan commented Apr 7, 2023

The more that I think about this pattern, the less I like it. It fells too complex/edge-case for a pattern, or maybe is a simple adaptation/equivalent for an "abstract-class" made by someone that is too stuck with the OOP mentality.

I understand the hesitation and value that feedback. I would say, that there are use cases, where you would like to use it, though. Maybe, this thought you have there is really something you could bring into a sentence as a warning when not to use it and which problems it can bring. I would say, that it's always better to explain these kinda things and let people gain that knowledge, than not documenting it at all and have a mess all over the place.

Comment on lines +1 to +104
In Rust, a sealed trait is a trait that can only be implemented within the same
crate where it is defined. It is achieved by making a public trait that depends
on a [supertrait](https://doc.rust-lang.org/rust-by-example/trait/supertraits.html)
that is private or only public on the local crate. This restricts the ability
to implement the trait from outside the crate and provides a form of interface
segregation.

## Motivation

Sealed traits can be used to create a set of behaviors that that allow future
addition of methods without breaking the API.

The sealed trait pattern helps to separate the public interface from the
implementation details. This makes it easier to maintain and evolve the
implementation over time without breaking existing code. It also provides a
level of abstraction that allows users to interact with the library or module at
a high level, without needing to know the implementation details.

Because users of this crate can't implement this trait directly, is possible to
add methods to it, without break existing code using this create. Only
implementations of this trait will need updated, what is assured by the
sealed trait to only happen locally.

## Example

One possible use of the sealed trait is to limit what kind of implementation a
function can receive, allowing only a limited number of types to be passed as
parameters.

```rust,ignore
pub(crate) mod private {
pub(crate) trait Sealed {}
}
// MyStruct is Sealed, and only this crate have access to it. Other crates will
// be able to implement it.
pub trait MyStruct: private::Sealed {...}
// auto implement Sealed for any type that implement MyStruct
impl<T: MyStruct> private::Sealed for T {}

pub struct MyStructA {...}
impl MyStruct for MyStructA {...}

pub struct MyStructB {...}
impl MyStruct for MyStructB {...}

// this function will only receive MyStructA or MyStructB because they are the
// only ones that implement the MyStruct trait
pub fn receive_my_struct(my_struct: impl MyStruct) {...}
```

The standard library makes use of a sealed trait, one example is the
`OsStrExt` trait for
[unix](https://doc.rust-lang.org/std/os/unix/ffi/trait.OsStrExt.html),
[windows](https://doc.rust-lang.org/std/os/windows/ffi/trait.OsStrExt.html) and
[wasi](https://doc.rust-lang.org/std/os/wasi/ffi/trait.OsStrExt.html).

Trait from `std::os::unix::ffi::OsStrExt`:

```rust,ignore
pub trait OsStrExt: Sealed {
fn from_bytes(slice: &[u8]) -> &Self;
fn as_bytes(&self) -> &[u8];
}
```

The `Sealed` trait is private and cannot be accessed from outside the standard
library. Not allowing users to implement `OsStrExt` for any type, except for the
implementations already present on the standard library.

The documentation describes it's motivation as the following:

> This trait is sealed: it cannot be implemented outside the standard library.
> This is so that future additional methods are not breaking changes.

## Advantages

By separating the public interface from the implementation details, it is
easier to maintain, ensure that the code remains correct without affecting
external users.

## Disadvantages

Although it usually reduces complexity and code duplication, the sealed trait
pattern can add complexity to the codebase, particularly if there are many
sealed traits that need to be managed.

## Discussion

Sealed traits are a useful tool for creating a set of related behaviors that are
intended to be used together without allowing other behaviors to be added from
outside the crate.

This restriction also allow future additions to the trait
without compromising the compatibility with existing code uses of it.

## See also

Blog post from
[Predrag](https://web.archive.org/web/20230406211349/https://predr.ag/blog/definitive-guide-to-sealed-traits-in-rust/)
about sealed, private and other pattern for traits.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file belongs to #354 as well. Do you want to PR it all in one PR, or you rather want it separated? If so, then removal of this file would be good.

@rbran rbran closed this Apr 8, 2023
@rbran rbran deleted the inner-trait branch April 8, 2023 12:16
@rbran
Copy link
Author

rbran commented Apr 8, 2023

I'll rethink this, if I got a more concise idea for a pattern, I'll create a new PR.

@simonsan simonsan added the C-zombie Category: A PR/an Issue that might be still useful but was closed label Apr 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pattern Area: Content about Patterns C-addition Category: Adding new content, something that didn't exist in the repository before C-zombie Category: A PR/an Issue that might be still useful but was closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants