rust-lang / rfcs

RFCs for changes to Rust
https://rust-lang.github.io/rfcs/
Apache License 2.0
5.9k stars 1.56k forks source link

Orphan rules are stricter than we would like #1856

Open nikomatsakis opened 7 years ago

nikomatsakis commented 7 years ago

The current orphan rules (described as "covered first" in my blog post) are too restrictive in some cases. The purpose of this RFC issue is to catalog cases where the rules are tighter than we would like and to also point at possible solutions.


Scenario. Type parameters restrict our flexibility

In the discussion around the try trait in https://github.com/rust-lang/rfcs/issues/1718, we realized that the orphan rules would prohibit one from adding an impl like this:

impl<T,E> Try<Poll<T, E>> for Result<T, E>

This is sort of annoying because if there were no type parameters involved, the impl would be permitted:

impl Try<Poll<(),()>> for Result<(),()> // OK

This roughly corresponds to the impl<T> BorrowFrom<Rc<T>> for T example from the Little Orphan Impls blog post. As that blog post describes, this is an artifact of introducing ordering into the algorithm; the post describes various alternative rules, notably the "covered rule", that would permit both of these impls, but at the cost of other kinds of rules.


Scenario. Wanting to implement a trait where the output type is not in current crate.

@Manishearth describes wanting to add an impl into libstd like so:

impl Add<str> for str {
    type Output = String;
    ...
}

By rights, this impl should live in libcore, since all the input types are in libcore, but the output type is defined in libstd, so of course it cannot.


Scenario. Optionally derive traits if desired by others, but without adding a required dependency.

A common example is that my crate foo defines a type Foo which can be serialized (e.g., using the serde crate). However, I do not want to add serde as a required dependency of foo. Currently, best practice is to define a Cargo.toml feature (e.g., with_serde) that adds a dependency on serde and includes the relevant impls. But this is a lot of manual work. Another example is how the rayon crate exports the ParallelIterator trait; other crates, like ndarray, might want to implement those, but without forcing a dependency on rayon on others

Proposals targeting this scenario:

This scenario was also described in https://github.com/rust-lang/rfcs/issues/1553.

Other scenarios?

Please leave comments and we can migrate those descriptions into this header.

withoutboats commented 7 years ago

The solution I like to the third scenario is to enable a greater variety of blanket impls through specialization & mutual exclusion.

For example, if there is a Sequence type in std serde had an impl<T> Serialize for T where T: Sequence, and Foo is a sequential type which implements Sequence, it now also implements Serialize.

Traits like Sequence can form a "pivot" between two cousin libraries.

This has pretty bad limitations when it comes to things like ndarray and rayon because the high performance expectation probably necessitates some sort of API access that wouldn't be contained in that ancestor trait (possibly even some unsafe code).

oli-obk commented 7 years ago

impl<T> Serialize for T where T: Sequence

this doesn't work, because we'd also want to have

impl<T> Serialize for T where T: Map

We already ran into this when we tried to

impl<I, T> Serialize for I where I: Iterator<Item = T>, T: Serialize
withoutboats commented 7 years ago

@oli-obk It does work with mutual exclusion, by making it incoherent for a single type to implement both Map and Sequence. That's exactly the feature I'm proposing. :-)

Ericson2314 commented 7 years ago

I've mused a kind of lattice system where if one uses crate foo with some type, and another crate bar with this trait, you must also use another (the "least upper bound" of foo and bar) containing orphan impls involving foo's type and bar's trait.

The motivation here is sort of a collaborative look at the expression problem: It's sort of arbitrary to say whether the trait or type should own the impls when its a joint endeavor. The traditional approach to the expression problem was about allowing everyone to work in isolation, which is what made it hard/impossible to resolve, here the idea is since the author of the type and the trait both have a stake, they both should be able to write the impl. Ideally this would be coupled with a jointly-owned crate on crates.io they both could publish.

I don't have a detailed design or informal argument that this is sound, but as this has long been my rough thinking on the problem from a software engineering perspective, and I figured I should share it.


For the String case (outputs not inputs more broadly), core should probably say "I defer to a crate called collections" giving it a non-orphan exception, along with maybe an input signature the "deferred impl" must obey (for purposes of core being able to define other blankets).

