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 support for default trait implementations #21689

Merged
merged 32 commits into from
Feb 24, 2015

Conversation

flaper87
Copy link
Contributor

This is one more step towards completing #13231

This series of commits add support for default trait implementations. The changes in this PR don't break existing code and they are expected to preserve the existing behavior in the compiler as far as built-in bounds checks go.

The PR adds negative implementations of Send/Sync for some types and it removes the special cases for Send/Sync during the trait obligations checks. That is, it now fully relies on the traits check rather than lang items.

Once this patch lands and a new snapshot is created, it'll be possible to add default impls for Send and Sync and remove entirely the use of BuiltinBound::{BoundSend,BoundSync} for positive implementations as well.

This PR also removes the restriction on negative implementations. That is, it is now possible to add negative implementations for traits other than Send/Sync

@flaper87
Copy link
Contributor Author

r? @nikomatsakis

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@nagisa
Copy link
Member

nagisa commented Jan 27, 2015

Since void structs are warned to be changed into struct Struct;, it would make sense to go for impl !Trait for Struct; syntax rather than impl !Trait for Struct {}.

@flaper87
Copy link
Contributor Author

@nagisa if we allow that, we should also allow trait Test;, impl Trait for Struct; and impl Trait for ..; otherwise it'd be inconsistent.

