Implement RFC 1268 #40097

sgrif · 2017-02-25T21:20:39Z

This patch allows overlap to occur between any two impls of a trait for
traits which have no associated items.

Several compile-fail tests around coherence had to be changed to add at
least one item to the trait they test against.

rust-highfive · 2017-02-25T21:20:52Z

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

The change in #709 had the side effect of re-introducing #104. With the design that we have right now, nullability isn't propagating upwards. This puts the issue of "expressions aren't validating that the type of its arguments haven't become nullable, and thus nulls are slipping in where they shouldn't be" at odds with "we can't use complex expressions in filters for joins because the SQL type changed". This semi-resolves the issue by restricting when we care about nullability. Ultimately the only time it really matters is when we're selecting data, as we need to enforce that the result goes into an `Option`. For places where we don't see the bytes in Rust (filter, order, etc), `NULL` is effectively `false`. This change goes back to fully fixing #104, but brings back a small piece of #621. I've changed everything that is a composite expression to only be selectable if the SQL type hasn't changed. This means that you won't be able to do things like `users.left_outer_join(posts).select(posts::id + 1)`, but you will be able to use whatever you want in `filter`. This change is also to support what I think will fix the root of all these issues. The design of "Here's the SQL type on this query source" is just fundamentally not what we need. There is only one case where the type changes, and that is to become null when it is on the right side of a left join, the left side of a right join, or either side of a full join. One of the changes that #709 made was to require that you explicitly call `.nullable()` on a tuple if you wanted to get `Option<(i32, String)>` instead of `(Option<i32>, Option<String>)`. This has worked out fine, and isn't a major ergonomic pain. The common case is just to use the default select clause anyway. So I want to go further down this path. The longer term plan is to remove `SqlTypeForSelect` entirely, and *not* implement `SelectableExpression` for columns on the nullable side of a join. We will then provide these two blanket impls: ```rust impl<Left, Right, T> SelectableExpression<LeftOuterJoin<Left, Right>> for Nullable<T> where T: SelectableExpression<Right>, {} impl<Left, Right, Head, Tail> SelectableExpression<LeftOuterJoin<Left, Right>> for Nullable<Cons<Head, Tail>> where Nullable<Head>: SelectableExpression<LeftOuterJoin<Left, Right>>, Nullable<Tail>: SelectableExpression<LeftOuterJoin<Left, Right>>, {} ``` (Note: Those impls overlap. Providing them as blanket impls would require rust-lang/rust#40097. Providing them as non-blanket impls would require us to mark `Nullable` and possibly `Cons` as `#[fundamental]`) The end result will be that nullability naturally propagates as we want it to. Given `sql_function!(lower, lower_t, (x: Text) -> Text)`, doing `select(lower(posts::name).nullable())` will work. `lower(posts::name)` will fail because `posts::name` doesn't impl `SelectableExpression`. `lower(posts::name.nullable())` will fail because while `SelectableExpression` will be met, the SQL type of the argument isn't what's expected. Putting `.nullable` at the very top level naturally follows SQL's semantics here.

oli-obk · 2017-02-27T08:27:02Z

src/librustc/ty/mod.rs

@@ -2222,6 +2222,16 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
            || self.sess.cstore.impl_trait_ref(self.global_tcx(), id))
    }

+    /// Returns true if the impl is positive and is for a triat which contains


typo: trait

oli-obk · 2017-02-27T08:29:38Z

src/test/run-pass/overlap-permitted-for-marker-traits.rs

