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

Discourage trait bounds on data structures unless necessary #6

Closed
dtolnay opened this issue Apr 4, 2017 · 5 comments
Closed

Discourage trait bounds on data structures unless necessary #6

dtolnay opened this issue Apr 4, 2017 · 5 comments
Assignees

Comments

@dtolnay
Copy link
Member

dtolnay commented Apr 4, 2017

Example from flate2:

pub struct EncoderReader<R: Read> {
    inner: EncoderReaderBuf<BufReader<R>>,
}

Bounds like this are annoying because they transitively infect any structs containing this type. Usually trait bounds should go just on impl blocks, not on data structures.

There are two exceptions where trait bounds on data structures are required.

  1. The data structure refers to an associated type of the trait. Cow is an example of this.
  2. The data structure has a Drop impl that requires trait bounds. Rust currently requires all bounds on the Drop impl to also be present on the data structure.

Relevant flate2 issue: rust-lang/flate2-rs#88

@brson
Copy link
Contributor

brson commented Jun 30, 2017

@brson
Copy link
Contributor

brson commented Jun 30, 2017

Mention the traits where bounds on types are desirable and why. Subtle issue.

@brson
Copy link
Contributor

brson commented Jun 30, 2017

"Flexibility" section.

@KodrAus KodrAus self-assigned this Aug 4, 2017
@KodrAus
Copy link
Contributor

KodrAus commented Aug 7, 2017

I'm looking at writing this one up. So, from a pure data vs behaviour split it seems like structures should only have trait bounds that inform data, that is associated types, makes sense. Of course with the exception of Drop.

One argument that's been raised is that trait bounds on structures can communicate that a structure has no use if its inner value doesn't implement a particular trait. Most examples I've seen for this are wrapper types around something like Read. I would still lean towards not requiring the bound on the type because that's still informing behavior rather than data and limiting your future options for extensibility.

Should we note that for build-in derives you don't need to add the bound yourself? Ie:

#[derive(Debug, Clone, Hash)]
struct Foo<T: Debug + Clone + Hash>(T);

is redundant?

@KodrAus
Copy link
Contributor

KodrAus commented Aug 7, 2017

And I guess there's also the un-bound ?Sized to consider.

LLFourn added a commit to LLFourn/bdk that referenced this issue Nov 24, 2020
LLFourn added a commit to LLFourn/bdk that referenced this issue Dec 3, 2020
arnauorriols pushed a commit to arnauorriols/streams that referenced this issue Sep 3, 2021
Trait bounds should generally be pushed to the latest moment when they are actually needed. That is, the functions that manipulate the data and that actually require the bound. It is usually avoided (to the reasonable extend) to put trait bounds in the type parameters of data types and trait declarations, as they unleash an unergonomic "trait bound hell" and are usually overly restrictive.

Some relevant references:
- [API Guidelines]
- [discussion API Guidelines]
- [flat2 case]
- [Haskell Programming Guidelines]

In this case, the trait bounds "Sync" and "Send" are already enforced (if necessary) when the async runtime actually schedule the Futures, and it is at this point (if actually enforced by the runtime) that all the types and intermediate functions that manipulate those types are required to enforce these markers; However, if at any point of the rabbit hole a type bound is placed on the data, hell breaks loose and all types that touch the bounded type get contaminated. [Implied bounds RFC] might mitigate the issue, but for the time being, this convention is gold one of those that is "all or nothing", because of the "viral" effect.

[API Guidelines]: https://rust-lang.github.io/api-guidelines/future-proofing.html#data-structures-do-not-duplicate-derived-trait-bounds-c-struct-bounds
[discussion API Guidelines]: rust-lang/api-guidelines#6
[Haskell Programming Guidelines]: https://wiki.haskell.org/Programming_guidelines#Types
[flate2 case]: rust-lang/flate2-rs#88
[Implied bounds RFC]: rust-lang/rust#44491
arnauorriols pushed a commit to arnauorriols/streams that referenced this issue Sep 3, 2021
Trait bounds should generally be pushed to the latest moment when they are actually needed.
That is, the functions that manipulate the data and that actually require the bound.
It is usually avoided (to the reasonable extend) to put trait bounds in the type parameters
of data types and trait declarations, as they unleash an unergonomic "trait bound hell" and are usually overly restrictive.

Some relevant references:
- [API Guidelines]
- [discussion API Guidelines]
- [flat2 case]
- [Haskell Programming Guidelines]

