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

Tighter coupling of Cargo workspaces #2315

Open
wants to merge 5 commits into
base: master
from

Conversation

@clarfon
Copy link
Contributor

commented Jan 30, 2018

This is a way to more tightly couple crates within a cargo workspace, allowing inherent impls and blanket trait impls across multiple crates in the same project. Its primary use case is for the standard library, but the feature is extended so that it can be used by the entire crate ecosystem.

Rendered

@CAD97

This comment has been minimized.

Copy link

commented Jan 30, 2018

FWIW, I think the default should be not to have this functionality turned on (though it would be nice to have it available). Part of the reason of splitting out crates in a large project like piston (another example) is to version the crates separately for those who only want to depend on the smaller crates.

@scottmcm

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

This reminds me of .Net's InternalsVisibleTo, that lets an assembly (roughly a crate) give privileged access to another assembly. I wonder if there's any learnings from that which could be useful here.

I feel like, after reading the RFC, I'm not certain exactly what's responsible for checking and ensuring the reasonableness of the combinations here. If crate A is the root, and crates B and C are in the workspace, what exactly happens if B and C both try to impl the same thing or add an inherent method with the same name to the same type? What forces the crates used by rustc to be ones that make sense together?

@clarfon

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

@scottmcm that's the same issue we have today when two crates don't interact well. What if crate1 provides:

impl Trait for Type1 {}

and crate2 provides:

impl Trait for Type2 {}

Currently, this is fine. But if crate1 adds the breaking change:

impl<T> Trait for T where T: Bound {}

Then these two types now conflict, and Rust fails to compile. If two crates provide the same name for a method or the same impl, it'll conflict like it already does. And this isn't unsound because all of the conflicts have to be resolved at once, and the entire workspace has to be published as a unit.

the type or the trait are in the same workspace, not just the same crate.
3. Workspaces which use virtual manifests must opt into this feature by
specifying `workspace.root` in `Cargo.toml`.
4. Workspaces can opt out of this with `workspace.coupled = false` in

This comment has been minimized.

Copy link
@mgattozzi

mgattozzi Jan 30, 2018

Member

I think default behavior should be considered. For instance build scripts named build.rs don't need to be named in Cargo.toml anymore.

I think we should err on the side of opt in rather than opt out, but I really do like this in general and would clean up some of my codebases if this was available.

This comment has been minimized.

Copy link
@clarfon

clarfon Jan 30, 2018

Author Contributor

This is an artefact of the fact that I wasn't sure what the default behaviour should be when writing this; in the guide-level explanation I don't list the default but in the reference-level I do.

I'll fix this later today; I agree that it should be opt-in, as it prevents the workspace from allowing multiple versions of crates.

@Centril

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

cc @SergioBenitez - isn't this somewhat the "crate groups" idea?

@kornelski

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2018

Can this be opt-in per trait, rather than for a crate/workspace as a whole? i.e. pub trait Foo not coupled, pub(workspace) trait Foo coupled?

@clarfon

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2018

@kornelski I don't think this would be useful; once crates are coupled, their versions are locked together too. All that coupling allows is the ability to provide extra impls for traits, not necessarily a requirement to do so.

@aturon

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

@aturon aturon self-assigned this Feb 7, 2018
@SergioBenitez

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

@Centril This is indeed in the same vain as what I've dubbed "crate friends". In fact, this RFC can be mostly subsumed by crate friends. Crate friends also offer a greater level of flexibility and allow for some important scenarios.

Crate friends allows you to mark specific crates as friends in Cargo.toml. Friends form a "crate group". Crates in a friend group are subject to different coherence rules than traditional crates. In particular, coherence checking is performed on the friend group as a whole instead of on each crate individually. As such, crates in a group may implement any trait for any type in the crate group.

Friend relationships must be reciprocal. Crates A and B only form part of a crate group if A is in B's friend list and B is in A's friend list.