+}
+
+fn main() {
+    assert_eq!(1, foo(1));


Shouldn't you also test a type which is Copy but not Eq?

and yeah it seems like it'd be better to have Clone || Display impls or something, since Vec<T> doesn't impl Display (but is Clone)

petrochenkov · 2017-02-27T09:31:22Z

This needs a feature gate at least, since this may be rolled back (rust-lang/rfcs#1268 (comment)):

Also, the implementation concerns are still very real -- it seems however that we expect to address many of the same questions as part of the specialization work, so hopefully we will find a nice solution. (If not, we'll rollback the RFC.)

sgrif · 2017-03-03T14:12:30Z

@petrochenkov I'm not entirely sure how to add a feature gate for something that has no explicit syntax or direct usage. Could you give me an example of when that was done in the past so I can see how it was implemented?

petrochenkov · 2017-03-03T15:22:41Z

@sgrif
Here's an example of a feature gate reported lately.
If two impls overlap then an error is reported, right? The feature error can be added somewhere in the same place and suppress the normal error - "error: conflicting implementation" => "conflicting implementation of empty traits are unstable".

The change in #709 had the side effect of re-introducing #104. With the design that we have right now, nullability isn't propagating upwards. This puts the issue of "expressions aren't validating that the type of its arguments haven't become nullable, and thus nulls are slipping in where they shouldn't be" at odds with "we can't use complex expressions in filters for joins because the SQL type changed". This semi-resolves the issue by restricting when we care about nullability. Ultimately the only time it really matters is when we're selecting data, as we need to enforce that the result goes into an `Option`. For places where we don't see the bytes in Rust (filter, order, etc), `NULL` is effectively `false`. This change goes back to fully fixing #104, but brings back a small piece of #621. I've changed everything that is a composite expression to only be selectable if the SQL type hasn't changed. This means that you won't be able to do things like `users.left_outer_join(posts).select(posts::id + 1)`, but you will be able to use whatever you want in `filter`. This change is also to support what I think will fix the root of all these issues. The design of "Here's the SQL type on this query source" is just fundamentally not what we need. There is only one case where the type changes, and that is to become null when it is on the right side of a left join, the left side of a right join, or either side of a full join. One of the changes that #709 made was to require that you explicitly call `.nullable()` on a tuple if you wanted to get `Option<(i32, String)>` instead of `(Option<i32>, Option<String>)`. This has worked out fine, and isn't a major ergonomic pain. The common case is just to use the default select clause anyway. So I want to go further down this path. The longer term plan is to remove `SqlTypeForSelect` entirely, and *not* implement `SelectableExpression` for columns on the nullable side of a join. We will then provide these two blanket impls: ```rust impl<Left, Right, T> SelectableExpression<LeftOuterJoin<Left, Right>> for Nullable<T> where T: SelectableExpression<Right>, {} impl<Left, Right, Head, Tail> SelectableExpression<LeftOuterJoin<Left, Right>> for Nullable<Cons<Head, Tail>> where Nullable<Head>: SelectableExpression<LeftOuterJoin<Left, Right>>, Nullable<Tail>: SelectableExpression<LeftOuterJoin<Left, Right>>, {} ``` (Note: Those impls overlap. Providing them as blanket impls would require rust-lang/rust#40097. Providing them as non-blanket impls would require us to mark `Nullable` and possibly `Cons` as `#[fundamental]`) The end result will be that nullability naturally propagates as we want it to. Given `sql_function!(lower, lower_t, (x: Text) -> Text)`, doing `select(lower(posts::name).nullable())` will work. `lower(posts::name)` will fail because `posts::name` doesn't impl `SelectableExpression`. `lower(posts::name.nullable())` will fail because while `SelectableExpression` will be met, the SQL type of the argument isn't what's expected. Putting `.nullable` at the very top level naturally follows SQL's semantics here.

nikomatsakis · 2017-03-03T21:25:24Z

@sgrif sorry, not trying to ignore you -- just been slammed. Just for some context, I'm starting work on a big refactoring of the trait system, that should support RFC 1268 out of the box (chalk, the prototype of sorts, already does). I'm...pretty sure that it will take more work than this PR, though I could be wrong, so I've been holding off on reading this PR until I have a bit of time to dedicate to thinking about it (it's really easy to take unfortunate shortcuts in this code if you're not careful).

sgrif · 2017-03-03T22:34:42Z

@nikomatsakis No worries, take your time. 😄

This removes the flawed "SQL type varies based on the query source" design in favor of "things become nullable if they're on the wrong side of an outer join". This means that columns from the right side of a left join can no longer directly be selected. `.nullable` must be manually called. This sounds like a huge deal, but that's already the case today with tuples (which is a more common case), and it hasn't caused a huge ergonomics issue. Ultimately most queries just use the default select clause. This also causes the nullability to bubble up. If you're building a compound expression that involves a column from the right side of a left join, you don't have to worry about that in the compound expression. You just have to make the whole thing nullable at the top level. This mirrors SQL's semantics quite nicely. One downside of this is that `.nullable` can no longer be used in arbitrary select clauses. Doing so is questionably useful at best, but I'd still like to bring back that support in the future. Ultimately doing so more requires rust-lang/rust#40097 or similar.

japaric · 2017-03-08T01:31:44Z

(sorry for the noise)

I just ran into the need for this today. Checked out this branch and worked flawlessly so I'm looking forward to this PR landing (it would let me write a single overloaded method instead of N differently named methods and N^2 very similar implementations). Thanks for working on this, @sgrif. 😄

nikomatsakis

OK, I'm sorry for the big delay. I read into this code. This definitely works out simpler than I had thought, in large part because specialization paved the way, which is cool. Kudos @sgrif.

This is leveraging the specialization pathway, where we defer "confirming" a candidate unless we know for sure that it will successfully apply. I generally plan to refactor the trait code to work like this overall, so leaning more on this pathway seems fine to me.

That said, I have a few suggestions for how we could clean things up (see comments below).

Also, we do need a feature-gate.

nikomatsakis · 2017-03-16T20:05:33Z

src/librustc/traits/specialize/mod.rs

+    if tcx.impl_always_allowed_to_overlap(impl1_def_id)
+        && tcx.impl_always_allowed_to_overlap(impl2_def_id) {
+        return true;
+    }


I think I would prefer not to modify specializes to return true. It makes the specialization graph "cyclic". I think we can make a more direct modification to select.rs so that where it calls specialized, it would do this instead:

return impls_are_allowed_to_ovelap(other_def, victim_def) || traits::specializes(tcx, other_def, victim_def);

This seems to more directly express what is going on.

nikomatsakis · 2017-03-16T20:05:36Z

src/librustc/ty/mod.rs

@@ -2222,6 +2222,16 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
            || self.sess.cstore.impl_trait_ref(self.global_tcx(), id))
    }

+    /// Returns true if the impl is positive and is for a trait which contains
+    /// no items
+    pub fn impl_always_allowed_to_overlap(self, def_id: DefId) -> bool {


I think we should change this to take two impls, and call it impls_are_allowed_to_overlap(impl1, impl2). For one thing, we always call it in pairs, but for another we probably want to permit overlap so long as the "polarity" of both impls is the same (that is, I see no reason you can't have overlapping negative impls as well, but indeed you do not want overlapping positive and negative impls).

nikomatsakis · 2017-03-16T20:24:35Z

src/librustc/traits/specialize/specialization_graph.rs

@@ -113,6 +113,11 @@ impl<'a, 'gcx, 'tcx> Children {
                                                        possible_sibling,
                                                        impl_def_id);
                if let Some(impl_header) = overlap {
+                    if tcx.impl_always_allowed_to_overlap(impl_def_id)
+                        && tcx.impl_always_allowed_to_overlap(possible_sibling) {
+                        return Ok((true, true));


I guess it doesn't matter, but I think Ok((false, false)) would be a better return. It'll do the same thing either way though.

sgrif · 2017-03-16T20:33:38Z

Excellent. I'll add a feature gate and implement the suggestions tomorrow

nikomatsakis · 2017-03-17T00:22:27Z

@sgrif it'd also be good to have some more tests. One thing that came to mind is tests around the interaction with negative impls (i.e., that a pos + neg still cannot overlap). Another would be some tests that cover specialization cases (i.e., impl<T> Marker for T and impl<T> Marker for Vec<T>, after enabling both feature gates), just to make sure that the two systems for permitting overlap don't fight with one another.

sgrif · 2017-03-17T18:23:23Z

Updated and added a feature gate. I've left the error message unchanged when the feature is not enabled, as it was just easier and I recall that occurring for similar feature gates in the past without issue.

bors · 2017-03-19T20:48:05Z

☔ The latest upstream changes (presumably #40651) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2017-03-20T18:59:58Z

src/test/compile-fail/feature-gate-overlapping_marker_traits.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+trait MyMarker {}


can you add a comment explaining that this test is targeting the absnce of a feature-gate?

Also, I think I'd prefer to have the T: Eq vs T: Debug example (or whatever) rather than this one, since this also works with specialization.

(well, I guess the comment isn't that imp't because of the filename...)

nikomatsakis · 2017-03-20T19:38:15Z

src/test/run-pass/overlap-permitted-for-marker-traits.rs

+impl<T: Copy> MyMarker for T {}
+impl<T: Eq> MyMarker for T {}
+
+struct MyStruct;


what is the role of MyStruct here? oh, to show that there can be two !Send impls? can we break that into a separate test, and add some comments describing what it's trying to test

nikomatsakis

looking good; I left a few further requests for modified tests

nikomatsakis · 2017-03-29T12:48:45Z

ping @sgrif -- you still have time to make those last few changes? If not, I could take a stab at it, pretty minor stuff. :)

sgrif · 2017-03-29T16:06:25Z

I should have time later this week. Been busy with the baby the past several days. If you don't mind making the changes, that'd be great.

nikomatsakis · 2017-04-03T15:52:00Z

@bors r+

bors · 2017-04-03T15:52:01Z

📌 Commit 665f852 has been approved by nikomatsakis

bors · 2017-04-03T18:18:20Z

⌛ Testing commit 665f852 with merge e6fc718...

bors · 2017-04-03T18:30:12Z

💔 Test failed - status-travis

frewsxcv · 2017-04-03T20:30:59Z

@bors retry

#40474

bors · 2017-04-04T20:18:47Z

⌛ Testing commit 665f852 with merge 7d09c65...

…matsakis Implement RFC 1268 This patch allows overlap to occur between any two impls of a trait for traits which have no associated items. Several compile-fail tests around coherence had to be changed to add at least one item to the trait they test against. Ref rust-lang#29864

frewsxcv · 2017-04-04T20:19:50Z

@bors retry

frewsxcv · 2017-04-04T20:21:43Z

This fails the tidy checker:

[00:02:39] Unstable feature 'overlapping_marker_traits' needs to have a section in The Unstable Book

You'll need to add a section for the unstable feature in src/doc/unstable-book/. If you need help with that let me know

@bors r-

bors · 2017-04-07T07:26:31Z

☔ The latest upstream changes (presumably #39987) made this pull request unmergeable. Please resolve the merge conflicts.

TimNN · 2017-04-09T11:52:06Z

For some reason, bors think's it's testing this...

@bors r- retry

frewsxcv · 2017-04-14T16:28:04Z

reopening to try to fix bors which is stuck on this PR.

aturon · 2017-04-14T19:09:04Z

I'm going to close this again...

aturon · 2017-04-14T19:10:35Z

OK, looks like every time this is re-opened, homu gets freshly confused.

@sgrif, sorry for the annoyance here, but can you open a fresh version of this PR?

frewsxcv · 2017-04-14T19:13:27Z

The only thing that's missing for this PR is the unstable docs stuff, which I'm willing to take on. So unless I hear from @sgrif, I can open a new PR with their commits and a new commit for the Unstable Book changes.

aturon · 2017-04-14T19:20:16Z

@frewsxcv sounds great, thanks!

frewsxcv · 2017-04-15T00:40:13Z

Rebased in #41309.

…ikomatsakis Implement RFC 1268. Rebased version of rust-lang#40097. Tracking issue: rust-lang#29864.

rust-highfive assigned nikomatsakis Feb 25, 2017

sgrif force-pushed the sg-implement-rfc-1268 branch 2 times, most recently from 8dbb775 to eb6af2d Compare February 25, 2017 23:09

sgrif mentioned this pull request Feb 26, 2017

Split SelectableExpression into two traits diesel-rs/diesel#764

Merged

oli-obk requested changes Feb 27, 2017

View reviewed changes

sgrif mentioned this pull request Mar 4, 2017

Remove the associated type from SelectableExpression diesel-rs/diesel#774

Merged

nikomatsakis requested changes Mar 16, 2017

View reviewed changes

sgrif force-pushed the sg-implement-rfc-1268 branch from 88d53f0 to ba7510b Compare March 17, 2017 18:23

sgrif force-pushed the sg-implement-rfc-1268 branch from 794d4d5 to 8d8f5c6 Compare March 19, 2017 23:00

nikomatsakis reviewed Mar 20, 2017

View reviewed changes

nikomatsakis requested changes Mar 20, 2017

View reviewed changes

nikomatsakis force-pushed the sg-implement-rfc-1268 branch from 8b20c25 to 665f852 Compare April 3, 2017 15:51

frewsxcv mentioned this pull request Apr 4, 2017

Rollup of 17 pull requests #41068

Closed

Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017

frewsxcv closed this Apr 14, 2017

frewsxcv reopened this Apr 14, 2017

aturon closed this Apr 14, 2017

aturon reopened this Apr 14, 2017

aturon closed this Apr 14, 2017

frewsxcv mentioned this pull request Apr 15, 2017

Implement RFC 1268 "Allow overlapping implementations for marker traits" #41309

Merged

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 15, 2017

Rollup merge of rust-lang#41309 - frewsxcv:sg-implement-rfc-1268, r=n…

3adcd1c

…ikomatsakis Implement RFC 1268. Rebased version of rust-lang#40097. Tracking issue: rust-lang#29864.

Implement RFC 1268 #40097

Implement RFC 1268 #40097

Conversation

sgrif commented Feb 25, 2017

rust-highfive commented Feb 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Feb 27, 2017

sgrif commented Mar 3, 2017

petrochenkov commented Mar 3, 2017

nikomatsakis commented Mar 3, 2017

sgrif commented Mar 3, 2017

japaric commented Mar 8, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgrif commented Mar 16, 2017

nikomatsakis commented Mar 17, 2017

sgrif commented Mar 17, 2017

bors commented Mar 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis commented Mar 29, 2017

sgrif commented Mar 29, 2017 • edited Loading

nikomatsakis commented Apr 3, 2017

bors commented Apr 3, 2017

bors commented Apr 3, 2017

bors commented Apr 3, 2017

frewsxcv commented Apr 3, 2017

bors commented Apr 4, 2017

frewsxcv commented Apr 4, 2017

frewsxcv commented Apr 4, 2017

bors commented Apr 7, 2017

TimNN commented Apr 9, 2017

frewsxcv commented Apr 14, 2017

aturon commented Apr 14, 2017

aturon commented Apr 14, 2017

frewsxcv commented Apr 14, 2017

aturon commented Apr 14, 2017

frewsxcv commented Apr 15, 2017

sgrif commented Mar 29, 2017 •

edited

Loading