[r+] Add rules for negative implementations #20972

flaper87 · 2015-01-11T23:40:17Z

This PR adds rules for negative implementations. It follows pretty much what the RFC says with 1 main difference:

Instead of positive implementations override negative implementations, this have been implemented in a way that a negative implementation of Trait for T will overlap with a positive implementation, causing a coherence error.

@nikomatsakis r?

cc #13231

[breaking-change]

rust-highfive · 2015-01-11T23:40:21Z

r? @pcwalton

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

flaper87 · 2015-01-11T23:44:34Z

(running stage2 checks locally, stage1 rpass/cfail passed)

UPDATE:

passed :)

bluss · 2015-01-11T23:45:09Z

src/liballoc/rc.rs

+impl<T> !marker::Send for Rc<T> {}
+
+#[cfg(not(stage0))] // NOTE remove cfg after next snapshot
+impl<T> !marker::Sync for Rc<T> {}


Isn't this redundant with raw pointers no longer being Send?

It's redundant unless, for some reason, NonZero ends up implementing Send. It doesn't now and I don't think it will so we could probably remove it. Although, the extra negative impl is harmless.

I think it's good practice though to be explicit; it serves as documentation. It'd be good to add some comments to the impls though saying why Rc is not sendable/sync. something like

/// `Rc` instances cannot be sent to other threads because they contain a shared reference count which is modified without atomic instructions. impl<T> !marker::Send for Rc<T> { } /// `Rc` instances cannot be sent to other threads because they contain a shared reference count which is modified without atomic instructions. impl<T> !marker::Sync for Rc<T> { }

An alternative, if we had such a thing, would be the equivalent of compile-fail unit tests though.

nikomatsakis · 2015-01-13T01:16:03Z

src/librustc_typeck/coherence/overlap.rs

+                if let Some(neg_impls) = self.tcx.trait_negative_impls.borrow().get(&k) {
+                    impls.push_all(neg_impls.borrow().as_slice());
+                }
+                (k, impls)


This seems a bit fishy. If there are only negative impls, it seems like the result will be the empty vec, but I think we just want the pos + the neg impls.

If there are no positive impls - trait_impls is empty - the result would be an empty vec, yes. Although, trait_impls will never be empty, afaict.

(by merging trait_impls and trait_negative_impls this won't be needed anymore)

nikomatsakis · 2015-01-13T01:33:04Z

So overall this looks really good. I'm happy about how small the diff became when we adopted the "unify pos and neg impl" approach. I feel like there should be more tests, but I'm not entirely sure what they ought to be. Maybe we can brainstorm some tricky scenarios. Here are some thoughts:

I'd like to see another slightly less trivial coherence overlap (e.g., unsafe impl<T> Send for MyType<T>; impl !Send for MyType<int>)
I'd like to see a test for coherence orphan rules (e.g., impl<T> !Send for Vec<T>)
I'd like to see a test where the "!send" thing is inside something else. Like, if Foo is !Send, test whether a struct Bar that wraps a Foo is send:

struct Foo;
struct Bar(Foo);
impl !Send for Foo;
fn is_send<T:Send>();
fn main() {
    is_send::<Bar>(); //~ ERROR ...
    is_send::<(int, Foo)>(); //~ ERROR ...
    is_send::<Box<Foo>>(); //~ ERROR ...
    is_send::<Box<Bar>>(); //~ ERROR ...

}

I put the one minor nit about keeping the pos/neg impls in one list instead of two -- do you think there is a good reason to keep them separated?

flaper87 · 2015-01-13T11:55:28Z

@nikomatsakis thanks for the review :)

I believe traits-negative-impls.rs covers part of what you suggested in the third point but I'll extend it to cover cases with Box and nested types. I'll also add cases for 1 and 2

nikomatsakis · 2015-01-15T10:48:02Z

r+

@nikomatsakis

…tsakis This PR adds rules for negative implementations. It follows pretty much what the [RFC](https://github.com/rust-lang/rfcs/blob/master/text/0019-opt-in-builtin-traits.md) says with 1 main difference: Positive implementations don't override negative implementations @nikomatsakis r? cc #13231 [breaking-change]

pythonesque · 2015-01-16T04:45:19Z

Why don't positive implementations override negative implementations? I think this is pretty important for some uses of unsafe traits, no?

flaper87 · 2015-01-16T07:23:14Z

@pythonesque I've updated the PR description with something that reflects the current implementation. The previous message corresponded to an older version of this implementation.

@nikomatsakis

…tsakis This PR adds rules for negative implementations. It follows pretty much what the [RFC](https://github.com/rust-lang/rfcs/blob/master/text/0019-opt-in-builtin-traits.md) says with 1 main difference: Instead of positive implementations override negative implementations, this have been implemented in a way that a negative implementation of `Trait` for `T` will overlap with a positive implementation, causing a coherence error. @nikomatsakis r? cc #13231 [breaking-change]

nikomatsakis · 2015-01-16T16:23:00Z

On Thu, Jan 15, 2015 at 08:45:23PM -0800, Joshua Yanovski wrote:

Why don't positive implementations override negative implementations? I think this is pretty important for some uses of unsafe traits, no?

It wasn't clear where this would be important. Do you have a use case?
Overall the code and rules worked out simpler this way, and it is
backwards compatible of course to extend later.

pythonesque · 2015-01-16T17:28:53Z

@nikomatsakis Maybe you are right and it doesn't, I think I had an example in mind but I can't remember what it was at the moment. I'll let you know if I remember.

ghost · 2015-01-18T18:38:08Z

It wasn't clear where this would be important. Do you have a use case?

@nikomatsakis there are some patterns involving type-level search that can't currently be expressed in Rust as far as I can tell, but might be possible with blanket negative impls allowing positive impl overrides (sealed traits with overlapping patterns would be another way).

One use case for that is generic programming with open sums (like the join type proposal). Another one would be statically checked dimensional analysis. Fancy database interfaces could also use that pattern for a number of things.

nikomatsakis · 2015-01-20T15:49:36Z

On Sun, Jan 18, 2015 at 10:38:12AM -0800, Darin Morrison wrote:

It wasn't clear where this would be important. Do you have a use case?

@nikomatsakis there are some patterns involving type-level search that can't currently be expressed in Rust as far as I can tell, but might be possible with blanket negative impls allowing positive impl overrides (sealed traits with overlapping patterns would be another way).

Interesting, I'd like to hear more details. Nonetheless, it feels good
to have the positive and negative impls follow a consistent set of
rules. I'd like to introduce some kind of support for specialization
which might also then apply across the board and allow support for
these patterns. Regardless the current semantics are conservative in
the sense that I believe we could change them in the future without
breaking code.

ghost · 2015-01-20T21:44:41Z

Interesting, I'd like to hear more details. Nonetheless, it feels good
to have the positive and negative impls follow a consistent set of
rules. I'd like to introduce some kind of support for specialization
which might also then apply across the board and allow support for
these patterns. Regardless the current semantics are conservative in
the sense that I believe we could change them in the future without
breaking code.

@nikomatsakis If I get a chance I'll try to put together some examples. Most of the ones I can think of at the moment can be encoded as a special case of a decidable equality on types by piggybacking on the definitional equality. Once equality predicates in where clauses are implemented, that might actually be enough, assuming they can be negated and that the coherence check could use that to determine disjointness of the implementations.

flaper87 · 2015-01-20T22:35:40Z

@darinmorrison please, keep me in the loop and thanks for bringing this up! 👍

rust-highfive assigned pcwalton Jan 11, 2015

flaper87 assigned nikomatsakis and unassigned pcwalton Jan 11, 2015

bluss reviewed Jan 11, 2015
View reviewed changes

flaper87 force-pushed the oibit-send-and-friends branch 2 times, most recently from dad2440 to 7fde099 Compare January 12, 2015 11:13

nikomatsakis reviewed Jan 13, 2015
View reviewed changes

flaper87 force-pushed the oibit-send-and-friends branch from 6fd3e62 to d8b507c Compare January 13, 2015 20:02

flaper87 force-pushed the oibit-send-and-friends branch from d8b507c to d87b71d Compare January 14, 2015 09:21

nikomatsakis changed the title ~~Add rules for negative implementations~~ [r+] Add rules for negative implementations Jan 14, 2015

flaper87 added 8 commits January 16, 2015 08:18

Record negative trait_impls separatedly

683d20c

Add test for missing default impl

199e0cd

push_impls_of_trait is not needed, use map

e644ca0

Negative impls are considered safe

39fe05f

Don't assemble bound impls if candidate's ambiguous

5aab863

Check for negative impls for Send and Sync

9eec782

Remove NoSend/NoSync

388e30f

Don't use NoSend/NoSync in liballoc

bb04121

flaper87 added 9 commits January 16, 2015 08:18

Don't use NoSend/NoSync in libstd

c6ab9a6

add a run-pass test that used to fail

9d42581

Fix coherence for negative implementations

094397a

Don't use NoSend/NoSync in tests

921ba5a

Allow negative impls just for Send and Sync

038aa0e

addressed comments

aa642b3

fix latest changes fallout

59775bb

populate impls *before* clonning the impls vec

2adc8b5

Docs fallout

7cd762a

flaper87 force-pushed the oibit-send-and-friends branch from 8544522 to 7cd762a Compare January 16, 2015 07:19

fix pretty test fallout

cb85223

bors merged commit cb85223 into rust-lang:master Jan 16, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[r+] Add rules for negative implementations #20972

[r+] Add rules for negative implementations #20972

flaper87 commented Jan 11, 2015

rust-highfive commented Jan 11, 2015

flaper87 commented Jan 11, 2015

bluss Jan 11, 2015

flaper87 Jan 12, 2015

nikomatsakis Jan 12, 2015

nikomatsakis Jan 12, 2015

nikomatsakis Jan 13, 2015

flaper87 Jan 13, 2015

flaper87 Jan 13, 2015

nikomatsakis commented Jan 13, 2015

flaper87 commented Jan 13, 2015

nikomatsakis commented Jan 15, 2015

pythonesque commented Jan 16, 2015

flaper87 commented Jan 16, 2015

nikomatsakis commented Jan 16, 2015

pythonesque commented Jan 16, 2015

ghost commented Jan 18, 2015

nikomatsakis commented Jan 20, 2015

ghost commented Jan 20, 2015

flaper87 commented Jan 20, 2015

[r+] Add rules for negative implementations #20972

[r+] Add rules for negative implementations #20972

Conversation

flaper87 commented Jan 11, 2015

rust-highfive commented Jan 11, 2015

flaper87 commented Jan 11, 2015

UPDATE:

bluss Jan 11, 2015

Choose a reason for hiding this comment

flaper87 Jan 12, 2015

Choose a reason for hiding this comment

nikomatsakis Jan 12, 2015

Choose a reason for hiding this comment

nikomatsakis Jan 12, 2015

Choose a reason for hiding this comment

nikomatsakis Jan 13, 2015

Choose a reason for hiding this comment

flaper87 Jan 13, 2015

Choose a reason for hiding this comment

flaper87 Jan 13, 2015

Choose a reason for hiding this comment

nikomatsakis commented Jan 13, 2015

flaper87 commented Jan 13, 2015

nikomatsakis commented Jan 15, 2015

pythonesque commented Jan 16, 2015

flaper87 commented Jan 16, 2015

nikomatsakis commented Jan 16, 2015

pythonesque commented Jan 16, 2015

ghost commented Jan 18, 2015

nikomatsakis commented Jan 20, 2015

ghost commented Jan 20, 2015

flaper87 commented Jan 20, 2015