Manishearth commented 7 years ago

integrating with cargo workspaces, by @nikomatsakis

I'm in favor of a boundary that isn't a crate boundary and contains multiple crates but is logically the same "project", where a different set of rules apply etc. I have never really pushed for this but I have often talked about separating the notion of a package from the notion of a crate.

However, this may only further muddle our already-confusing crate/mod system.

bluss commented 7 years ago

I'd like such a wider boundary, a way for a parent crate to delegate to other crates, so that they are permitted to implement traits for types in the parent crate “in its stead”.

Something like this:

[package] name = "ndarray" trait_delegate = ["ndarray-serde"]

Now ndarray-serde will depend on ndarray and is allowed to define foreign traits (serde::Serialize in this case) for the types in ndarray.

strega-nil commented 7 years ago

@bluss

I'd like such a wider boundary, a way for a parent crate to delegate to other crates, so that they are permitted to implement traits for types in the parent crate “in its stead”.

I would rather have a way for a child crate, of two parent crates, to define interop code. This means that ndarray doesn't have to know all of the crates that will be interopped with it; there's just an interop crate. This crate would have metadata, saying that it's for interoperability between two crates, and may be decided upon by the final consumer, or any link up the chain. For example, say I want to interop ndarray and serde with the crate ndarray-serde. ndarray-serde would look something like:

[package]
name = "ndarray-serde"

[interop]
ndarray = "0.N"
serde = "0.M" # note, these must be the versions in the final binary, and imply dependencies
[dependencies]
ndarray = "0.N"
serde = "0.M"
ndarray-serde = "0.L"

that was a lot of "interop"s.

Manishearth commented 7 years ago