@@ -126,6 +126,7 @@ enum Family {
TupleVariant, // v
StructVariant, // V
Impl, // i
DefTrait, // d
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call this DefaultedTrait

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, let's call it DefaultImpl or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm terrible with naming things. If we rename this to DefaultImpl, we should probably also rename the Item_ as DefaultImpl instead of DefaultTrait

@flaper87 flaper87 force-pushed the oibit-send-and-friends branch 5 times, most recently from 0f05cde to 5c78f48 Compare February 8, 2015 10:44
@nikomatsakis
Copy link
Contributor

This is looking really good. Here is a checklist of things I noticed while reading (it's hard to leave comments on individual commits because I was reading the collapsed diff):

  • We need to get the coherence rules right. In particular, for a defaulted trait, you should only be able to do user impls on structs/enums. This is basically a generalization of the rules in librustc_typeck/coherence/impls.rs to apply to defaulted traits as well as builtin bounds.
  • I'd like to see more systematic testing, perhaps with a more regular naming convention. That said, I think your tests are about as good as what we currently have anyway. It just seems like there are a lot of cases in constituent_types that we should cover.
  • I wonder if we can make send/sync go through the defaulted code paths, even though we can't (yet) add the actual impls we want. That would help us gain confidence in those code paths. We can discuss on IRC what I mean.
  • I think the supertrait code in constituent_types is wrong, we shouldn't need to elaborate the supertraits. Defaulted traits do not apply to object types, in fact, since they can close over anything (including some type that opted out of the defaulted trait).

UPDATED the supertrait bullet point

@nikomatsakis
Copy link
Contributor

Some further comments from IRC:

[03:57:43] <nmatsakis> if you have a defaulted trait
[03:57:48] <nmatsakis> right now you just unconditionally add an entry
[03:57:49] <nmatsakis> that's not quite right
[03:58:08] <nmatsakis> I think we want to match on the (shallowly resolved) self-type
[03:58:16] <nmatsakis> if it is a tyvar, we want to set the ambigiuous flag and bail out
[03:58:40] <nmatsakis> if it is an object, we do not add a default candidate (but see later)
[03:58:45] <nmatsakis> if it is anything else, we add a default candidate
[03:59:12] <nmatsakis> for objects, there exists already assemble_candidates_from_object_ty()
[03:59:18] <nmatsakis> I think that should be extended to cover the builtin bounds case
[03:59:37] <nmatsakis> eventually object types should be arbitrary sums
[03:59:41] <nmatsakis> Foo+Bar whre Foo and Bar are traits
[04:00:05] <nmatsakis> but that seems like the most logical place to put it
[04:01:09] <nmatsakis> we'd have to modify the builtin bounds code then to remove that logic from `builtin_bound` -- which should just return ParameterBuiltin for object types, I think
[04:01:18] <nmatsakis> (I'm going to copy this to the PR for future reference)

@flaper87 flaper87 force-pushed the oibit-send-and-friends branch 6 times, most recently from 892c4fd to a163c23 Compare February 19, 2015 07:56
@bors
Copy link
Contributor

bors commented Feb 22, 2015

⌛ Testing commit 3dcc631 with merge d15a06a...

@bors
Copy link
Contributor

bors commented Feb 22, 2015

💔 Test failed - auto-win-64-nopt-t

@Manishearth
Copy link
Member

@bors : retry

@bors
Copy link
Contributor

bors commented Feb 23, 2015

⌛ Testing commit 3dcc631 with merge 812dd9f...

@bors
Copy link
Contributor

bors commented Feb 23, 2015

💔 Test failed - auto-linux-32-nopt-t

@bors
Copy link
Contributor

bors commented Feb 23, 2015

💔 Test failed - auto-linux-32-opt

@bors
Copy link
Contributor

bors commented Feb 23, 2015

💔 Test failed - auto-win-64-opt

@bors
Copy link
Contributor

bors commented Feb 23, 2015

💔 Test failed - auto-win-32-opt

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Feb 23, 2015

⌛ Testing commit 3dcc631 with merge ca9b37e...

@bors
Copy link
Contributor

bors commented Feb 23, 2015

💔 Test failed - auto-mac-64-nopt-t

@nikomatsakis
Copy link
Contributor

@bors retry

curse you!

@bors
Copy link
Contributor

bors commented Feb 23, 2015

⌛ Testing commit 3dcc631 with merge dc6e06e...

bors added a commit that referenced this pull request Feb 23, 2015
This is one more step towards completing #13231

This series of commits add support for default trait implementations. The changes in this PR don't break existing code and they are expected to preserve the existing behavior in the compiler as far as built-in bounds checks go.

The PR adds negative implementations of `Send`/`Sync` for some types and it removes the special cases for `Send`/`Sync` during the trait obligations checks. That is, it now fully relies on the traits check rather than lang items.

Once this patch lands and a new snapshot is created, it'll be possible to add default impls for `Send` and `Sync` and remove entirely the use of `BuiltinBound::{BoundSend,BoundSync}` for positive implementations as well.

This PR also removes the restriction on negative implementations. That is, it is now possible to add negative implementations for traits other than `Send`/`Sync`
@bors
Copy link
Contributor

bors commented Feb 23, 2015

💔 Test failed - auto-win-32-nopt-t

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Feb 24, 2015

⌛ Testing commit 3dcc631 with merge 2890508...

bors added a commit that referenced this pull request Feb 24, 2015
This is one more step towards completing #13231

This series of commits add support for default trait implementations. The changes in this PR don't break existing code and they are expected to preserve the existing behavior in the compiler as far as built-in bounds checks go.

The PR adds negative implementations of `Send`/`Sync` for some types and it removes the special cases for `Send`/`Sync` during the trait obligations checks. That is, it now fully relies on the traits check rather than lang items.

Once this patch lands and a new snapshot is created, it'll be possible to add default impls for `Send` and `Sync` and remove entirely the use of `BuiltinBound::{BoundSend,BoundSync}` for positive implementations as well.

This PR also removes the restriction on negative implementations. That is, it is now possible to add negative implementations for traits other than `Send`/`Sync`
@eddyb
Copy link
Member

eddyb commented Feb 24, 2015

This... renamed def::DefTrait to def::DefaultImpl and nobody noticed?!

@eddyb
Copy link
Member

eddyb commented Feb 24, 2015

I am not killing the build because the rest of this patch is important, but someone will have to revert that change.

@bors bors merged commit 3dcc631 into rust-lang:master Feb 24, 2015
@flaper87
Copy link
Contributor Author

@eddyb that change was obviously unintentional and no-one noticed because the patch is quite big and the rename came in the middle. In other words, assume good faith ;)

I'll revert that change.

@eddyb
Copy link
Member

eddyb commented Feb 24, 2015

@flaper87 I realized it was sed gone wrong. My UFCS patch is rebased on top of a revert, otherwise rebasing would've been much more painful. I guess I noticed only because I was looking for potential conflicts.

@flaper87
Copy link
Contributor Author

@eddyb oh awesome, thanks for fixing it, you saved me some revert time :D

@nikomatsakis
Copy link
Contributor

Hey, it landed! 👯

@brson
Copy link
Contributor

brson commented Mar 1, 2015

🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants