Pre-RFC describing mechanism to optionally loosen some orphan rule constraints #3482
Conversation
|
The reason the orphan rules exist is precisely to avoid the situation where two crates cannot be used together because they have conflicting implementations. If there's a way to disable the orphan rules then you may as well just not have them, so I don't think this is a good idea. |
|
I think that the need to restrict this feature to a local workspace shows how dangerous disabling the orphan rule really is. I personally do not want to have to think about any two crates creating an impl conflict, since it massively increases the complexity of crate relationships and possible impl-overlap compilation errors. Perhaps the trait-defining crate could be required to declare (but not use-depend on) all crates which can impl-depend on it. This at least puts the task of avoiding conflicts in the court of the crate author instead of any user of the crates. Even if such crates are then published on crates.io, any conflicts could be detected within the limited list of crates before allowing publication. Overall, I still think that the increase in complexity and nasty compilation errors is not merited. I feel like using features and improving parallel compilation itself are better approaches here. |
ehuss
added
the
T-lang
|
I'd think "automatic dependencies" would address the usual issue here better: This says I'll impl whatever traits unwanted_dependency creates, but only if you use unwanted_dependency elsewhere. After these cases, we need some "classification" of the possible newtype deriving stories, so folks can finally feel like they know the proper solutions there. If you really need this, then you might do roughly this under the second orphan rule. We've a type and trait like We then define A priori, we'd want |
|
With respect to features, if some crate That's not to say I haven't personally found orphan rules painful, but they'd need to be replaced and not just removed for the same reason that we don't have type inference beyond the function boundary. This doesn't sell me on it because it doesn't seem to make things take less work and it does definitely allow for spooky type action at a distance. I don't want to patch one or more deep chains of dependencies because two semver-compatible versions now happen to provide conflict impls. It's not just patch Anecdotally, I had to try to optimize build times for a large Rust codebase at work. Removing even large numbers of dependencies was worth very little once sccache was running. Surprisingly so in fact. It was worth maybe a couple percent no matter what I tried in that regard (in that case the most gains were found helping Warp not to monomorphize as much and getting CI using sccache with s3). Now to be clear: I'd love something less restrictive than the orphan rules that solves the same problem. I'm not saying "O this is bad", more "O this will cause chaos". If something was coming in to replace them, I'd feel more confident. |
|
Oh, wow wasn't expecting this to attract attention so quickly. The fundamental part of this proposal is the language-level mechanism, as a starting point for experimentation. The secondary part was a limited means to use it via Cargo so that there's something to work with. As I said numerous times in the RFC text, I do not think this should be considered for widespread use without a lot of extra discussion. I'll reply to each person separately to help with discussion
Right, but this does not propose disabling them; this RFC requires precisely the same coherence invariants that the current orphan rules do. All it proposes is implementing them with a bit more flexibility. |
One of the other key points of this proposal is that any conflict is detected and reported as close as possible to the conflicting crates. In principle one could do all coherence checking when linking the final
Yes, I did consider that - that was the initial design. The problem is that since Rust has a flat crate namespace, there's no way to actually identify a crate without taking a dependency on it. Something of the form I also experimented with the idea of a "witness" crate-like object which takes a dependency on the defining and impl crates, and will only compile if the coherence invariants are correct. While that would work, it would also undermine all the other benefits of this proposal (since we'd still end up with binaries depending on everything even if they're not needed).
There's two classes of parallel compilation - "internal", where rustc has internal parallelism to get better performance, and "external" where the build system can invoke multiple instances of rustc in parallel. The latter is much more scalable than the former. Internal parallelism can make use of at most one machine's cores, assuming there's parallelism to be successfully extracted. External parallelism can make use of a whole build-farm's worth of machines. In large scale systems, a build system like buck2 can have many thousands parallel compiler running at once, so long as the dependency graph allows it. In C++ this is relatively easy because of the decoupling header files give, but Rust is currently strongly constrained here. This is one of the things I'd like this RFC to improve. That said, one of the "future possibilities" I propose is auto-splitting a crate into multiple subcrates. This effectively converts internal parallelism to external, and also mitigates all the complexity and eco system problems, since it's necessarily purely local. |
This looks like a "weak dependency" analogous to a weak symbol - the dependency here is not enough to bring in the crate, but some other strong dependency will. Unfortunately this idea only work for small-scale build systems like Cargo, where you only have a single program within the dependency graph. If you have a build system for a large system, then these weak dependencies can be hard to implement, if its possible at all, and even if you do have them they're very hard to reason about. Neither Buck nor Bazel (I think) support this kind of dependency. I haven't thought about it in detail, but it seems quite possible you could get paradoxical situations: eg where you have an acyclic dependency graph, then you add one new dependency, which causes a weak dependency to be brought in, which in turn causes a cyclic dependency. Avoiding this means that you still need to analyze the entire dependency graph even for the "optional" parts you're not using, which add constraints on what you can depend on even though they're from things you're not using. It doesn't seem any simpler, at any rate. |
Could you expand on this? What do you mean by "the convergence problem"?
Yeah, in the environments I'm working in, small crates are vastly preferable, from the perspective of:
On the other hand, there's no requirement to use this mechanism if it doesn't solve any problems. It might make debugging issues with upstream dependencies harder if they misuse it; it doesn't feel to me like it would be much harder than feature combinetrics (ie the "how did this feature get enabled?" game).
Yes I agree entirely - I would not want this to be available in the form of "any package can implement traits for any other package for types of any package" - ie, the "general third-party impls" problem. That would be a disaster. But that's orthogonal to what this RFC discusses, which is a basic mechanism which could be (mis)used this way, but also for many other much less problematic uses.
Yes, to an extent. The environment I'm most familiar with uses Buck, which caches much more aggressively than Cargo (even with sccache). Warm builds are fine, but coldish builds are much more common than one would like - in a large multi-developer environment, someone is always touching something, and so we need to make those builds as quick as possible. But even if everything is cached, even analyzing the dependency graph to work out what even needs to be considered can take a considerable amount of time, so shrinking the dependency graph can be a build speed win in itself. See also my comments above about internal vs external parallelism, where having a wide dependency graph which allows many build actions to be run in parallel has much better scaling. |
|
Sorry, convergence was a bad word. I mean the dependency explosion that happens when someone brings in (say) serde just to support its traits. The thing is that the Rust parallel compilation model can be improved more and while it is indeed the case that clean builds happen more often than we would like, it's hard for me to see this as anything but a short-term hack to deal with such. Typically, there's a way to design so that even local crate hierarchies don't have such issues. For example, why not just pull your traits to a super-lightweight trait-defining crate, depend on that, and then implement them everywhere? If we ignore everything about good code design and we say that this is entirely for build times/parallelism, then a If you're rebuilding from scratch all the time (including dependencies) then it seems to me that your build would always be dominated by external crates anyway, unless you're in an environment in which dependencies are disallowed, right? Finally, with respect to parallel compilation and in full acknowledgement that I haven't done this, I would intuitively think that a build large enough to saturate more than one machine is large enough that the fact that there are dependency chains isn't going to be much of a factor. There's bottlenecks, but I'm skeptical it's as clear-cut as "C++ headers good". I also feel like there's probably a set of proper solutions to that which aren't allowing workspace-local breaking of the orphan rules. I think if I was going to put it simply, what's the common use case this solves? Right now I'm seeing this as primarily build time for a set of somewhat niche use cases. Not invalid ones, it just feels like "turn off orphan rules" is really drastic to solve them. I also don't personally see how this gets past experimentation to a point where it can interact with crates.io. The RFC says it's leaving crates.io for the future, but it's hard for me to see this as a general solution to ecosystem problems because I don't see that future working out. You're right that what replaces the orphan rules in that future could be deferred indefinitely, but without some idea of what that looks like I don't see how that's even a possible future; I'm not convinced the orphan rules can be relaxed, and convincing me that they could be would go a long way toward me seeing this as something general. |
|
@jsgf This came up in today's @rust-lang/lang meeting. We have had some discussions in the past about a more general solution for orphan instances in the ecosystem (beyond just workspaces), and the requirements and tradeoffs of doing so. If @rust-lang/lang were able to provide some design constraints/requirements for this, would you be interested in working on the more general lang-level solution? (If so, we could start some further design conversations on this.) |
jsgf
changed the title
|
@joshtriplett Yes, definitely. I'd really like to find a path forward here, but I also know it's important to get right. But
I have to say this is a secondary concern for me. My priority is to have a very solid solution for the local case, and then possibly build on that for a more general solution. I'm doubtful there's a path to a general solution without having a settled local solution first. |
I don't agree with that framing of the RFC. The RFC allows any crate to essentially "opt out" of the orphan rules for a specific dependency, and as a downstream consumer of the crate I have no control over this. For example:
If Bob had instead made a newtype wrapper, then the code might be slightly more complex, but there would be no risk of breakage down the line. I think there is an argument to be had about whether the orphan rules should exist (it's certainly a trade-off). However, having the orphan rules but also having an opt-out with no apparant downside to the opt-out is the worst of both worlds, since you get both the confusing error messages, and a high risk of future breakage. Personally I think the orphan rules are better than no orphan rules, but that there is still room for improvement. First: making it easier for maintainers to structure crates how they want. This can be solved with a similar approach to this RFC but in reverse: the upstream crate can "nominate" downstream crates who are allowed to add implementations - this way you add flexibility without introducing the possibility for breakage. Second: a mechanism for "fallback implementations". Fallback implementations can be provided for any type/trait pair but will only be used as a last resort if a "real" implementation is not found. If multiple fallback implementations exist, then the "root crate" can disambiguate: failure to do so is a compiler error. This mechanism should still be considered a "last resort" though. Obeying the coherenece rules is better for everyone involved if at all possible. |
|
@jsgf In my opinion, the crates.io/ecosystem solution is what moves this from hacky to cohesive. Otherwise, it's hard for me to see this as anything but a build time workaround or something like that. Locally, you have full control of what's where and I don't think I know of a case where this can't be made to work out. Can you more clearly articulate the thing you can't do at all without this proposal? @Diggsey I kinda like your solution re: fallback implementations, though I find myself asking how often they would be used in practice. It sounds like specialization however. Isn't what you're saying isomorphic to adding a rule/attribute to specialization that says "this trait is of the lowest priority"? |
True, but that is the natural progression if this RFC goes forward, and without the ability to publish such crates several of the main motivations for the RFC are not solved.
Ideally as infrequently as possible...
As it stands, specialization does not relax the orphan rules, so you still can't define implementations outside of the crate defining the type or trait. |
My proposal here is that 1) I don't have a solution to the general third-party implementation problem which would for crates.io, for all the reasons that you're concerned about. I'm putting this forward as a mechanism which is locally useful, that could be part of a more complete solution. But it also enables a bunch of interesting possibilities even if it's never directly exposed to users, such as splitting std or auto-crate-splitting.
Yeah, I'm very aware of those solutions. My principle build system is Buck2 which aggressively distributes and caches, and does a very good job of extracting as much concurrency from a build as is possible to extract with the current model. It's just not enough. And more generally it's just very frustrating having a hard language-imposed bound on how I can factor functionality to crates.
Having a separate ad-hoc mechanism for every code generator is a specific example of this. Fundamentally, I see this proposal as being a bit like specialization: its in the language, but not generally available without enabling a feature. It can be used in, say, libstd with a particular awareness of the problems and limitations, and maybe eventually it will be a generally available feature once there's a coherent story around how it can be made to work in the large. |
|
@jsgf Perhaps I should clarify, however, that I have no decision-making power either way, and am speaking only from a general project planning perspective; if Rust folks are happy with an unscoped experimental initiative for this, more power to them. |
|
It's maybe helpful if people cc rust-lang/rust#29635 whenever they'd consider using |
|
Maybe an alternative is to make it easier to create a wrapper type, which is how the orphan rule is usually worked around: #[wrapper]
struct Foo(crate1::Bar);
impl crate2::Baz for Foo { /* ... */ }
This could be generally helpful as well. |
|
@tgross35 Yeah I should add a wrapper types discussion to the "alternatives" section. I think the main problem is getting all the wrappers confused with each other and the wrapped type. I guess we'd end up with lots of signatures of the form
Do you mean just libstd traits, or all traits? What would this look like for serde, for example? |
Not a very refined idea :) I was initially thinking all traits but know that would come with complications. However...
It seems like this idea has already been floating around (thanks for sharing today's vocab word) |
Well take this for example. In C++, when I want to depend on another module I do In addition to this, it is straightforward to factor the code for a given class's member functions into as many compilation units as one likes, either for code organization purposes, build speed and/or whatever other criteria one desires. In Rust, the maximum compilation parallelism is limited by the shape of the dependency graph - at greatest the amount of parallelism is the widest part of the dependency graph, which often isn't all that wide. But worse, there are often bottleneck dependencies, which depend on lots of things, and are depended on by lots of things - if they need to be rebuilt then everything comes to a standstill while that happens. Rustc has a few techniques to mitigate this:
These all help to some degree. In many cases they help enough that one might not consider this to be a big issue. But at a larger scale they are literally orders of magnitude away from being able to achieve the same build throughput that's possible with C++. Caching aside, none of those techniques - individually or collectively - are going to come anywhere close, because they're fundamentally limited by the structure of the dependency graph. Now obviously C++ has a bunch of disadvantages here - the primary one being that you need to manually keep your headers and your implementations in sync, it's easy to cause UB by violating the One Definition Rule, etc - we absolutely do not want to let Rust be subject to any of these problems. So what I'd like to be able to do is define "interface" crates, which just have a bunch of type/struct/trait definitions in them, and ideally no implementations at all. Any crate which needs those definitions need only depend on this crate. Since it does not contain code, its effectively equivalent to a .rmeta. The implementations are all split into separate crates in any organization that makes sense to the developer. Consumers can depend on any subset of those implementation crates that they actually need, and so take on only the build cost and transitive dependency cost of the functionality they need. This structure not only inherently improves build performance, it also enhances all the other speedup techniques that rustc employs, mostly by letting them all operate on a finer grain:
This last point is particularly important - changing a type definition is clearly going to cause a lot of downstream fallout to adjust. But changing the implementation of a seldom used function can be limited to that crate and its dependencies, even if the rest of the associated functionality is widely used. This still has the coupling to the shape of the dependency graph, but its limited to the interface crates. It's equivalent to C++ with modules. The other limitation here is that (Note here I'm talking about a more general form of this RFC that also applies to intrinsic implementations as well as trait implementations. I included that in earlier drafts but it raised enough secondary complexity that it can be deferred to later.) So, no, this change isn't essential in the sense that there's some lost expressiveness at the language level. But at some point, the effect of being able to improve the practical experience of using Rust counts for something - if we went the other way, and removed the notion of separate compilation entirely it would certainly simplify things, and open up a lot of opportunities for internal parallelism and incrementality and so on. And similarly incremental building is a fairly brittle, somewhat ad-hoc hack to improve inner loop compilation speed, which is nevertheless probably worthwhile. |
|
@jsgf A few things I think are worth noting:
It's not like a giant drop in code quality but using this is still generally a drop in code quality especially given all the ways you can use it wrong/too much in a given codebase. I think it should be considered a very big open question whether or not this even fixes the build time problem as well. What evidence exists that this is a large gain? Looking through the RFC I don't see that addressed. If it was (say) a 50% gain for lots of common codebases, I'd be more for this. But everyone will probably be very sad if this is added and 6 months down the line it turns out it's only a 10% gain or something, but now we're stuck with it. Also, what evidence is there that this time next year there won't have been some big initiative for compile times that renders this use obsolete? Even if neither of those happens, this feels like a limited-time feature that "expires" at least as regards the original motivation whenever Rust fixes build times as a whole. |
|
I'm focusing on local use simply because I agree that it could cause deep problems at scale. I think the tooling around it should at the very least encourage, and probably enforce local use, in order to keep refactoring and so on tractable. That's one of the reasons I'm hesitant to tie this to a "do this at crates.io scale" because I'm doubtful that there's a way to do this - I think the current orphan rule works pretty well ecosystem wide, on balance.
This is the "everyone will use unsafe to avoid the borrow checker" argument. I don't think it follows.
Right, so don't do that. There's lots of ways to influence how a feature gets used to make sure it gets used in the ways one would like, and avoid bad patterns. There's a reason I keep saying "small scale", "limited", "local scope", "opt in", precisely because this is something that should be used carefully, and all the tooling should push users in that direction. But also because it hasn't been implemented yet, so it will take some practical experience to really understand the implications. I understand your concerns, I really do. I'd also push back if this were "let's stabilize this tomorrow". I posted this because I want feedback on the technical aspects, to see if there's a fundamental soundness problem with the proposal, or something that makes it surprisingly hard to implement within current rustc. If it passes that bar, and we get to prototyping it, and merging it, it ends up as one of the many experiments currently live within rustc. Either we'll find value for it, in which case it stays (perhaps perma-unstable) or we drop it. I just don't think we're at the "this is an inherently flawed idea that is morally wrong" phase of the conversation yet.
Yeah, that will need a working prototype to answer. Or maybe a local hack to just disable the orphan rule altogether so we can try it out, but that's later. And while build perf isn't the only consideration for this feature, there's examples like this where useful functionality in a common crate is dropped in service of build time. Something like this RFC would make it much easier to have opt-in functionality.
Sure. |
|
|
||
| ``` | ||
| [dependency] | ||
| my_definitions = { version = "0.1", path = "../my_definitions", impl = True } |
There was a problem hiding this comment.
Nit: booleans are lower case in toml
https://toml.io/en/v1.0.0#boolean
Edit: This applies to every "True" in the RFC.
| my_definitions = { version = "0.1", path = "../my_definitions", impl = True } | |
| my_definitions = { version = "0.1", path = "../my_definitions", impl = true } |
| fine at small scalle, but at large scales it would mean that if there were two | ||
| crates with conflicting implementations, then could never appear in the same | ||
| dependency graph. That is a change to private dependencies, which wouldn't | ||
| usually be considered compatibility breaking, would cause downstream build |
There was a problem hiding this comment.
This might be clearer phrasing?
| usually be considered compatibility breaking, would cause downstream build | |
| usually be considered compatibility breaking, but can now cause downstream build |
Unsafe doesn't disable the borrow checker though. If it did, then people using unsafe incorrectly would be a much bigger issue than it is. |
|
It's not the "everyone will use unsafe" argument because unsafe is hard to use even if you're doing it right, and the people who find Rust come to Rust to avoid unsafe. Someone can't easily decide to use unsafe because using unsafe is hard. Additionally using unsafe is local: it might reduce the code quality of one file/module/crate but it doesn't change anything much about project organization. All operations one might wish to perform at the crate level can still work. Additionally it's very easy to understand why unsafe introduces bugs. On top of all of that, it's trivially easy to explain why unsafe is necessary and why Rust as a language would be impossible without it. This flag takes an error that is hard to explain to new Rust users which exists for good-yet-esoteric reasons and silently makes it go away. It doesn't introduce bugs today. It's not syntactic in the code; everyone who comes after will "use" it without knowing that it was turned on. I would agree that it's the "everyone will use unsafe" argument if it wasn't entirely invisible and/or was a holistic solution to the problem. Also, that said, for something which appears to be mostly about build times, I think an RFC without some sort of prototype/proof that it will help isn't a great idea. A proper implementation is likely not a small change in the compiler even though it seems like it should be, and (for example) probably means modifying the outputted crate metadata. Perhaps there is a hacky way to prototype it instead. We have one of these large monorepos at work which takes 12 minutes or so to build from scratch in CI, and we wouldn't benefit from this RFC for build times because:
Which is why I'm somewhat skeptical. It requires a lot of assumptions before we can take it as a given that build times go down. |
|
I would propose a very different approach that could solve the basic requirements and treats /// Module in pkg3
pub mod my_mod {
use pkg1::my_trait
use pkg2::my_type
// This implementation override ONLY LOCALLY to this module the implementation in my_trait or in my_type (if it exists)
// This impl block has no effect outside this module and it cannot be referenced by any other module.
impl my_trait for my_type {
....
}
....
}It should be possible to make public the implementation to allow usage in different modules/packages using /// Module in pkg3
pub mod my_mod {
use pkg1::my_trait
use pkg2::my_type
// This implementation override ONLY LOCALLY to this module the implementation in my_trait or in my_type if it exist
// Now it can be used in other modules
pub impl my_trait for my_type {
....
}
....
}If a developer would like to previous implementation it has to declared explicitly /// Module in pkg4
mod another_mod {
// Override the implementation in my_trait or my_type and bring automatically in scope pkg1::my_trait and pkg2::my_type
use pkg3::impl(pkg1::my_trait,pkg2::my_type) // Likely there is a different nicer syntax :-)
// Equivalent code
use pkg1::my_trait
use pkg2::my_type
use pkg3::impl(my_trait,my_type)
}I see the following benefits:
|
|
I think "local impls" should be done by delegation, but.. There is a large design space for delegation, so someone should explore this space. In particular you've this flavor: All traits of |
|
For the very specific goal of optimizing build times of code which can be theoretically built in parallel after certain shared trait/type definitions are built first... I feel like crates and orphan rules are a red herring, and we should be asking if the compiler can be told that information directly. The same way we use As a pure strawman, I'm imagining a |
|
Very, very weird case for this that came up when discussing the situation where It might be interesting to loosen this to "deferred impls" where the crate explicitly states that the implementation is "deferred" to some dependency. This both strengthens and weakens the constraints, by allowing you to place restrictions on arbitrary types while requiring you to explicitly enumerate them. Ultimately, the way this would be handled on the compiler side is by using either negative impls or reserved impls in the crate that can then be overridden in the explicitly allowed crate with a proper implementation. Of course, the main issue here is that it defeats the original purpose: depending on crates which have these types. So, it would require some shenanigans where cargo lets you reference this crate from the same workspace for the sake of listing the allowed impls, but doesn't actually compile that code unless the dependency is visible anywhere in the tree. Something along the lines of a special, automatically enabled crate feature. |
|
What if, at least, developers could entirely disable the orphan rules for crates that will never be a dependency to another? The idea: Orphan rules should only be enforced on crates published on crates.io There's no reason why the "end-stream" user should have to abide to such rules. |
This RFC proposes a mechanism to allow
impls for a type or trait to be in aseparate crate, while still meeting the soundness guarantees currently enforced
by the orphan rule. With this, the crate structure (and thus build system
dependency graph) can be decoupled from the actual implementations.
Rendered