I expect the syntax for declaring crate friends to be similar to the syntax for declaring dependencies. Imagine, for instance, that rocket and serde_json want to be friends. This would allow Rocket (or serde_json) to implement the rocket::Responder trait for serde_json::Value without providing a wrapper type. Their Cargo.tomls may look like this:

Rocket's Cargo.toml:

[friends]
serde_json = "1.0"

serde_json's Cargo.toml:

[friends]
rocket = "*"

Removing a friend is a breaking change.

If all members of a workspace are considered friends by default, this RFC is mostly subsumed. The exception is the provision in this RFC that would allow all members of a friend group to access an item declared as pub(crate). The "crate group" proposal doesn't allow for this, though a pub(friends) visibility class would be very useful. In fact, Rocket already establishes an ad-hoc "friend" relationship with serde for exactly this purpose: Rocket uses hidden, private types (with the consent of serde) to enable functionality.

@clarfon

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2018

I'm not opposed to the idea of crate friends, but I'm also unsure why in that particular case From<io::Error> can't be public.

@SergioBenitez

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

@clarcharr See SergioBenitez/Rocket#547 (in particular SergioBenitez/Rocket#547 (comment)) and serde-rs/json#407 for details on that particular issue.

@clarfon

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2018

I was thinking about this a bit, and one particularly big downside to crate friends (imo) is that it feels a bit more like crate cliques in the sense that some of the larger crates may be able to more easily interact, whereas smaller ones won't. Why should Rocket have preferential access to From<io::Error> for serde::Error when the rest of the ecosystem doesn't?

To me it just seems like some APIs may need to be exposed differently, or crates should be split to allow interaction with lower-level error types but not higher-level ones. Of course, this is only after a glimpse into the situation you described, and I'm still open to reworking this RFC to make use cases like that more easy to work with.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2018

With #1133 or it's successor, I'd hope standard library crates would not be pre-built in their own workspace but regular deps like any other of crates in the user's workspace. Then this trick wouldn't work for core?

I rather expose built-in types with struct or mock-enum definitions that have #[lang = ...] annotations. We can then do regular inherent impls on those lang items. We can also then cfg them out where they are not available (e.g. no soft or hard float). I think there's some clever sophistry we can do to make that forwards compatible with the current rule that builtin types are always in scope and never shadowed.

@AGausmann

This comment has been minimized.

Copy link

commented Jul 16, 2018

I'm glad someone brought up the issue of version synchronization for all crates in a workspace, as that is a feature that I do miss coming originally from Maven. However, I don't see how any of the other features are actually useful or reasonable. Crates are not meant to be tightly coupled to each other; if you are trying to do that, those features probably belong in the same crate. Crates are meant to either start from scratch or build on top of existing crates in an acyclical way.

I don't think this is the right way to resolve the "mess" that we have with core, alloc, and std. The correct way, in my opinion, to add functionality to a type in another crate is to explicitly define the extension trait, and if you want to expose that existing type as a part of the new API, reexport it with #[doc(inline)] (which should, in theory, also add the newly-implemented traits to the documentation). If new inherent impls are a must, we could add another inheritance model that allows new type definitions to build their own impls/fields upon a parent type.

If this were to be implemented in Cargo as it is currently proposed, it would probably mean that the entire workspace would have to be treated as a single crate, which means "crates" would now just be glorified modules. Skip the extra complexity and turn your codependent crates into modules, or even better, improve your design by removing such cycles.

@ehuss

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2019

We discussed this RFC in the Cargo team meeting, and have decided to move to postpone this. This will likely require deep changes, and we do not think there is the necessary bandwidth or priority to move forward with this at this time.

@rfcbot fcp postpone

@rfcbot

This comment has been minimized.

Copy link

commented Sep 21, 2019

Team member @ehuss has proposed to postpone this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Eh2406

This comment has been minimized.

Copy link

commented Sep 21, 2019

@rfcbot reviewed

1 similar comment
@Centril

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

@rfcbot reviewed

@rfcbot

This comment has been minimized.

Copy link

commented Oct 11, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.