In this case, the trait bounds "Sync" and "Send" are already enforced (if necessary) when the async runtime actually schedule the Futures,
and it is at this point (if actually enforced by the runtime) that all the types and intermediate functions that manipulate those types are required
to enforce these markers; However, if at any point of the rabbit hole a type bound is placed on the data, hell breaks loose and all types that touch
the bounded type get contaminated. [Implied bounds RFC] might mitigate the issue, but for the time being, this convention is gold one of those that
is "all or nothing", because of the "viral" effect.

[API Guidelines]: https://rust-lang.github.io/api-guidelines/future-proofing.html#data-structures-do-not-duplicate-derived-trait-bounds-c-struct-bounds
[discussion API Guidelines]: rust-lang/api-guidelines#6
[Haskell Programming Guidelines]: https://wiki.haskell.org/Programming_guidelines#Types
[flate2 case]: rust-lang/flate2-rs#88
[Implied bounds RFC]: rust-lang/rust#44491
bgw added a commit to vercel/next.js that referenced this issue Nov 15, 2024
… types (#72823)

`Vc<T>` had a type bound that `T: Send + ?Sized`. The general recommendation is to remove all type bounds from the struct that aren't necessary:

- https://rust-lang.github.io/api-guidelines/future-proofing.html#c-struct-bounds
- rust-lang/api-guidelines#6
- rust-lang/rust-clippy#1689

The reasoning is that type bounds on structs are "infectious". Any generic function, trait, or struct referring to `Vc<T>` was required to add `T: Send` bounds.

*Sidenote: The [`implied_bounds` feature](https://rust-lang.github.io/rfcs/2089-implied-bounds.html) might mitigate some of this if ever stabilized.*

Removing the `T: Send` type bound from `Vc<T>` means that there's less places where we need to specify it.

This pattern can be seen in many parts of the stdlib. For example, `Arc<T>` doesn't require that `T: Send + Sync`, though in reality to do anything useful with it, you need `T: Send + Sync`.

## Is this safe?

Yes, bounds are checked during cell construction, and the lower-level APIs used for creating cells require `Send + Sync`, so this would be hard to mess up without explicitly unsafe code.

Also, `Vc<T>` also technically requires that `T: Sync`, but that wasn't enforced in the struct definition (only during cell construction). We did just fine without that bound on the struct.

## Why are you leaving `?Sized`?

There's an implicit `Sized` bound on type parameters in struct definitions unless explicitly specified otherwise with `?Sized`.

Right now this bound isn't used. We use a box, e.g. `Vc<Box<dyn Foo>>`, but in the future we might be able to drop that intermediate box from the type signature. I'm leaving the `?Sized` bound in place (and adding it in a few places where it was missing) in hopes of that.
wyattjoh pushed a commit to vercel/next.js that referenced this issue Nov 28, 2024
… types (#72823)

`Vc<T>` had a type bound that `T: Send + ?Sized`. The general recommendation is to remove all type bounds from the struct that aren't necessary:

- https://rust-lang.github.io/api-guidelines/future-proofing.html#c-struct-bounds
- rust-lang/api-guidelines#6
- rust-lang/rust-clippy#1689

The reasoning is that type bounds on structs are "infectious". Any generic function, trait, or struct referring to `Vc<T>` was required to add `T: Send` bounds.

*Sidenote: The [`implied_bounds` feature](https://rust-lang.github.io/rfcs/2089-implied-bounds.html) might mitigate some of this if ever stabilized.*

Removing the `T: Send` type bound from `Vc<T>` means that there's less places where we need to specify it.

This pattern can be seen in many parts of the stdlib. For example, `Arc<T>` doesn't require that `T: Send + Sync`, though in reality to do anything useful with it, you need `T: Send + Sync`.

## Is this safe?

Yes, bounds are checked during cell construction, and the lower-level APIs used for creating cells require `Send + Sync`, so this would be hard to mess up without explicitly unsafe code.

Also, `Vc<T>` also technically requires that `T: Sync`, but that wasn't enforced in the struct definition (only during cell construction). We did just fine without that bound on the struct.

## Why are you leaving `?Sized`?

There's an implicit `Sized` bound on type parameters in struct definitions unless explicitly specified otherwise with `?Sized`.

Right now this bound isn't used. We use a box, e.g. `Vc<Box<dyn Foo>>`, but in the future we might be able to drop that intermediate box from the type signature. I'm leaving the `?Sized` bound in place (and adding it in a few places where it was missing) in hopes of that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants