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

Early trait bounds on generic types #590

Closed
wants to merge 1 commit into from

Conversation

Kimundi
Copy link
Member

@Kimundi Kimundi commented Jan 17, 2015

Summary

Make it an API convention that generic types that have an interface
that always requires the same trait bounds on the same type parameters
should have those bounds directly in the type definition. Allow exceptions
from this rule for cases like planned changes or upholding backwards
compatibility.

Rendered

that always requires the same trait bounds on the same type parameters
_should_ have those bounds directly in the type definition. Allow exceptions
from this rule for cases like planned changes or upholding backwards
compatibility.
@aturon
Copy link
Member

aturon commented Feb 4, 2015

@Kimundi Thanks for this RFC, and sorry to be slow commenting on it.

I quite like this idea, and I think you've very carefully thought out the tradeoffs and nuances for rolling out; I'm basically happy to take the RFC as-is.

I would note that this has some interaction with implies bounds, though it's not clear if/when we'll get those. But essentially, that change would put even more pressure to follow the convention you're laying out here.

@huonw
Copy link
Member

huonw commented Mar 8, 2015

rust-lang/rust#23176 is related: it (unfortunately) "removes" the motivating example, by making Mutex<T> only consider the Send/Sync nature of T when considering that of the whole Mutex<T>, i.e. making the methods themselves not care about/require T: Send.

@nikomatsakis
Copy link
Contributor

One thing that was not clear to me: the RFC mentions adding these bounds after the fact, but that would be a breaking change -- I presume we intend to follow this only for newer types, if we accept it now?

@Gankra
Copy link
Contributor

Gankra commented May 27, 2015

Arguably hoisting "universal" requirements to the struct level is a minor breaking change in that:

  • everyone genuinely using the type would have had those constraints already
  • adding those type constraints in client code is forward-backward compatible

@Gankra
Copy link
Contributor

Gankra commented May 27, 2015

On the RFC as a whole, I feel like this could largely be alleviated by better error message tooling overall. Right now we have a nasty habit of telling you the "last detail" of why your code is wrong. e.g. in this case we're telling you you're calling a non-existent method, where really this is just a consequence of missing the bound. I'm not familiar enough with the error-reporting tooling, but it seems plausible in theory to "explore" the type's impl's for matching names and construct plausible causes.

See also rust-lang/rust#21793

@Gankra
Copy link
Contributor

Gankra commented May 27, 2015

Note: This RFC entered its Final Comment Period as of yesterday.

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label May 27, 2015
@alexcrichton
Copy link
Member

This RFC does seem to unfortunately contradict the decision made in rust-lang/rust#23176, the rationale there being that operating generically over these types is much easier if the bounds are not required (as sometimes they may not be, for example deriving).

I think that the motivation here was also solved by that PR by only using the Send and Sync bounds when they're actually needed to cross a thread boundary of some form. For example the lock method of Mutex no longer requires that the type is Send.

Also, 1.0 has been released since this RFC was written, so I would personally prefer to take the opposite route of adding bounds to type definitions as soon as possible. Bounds should only be required when absolutely necessary, for example Send and Sync should usually only be necessary when determining whether a type itself is Send or Sync, not for the instance methods.

@Kimundi
Copy link
Member Author

Kimundi commented May 28, 2015

Yeah, with the new stance to have impls be as minimal as possible this RFC is largely obsoleted

@Kimundi
Copy link
Member Author

Kimundi commented May 30, 2015

@nikomatsakis: Ah, just re-read your comment. Such a change is indeed a breaking change after-the-fact, this should ideally have been a pre-1.0 thing... Though Gankro's comment also holds true.

@bluss
Copy link
Member

bluss commented May 30, 2015

I think it's annoying to implement types this way, and by extension use them this way. Ironically (to me), trait bounds on a struct don't help you, because instead of being the default for any use of the struct, they simply become mandatory to repeat for any use of it. In a way, you lose when you give the compiler more information.

I prefer the minimal route. Usually bounds on the struct definition is only required for A) Correspondance with Drop B) Using associated types in fields.

The Foo::new() function is a good place to use the trait bounds you wish the user to encounter when trying to use your type!

@BlacklightShining
Copy link

@alexcrichton I don't see that decision as being relevant here. As @huonw said there,

It is perfectly safe for an Arc<T> to stay on and be used on a single thread if T doesn't satisfy the necessary bounds. It just acts like a (expensive) Rc<T>.

In contrast, some types like HashSet<T> don't make any sense if T: !Hash—you can't even construct an instance with new(). I think types like that shouldn't be allowed at all—the alternative, as @Kimundi said, would still give errors late.

@glaebhoerl
Copy link
Contributor

Ironically (to me), trait bounds on a struct don't help you, because instead of being the default for any use of the struct, they simply become mandatory to repeat for any use of it.

This happens to be the same reason why DatatypeContexts is going to be (or has been?) removed from Haskell.

@alexcrichton
Copy link
Member

The state of the standard library has definitely changed since this RFC was first opened, and the specific motivation listed has since been fixed. The policy in the standard library for now will likely be "do not mention bounds in datatype definitions unless the compiler requires it", but this is perhaps subject to change over time. Regardless, however, existing APIs cannot be altered due to backwards compatibility concerns, so I'm going to close this RFC now.

Thanks regardless though for it @Kimundi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants