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

Arbitrary : negative impl of Arbitrary on never type (!) #332

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthew-russo
Copy link
Member

fixes #331

@@ -56,6 +56,8 @@ atomic64bit = []
# passed (see https://github.com/proptest-rs/proptest/issues/124).
break-dead-code = []

never-arbitrary = []
Copy link
Member Author

Choose a reason for hiding this comment

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

Wasn't sure whether or not to just roll this in to the unstable feature flag. Opinions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a reasonable idea for stability as this is a fairly new area in the rust lang - (see my message below about coherence rules)

@@ -56,6 +56,8 @@ atomic64bit = []
# passed (see https://github.com/proptest-rs/proptest/issues/124).
break-dead-code = []

never-arbitrary = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a reasonable idea for stability as this is a fairly new area in the rust lang - (see my message below about coherence rules)

}
}

impl Arbitrary for Wrapper<!> {
Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunately, due to coherence rules, this won't be possible to implement if Wrapper is defined in another crate as the rust trait coherence rules considers it to be a conflicting implementation.

Without this PR the error looks like this

19 | impl<T: Arbitrary> Arbitrary for Wrapper<T> {
   | ------------------------------------------- first implementation here
...
28 | impl Arbitrary for Wrapper<!> {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `Wrapper<!>`
   |
   = note: upstream crates may add a new impl of
trait `_IMPL_ARBITRARY_FOR_Wrapper::_proptest::arbitrary::Arbitrary` for type `!` in future versions

With this PR it still doesn't work, and the error looks like this

error[E0119]: conflicting implementations of trait `_IMPL_ARBITRARY_FOR_Wrapper::_proptest::arbitrary::Arbitrary` for type `Wrapper<!>`
  --> src/main.rs:13:10
   |
13 | #[derive(proptest_derive::Arbitrary, Clone, Debug)]
   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `Wrapper<!>`
...
32 | impl Arbitrary for Wrapper<!> {
   | ----------------------------- first implementation here
   |
   = note: this error originates in the derive macro
`proptest_derive::Arbitrary`
(in Nightly builds, run with -Z macro-backtrace for more info)

There is an initiative inside of rustc to integrate negative impls into trait coherence rules, but until then this will be insufficient.
rust-lang/negative-impls-initiative#1

I originally thought this would be a strict improvement, but actually judging from the error messages (IMO they end up regressing) - I think this is probably premature and should wait until the negative impls initiative furthers.

Perhaps hiding it behind another feature flag like you did is a good way to mitigate this concern!

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.

Handling Arbitrary on structs containing the Never type (!)
2 participants