FWIW the conditional features rfc eases the interop use case (but doesn't solve it completely)

withoutboats commented 7 years ago

@Manishearth and I talked about orphan issues tonight, and he gave a good example from servo, which I admit I may have badly misunderstood. But based on my understanding, I discovered a pattern for 'smuggling' code for foreign types. There are some restrictions on this pattern, but it seems quite powerful.

An example is FFI conversion. Maybe you have a single crate which defines all of your FFI types (possibly in an automatically generated fashion), and its shared by many other crates in your project. So you want all your FFI types to implement from/into their real types, and for your real types to implement from/into your FFI types. This runs into orphan rules because your FFI library doesn't know about the real types, because its a shared dependency of all the crates containing the real types.

Here's a way of defining those traits (and a 'smuggler' impl) which totally compiles and works (and it blows my mind):

trait Real: Sized {
    type Ffi: Ffi<Self>;
    fn from_ffi(source: Self::Ffi) -> Self;
    fn into_ffi(self) -> Self::Ffi;
}

trait Ffi<T>: Sized {
    fn into_real(self) -> T;
    fn from_real(real: T) -> Self;
}

impl<T> Ffi<T> for T::Ffi where T: Real {
    fn into_real(self) -> T {
        T::from_ffi(self)
    }

    fn from_real(real: T) -> T::Ffi {
        real.into_ffi()
    }
}

You can implement the Real trait for all the real types locally where they live, and declare their FFI type to be the foreign type from the FFI library, and that FFI type magically gets the Ffi trait implemented for it.

I was doubtful that this could be coherent, but because Ffi is parameterized over its Real, implementing it for an associated type just works!

You can actually smuggle arbitrary code (not only conversions) into the FFI crate by just having it delegate to a function on the Real trait.

glaebhoerl commented 7 years ago

@Ericson2314

I've mused a kind of lattice system where if you use a crate with this type and another with this crate, you must also use another (their "least upper bound") with the impl.

Could you please rephrase this, or maybe with example code? I confess I don't understand at all what each "this" and "another" is referring to.

bluss commented 7 years ago

The automatic features RFC (did you mean this one @Manishearth?) seems like it doesn't solve any of the hard issues around either version coupling or orphan rules. It is like a convenience around what we can already achieve, while we want something that really bends the current rules a bit.

Manishearth commented 7 years ago

did you mean this one @Manishearth?

yep. used to be called "conditional dependencies", now is "automatic features", mixed the names.

seems like it doesn't solve any of the hard issues around either version coupling or orphan rules.

Yeah, it doesn't, but it smoothens some of the more common annoying use cases.

Ericson2314 commented 7 years ago

@glaebhoerl sorry, did a sloppy job writing on my phone. I've gone back and cleaned up that post so it should be understandable now.

ztrash commented 7 years ago

The current orphan rules (described as "covered first" in my blog post) are too restrictive in some cases.

I wonder if you would consider a way to make binary operator traits like this work:

impl<T> Mul<Matrix<T>> for T

It would be a much cleaner way of implementing these operators for numerical programming.

burdges commented 7 years ago

Isn't there a case that binary operators should've been defined on 2-tuples? Ala impl Mul for (A,B) { .. } not impl Mul<B> for A?

I donno if this could changed in a backward compatible way with a blanket impl like impl<A,B> MulPair for (A,B) where A: Mul<B> { } or impl<A,B> Mul<B> for A where (A,B): MulPair { }.

eddyb commented 7 years ago

You've literally just decomposed the problem into figuring out the exact same predicate, but specifically for tuples. It's good to have the two use the same rules, but you don't have a simpler problem to solve.

snuk182 commented 6 years ago

Would like to have something like this allowed:

impl ForeignTrait2 for ForeignWrapper<MyStruct> { ... }

Currently results in E0117.

shepmaster commented 6 years ago

See Re-Rebalancing Coherence (2451).

axos88 commented 5 years ago

Are there any caveats I'm not considering for completely ignoring the orphan rule for BINARY crates? For example: I'd love to be able to impl Debug, or Serialize for types coming from used libraries (or even overwrite their implementation - could be useful for mock testing and debugging, but probably with a keyword such as reimpl Debug for Foo), and since I'm talking about leaf crates, the only thing that could break is the application itself compiling IF a dependency is upgraded where that trait is already implemented (unless you can overwrite the implementation), but then again that only happens if someone upgrades that dependency, so they are already touching the application code, so I would say it's not unexpected to break the code if you start upgrading your libraries.

Ixrec commented 5 years ago

afaik permitting leaf crates to completely ignore the orphan rules would be perfectly sound and not create any of the subtle composability issues we usually talk about on issues like this. The concerns there would be about it preventing you from easily refactoring the leaf/binary code into a non-leaf library, or creating a surprisingly huge and fundamental difference between what is allowed in libraries versus binaries that's especially likely to catch newcomers off guard. In particular, I'm told it's common to design Rust executables as mostly a library crate with only a very thin wrapper in the actual binary crate, in which case binary-only powers wouldn't help much.

axos88 commented 5 years ago

I think the part about catching newcomers off guard can be helped with good error messages, and warnings (such as a link to what the orphan rule is, and why it's needed for library crates, also explain that this limitation does not hold for binary crates, and why that is so).

As far as I see - although I still consider myself a newcomer to rust - is that the habit of having a small wrapper as the binary and having most of the code in a library is getting less common, especially since cargo now defaults to binary crates. Never really understood what was the point of that, I really think it's a fallacy to think that a full-blown application would/(or even should!) be easily converted to a library, other than some very small tools.

And about introducing a fundamental difference between them.... Yeah, I totally see your point there, however the use cases for these crates are fundamentally different (one is for reuse in other components, the other is for distributing and running), so I'm not convinced this is a bad thing. And allowing it gives us a lot of expressiveness in return, which in some cases cannot be worked around (such as impl Operator<MyType> for ForeignType), and in other cases very circumstantial (wrapping with NewTypes, etc).

So all in IMHO the orphan rule was introduced as a "necessary limitation" to circumvent the composability issues, however it's a limitation that I would think is not needed for binary crates.

Ekleog commented 5 years ago

The orphan rules are required for binary crates in order to keep semver working correctly, as an application is not necessarily distributed with its Cargo.lock, even though it is the best practice around the Rust community.

(also, I have noticed that until now every time I tried designing an application as a library wrapped by a binary I actually somehow managed it, and it makes for much cleaner code IMO as libraries make coupling and dependencies much more explicit)

axos88 commented 5 years ago

Well to me that seems like the cause of the incompatibility with semver is not really caused by not enforcing the orphan rule, but by not distributing the Cargo.lock, which is a very questionable thing to do - basically that means you trust every dependency (direct and transitive) to adhere to semver, which I think everyone already realized that unfortunately there are and always will be people, who don't. Also there's this: https://gist.github.com/jashkenas/cbd2b088e20279ae2c8e, which I don't agree with in full extent, but there are some very interesting points to consider.

Furthermore, I don't think we should restrict the experessiveness of a language in order to "be compatible with" an arbitrarily chosen versioning scheme, unless it is established, that it is a convention that all the rust things are to follow that versioning scheme.

We can issue warnings that hey, here be lions, be careful, we can put this behind a special keyword / compiler flag / rustc cmdline argument / whatever, but the amount of help we can get while debugging or just extending stuff without having to fork every single dependency I just want to make Serializeable or Debuggable is hard to ignore. And this opens the door to the possibility of overwriting implementations of traits in leaf crates (something along the lines of reimpl Trait for Struct {}), that could be a very powerful tool during mock testing, or debugging nasty issues, or just simply working around a bug found in one of the libraries / extending their functionality a bit without having to necessarily fork it (this latter part is questionable though).

withoutboats commented 5 years ago

@axos88 The fundamental problem isn't cargo and semver of third party library crates, but the versioning of Rust itself. We've made backward compatibility guarantees regarding the standard library that would mean we wouldn't be able to add impls to it if orphan impls were allowed, since someone could have written than impl as an orphan impl.

DanielJoyce commented 5 years ago

This really needs to be fixed instead of pestering crate owners to remember to implement/derive Debug all the time...

axos88 commented 5 years ago

@withoutboats That could be circumvented, if we'd say that orphan impls always override default impls.

cloudhan commented 5 years ago

@axos88 I think just refuse to compile is good enough. In many package managers, they have multiple packages provide the same functionality, when installing both, a conflict will occur and the package manager will ask the user to choose either one. Here, if the user want to use new crate with additional impls and keep the homemade impls, the compiler panics!

axos88 commented 5 years ago

@cloudhan that's not a good way because of the reasons @withoutboats described above. An application may stop compiling only because of a seemingly non-breaking change in rust, or another dependency, which is not acceptable for obvious reasons.

However if we specify a clear overriding order (and possibly still issue compiler warnings, so they don't go unnoticed, perhaps unless somebody clearly describes that they WANT to override), things won't break.

withoutboats commented 5 years ago

overriding violates the fundamental property of coherence we are trying to maintain - upstream code may already have been compiled using the impl which you have overriden, meaning that the same type impl is dispatched differently in different parts of your program.

burdges commented 5 years ago

impl Operator<MyType> for ForeignType works but not impl<T: Trait> Operator<MyType> for T.

As a rule, you could never add Debug etc to another crate because you cannot access the type's contents usually. It's simpler, more readable, and better for the community if you send the other a PR adding the functionality. And it'll catch bugs because they maybe omitted that impl for good reason. As an example, Rust ecosystem for zero-knowledge proofs is by far the best, in part due to session types, which adding Clone breaks.

cloudhan commented 5 years ago

@burdges

in part due to session types, which adding Clone breaks. then C++'s delete would make more sense

impl Trait for MyType = delete

maybe better than simply left the trait unimplemented and implicitly imply it.

Ixrec commented 5 years ago

I'd certainly support adding comments about deliberately unimplemented traits, but I don't see any benefits to adding a language-level syntax for it (C++ has similar syntax because, unlike Rust, it has implicitly provided methods and method overriding).

burdges commented 5 years ago

You cannot simply ban specific traits like Clone because another trait like ToOwned could provide similar functionality. You'd never catch them all throughout the ecosystem. We'd always prevent these with data hiding in my example, but then data hiding prevents many cases about which you care too.

It's only auto-traits like Send and Sync that developers must unimplement now, which although annoying only comes up in niche cases, and people can reasonable learn about.

We communicate performance characteristics with missing impls too: You do algebra on elliptic curve points in projective coordinates, but you cannot hash or order them without converting to affine coordinates, which takes time. If you permit adding comparison operators, then you'll wind up with folks writing amazingly slow hash and btree maps.

Instead, we should add a mechanism for trait warnings because we do want HashMap: Clone but it's mildly insecure, due to cloning the HashMap's BuildHasher. Ideally, any code that invokes HashMap: Clone should generate a warning, whether through ToOwned or whatever, so that each specific indirect call site gets annotated with an #[allow()]. We need trait warnings to disambiguate unclear cases like HashMap: Clone so that developers can be more fearless about providing iffy impls.

Just fyi, these trait warnings would not suffice for my previous use cases because say cloning a session type for a multi-signature will compromise the user's private key, not just enable some DoS attack.

alercah commented 5 years ago

Random spitball I thought of and figured I'd mention here, though it has a big problem: what if we explicitly allowed orphans in designated crates, to bridge compatibility gaps? Succintly, we have a rule that when crates c and d are not descendants of one another, then there is no way to implement traits from c on types from d. One of c or d must be changed to depend on the other.

In my view, the restriction I mentioned should be viewed as a problem in and of itself, as it's imposing maintenance burden on the author of the smart pointer library, even small maintenance burden, for users to use it in conjunction with another library. The only available workaround is a wrapper type with its massive usability hit, which sucks. And the author may not implement it, because they are too busy, away, burned out, etc. Or maybe they just hate the trait and refuse to implement it. And the bigger the ecosystem of useful utility traits grows, the worse the problem gets. We don't want to reach a point where a library can't be considered useful until it implements all of 25 different utility crates' traits, with a feature flag for each.

Additionally, workarounds currently in use, particularly feature flags, may work well when there's a clear dependency ordering of "coreness" of libraries. serde, for instance, is generally considered to be extremely core, and so the expectation is that everyone else implements serde's traits. But in the wider ecosystem, it's less clear whether someone wishing to add a new trait like stable_deref_trait that are of specialized use shouldn't go and implement them for all the appropriate types in common use; in this case smart pointer types.

This problem only gets worse when there are multiple types in the traits, such as From<T> for U, because we couldn't even have a rule like "the type definer should implement traits"---is the author of T or of U responsible for the instance? It's sadly not at all difficult to imagine the maintainers arguing that the other should be responsible for the instance, and therefore it never gets added.


A concrete idea to address this problem is that a crate can have a mapping of crates for which it specifies it provides compatibility, and this relaxes orphan rules. For instance, suppose that foo does not implement serialization traits. Then you could write provides_compatibility = [[serde, lfd]] in Cargo.toml. The list can have multiple sets of crates, as in [[serde, foo], [serde, bar]], to allow compatibility to be provided between multiple pairs (or larger sets) of crates without it counting as [foo, bar].

The effect of saying that a crate provides compatibility between a set of crates is that it is permitted to write instances for a trait defined in one of the crates, provided that it meets the local type requirements for all of the other crates. For instance, in the case of [serde, foo], we could:

  1. Implement traits from serde on types local to foo.
  2. Implement traits from foo on types local to serde.

In the case of [serde, foo, bar], we could only implement traits from serde provided that types local to both foo and bar, were included, to avoid conflict with foo.

There are three rules to be followed:

  1. Compatibility of one crate with another must be provided by at most one crate in a program. Trying to include two crates implementing the same compatibility is an error.
  2. Compatibility cannot include crates that depend on one another.

This maintains coherence because the only instances allowed are those that would require a dependency relationship between the named crates that must be forbidden by rule 2. Some issues mentioned upthread do not arise here: libraries are not restricted in adding implementations, because a library can still not add single-crate orphan instances, and there's no risk of ordering issues in compile because the rules are enforced before the compile begins and guarantee coherence when they are followed. I think there's some fuziness about the common dependency permission (and specifically, what if another crate implements compatibility for all of the crates and the common dependency), but it could be removed if it can't be made to work.

These rules can probably be relaxed, for instance to allow implementations of From<T> for U, but more care would be required in doing so and it's not worth doing in the first spitball.

There's a minor interaction with semver that ought to be noted here: since different major versions of the same crate are treated as different crates altogether, there's some ambiguity. The easiest resolution is simply to make it a hard rule that a crate that provides_compatibility for another crate be restricted to a single major version of that crate, allowing multiple major versions of the compatibility crate to provide compatibility between multiple combinations of major versions of the dependencies. It's best practice anyway, so enforcing it seems fine.


Unfortunately, there are two major limitations to this approach, both of which present serious difficulties. First, collisions when multiple compatibility crates exist could damage and fragment the ecosystem. Second, the semver implications on trait authors may be too much of a problem.

The issue with collisions is a particularly nasty dependency hell, where two different libraries might choose to depend on different crates offering compatibility between two other crates. These libraries cannot be used together. The problem isn't entirely unique, as there are definitely instances in the wild of competing crates, and the frustrations caused by trying to mix packages that made different choices. But these situations, although frustrating, can be worked around. Collisions between compatibility crates, on the other hand, don't seem to admit workarounds.

One approach we might consider would be the use of overrides in the top-level Cargo.toml, but this falls short on several aspects. There is no guarantee that compatibility crates implement only instances; they may have other contents which crates rely on. It could be a rule that nothing can be exported except for instances, but that still leaves problems. There is nothing ensuring that the two conflicting crates implement all of the same instances. And another crate may rely on a specific associated type definition or, worse, behaviour of a given instance, and substituting another instance would lead to compile errors if you're lucky, and possibly logic errors.

The issue with semver is that the relaxations to the coherence rules only work if the principal crate cannot add those instances themselves. In an individual compile, ensuring an absence of a dependency relationship is enough for this, but the implication there is that adding a dependency becomes a breaking change! I had originally thought that this would be limited to direct dependencies, and might be a little bit more workable perhaps, but unfortunately it's not. Because of type aliases and re-exports, it's possible for an instance to be defined on an indirect dependency. Thus, depending on another package which added a dependency could be enough for a breaking change. Maybe there's a kernel of workability in here somewhere, but I don't immediately see it and this comment is long enough already.

Ixrec commented 5 years ago

what if we explicitly allowed orphans in designated crates, to bridge compatibility gaps?

It's definitely come up before. My preferred nickname for this family of solutions is "official orphans", and I've previously attempted to gather some relevant discussion links on https://github.com/Ixrec/rust-orphan-rules/issues/7 in the hopes of defragmenting the broader conversation.

I believe we can categorically rule out any proposal that only solves "first-order orphans" by allowing higher-order orphans to break coherence / cause dependency hells (because if we're willing to break coherence, it'd be much simpler to just allow all orphans). That immediately implies any official orphans proposal must have the "non-parent" crates explicitly declare which crates may provide which orphan impls (and the toolchain validate those declarations are overlap-free), so that there can never be conflicting orphans in the ecosystem. The semver details you discuss would certainly have to be part of any concrete proposal, but imo they're probably solvable (if we avoid thinking about any first-order-only "solutions"), and not the blocking issue anyway.

AFAICT the reason no one's pursuing "official orphan" solutions is simply that core crate maintainers don't want to add official orphan lists/declarations to their maintenance burden. I believe I've seen @sgrif express this in a few past discussions. I'm failing to locate a perfectly explicit confirmation of this (EDIT: found this and this), but I'm pretty sure that's what he meant by e.g. the "completely independently" in "I’d like to be able to have a chrono_diesel crate exist completely independently in the ecosystem." Unfortunately I'm not aware of any other core crate maintainers chiming in on this either way (if I've missed something, please mention it here). Or in other words, I strongly suspect this part of the orphan rules design space is more of a social "who maintains what" problem than a technical problem.

alercah commented 5 years ago

I mean, I'm of the opinion that coherence was a mistake, at the very least for compile time :P. But yeah, fundamentally there is a social problem here, and it sucks that there isn't a good technical way we've found to at least reduce the issues. Not that we can fully eliminate them---there will always be examples of some crates just refusing to work together in any reasonable manner and that's okay. I think it's all about isolating them.

I'm also a bit skeptical that you can safely assume uniqueness across the entire ecosystem, when there is more than one of those too. The other thing I could think of is that while dependency hell can exist, and overriding might genuinely be a problem, maybe we can contain them to the point where they're unlikely to cause major damage. For instance, the following seem likely true to me:

  1. Most demand for orphans will near the root of the dependency tree, because libraries can often just define their own traits and shove the problem downwards. In practice I would expect that we can move most of the demand to last-step library packages (that is, the ones implementing most of the logic for a binary).
  2. Most library authors will not (intentionally) depend on specific behaviour of implementations. Not counting depending on the trait contract, but specific implementations.

If these both hold, then maybe we can manage to get away with some additional restrictions, such as a crate being able to explicitly declare which orphan implementations it wants to have exist, and can this asserts a contract that that crate is okay with substitute impls that meet the trait contract. Then we allow overrides as I discussed above, and this would work because the crate has promised it is okay with either option. We could also expect that orphan crates try to limit their scope so that there's less collision in the ecosystem (possibly enforced with restrictions on e.g. exporting non-impl items, in a manner similar to proc macro crates).

burdges commented 5 years ago

I think the automatic features proposal linked above still sounds by far the most promising.

In particular, there is nothing specific to traits about this problem: You could need an inherent method, wrapper type, type alias, mixed trait impl, macros, etc. that requires another crate, while you avoid actually depending upon that crate.

There is certainly an elegance in neither chrono nor diesel declaring the other to be higher level, by chrono_diesel being their cross over, but any advantage is lost once authors force everything into traits unnecessarily.

I think any chrono_diesel solution only really becomes interesting when crates delegate all their powers, including privileged visibility, so like chrono_diesel could handle sealed traits and private macros from both chrono and diesel together. At that point, chrono_diesel cannot "exist completely independently in the ecosystem" but becomes an integral part of both.

Ixrec commented 5 years ago

Just for completeness: my current opinion is that insisting on coherence is a big win, and we can "solve" these problems (to the extent that it's possible and worthwhile to solve them at all) through several indirectly related proposals with independent motivation. The big ones being:

I strongly suspect that when all of these are completed, the orphan rules will be a small enough pain point that no escape hatches or other direct weakening of the rules will be needed, with one exception. But there's no good way to evaluate that claim until these make significantly more progress, so there's probably not much else to say at this level.

IMO the "official orphans" like chrono_diesel are the most interesting cases precisely because they are that one exception, the one big pain point in the whole design space that none of these other proposals really help with. But I still think that's because--as @burdges said--"existing completely independently" just isn't possible without giving up coherence (this post appears to disagree, but IIUC what it's proposing is yet another "first-order only" solution), hence we're stuck on the social problem there.

alercah commented 5 years ago

I think any chrono_diesel solution only really becomes interesting when crates delegate all their powers, including privileged visibility, so like chrono_diesel could handle sealed traits and private macros from both chrono and diesel together.

I disagree quite strongly. There's no reason that an impl crate like a hypothetical chrono_diesel should need access to private functionality from either chrono or from diesel. All functionality should be already a part of the public interfaces of those crates, and the traits implemented elsewhere only glue them together. impl Chrono for Diesel is only privileged to chrono or diesel to implement because of coherence---the need to ensure that there is only one implementation. This is the only reason why some downstream crate has to write struct SuperDiesel(Diesel); impl Chrono for SuperDiesel instead.

Your proposal makes the problem worse, by requiring both chrono and diesel authors to work together. This is unworkable; @dtolnay is not going to sign off on every single package that wants to implement serde traits, for instance. It also has serious problems for unsafe code: if we can violate visibility, then suddenly unsafe code that relies on things not being accessed from outside the crate (or even from other modules within the crate) can't make these assumptions any more, and the proof guarantees can't be met. This would force all the crates involved to be evaluated with respect to each other to verify safety guarantees and to release in lockstep.

Just for completeness: my current opinion is that insisting on coherence is a big win, and we can "solve" these problems (to the extent that it's possible and worthwhile to solve them at all) through several indirectly related proposals with independent motivation. The big ones being:

  • automatic features
  • getting specialization stabilized
  • some sort of delegation
  • mutually exclusive traits (nothing to link for now, afaik)

Automatic features improves usability, but does nothing to solve the problem of a linear number of maintainers having to maintain a quadratic number of instances. Specialization and mutually exclusive traits, I may be missing something, but I don't see how they help with this problem. If you mean that they help with some of the other problems in the first comment, I agree there, but those are really different problems. Delegation would significantly ease on the level of boilerplate required to create a wrapper type, but it still brings with it ugly wrapping/unwrapping to convert between the various wrapped versions. It's a big help, but it's a workaround and not a solution.

As for coherence, while I think it was the wrong choice for the language to require coherence to begin with, I'm skeptical that Rust really has the capacity to accept a relaxation of coherence without causing other issues. I do think that we can insist on coherence easily with official orphans, as I said above, but at the cost of dependency hell, and I think that trying to find workarounds to that is possibly more fruitful than relaxing coherence. But I might be wrong here.

P.S. I don't know what you mean by "first-order". Could you define it for me? Thanks!

alercah commented 5 years ago

(this post appears to disagree, but IIUC what it's proposing is yet another "first-order only" solution)

Forgot to reply to this---if I understand what it is proposing, it's that a crate can "cheat' the orphan rule by using a potentially coherence-violating instance internally. As long as it doesn't expose anything to any other crate, then a collision can't be visible and thus there's no issues with coherence. I thought there was an RFC proposing to do this for inherent impls (which are much simpler because neither of the below problems apply) but I can't seem to find it.

The obvious and immediate restriction is that you can't create a trait object from such an impl, or at least you can't let it escape your crate, because then you have a different vtable pointer and so you'll look like a different type.

The more subtle and nasty problem is the interaction with generics in the crate. If I have a private implementation of, say Serializable for Foo, and also a public generic function taking T: Serializable, we have a problem. Some other crate could also have an implementation of Serializable for Foo and pass it in to my function. The coherence violation has snuck into my crate.This might be a surmountable problem if Rust was parametric, but it's not, and so we can try to do something like put them both into the same Vec<Foo> and at that point it's game over because we can't tell which implementation to use any more.

burdges commented 5 years ago

There's no reason that an impl crate like a hypothetical chrono_diesel should need access to private functionality from either chrono or from diesel.

There is afaik no reason for chrono_diesel to exist since diesel with an automatic chrono feature handles everything far better. Automatic features are both a far more conservative and vastly more useful.

Automatic features cannot provide access to private items from both sides however, so I'm only highlighting their one real limitation, especially if both sides employ sealed traits.

This is unworkable; @dtolnay is not going to sign off on every single package that wants to implement serde traits, for instance.

If either side's public interface suffices, then automatic features are far better than another microcrate that breaks rustdoc, requires using traits when inherent items fit better, etc.

I never suggested chrono_diesel as an alternative to automatic features, and I'd argue against chrono_diesel even with privileged visibility, but I think the privileged visibility variant sounds like the interesting case.

Automatic features improves usability, but does nothing to solve the problem of a linear number of maintainers having to maintain a quadratic number of instances.

There are many harmful or questionable impls, like say HashMap needs to be Clone but this causes security bugs. At least automatic features place blame firmly onto one project. I've zero faith in any community managing literally quadratic impls. Yet, individual projects can eventually navigate the painful human scaling process for more reasonable goals.

In particular, I'd expect tiny ad hoc (non-permission/workspaced/etc) joiner crates would become a bug minefield with "maintainers" who often either do not care about miss-use or else abandon them preventing fixes. And good luck winning the crate name chrono_diesel from the version with an sql injection exploit whose maintainer disappears or whatever.

As an aside, we need "impl warnings" so that iffly impls like HashMap: Clone can generate a warning with every use, ideally meaning #[allow(..)] statement get inserted explaining the usage.

Delegation would significantly ease on the level of boilerplate required to create a wrapper type, but it still brings with it ugly wrapping/unwrapping to convert between the various wrapped versions.

I've sent enough pull request adding core traits to types in third party crates, but I only rarely want a third party trait defined for a type from a third party crate. There are third party crates like serde designed well enough to warrant this, but among those many like rand rarely fit external types. And most crates' traits prove too bespoke without some wrapper type, so..

It's actually delegation that addresses the most common case!

sgrif commented 5 years ago

There is afaik no reason for chrono_diesel to exist since diesel with an automatic chrono feature handles everything far better.

That feature exists as a workaround only. It should not have to live in Diesel