From 05cf67113f6b59ea00fb141d9ce457bf55fc3620 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 13 Oct 2023 09:25:50 -0500 Subject: [PATCH 01/43] feat: Fork pub/private dependencies RFC --- text/0000-public-private-dependencies.md | 451 +++++++++++++++++++++++ 1 file changed, 451 insertions(+) create mode 100644 text/0000-public-private-dependencies.md diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md new file mode 100644 index 00000000000..a95a78799b1 --- /dev/null +++ b/text/0000-public-private-dependencies.md @@ -0,0 +1,451 @@ +- Feature Name: `public_private_dependencies` +- Start Date: 2017-04-03 +- RFC PR: [rust-lang/rfcs#1977](https://github.com/rust-lang/rfcs/pull/1977) +- Rust Issue: [rust-lang/rust#44663](https://github.com/rust-lang/rust/issues/44663) + +# Summary +[summary]: #summary + +Introduce a public/private distinction to crate dependencies. + +# Motivation +[motivation]: #motivation + +The crates ecosystem has greatly expanded since Rust 1.0. With that, a few patterns for +dependencies have evolved that challenge the currently existing dependency declaration +system in Cargo and Rust. The most common problem is that a crate `A` depends on another +crate `B` but some of the types from crate `B` are exposed through the API in crate `A`. +This causes problems in practice if that dependency `B` is also used by the user's code +itself, crate `B` resolves to different versions for each usage, and the values of types +from the two crate `B` instances need to be used together but don't match. In this case, +the user's code will refuse to compile because different versions of those libraries are +requested, and the compiler messages are less than clear. + +The introduction of an explicit distinction between public and private dependencies can +solve some of these issues. This distinction should also let us lift some restrictions and +make some code compile that previously was prevented from compiling. + +**Q: What is a public dependency?**
+A: A dependency is public if some of the types or traits of that dependency are themselves +exported through the public API of main crate. The most common places where this happens +are return values and function parameters. The same applies to trait implementations and +many other things. Because "public" can be tricky to determine for a user, this RFC +proposes to extend the compiler infrastructure to detect the concept of a "public +dependency". This will help the user understand this concept so they can avoid making +mistakes in the `Cargo.toml`. + +Effectively, the idea is that if you bump a public dependency's version, it's a breaking +change of your *own* crate. + +**Q: What is a private dependency?**
+A: On the other hand, a private dependency is contained within your crate and effectively +invisible for users of your crate. As a result, private dependencies can be freely +duplicated in the dependency graph and won't cause compilation errors. This distinction +will also make it possible to relax some restrictions that currently exist in Cargo which +sometimes prevent crates from compiling. + +**Q: Can public become private later?**
+A: Public dependencies are public within a reachable subgraph but can become private if a +crate stops exposing a public dependency. For instance, it is very possible to have a +family of crates that all depend on a utility crate that provides common types which is a +public dependency for all of them. However, if your own crate ends up being a user of this +utility crate but none of its types or traits become part of your own API, then this +utility crate dependency is marked private. + +**Q: Where is public / private defined?**
+Dependencies are private by default and are made public through a `public` flag on the +dependency in the `Cargo.toml` file. This also means that crates created before the +implementation of this RFC will have all their dependencies private. + +**Q: How is backwards compatibility handled?**
+A: It will continue to be permissible to "leak" dependencies (and there are even some use +cases of this), however, the compiler or Cargo will emit warnings if private dependencies +are part of the public API. Later, it might even become invalid to publish new crates +without explicitly silencing these warnings or marking the dependencies as public. + +**Q: Can I export a type from a private dependency as my own?**
+A: For now, it will not be strictly permissible to privately depend on a crate and export a +type from there as your own. The reason for this is that at the moment it is not possible +to force this type to be distinct. This means that users of the crate might accidentally +start depending on that type to be compatible if the user starts to depend on the crate +that actually implements that type. The limitations from the previous answer apply (e.g.: +you can currently overrule the restrictions). + +**Q: How do semver and dependencies interact?**
+A: It is already the case that changing your own dependencies would require a semver bump +for your own library because your API contract to the outside world changes. This RFC, +however, makes it possible to only have this requirement for public dependencies and would +permit Cargo to prevent new crate releases with semver violations. + +# Detailed design +[design]: #detailed-design + +There are a few areas that need to be changed for this RFC: + +* The compiler needs to be extended to understand when crate dependencies are + considered a public dependency +* The `Cargo.toml` manifest needs to be extended to support declaring public + dependencies. This will start as an unstable cargo feature available on nightly + and only via opt-in. +* The `public` attribute of dependencies needs to appear in the Cargo index in order + to be used by Cargo during version resolution +* Cargo's version resolution needs to change to reject crate graph resolutions where + two versions of a crate are publicly reachable to each other. +* The `cargo publish` process needs to be changed to warn (or prevent) the publishing + of crates that have undeclared public dependencies +* `cargo publish` will resolve dependencies to the *lowest* possible versions in order to + check that the minimal version specified in `Cargo.toml` is correct. +* Crates.io should show public dependencies more prominently than private ones. + +## Compiler Changes + +The main change to the compiler will be to accept a new parameter that Cargo +supplies which is a list of public dependencies. The flag will be called +`--extern-public`. The compiler then emits warnings if it encounters private +dependencies leaking to the public API of a crate. `cargo publish` might change +this warning into an error in its lint step. + +Additionally, later on, the warning can turn into a hard error in general. + +In some situations, it can be necessary to allow private dependencies to become +part of the public API. In that case one can permit this with +`#[allow(external_private_dependency)]`. This is particularly useful when +paired with `#[doc(hidden)]` and other already existing hacks. + +This most likely will also be necessary for the more complex relationship of +`libcore` and `libstd` in Rust itself. + +## Changes to `Cargo.toml` + +The `Cargo.toml` file will be amended to support the new `public` parameter on +dependencies. Old Cargo versions will emit a warning when this key is encountered +but otherwise continue. Since the default for a dependency to be private only, +public ones will need to be tagged which should be the minority. + +This will start as an unstable Cargo feature available on nightly only that authors +will need to opt into via a feature specified in `Cargo.toml` before Cargo will +start using the `public` attribute to change the way versions are resolved. The +Cargo unstable feature will turn on a corresponding rustc unstable feature for +the compiler changes noted above. + +Example dependency: + +```toml +[dependencies] +url = { version = "1.4.0", public = true } +``` + +## Changes to the Cargo Index + +The [Cargo index](https://github.com/rust-lang/crates.io-index) used by Cargo when +resolving versions will contain the `public` attribute on dependencies as specified +in `Cargo.toml`. For example, an index line for a crate named `example` that +publicly depends on the `url` crate would look like (JSON prettified for legibility): + +```json +{ + "name":"example", + "vers":"0.1.0", + "deps":[ + { + "name":"url", + "req":"^1.4.0", + "public":"true", + "features":[], + "optional":false, + "default_features":true, + "target":null, + "kind":"normal" + } + ] +} +``` + +## Changes to Cargo Version Resolution + +Cargo will specifically reject graphs that contain two different versions of the +same crate being publicly depended upon and reachable from each other. This will +prevent the strange errors possible today at version resolution time rather than at +compile time. + +How this will work: + +* First, a resolution graph has a bunch of nodes. These nodes are "package ids" + which are a triple of (name, source, version). Basically this means that different + versions of the same crate are different nodes, and different sources of the same + name (e.g. git and crates.io) are also different nodes. +* There are *directed edges* between nodes. A directed edge represents a dependency. + For example if A depends on B then there's a directed edge from A to B. +* With public/private dependencies, we can now say that every edge is either tagged + with public or private. +* This means that we can have a collection of subgraphs purely connected by public + dependency edges. The directionality of the public dependency edges within the + subgraph doesn't matter. Each of these subgraphs represents an "ecosystem" of + crates publicly depending on each other. These subgraphs are "pools of public + types" where if you have access to the subgraph, you have access to all types + within that pool of types. +* We can place a constraint that each of these "publicly connected subgraphs" are + required to have exactly one version of all crates internally. For example, each + subgraph can only have one version of Hyper. +* Finally, we can consider all pairs of edges coming out of one node in the + resolution graph. If the two edges point to *two distinct publicly connected + subgraphs from above* and those subgraphs contain two different versions of the + same crate, we consider that an error. This basically means that if you privately + depend on Hyper 0.3 and Hyper 0.4, that's an error. + +## Changes to Cargo Publish: Warnings + +When a new crate version is published, Cargo will warn about types and traits that +the compiler determined to be public but did not come from a public dependency. For +now, it should be possible to publish anyways but in some period in the future it +will be necessary to explicitly mark all public dependencies as such or explicitly +mark them with `#[allow(external_private_dependency)]`. + +## Changes to Cargo Publish: Lowest Version Resolution + +A very common situation today is that people write the initial version of a +dependency in their Cargo.toml, but never bother to update it as they take advantage +of new features in newer versions. This works out okay because (1) Cargo will +generally use the largest version it can find, compatible with constraints, and (2) +upper bounds on constraints (at least within a particular minor version) are +relatively rare. That means, in particular, that Cargo.toml is not a fully accurate +picture of version dependency information; in general it's a lower bound at best. +There can be "invisible" dependencies that don't cause resolution failures but can +create compilation errors as APIs evolve. + +Public dependencies exacerbate the above problem, because you can end up relying on +features of a "new API" from a crate you didn't even know you depended on! For +example: + +- A depends on: + - B 1.0 which publicly depends on C ^1.0 + - D 1.0, which has no dependencies +- When A is initially built, it resolves to B 1.0 and C 1.1. + - Because C's APIs are available to A via re-exports in B, A effectively depends + on C 1.1 now, even though B only claims to depend on C ^1.0 + - In particular, the code in A might depend on APIs only available in C 1.1 + - However, if A is a library, we don't check in any lockfile for it, so this + information is lost. +- Now we change A to depend on D 1.1, which depends on C =1.0 + - A fresh copy of A, when built, will now resolve the crate graph to B 1.0, D 1.1, + C 1.0 + - But now A may suddenly fail to compile, because it was implicitly depending on C + 1.1 features via B. + +This example and others like it rely on a common ingredient: a crate somewhere using +an API that only is available in a newer version of a crate than the version listed +in Cargo.toml. + +To attempt to surface this problem earlier, `cargo publish` will attempt to resolve +the graph while picking the smallest versions compatible with constraints. If the +crate fails to build with this resolution graph, the publish will fail. + +# How We Teach This +[how-we-teach-this]: #how-we-teach-this + +From the user's perspective, the initial scope of the RFC will be quite transparent, +but it will definitely show up for users as a question of what the new restrictions +mean. In particular, a common way to leak out types from APIs that most crates do is +error handling. Quite frequently it happens that users wrap errors from other +libraries in their own types. It might make sense to identify common cases of where +type leakage happens and provide hints in the lint about how to deal with it. + +Cases that I anticipate that should be explained separately: + +* Type leakage through errors: This should be easy to spot for a lint because the + wrapper type will implement `std::error::Error`. The recommendation should most + likely be to encourage wrapping the internal error. +* Traits from other crates: In particular, serde and some other common crates will + show up frequently. It might make sense to separately explain types and traits. +* Type leakage through derive: Users might not be aware they have a dependency on + a type when they derive a trait (think `serde_derive`). The lint might want to + call this out separately. + +The feature will be called `public_private_dependencies` and it comes with one +lint flag called `external_private_dependency`. For all intents and purposes, this +should be the extent of the new terms introduced in the beginning. This RFC, however, +lays the groundwork for later providing aliasing so that a private dependency could +be forcefully re-exported as the crate's own types. As such, it might make sense to +consider how to refer to this. + +It is assumed that this feature will eventually become quite popular due to patterns +that already exist in the crate ecosystem. It's likely that it will evoke some +negative opinions initially. As such, it would be a good idea to make a run with +cargobomb/crater to see what the actual impact of the new linter warnings is and +how far away we are from making them errors. + +Crates.io should be updated to render public and private dependencies separately. + +# End user experience +[end-user-experience]: #end-user-experience + +## Author of a crate with one dependency + +Assume today that an author of a library crate `onedep` has a +dependency on the `url` crate and the `url::Url` type is exposed in +`onedep`'s public API. + +`onedep`'s `Cargo.toml`: + +```toml +[package] +name = "onedep" +version = "0.1.0" + +[dependencies] +url = "1.0.0" +``` + +`onedep`'s `src/lib.rs`: + +```rust +extern crate url; +use url::Origin; + +use std::collections::HashMap; + +#[derive(Default)] +pub struct OriginTracker { + origin_counts: HashMap, +} + +impl OriginTracker { + pub fn log_origin(&mut self, origin: Origin) { + let counter = self.origin_counts.entry(origin).or_insert(0); + *counter += 1; + } +} +``` + +When the author of `onedep` upgrades Rust/Cargo to a version where this RFC is +completely implemented, the author will notice two changes: + +1. When they run `cargo build`, the build will succeed but they will get a warning +that a private dependency (the `url` crate specifically) is used in their public API +(the `url::Origin` type in the `pub fn log_origin` function specifically) and that +they should consider adding `public = true` to their `Cargo.toml`. Ideally the +warning would say something like: + + ``` + consider changing dependency: + + ``` + url = "1.0.0" + ``` + + to: + + ``` + url = { version = "1.0.0", public = true } + ``` + ``` + +The warning could also encourage the author to then bump their crate's major +version since adding public dependencies is a breaking change. + +2. When they run `cargo publish`, the build check that happens after packaging will +fail and the publish will fail. This is because [deriving `Hash` on `url::Origin` +wasn't added until v1.5.1 of the url +crate](https://github.com/servo/rust-url/commit/42603254fac8d4c446183cba73bbaeb2c3b416c2). +The author of `onedep` has been running `cargo update` periodically, and their +`Cargo.lock` has url 1.5.1, but they never updated `Cargo.toml` to indicate that +they have a new lower bound. Since `cargo publish` will try to resolve dependencies +to the lowest possible versions, it will choose version 1.0.0 of the url crate, +which doesn't implement `Hash` on `Origin`. + +There should be a clear error message for this case that indicates Cargo has +resolved crates to their lowest possible versions, that this might be the cause of +the compilation failure, and that the author should investigate the versions of +their dependencies in `Cargo.toml` to see if they should be updated. This command +should change the Cargo.lock so that running `cargo build` will reproduce the error +for the author to fix. + +## Author of a crate with multiple dependencies + +`twodep`'s `Cargo.toml`: + +```toml +[package] +name = "twodep" +version = "0.1.0" + +[dependencies] +// this is the version of onedep above using a public dep on url 1.5.1 +onedep = "1.0.0" +url = "1.0.0" +``` + +`twodep`'s `src/main.rs`: + +```rust +extern crate url; +use url::Origin; + +extern crate onedep; + +fn main() { + let mut origin_tracker = onedep::OriginTracker::default(); + + loop { + println!("Please enter a URL!"); + // pseudocode because I'm lazy + let url = stdin::readline().unwrap(); + let url = Url::parse(url).unwrap(); + origin_tracker.log_origin(url.origin()); + // other stuff + } + println!("Here are all the origins you mentioned: {:#?}", origin_tracker); +} +``` + +Before upgrading Rust/Cargo to a version where this RFC has been implemented, this +code might have been getting a compilation error if Cargo had resolved the direct +dependency on the url crate to a different version than the version of onedep +resolved to. Or it might have been resolving and compiling fine if the versions had +resolved to be the same. + +After upgrading Rust/Cargo, if this code had a compilation error, it would now have +a version resolution problem that cargo would either automatically resolve or prompt +the user to change version constraints/`cargo update` to resolve. If the code was +compiling before, that must mean the previous resolution graph was good, so nothing +will change on upgrading. + +This crate is a binary and doesn't have a public API, so it won't get any warnings +about crates not being marked public. + +If the author publishes to crates.io after upgrading Rust/Cargo, since onedep's +public dependency on url now has a lower bound of 1.5.1, the only valid graphs that +Cargo will generate will be with url 1.5.1 or greater, which is also compatible with +the url 1.0.0 direct dependency. Publish will work without any errors or further +changes. + +# Drawbacks +[drawbacks]: #drawbacks + +I believe that there are no drawbacks if implemented well (this assumes good +linters and error messages). + +# Alternatives +[alternatives]: #alternatives + +For me, the biggest alternative to this RFC would be a variation of it where type +and trait aliasing becomes immediately part of it. This would mean that a crate +can have a private dependency and re-export it as its own type, hiding where it +came from originally. This would most likely be easier to teach users and can get +rid of a few "cul-de-sac" situations users can end up in where their only way +out is to introduce a public dependency for now. The assumption is that if trait +and type aliasing is available, the `external_public_dependency` would not need to +exist. + +# Unresolved questions +[unresolved]: #unresolved-questions + +There are a few open questions about how to best hook into the compiler and Cargo +infrastructure: + +* What is the impact of this change going to be? This most likely can be answered + running cargobomb/crater. +* Since changing public dependency pins/ranges requires a change in semver, it might + be worth exploring if Cargo could prevent the user from publishing new crate + versions that violate that constraint. +* If this is implemented before [the RFC to deprecate `extern crate`](https://github.com/rust-lang/rfcs/pull/2126), how would this work if you're not using `--extern`? From 2012050a1133dff71a3c388a9ba11fc1105f31bf Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 13 Oct 2023 09:35:19 -0500 Subject: [PATCH 02/43] refactor: Update to latest RFC template --- text/0000-public-private-dependencies.md | 346 ++++++++++++----------- 1 file changed, 176 insertions(+), 170 deletions(-) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index a95a78799b1..85d6b531ea8 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -77,171 +77,8 @@ for your own library because your API contract to the outside world changes. Thi however, makes it possible to only have this requirement for public dependencies and would permit Cargo to prevent new crate releases with semver violations. -# Detailed design -[design]: #detailed-design - -There are a few areas that need to be changed for this RFC: - -* The compiler needs to be extended to understand when crate dependencies are - considered a public dependency -* The `Cargo.toml` manifest needs to be extended to support declaring public - dependencies. This will start as an unstable cargo feature available on nightly - and only via opt-in. -* The `public` attribute of dependencies needs to appear in the Cargo index in order - to be used by Cargo during version resolution -* Cargo's version resolution needs to change to reject crate graph resolutions where - two versions of a crate are publicly reachable to each other. -* The `cargo publish` process needs to be changed to warn (or prevent) the publishing - of crates that have undeclared public dependencies -* `cargo publish` will resolve dependencies to the *lowest* possible versions in order to - check that the minimal version specified in `Cargo.toml` is correct. -* Crates.io should show public dependencies more prominently than private ones. - -## Compiler Changes - -The main change to the compiler will be to accept a new parameter that Cargo -supplies which is a list of public dependencies. The flag will be called -`--extern-public`. The compiler then emits warnings if it encounters private -dependencies leaking to the public API of a crate. `cargo publish` might change -this warning into an error in its lint step. - -Additionally, later on, the warning can turn into a hard error in general. - -In some situations, it can be necessary to allow private dependencies to become -part of the public API. In that case one can permit this with -`#[allow(external_private_dependency)]`. This is particularly useful when -paired with `#[doc(hidden)]` and other already existing hacks. - -This most likely will also be necessary for the more complex relationship of -`libcore` and `libstd` in Rust itself. - -## Changes to `Cargo.toml` - -The `Cargo.toml` file will be amended to support the new `public` parameter on -dependencies. Old Cargo versions will emit a warning when this key is encountered -but otherwise continue. Since the default for a dependency to be private only, -public ones will need to be tagged which should be the minority. - -This will start as an unstable Cargo feature available on nightly only that authors -will need to opt into via a feature specified in `Cargo.toml` before Cargo will -start using the `public` attribute to change the way versions are resolved. The -Cargo unstable feature will turn on a corresponding rustc unstable feature for -the compiler changes noted above. - -Example dependency: - -```toml -[dependencies] -url = { version = "1.4.0", public = true } -``` - -## Changes to the Cargo Index - -The [Cargo index](https://github.com/rust-lang/crates.io-index) used by Cargo when -resolving versions will contain the `public` attribute on dependencies as specified -in `Cargo.toml`. For example, an index line for a crate named `example` that -publicly depends on the `url` crate would look like (JSON prettified for legibility): - -```json -{ - "name":"example", - "vers":"0.1.0", - "deps":[ - { - "name":"url", - "req":"^1.4.0", - "public":"true", - "features":[], - "optional":false, - "default_features":true, - "target":null, - "kind":"normal" - } - ] -} -``` - -## Changes to Cargo Version Resolution - -Cargo will specifically reject graphs that contain two different versions of the -same crate being publicly depended upon and reachable from each other. This will -prevent the strange errors possible today at version resolution time rather than at -compile time. - -How this will work: - -* First, a resolution graph has a bunch of nodes. These nodes are "package ids" - which are a triple of (name, source, version). Basically this means that different - versions of the same crate are different nodes, and different sources of the same - name (e.g. git and crates.io) are also different nodes. -* There are *directed edges* between nodes. A directed edge represents a dependency. - For example if A depends on B then there's a directed edge from A to B. -* With public/private dependencies, we can now say that every edge is either tagged - with public or private. -* This means that we can have a collection of subgraphs purely connected by public - dependency edges. The directionality of the public dependency edges within the - subgraph doesn't matter. Each of these subgraphs represents an "ecosystem" of - crates publicly depending on each other. These subgraphs are "pools of public - types" where if you have access to the subgraph, you have access to all types - within that pool of types. -* We can place a constraint that each of these "publicly connected subgraphs" are - required to have exactly one version of all crates internally. For example, each - subgraph can only have one version of Hyper. -* Finally, we can consider all pairs of edges coming out of one node in the - resolution graph. If the two edges point to *two distinct publicly connected - subgraphs from above* and those subgraphs contain two different versions of the - same crate, we consider that an error. This basically means that if you privately - depend on Hyper 0.3 and Hyper 0.4, that's an error. - -## Changes to Cargo Publish: Warnings - -When a new crate version is published, Cargo will warn about types and traits that -the compiler determined to be public but did not come from a public dependency. For -now, it should be possible to publish anyways but in some period in the future it -will be necessary to explicitly mark all public dependencies as such or explicitly -mark them with `#[allow(external_private_dependency)]`. - -## Changes to Cargo Publish: Lowest Version Resolution - -A very common situation today is that people write the initial version of a -dependency in their Cargo.toml, but never bother to update it as they take advantage -of new features in newer versions. This works out okay because (1) Cargo will -generally use the largest version it can find, compatible with constraints, and (2) -upper bounds on constraints (at least within a particular minor version) are -relatively rare. That means, in particular, that Cargo.toml is not a fully accurate -picture of version dependency information; in general it's a lower bound at best. -There can be "invisible" dependencies that don't cause resolution failures but can -create compilation errors as APIs evolve. - -Public dependencies exacerbate the above problem, because you can end up relying on -features of a "new API" from a crate you didn't even know you depended on! For -example: - -- A depends on: - - B 1.0 which publicly depends on C ^1.0 - - D 1.0, which has no dependencies -- When A is initially built, it resolves to B 1.0 and C 1.1. - - Because C's APIs are available to A via re-exports in B, A effectively depends - on C 1.1 now, even though B only claims to depend on C ^1.0 - - In particular, the code in A might depend on APIs only available in C 1.1 - - However, if A is a library, we don't check in any lockfile for it, so this - information is lost. -- Now we change A to depend on D 1.1, which depends on C =1.0 - - A fresh copy of A, when built, will now resolve the crate graph to B 1.0, D 1.1, - C 1.0 - - But now A may suddenly fail to compile, because it was implicitly depending on C - 1.1 features via B. - -This example and others like it rely on a common ingredient: a crate somewhere using -an API that only is available in a newer version of a crate than the version listed -in Cargo.toml. - -To attempt to surface this problem earlier, `cargo publish` will attempt to resolve -the graph while picking the smallest versions compatible with constraints. If the -crate fails to build with this resolution graph, the publish will fail. - -# How We Teach This -[how-we-teach-this]: #how-we-teach-this +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation From the user's perspective, the initial scope of the RFC will be quite transparent, but it will definitely show up for users as a question of what the new restrictions @@ -276,10 +113,10 @@ how far away we are from making them errors. Crates.io should be updated to render public and private dependencies separately. -# End user experience +## End user experience [end-user-experience]: #end-user-experience -## Author of a crate with one dependency +### Author of a crate with one dependency Assume today that an author of a library crate `onedep` has a dependency on the `url` crate and the `url::Url` type is exposed in @@ -360,7 +197,7 @@ their dependencies in `Cargo.toml` to see if they should be updated. This comman should change the Cargo.lock so that running `cargo build` will reproduce the error for the author to fix. -## Author of a crate with multiple dependencies +### Author of a crate with multiple dependencies `twodep`'s `Cargo.toml`: @@ -419,14 +256,177 @@ Cargo will generate will be with url 1.5.1 or greater, which is also compatible the url 1.0.0 direct dependency. Publish will work without any errors or further changes. +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +There are a few areas that need to be changed for this RFC: + +* The compiler needs to be extended to understand when crate dependencies are + considered a public dependency +* The `Cargo.toml` manifest needs to be extended to support declaring public + dependencies. This will start as an unstable cargo feature available on nightly + and only via opt-in. +* The `public` attribute of dependencies needs to appear in the Cargo index in order + to be used by Cargo during version resolution +* Cargo's version resolution needs to change to reject crate graph resolutions where + two versions of a crate are publicly reachable to each other. +* The `cargo publish` process needs to be changed to warn (or prevent) the publishing + of crates that have undeclared public dependencies +* `cargo publish` will resolve dependencies to the *lowest* possible versions in order to + check that the minimal version specified in `Cargo.toml` is correct. +* Crates.io should show public dependencies more prominently than private ones. + +## Compiler Changes + +The main change to the compiler will be to accept a new parameter that Cargo +supplies which is a list of public dependencies. The flag will be called +`--extern-public`. The compiler then emits warnings if it encounters private +dependencies leaking to the public API of a crate. `cargo publish` might change +this warning into an error in its lint step. + +Additionally, later on, the warning can turn into a hard error in general. + +In some situations, it can be necessary to allow private dependencies to become +part of the public API. In that case one can permit this with +`#[allow(external_private_dependency)]`. This is particularly useful when +paired with `#[doc(hidden)]` and other already existing hacks. + +This most likely will also be necessary for the more complex relationship of +`libcore` and `libstd` in Rust itself. + +## Changes to `Cargo.toml` + +The `Cargo.toml` file will be amended to support the new `public` parameter on +dependencies. Old Cargo versions will emit a warning when this key is encountered +but otherwise continue. Since the default for a dependency to be private only, +public ones will need to be tagged which should be the minority. + +This will start as an unstable Cargo feature available on nightly only that authors +will need to opt into via a feature specified in `Cargo.toml` before Cargo will +start using the `public` attribute to change the way versions are resolved. The +Cargo unstable feature will turn on a corresponding rustc unstable feature for +the compiler changes noted above. + +Example dependency: + +```toml +[dependencies] +url = { version = "1.4.0", public = true } +``` + +## Changes to the Cargo Index + +The [Cargo index](https://github.com/rust-lang/crates.io-index) used by Cargo when +resolving versions will contain the `public` attribute on dependencies as specified +in `Cargo.toml`. For example, an index line for a crate named `example` that +publicly depends on the `url` crate would look like (JSON prettified for legibility): + +```json +{ + "name":"example", + "vers":"0.1.0", + "deps":[ + { + "name":"url", + "req":"^1.4.0", + "public":"true", + "features":[], + "optional":false, + "default_features":true, + "target":null, + "kind":"normal" + } + ] +} +``` + +## Changes to Cargo Version Resolution + +Cargo will specifically reject graphs that contain two different versions of the +same crate being publicly depended upon and reachable from each other. This will +prevent the strange errors possible today at version resolution time rather than at +compile time. + +How this will work: + +* First, a resolution graph has a bunch of nodes. These nodes are "package ids" + which are a triple of (name, source, version). Basically this means that different + versions of the same crate are different nodes, and different sources of the same + name (e.g. git and crates.io) are also different nodes. +* There are *directed edges* between nodes. A directed edge represents a dependency. + For example if A depends on B then there's a directed edge from A to B. +* With public/private dependencies, we can now say that every edge is either tagged + with public or private. +* This means that we can have a collection of subgraphs purely connected by public + dependency edges. The directionality of the public dependency edges within the + subgraph doesn't matter. Each of these subgraphs represents an "ecosystem" of + crates publicly depending on each other. These subgraphs are "pools of public + types" where if you have access to the subgraph, you have access to all types + within that pool of types. +* We can place a constraint that each of these "publicly connected subgraphs" are + required to have exactly one version of all crates internally. For example, each + subgraph can only have one version of Hyper. +* Finally, we can consider all pairs of edges coming out of one node in the + resolution graph. If the two edges point to *two distinct publicly connected + subgraphs from above* and those subgraphs contain two different versions of the + same crate, we consider that an error. This basically means that if you privately + depend on Hyper 0.3 and Hyper 0.4, that's an error. + +## Changes to Cargo Publish: Warnings + +When a new crate version is published, Cargo will warn about types and traits that +the compiler determined to be public but did not come from a public dependency. For +now, it should be possible to publish anyways but in some period in the future it +will be necessary to explicitly mark all public dependencies as such or explicitly +mark them with `#[allow(external_private_dependency)]`. + +## Changes to Cargo Publish: Lowest Version Resolution + +A very common situation today is that people write the initial version of a +dependency in their Cargo.toml, but never bother to update it as they take advantage +of new features in newer versions. This works out okay because (1) Cargo will +generally use the largest version it can find, compatible with constraints, and (2) +upper bounds on constraints (at least within a particular minor version) are +relatively rare. That means, in particular, that Cargo.toml is not a fully accurate +picture of version dependency information; in general it's a lower bound at best. +There can be "invisible" dependencies that don't cause resolution failures but can +create compilation errors as APIs evolve. + +Public dependencies exacerbate the above problem, because you can end up relying on +features of a "new API" from a crate you didn't even know you depended on! For +example: + +- A depends on: + - B 1.0 which publicly depends on C ^1.0 + - D 1.0, which has no dependencies +- When A is initially built, it resolves to B 1.0 and C 1.1. + - Because C's APIs are available to A via re-exports in B, A effectively depends + on C 1.1 now, even though B only claims to depend on C ^1.0 + - In particular, the code in A might depend on APIs only available in C 1.1 + - However, if A is a library, we don't check in any lockfile for it, so this + information is lost. +- Now we change A to depend on D 1.1, which depends on C =1.0 + - A fresh copy of A, when built, will now resolve the crate graph to B 1.0, D 1.1, + C 1.0 + - But now A may suddenly fail to compile, because it was implicitly depending on C + 1.1 features via B. + +This example and others like it rely on a common ingredient: a crate somewhere using +an API that only is available in a newer version of a crate than the version listed +in Cargo.toml. + +To attempt to surface this problem earlier, `cargo publish` will attempt to resolve +the graph while picking the smallest versions compatible with constraints. If the +crate fails to build with this resolution graph, the publish will fail. + # Drawbacks [drawbacks]: #drawbacks I believe that there are no drawbacks if implemented well (this assumes good linters and error messages). -# Alternatives -[alternatives]: #alternatives +# Rationale and alternatives +[rationale-and-alternatives]: #rationale-and-alternatives For me, the biggest alternative to this RFC would be a variation of it where type and trait aliasing becomes immediately part of it. This would mean that a crate @@ -437,6 +437,9 @@ out is to introduce a public dependency for now. The assumption is that if trait and type aliasing is available, the `external_public_dependency` would not need to exist. +# Prior art +[prior-art]: #prior-art + # Unresolved questions [unresolved]: #unresolved-questions @@ -449,3 +452,6 @@ infrastructure: be worth exploring if Cargo could prevent the user from publishing new crate versions that violate that constraint. * If this is implemented before [the RFC to deprecate `extern crate`](https://github.com/rust-lang/rfcs/pull/2126), how would this work if you're not using `--extern`? + +# Future possibilities +[future-possibilities]: #future-possibilities From c0eb5bfdd9091e1b481307eab72924401c9d1ca9 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 13 Oct 2023 11:39:11 -0500 Subject: [PATCH 03/43] feat: Initial draft for new proposal --- text/0000-public-private-dependencies.md | 527 ++++++++--------------- 1 file changed, 173 insertions(+), 354 deletions(-) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index 85d6b531ea8..ecc7ec3d48e 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -1,6 +1,7 @@ - Feature Name: `public_private_dependencies` -- Start Date: 2017-04-03 -- RFC PR: [rust-lang/rfcs#1977](https://github.com/rust-lang/rfcs/pull/1977) +- Start Date: 2023-10-13 +- Prior RFC PR: [rust-lang/rfcs#1977](https://github.com/rust-lang/rfcs/pull/1977) +- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) - Rust Issue: [rust-lang/rust#44663](https://github.com/rust-lang/rust/issues/44663) # Summary @@ -8,6 +9,11 @@ Introduce a public/private distinction to crate dependencies. +Note: this supersedes [RFC 1977] +Enough has changed in the time since that RFC was approved that we felt we needed to go back and get wider input on this, rather than handling decisions through the tracking issue. +- [RFC 1977] was written before Editions, `cfg` dependencies, and package renaming which can all affect it +- The resolver changes were a large part of [RFC 1977] but there are concerns with it and we feel it'd be best to decouple it, offering a faster path to stabilization + # Motivation [motivation]: #motivation @@ -15,272 +21,132 @@ The crates ecosystem has greatly expanded since Rust 1.0. With that, a few patte dependencies have evolved that challenge the currently existing dependency declaration system in Cargo and Rust. The most common problem is that a crate `A` depends on another crate `B` but some of the types from crate `B` are exposed through the API in crate `A`. -This causes problems in practice if that dependency `B` is also used by the user's code -itself, crate `B` resolves to different versions for each usage, and the values of types -from the two crate `B` instances need to be used together but don't match. In this case, -the user's code will refuse to compile because different versions of those libraries are -requested, and the compiler messages are less than clear. - -The introduction of an explicit distinction between public and private dependencies can -solve some of these issues. This distinction should also let us lift some restrictions and -make some code compile that previously was prevented from compiling. - -**Q: What is a public dependency?**
-A: A dependency is public if some of the types or traits of that dependency are themselves -exported through the public API of main crate. The most common places where this happens -are return values and function parameters. The same applies to trait implementations and -many other things. Because "public" can be tricky to determine for a user, this RFC -proposes to extend the compiler infrastructure to detect the concept of a "public -dependency". This will help the user understand this concept so they can avoid making -mistakes in the `Cargo.toml`. - -Effectively, the idea is that if you bump a public dependency's version, it's a breaking -change of your *own* crate. - -**Q: What is a private dependency?**
-A: On the other hand, a private dependency is contained within your crate and effectively -invisible for users of your crate. As a result, private dependencies can be freely -duplicated in the dependency graph and won't cause compilation errors. This distinction -will also make it possible to relax some restrictions that currently exist in Cargo which -sometimes prevent crates from compiling. - -**Q: Can public become private later?**
-A: Public dependencies are public within a reachable subgraph but can become private if a -crate stops exposing a public dependency. For instance, it is very possible to have a -family of crates that all depend on a utility crate that provides common types which is a -public dependency for all of them. However, if your own crate ends up being a user of this -utility crate but none of its types or traits become part of your own API, then this -utility crate dependency is marked private. - -**Q: Where is public / private defined?**
-Dependencies are private by default and are made public through a `public` flag on the -dependency in the `Cargo.toml` file. This also means that crates created before the -implementation of this RFC will have all their dependencies private. - -**Q: How is backwards compatibility handled?**
-A: It will continue to be permissible to "leak" dependencies (and there are even some use -cases of this), however, the compiler or Cargo will emit warnings if private dependencies -are part of the public API. Later, it might even become invalid to publish new crates -without explicitly silencing these warnings or marking the dependencies as public. - -**Q: Can I export a type from a private dependency as my own?**
-A: For now, it will not be strictly permissible to privately depend on a crate and export a -type from there as your own. The reason for this is that at the moment it is not possible -to force this type to be distinct. This means that users of the crate might accidentally -start depending on that type to be compatible if the user starts to depend on the crate -that actually implements that type. The limitations from the previous answer apply (e.g.: -you can currently overrule the restrictions). - -**Q: How do semver and dependencies interact?**
-A: It is already the case that changing your own dependencies would require a semver bump -for your own library because your API contract to the outside world changes. This RFC, -however, makes it possible to only have this requirement for public dependencies and would -permit Cargo to prevent new crate releases with semver violations. + +- Poor error messages when a user directly depends on `A` and `B` but with a version requirement on `B` that is semver incompatible with `A`s version requirement on `B` +- Brittle semver compatibility as `A` might not have intended to expose `B`, like with `impl From for AError` +- When self-hosting documentation, you may want to render documentation for all of your public dependencies as well +- When running `cargo doc`, users may way to render [documentation for their accessible dependencies](https://github.com/rust-lang/cargo/issues/2025) [without the cost of their inaccessible dependencies](https://github.com/rust-lang/cargo/issues/4049) +- When linting for semver compatibility [there isn't enough information](https://github.com/obi1kenobi/cargo-semver-checks/issues/121) # Guide-level explanation [guide-level-explanation]: #guide-level-explanation -From the user's perspective, the initial scope of the RFC will be quite transparent, -but it will definitely show up for users as a question of what the new restrictions -mean. In particular, a common way to leak out types from APIs that most crates do is -error handling. Quite frequently it happens that users wrap errors from other -libraries in their own types. It might make sense to identify common cases of where -type leakage happens and provide hints in the lint about how to deal with it. - -Cases that I anticipate that should be explained separately: - -* Type leakage through errors: This should be easy to spot for a lint because the - wrapper type will implement `std::error::Error`. The recommendation should most - likely be to encourage wrapping the internal error. -* Traits from other crates: In particular, serde and some other common crates will - show up frequently. It might make sense to separately explain types and traits. -* Type leakage through derive: Users might not be aware they have a dependency on - a type when they derive a trait (think `serde_derive`). The lint might want to - call this out separately. - -The feature will be called `public_private_dependencies` and it comes with one -lint flag called `external_private_dependency`. For all intents and purposes, this -should be the extent of the new terms introduced in the beginning. This RFC, however, -lays the groundwork for later providing aliasing so that a private dependency could -be forcefully re-exported as the crate's own types. As such, it might make sense to -consider how to refer to this. - -It is assumed that this feature will eventually become quite popular due to patterns -that already exist in the crate ecosystem. It's likely that it will evoke some -negative opinions initially. As such, it would be a good idea to make a run with -cargobomb/crater to see what the actual impact of the new linter warnings is and -how far away we are from making them errors. - -Crates.io should be updated to render public and private dependencies separately. - -## End user experience -[end-user-experience]: #end-user-experience - -### Author of a crate with one dependency - -Assume today that an author of a library crate `onedep` has a -dependency on the `url` crate and the `url::Url` type is exposed in -`onedep`'s public API. - -`onedep`'s `Cargo.toml`: - +As a trivial example: ```toml [package] -name = "onedep" -version = "0.1.0" +name = "diagnostic" +version = "1.0.0" [dependencies] -url = "1.0.0" +serde = { version = "1", features = ["derive"] } +serde_json = "1" ``` - -`onedep`'s `src/lib.rs`: - ```rust -extern crate url; -use url::Origin; - -use std::collections::HashMap; - -#[derive(Default)] -pub struct OriginTracker { - origin_counts: HashMap, +#[derive(Clone, Debug, PartialEq, Eq, serde::Deserialize, serde::Serialize)] +pub struct Diagnostic { + code: String, + message: String, + file: std::path::PathBuf, + span: std::ops::Range, } -impl OriginTracker { - pub fn log_origin(&mut self, origin: Origin) { - let counter = self.origin_counts.entry(origin).or_insert(0); - *counter += 1; +impl std::str::FromStr for Diagnostic { + type Err = serde_json::Error; + + fn from_str(s: &str) -> Result { + serde_json::from_str(s) } } ``` -When the author of `onedep` upgrades Rust/Cargo to a version where this RFC is -completely implemented, the author will notice two changes: - -1. When they run `cargo build`, the build will succeed but they will get a warning -that a private dependency (the `url` crate specifically) is used in their public API -(the `url::Origin` type in the `pub fn log_origin` function specifically) and that -they should consider adding `public = true` to their `Cargo.toml`. Ideally the -warning would say something like: - - ``` - consider changing dependency: - - ``` - url = "1.0.0" - ``` - - to: - - ``` - url = { version = "1.0.0", public = true } - ``` - ``` - -The warning could also encourage the author to then bump their crate's major -version since adding public dependencies is a breaking change. +The dependencies `serde` and `serde_json` are both public dependencies, meaning their types are referenced in the public API. +Effectively, the idea is that if you do a semver incompatible upgrade to a +public dependency, it's a breaking change of your *own* crate. -2. When they run `cargo publish`, the build check that happens after packaging will -fail and the publish will fail. This is because [deriving `Hash` on `url::Origin` -wasn't added until v1.5.1 of the url -crate](https://github.com/servo/rust-url/commit/42603254fac8d4c446183cba73bbaeb2c3b416c2). -The author of `onedep` has been running `cargo update` periodically, and their -`Cargo.lock` has url 1.5.1, but they never updated `Cargo.toml` to indicate that -they have a new lower bound. Since `cargo publish` will try to resolve dependencies -to the lowest possible versions, it will choose version 1.0.0 of the url crate, -which doesn't implement `Hash` on `Origin`. - -There should be a clear error message for this case that indicates Cargo has -resolved crates to their lowest possible versions, that this might be the cause of -the compilation failure, and that the author should investigate the versions of -their dependencies in `Cargo.toml` to see if they should be updated. This command -should change the Cargo.lock so that running `cargo build` will reproduce the error -for the author to fix. - -### Author of a crate with multiple dependencies - -`twodep`'s `Cargo.toml`: +With this RFC, in pre-2024 editions, this will warn saying that `serde` and `serde_json` are private dependencies in a public API. +In 2024+ editions, this will be an error. +To resolve this in a semver compatible way, they would need to declare both dependencies as public: ```toml [package] -name = "twodep" -version = "0.1.0" +name = "diagnostic" +version = "1.0.0" [dependencies] -// this is the version of onedep above using a public dep on url 1.5.1 -onedep = "1.0.0" -url = "1.0.0" +serde = { version = "1", features = ["derive"], pub = true } +serde_json = { version = "1", pub = true } ``` +For edition migrations, `cargo fix` will look for the warning code and mark those dependencies as `pub`. -`twodep`'s `src/main.rs`: +However, for this example, it was an oversight in exposing `serde_json` in the public API. +Removing it from the public API is a semver incompatible change. +```toml +[package] +name = "diagnostic" +version = "1.0.0" +[dependencies] +serde = { version = "1", features = ["derive"], pub = true } +serde_json = "1" +``` ```rust -extern crate url; -use url::Origin; - -extern crate onedep; +#[derive(Clone, Debug, PartialEq, Eq, serde::Deserialize, serde::Serialize)] +pub struct Diagnostic { + code: String, + message: String, + file: std::path::PathBuf, + span: std::ops::Range, +} -fn main() { - let mut origin_tracker = onedep::OriginTracker::default(); +impl std::str::FromStr for Diagnostic { + type Err = Error - loop { - println!("Please enter a URL!"); - // pseudocode because I'm lazy - let url = stdin::readline().unwrap(); - let url = Url::parse(url).unwrap(); - origin_tracker.log_origin(url.origin()); - // other stuff + fn from_str(s: &str) -> Result { + serde_json::from_str(s).map_err(Error) } - println!("Here are all the origins you mentioned: {:#?}", origin_tracker); } -``` - -Before upgrading Rust/Cargo to a version where this RFC has been implemented, this -code might have been getting a compilation error if Cargo had resolved the direct -dependency on the url crate to a different version than the version of onedep -resolved to. Or it might have been resolving and compiling fine if the versions had -resolved to be the same. -After upgrading Rust/Cargo, if this code had a compilation error, it would now have -a version resolution problem that cargo would either automatically resolve or prompt -the user to change version constraints/`cargo update` to resolve. If the code was -compiling before, that must mean the previous resolution graph was good, so nothing -will change on upgrading. +pub struct Error(serde_json::Error); +``` -This crate is a binary and doesn't have a public API, so it won't get any warnings -about crates not being marked public. +If you then had a public dependency on `diagnostic`, +then `serde` would automatically be considered a public dependency of yours. -If the author publishes to crates.io after upgrading Rust/Cargo, since onedep's -public dependency on url now has a lower bound of 1.5.1, the only valid graphs that -Cargo will generate will be with url 1.5.1 or greater, which is also compatible with -the url 1.0.0 direct dependency. Publish will work without any errors or further -changes. +At times, some public dependencies are effectively private. +Take this code from older versions of `clap` +```rust +#[doc(hidden)] +#[cfg(feature = "derive")] +pub mod __derive_refs { + #[doc(hidden)] + pub use once_cell; +} +``` +Since the proc-macro can only guarantee that the namespace `clap` is accessible, +`clap` must re-export any functionality that is needed at runtime. +As a last-ditch way of dealing with this, a user may allow the error: +```rust +#[doc(hidden)] +#[allow(external_private_dependency)] +#[cfg(feature = "derive")] +pub mod __derive_refs { + #[doc(hidden)] + pub use once_cell; +} +``` +I say "last ditch" because in cases outside of `#[doc(hidden)]` items for macros, +a user would be better served by other features, +like `impl Trait` in type aliases if we had it. # Reference-level explanation [reference-level-explanation]: #reference-level-explanation -There are a few areas that need to be changed for this RFC: - -* The compiler needs to be extended to understand when crate dependencies are - considered a public dependency -* The `Cargo.toml` manifest needs to be extended to support declaring public - dependencies. This will start as an unstable cargo feature available on nightly - and only via opt-in. -* The `public` attribute of dependencies needs to appear in the Cargo index in order - to be used by Cargo during version resolution -* Cargo's version resolution needs to change to reject crate graph resolutions where - two versions of a crate are publicly reachable to each other. -* The `cargo publish` process needs to be changed to warn (or prevent) the publishing - of crates that have undeclared public dependencies -* `cargo publish` will resolve dependencies to the *lowest* possible versions in order to - check that the minimal version specified in `Cargo.toml` is correct. -* Crates.io should show public dependencies more prominently than private ones. - -## Compiler Changes - -The main change to the compiler will be to accept a new parameter that Cargo -supplies which is a list of public dependencies. The flag will be called -`--extern-public`. The compiler then emits warnings if it encounters private +## rustc + +The main change to the compiler will be to accept a new modifier on the `--extern` flag that Cargo +supplies which is a list of public dependencies. +The mode will be called `priv` (e.g. `--extern priv:serde`). +The compiler then emits warnings if it encounters private dependencies leaking to the public API of a crate. `cargo publish` might change this warning into an error in its lint step. @@ -294,53 +160,86 @@ paired with `#[doc(hidden)]` and other already existing hacks. This most likely will also be necessary for the more complex relationship of `libcore` and `libstd` in Rust itself. -## Changes to `Cargo.toml` +## cargo -The `Cargo.toml` file will be amended to support the new `public` parameter on -dependencies. Old Cargo versions will emit a warning when this key is encountered -but otherwise continue. Since the default for a dependency to be private only, -public ones will need to be tagged which should be the minority. +A new dependency field, `pub = ` will be added that defaults to `false`. +Old cargo version will emit a warning when this key is encountered but otherwise continue. +Cargo will use use the `priv` modifier with `--extern` for all private dependencies when building a `lib`. +What is private is what is left after recursively walking public dependencies. +We'll ignore this for other build target kinds (e.g. `bin`) as that would create extra noise. -This will start as an unstable Cargo feature available on nightly only that authors -will need to opt into via a feature specified in `Cargo.toml` before Cargo will -start using the `public` attribute to change the way versions are resolved. The -Cargo unstable feature will turn on a corresponding rustc unstable feature for -the compiler changes noted above. +Cargo will not force a `rust-version` bump when using this feature as someone +building with an old version of cargo depending on packages that set `pub = +true` will not start to fail when upgrading to new versions of cargo. -Example dependency: +`cargo add` will gain `--pub ` flags to control this field. +When adding a dependency today, the version requirement is reused from other dependency tables within your manifest. +With this RFC, that will be extended to also checking your dependencies for any `pub` dependencies, and reusing their version requirement. +This would be most easily done by having the field in the Index but `cargo add` could also read the `.crate` files as a fallback. -```toml -[dependencies] -url = { version = "1.4.0", public = true } -``` +## crates.io -## Changes to the Cargo Index - -The [Cargo index](https://github.com/rust-lang/crates.io-index) used by Cargo when -resolving versions will contain the `public` attribute on dependencies as specified -in `Cargo.toml`. For example, an index line for a crate named `example` that -publicly depends on the `url` crate would look like (JSON prettified for legibility): - -```json -{ - "name":"example", - "vers":"0.1.0", - "deps":[ - { - "name":"url", - "req":"^1.4.0", - "public":"true", - "features":[], - "optional":false, - "default_features":true, - "target":null, - "kind":"normal" - } - ] -} -``` +Crates.io should show public dependencies more prominently than private ones. + +# Drawbacks +[drawbacks]: #drawbacks + +This doesn't cover the case where a dependency is public only if a feature is enabled. + +In the case where you depend on `foo = "300"`, there isn't a way to clarify that what is public is actually from `foo-core = "1"` without explicitly depending on it. + +You can't definitively lint when a `pub = true` is unused since it may depend on which platform or features. + +The warning is emitted even when a `pub` item isn't accessible. + +# Rationale and alternatives +[rationale-and-alternatives]: #rationale-and-alternatives + +## Misc + +- `cargo add`: instead of `--pub ` it could be `--pub` / `--no-pub` like `--optional` or `--public` / `--private` +- `cargo add`: when adding a dependency, we could automatically add all of its `pub` dependencies. + - This was passed up as being too noisy, especially when dealing with facade crates, those that fully re-export their `pub = true` dependency +- `Cargo.toml`: Instead of `pub = false` being the default and changing the warning level on an edition boundary, we could instead start with `pub = true` and change the default on an edition boundary. + - This would require `cargo fix` marking all dependencies as `pub = true`, while using the warning means we can limit it to only those dependencies that need it. +- `Cargo.toml`: Instead of `pub = false` being the default, we could have a "unchecked" / "unset" state + - This would require `cargo fix` marking all dependencies as `pub = true`, while using the warning means we can limit it to only those dependencies that need it. +- `Cargo.toml`: In the long term, we decided on the default being `pub = false` as that is the common case and gives us more information than supporting a `pub = "unchecked"` and having that be the long term solution. +- `Cargo.toml`: instead of `pub` (named after the [Rust keyword](https://doc.rust-lang.org/reference/visibility-and-privacy.html), we could name the field `public` (like [RFC 1977]) or name the field `visibility = ""` + - The parallel with Rust seemed worth pursuing + - While `visibility` would offer greater flexibility, it is unclear if we need that flexibility and if the friction of any feature leveraging it would be worth it + +## Minimal version resolution + +[RFC 1977] included the idea of verifying version requirements are high enough. +This is a problem whether the dependency is private or not. +This should be handled independent of this RFC. + +## Dependency visibility and the resolver + +This is deferred to [Future possibilities](#future-possibilities) +- This has been the main hang-up for stabilization over the last 6 years since the RFC was approved + - For more on the complexity involved, see the thread starting at [this comment](https://github.com/rust-lang/rust/issues/44663#issuecomment-881965668) +- More thought is needed as we found that making a dependency `pub = true` can be a breaking change if the caller also depends on it but with a different semver incompatible version +- More thought is needed on what happens if you have multiple versions of a package that are public (via renaming like `tokio_03` and `tokio_1`) + +Affects of deferring this out +- It is hoped that the resolver change would help with [cargo#9029](https://github.com/rust-lang/cargo/issues/9029) +- If we allow duplication of private semver compatible dependencies, it would help with [cargo#10053](https://github.com/rust-lang/cargo/issues/10053) + +# Prior art +[prior-art]: #prior-art + +Within the cargo ecosystem: +- [cargo public-api-crates](https://github.com/davidpdrsn/cargo-public-api-crates) + +# Unresolved questions +[unresolved]: #unresolved-questions + +# Future possibilities +[future-possibilities]: #future-possibilities -## Changes to Cargo Version Resolution +## Dependency visibility and the resolver Cargo will specifically reject graphs that contain two different versions of the same crate being publicly depended upon and reachable from each other. This will @@ -372,86 +271,6 @@ How this will work: same crate, we consider that an error. This basically means that if you privately depend on Hyper 0.3 and Hyper 0.4, that's an error. -## Changes to Cargo Publish: Warnings - -When a new crate version is published, Cargo will warn about types and traits that -the compiler determined to be public but did not come from a public dependency. For -now, it should be possible to publish anyways but in some period in the future it -will be necessary to explicitly mark all public dependencies as such or explicitly -mark them with `#[allow(external_private_dependency)]`. - -## Changes to Cargo Publish: Lowest Version Resolution - -A very common situation today is that people write the initial version of a -dependency in their Cargo.toml, but never bother to update it as they take advantage -of new features in newer versions. This works out okay because (1) Cargo will -generally use the largest version it can find, compatible with constraints, and (2) -upper bounds on constraints (at least within a particular minor version) are -relatively rare. That means, in particular, that Cargo.toml is not a fully accurate -picture of version dependency information; in general it's a lower bound at best. -There can be "invisible" dependencies that don't cause resolution failures but can -create compilation errors as APIs evolve. - -Public dependencies exacerbate the above problem, because you can end up relying on -features of a "new API" from a crate you didn't even know you depended on! For -example: - -- A depends on: - - B 1.0 which publicly depends on C ^1.0 - - D 1.0, which has no dependencies -- When A is initially built, it resolves to B 1.0 and C 1.1. - - Because C's APIs are available to A via re-exports in B, A effectively depends - on C 1.1 now, even though B only claims to depend on C ^1.0 - - In particular, the code in A might depend on APIs only available in C 1.1 - - However, if A is a library, we don't check in any lockfile for it, so this - information is lost. -- Now we change A to depend on D 1.1, which depends on C =1.0 - - A fresh copy of A, when built, will now resolve the crate graph to B 1.0, D 1.1, - C 1.0 - - But now A may suddenly fail to compile, because it was implicitly depending on C - 1.1 features via B. - -This example and others like it rely on a common ingredient: a crate somewhere using -an API that only is available in a newer version of a crate than the version listed -in Cargo.toml. - -To attempt to surface this problem earlier, `cargo publish` will attempt to resolve -the graph while picking the smallest versions compatible with constraints. If the -crate fails to build with this resolution graph, the publish will fail. - -# Drawbacks -[drawbacks]: #drawbacks - -I believe that there are no drawbacks if implemented well (this assumes good -linters and error messages). - -# Rationale and alternatives -[rationale-and-alternatives]: #rationale-and-alternatives +As an alternative, when declaring dependencies, a user could [explicitly delegate the version requirement to another package](https://github.com/rust-lang/cargo/issues/4641) -For me, the biggest alternative to this RFC would be a variation of it where type -and trait aliasing becomes immediately part of it. This would mean that a crate -can have a private dependency and re-export it as its own type, hiding where it -came from originally. This would most likely be easier to teach users and can get -rid of a few "cul-de-sac" situations users can end up in where their only way -out is to introduce a public dependency for now. The assumption is that if trait -and type aliasing is available, the `external_public_dependency` would not need to -exist. - -# Prior art -[prior-art]: #prior-art - -# Unresolved questions -[unresolved]: #unresolved-questions - -There are a few open questions about how to best hook into the compiler and Cargo -infrastructure: - -* What is the impact of this change going to be? This most likely can be answered - running cargobomb/crater. -* Since changing public dependency pins/ranges requires a change in semver, it might - be worth exploring if Cargo could prevent the user from publishing new crate - versions that violate that constraint. -* If this is implemented before [the RFC to deprecate `extern crate`](https://github.com/rust-lang/rfcs/pull/2126), how would this work if you're not using `--extern`? - -# Future possibilities -[future-possibilities]: #future-possibilities +[RFC 1977]: https://github.com/rust-lang/rfcs/pull/1977 From 8774ca668147662a6f3e50c865314f7e06379d8a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 13 Oct 2023 12:30:44 -0500 Subject: [PATCH 04/43] fix: Clarify the proc macro language --- text/0000-public-private-dependencies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index ecc7ec3d48e..6f6e054edca 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -123,7 +123,7 @@ pub mod __derive_refs { } ``` Since the proc-macro can only guarantee that the namespace `clap` is accessible, -`clap` must re-export any functionality that is needed at runtime. +`clap` must re-export any functionality that is needed at runtime by the generated code. As a last-ditch way of dealing with this, a user may allow the error: ```rust #[doc(hidden)] From ac7c261924ea5be88cfb83afb3b767924073687d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 13 Oct 2023 12:48:16 -0500 Subject: [PATCH 05/43] fix: Expand more on the old cargo situation --- text/0000-public-private-dependencies.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index 6f6e054edca..81904019771 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -163,11 +163,14 @@ This most likely will also be necessary for the more complex relationship of ## cargo A new dependency field, `pub = ` will be added that defaults to `false`. -Old cargo version will emit a warning when this key is encountered but otherwise continue. Cargo will use use the `priv` modifier with `--extern` for all private dependencies when building a `lib`. What is private is what is left after recursively walking public dependencies. We'll ignore this for other build target kinds (e.g. `bin`) as that would create extra noise. +Old cargo version will emit a warning when this key is encountered but otherwise continue, +even if the feature is present but unstable. +While it is unstable, `cargo publish` will strip the field. + Cargo will not force a `rust-version` bump when using this feature as someone building with an old version of cargo depending on packages that set `pub = true` will not start to fail when upgrading to new versions of cargo. From e59b86517477d051137c855f6461936d0d7c30c4 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 13 Oct 2023 20:21:30 -0500 Subject: [PATCH 06/43] fix: Call out rustc/cargo risks --- text/0000-public-private-dependencies.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index 81904019771..61c7f1556e2 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -195,6 +195,14 @@ You can't definitively lint when a `pub = true` is unused since it may depend on The warning is emitted even when a `pub` item isn't accessible. +The warning message might not be the clearest in how to resolve as its emitted +by rustc but is resolved by changing information in the build system, +generally, but not always, cargo. + +There are risks with the `cargo fix` approach is it requires us to take a non-machine applicable lint, +parsing out the information we need to identify the corresponding `Cargo.toml`, +and translate it into a change for `Cargo.toml`. + # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives From a78452877b53d356f042b8869eeaa931bb61b9f3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 14 Oct 2023 19:50:30 -0500 Subject: [PATCH 07/43] feat: Expand on preventing version mismatch errors --- text/0000-public-private-dependencies.md | 66 ++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 4 deletions(-) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index 61c7f1556e2..109bf3b3ba1 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -22,12 +22,16 @@ dependencies have evolved that challenge the currently existing dependency decla system in Cargo and Rust. The most common problem is that a crate `A` depends on another crate `B` but some of the types from crate `B` are exposed through the API in crate `A`. -- Poor error messages when a user directly depends on `A` and `B` but with a version requirement on `B` that is semver incompatible with `A`s version requirement on `B` -- Brittle semver compatibility as `A` might not have intended to expose `B`, like with `impl From for AError` +- Brittle semver compatibility as `A` might not have intended to expose `B`, like when adding `impl From for AError` for your own convenience - When self-hosting documentation, you may want to render documentation for all of your public dependencies as well - When running `cargo doc`, users may way to render [documentation for their accessible dependencies](https://github.com/rust-lang/cargo/issues/2025) [without the cost of their inaccessible dependencies](https://github.com/rust-lang/cargo/issues/4049) - When linting for semver compatibility [there isn't enough information](https://github.com/obi1kenobi/cargo-semver-checks/issues/121) +A related problem not covered by this RFC is helping with the poor error +messages when a user directly depends on `A` and `B` but with a version +requirement on `B` that is semver incompatible with `A`s version requirement on +`B`. + # Guide-level explanation [guide-level-explanation]: #guide-level-explanation @@ -250,7 +254,27 @@ Within the cargo ecosystem: # Future possibilities [future-possibilities]: #future-possibilities -## Dependency visibility and the resolver +## Help keep versions in-sync + +When upgrading one dependency, you might need to upgrade another because you +use it to interact with the first, like `clap` and `clap_complete`. +The existing error messages are not great, along the lines of "expected `clap::Command`, found `clap::Command`". +Ideally, you would be presented instead with a message saying "clap_complete +3.4 is not compatiblw with clap 4.0, try upgrading to clap_complete 4.0". +Even better if we could help users do this upgrade automatically. + +As solving this, via the resolver, has been the main sticking point for [RFC 1997], +this was deferred out to take smaller, +more incremental steps, +that open the +door for more experimentation in the future to understand how best to solve +these problems. + +Some possible routes: + +### Dependency visibility and the resolver + +[RFC 1977] originall proposed handling this within the resolver Cargo will specifically reject graphs that contain two different versions of the same crate being publicly depended upon and reachable from each other. This will @@ -282,6 +306,40 @@ How this will work: same crate, we consider that an error. This basically means that if you privately depend on Hyper 0.3 and Hyper 0.4, that's an error. -As an alternative, when declaring dependencies, a user could [explicitly delegate the version requirement to another package](https://github.com/rust-lang/cargo/issues/4641) +If we want to go this route, some hurdles to overcome include: +- Difficulties in working with cargo's resolver as this has been the main hang-up for stabilization over the last 6 years since the [RFC 1977] was approved + - For more on the complexity involved, see the thread starting at [this comment](https://github.com/rust-lang/rust/issues/44663#issuecomment-881965668) +- More thought is needed as we found that making a dependency `pub = true` can be a breaking change if the caller also depends on it but with a different semver incompatible version +- More thought is needed on what happens if you have multiple versions of a package that are public (via renaming like `tokio_03` and `tokio_1`) + +### Caller-declared relations + +As an alternative, when declaring dependencies, +a user could [explicitly delegate the version requirement to another package](https://github.com/rust-lang/cargo/issues/4641) + +One possible approach for this: +```toml +[package] +name = "some-cli" + +[dependencies] +clap = { version.from ["clap_complete"] } +clap_complete = "3.4" +``` +When resolving the dependencies for `some-cli`, +the resolver will not explicitly choose a version for `clap` but will continue resolving the graph. +Upon completion, it will look to see what version of `clap_complete` was +resolved and act as if that was what was specified inside of the in-memory +`clap` dependency. + +The packakge using `version.from` must be a public dependency of the `from` package. +In this case, `clap` must be a public dependency of `clap_complete`. +If the different packages in `version.from` do not agree on what the package +version should resolve to (clap 3.4 vs clap 4.0), then it is an error. + +Compared to the resolver doing this implicitly +- It is unclear if this would be any more difficult to implement in the resolver +- Changing a dependency from `pub = false` to `pub = true` is backwards compatible because it has no affect on existing callers. +- It is unclear how this would hanlde multiple versions of a package that are public [RFC 1977]: https://github.com/rust-lang/rfcs/pull/1977 From 86ea56c671ab6293bccf85a05cf4edfcaee5b87f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 14 Oct 2023 19:51:53 -0500 Subject: [PATCH 08/43] fix: Re-order drawbacks by priority --- text/0000-public-private-dependencies.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index 109bf3b3ba1..bc5bd5c896f 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -191,14 +191,6 @@ Crates.io should show public dependencies more prominently than private ones. # Drawbacks [drawbacks]: #drawbacks -This doesn't cover the case where a dependency is public only if a feature is enabled. - -In the case where you depend on `foo = "300"`, there isn't a way to clarify that what is public is actually from `foo-core = "1"` without explicitly depending on it. - -You can't definitively lint when a `pub = true` is unused since it may depend on which platform or features. - -The warning is emitted even when a `pub` item isn't accessible. - The warning message might not be the clearest in how to resolve as its emitted by rustc but is resolved by changing information in the build system, generally, but not always, cargo. @@ -207,6 +199,14 @@ There are risks with the `cargo fix` approach is it requires us to take a non-ma parsing out the information we need to identify the corresponding `Cargo.toml`, and translate it into a change for `Cargo.toml`. +In the case where you depend on `foo = "300"`, there isn't a way to clarify that what is public is actually from `foo-core = "1"` without explicitly depending on it. + +This doesn't cover the case where a dependency is public only if a feature is enabled. + +The warning is emitted even when a `pub` item isn't accessible. + +You can't definitively lint when a `pub = true` is unused since it may depend on which platform or features. + # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives From ddce8452ea961a04d754177cce4c12cab8744aba Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 14 Oct 2023 20:16:12 -0500 Subject: [PATCH 09/43] fix: Clarify the migration path --- text/0000-public-private-dependencies.md | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index bc5bd5c896f..3dc362d8b20 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -14,6 +14,8 @@ Enough has changed in the time since that RFC was approved that we felt we neede - [RFC 1977] was written before Editions, `cfg` dependencies, and package renaming which can all affect it - The resolver changes were a large part of [RFC 1977] but there are concerns with it and we feel it'd be best to decouple it, offering a faster path to stabilization +Note: The 2024 Edition is referenced in this RFC but that is a placeholder for whatever edition next comes up. + # Motivation [motivation]: #motivation @@ -150,17 +152,20 @@ like `impl Trait` in type aliases if we had it. The main change to the compiler will be to accept a new modifier on the `--extern` flag that Cargo supplies which is a list of public dependencies. The mode will be called `priv` (e.g. `--extern priv:serde`). -The compiler then emits warnings if it encounters private -dependencies leaking to the public API of a crate. `cargo publish` might change -this warning into an error in its lint step. +The compiler then emits a lint if it encounters private +dependencies leaking to the public API of a crate. -Additionally, later on, the warning can turn into a hard error in general. +While unstable, this lint will be `warn` by default. +If the presentatuion of the lint is what holds us back from stabilization, +one route to speed up the process is to change the level to `allow`. +Once we feel comfortable with the presentation, we could then move it back towards +`warn`. +In the 2024 edition, we would change this lint to `deny`. In some situations, it can be necessary to allow private dependencies to become part of the public API. In that case one can permit this with `#[allow(external_private_dependency)]`. This is particularly useful when paired with `#[doc(hidden)]` and other already existing hacks. - This most likely will also be necessary for the more complex relationship of `libcore` and `libstd` in Rust itself. From cefbcdf1d89c617ae470fe0cbb609161cbe6e025 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 16 Oct 2023 09:44:52 -0500 Subject: [PATCH 10/43] fix: Exclude independent resolution --- text/0000-public-private-dependencies.md | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index 3dc362d8b20..7dcc67cc310 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -29,10 +29,15 @@ crate `B` but some of the types from crate `B` are exposed through the API in cr - When running `cargo doc`, users may way to render [documentation for their accessible dependencies](https://github.com/rust-lang/cargo/issues/2025) [without the cost of their inaccessible dependencies](https://github.com/rust-lang/cargo/issues/4049) - When linting for semver compatibility [there isn't enough information](https://github.com/obi1kenobi/cargo-semver-checks/issues/121) -A related problem not covered by this RFC is helping with the poor error -messages when a user directly depends on `A` and `B` but with a version -requirement on `B` that is semver incompatible with `A`s version requirement on -`B`. +Excluded motivations: +- Poor error messages when a user directly depends on `A` and `B` but with a + version requirement on `B` that is semver incompatible with `A`s version + requirement on `B`. + - See [Dependency visibility and the resolver](#rationale-and-alternatives) for why this is excluded. +- Allow mutually exclusiev features or overly-constrained version requirements + by not requiring private dependencies to be unified. + - Private dependencies are not sufficient on their own for this + - There are likely better alternatives, like [Pre-RFC: Mutually-exclusive, global features](https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618) # Guide-level explanation [guide-level-explanation]: #guide-level-explanation From c6cc6caf911eeebf13da777924a54e9fd42666ff Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 16 Oct 2023 09:46:45 -0500 Subject: [PATCH 11/43] fix: Clean up some language --- text/0000-public-private-dependencies.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index 7dcc67cc310..09782400367 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -71,8 +71,7 @@ impl std::str::FromStr for Diagnostic { ``` The dependencies `serde` and `serde_json` are both public dependencies, meaning their types are referenced in the public API. -Effectively, the idea is that if you do a semver incompatible upgrade to a -public dependency, it's a breaking change of your *own* crate. +This as the implication that a semver incompatible upgrade of these dependencies is a breaking change for this package. With this RFC, in pre-2024 editions, this will warn saying that `serde` and `serde_json` are private dependencies in a public API. In 2024+ editions, this will be an error. From 14886358091896c947182beb8f9e1647ac1b7f56 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 16 Oct 2023 09:48:05 -0500 Subject: [PATCH 12/43] fix: Mention ambiguity --- text/0000-public-private-dependencies.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index 09782400367..9d3371a55e1 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -231,6 +231,7 @@ You can't definitively lint when a `pub = true` is unused since it may depend on - `Cargo.toml`: In the long term, we decided on the default being `pub = false` as that is the common case and gives us more information than supporting a `pub = "unchecked"` and having that be the long term solution. - `Cargo.toml`: instead of `pub` (named after the [Rust keyword](https://doc.rust-lang.org/reference/visibility-and-privacy.html), we could name the field `public` (like [RFC 1977]) or name the field `visibility = ""` - The parallel with Rust seemed worth pursuing + - `pub` could be seen as ambiguous with `publish` - While `visibility` would offer greater flexibility, it is unclear if we need that flexibility and if the friction of any feature leveraging it would be worth it ## Minimal version resolution From 8c26107436e08f8c63a548d11c06cce2cd3b2ff7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 16 Oct 2023 09:55:08 -0500 Subject: [PATCH 13/43] fix: Add notes on missing feature declaration checks --- text/0000-public-private-dependencies.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index 9d3371a55e1..aa7f1b1f281 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -38,6 +38,8 @@ Excluded motivations: by not requiring private dependencies to be unified. - Private dependencies are not sufficient on their own for this - There are likely better alternatives, like [Pre-RFC: Mutually-exclusive, global features](https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618) +- Help check for missing feature declarations by duplicating dependencies, rather than unifiying features + - See [Missing feature declaration check](#future-possibilities) # Guide-level explanation [guide-level-explanation]: #guide-level-explanation @@ -352,4 +354,18 @@ Compared to the resolver doing this implicitly - Changing a dependency from `pub = false` to `pub = true` is backwards compatible because it has no affect on existing callers. - It is unclear how this would hanlde multiple versions of a package that are public +## Missing feature declaration check + +It is easy for packages to accidentally rely on a dependency enabling a feature for them. +We could add a mode that limits feature unification to reachable dependencies, +forcing duplication and longer builds for the sake of checking if any features +need specifying. + +However, this will still likely miss a lot of cases, making the pay off questionable. +This also has the risk of being abused as a workaround so people can use +mutually exclusive features. +If packages start relying on it, +it could coerce callers into abusing this mechanism, +having a cascading effect in the ecosystem in the wrong direction. + [RFC 1977]: https://github.com/rust-lang/rfcs/pull/1977 From da3c1984601bfb25a5c5e97a240f64c3767dff51 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 16 Oct 2023 13:14:18 -0500 Subject: [PATCH 14/43] fix: Add note for version.from --- text/0000-public-private-dependencies.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index aa7f1b1f281..2924a04dbf6 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -354,6 +354,10 @@ Compared to the resolver doing this implicitly - Changing a dependency from `pub = false` to `pub = true` is backwards compatible because it has no affect on existing callers. - It is unclear how this would hanlde multiple versions of a package that are public +The downside is it feels like the declaration is backwards. +If you have one core crate (e.g. `clap`) and many crates branching off (e.g. `clap_complete`, `clap_mangen`), +it seems like those helper crates should have their version picked from `clap`. + ## Missing feature declaration check It is easy for packages to accidentally rely on a dependency enabling a feature for them. From 0ff78bd2b4f7f9c508fc27d0374383300ca325d3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 16 Oct 2023 13:45:03 -0500 Subject: [PATCH 15/43] fix: Quick edit pass --- text/0000-public-private-dependencies.md | 67 ++++++++++++++---------- 1 file changed, 40 insertions(+), 27 deletions(-) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index 2924a04dbf6..87fb77962a2 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -14,22 +14,24 @@ Enough has changed in the time since that RFC was approved that we felt we neede - [RFC 1977] was written before Editions, `cfg` dependencies, and package renaming which can all affect it - The resolver changes were a large part of [RFC 1977] but there are concerns with it and we feel it'd be best to decouple it, offering a faster path to stabilization -Note: The 2024 Edition is referenced in this RFC but that is a placeholder for whatever edition next comes up. +Note: The 2024 Edition is referenced in this RFC but that is a placeholder for +whatever edition next comes up after stabilization. # Motivation [motivation]: #motivation The crates ecosystem has greatly expanded since Rust 1.0. With that, a few patterns for -dependencies have evolved that challenge the currently existing dependency declaration +dependencies have evolved that challenge the existing dependency declaration system in Cargo and Rust. The most common problem is that a crate `A` depends on another crate `B` but some of the types from crate `B` are exposed through the API in crate `A`. -- Brittle semver compatibility as `A` might not have intended to expose `B`, like when adding `impl From for AError` for your own convenience +- Brittle semver compatibility as `A` might not have intended to expose `B`, + like when adding `impl From for AError` for convenience in using `?` in the implementation of `A`. - When self-hosting documentation, you may want to render documentation for all of your public dependencies as well - When running `cargo doc`, users may way to render [documentation for their accessible dependencies](https://github.com/rust-lang/cargo/issues/2025) [without the cost of their inaccessible dependencies](https://github.com/rust-lang/cargo/issues/4049) -- When linting for semver compatibility [there isn't enough information](https://github.com/obi1kenobi/cargo-semver-checks/issues/121) +- When linting for semver compatibility [there isn't enough information to reason about dependencies](https://github.com/obi1kenobi/cargo-semver-checks/issues/121) -Excluded motivations: +Related problems not with this scenario not handled by this RFC: - Poor error messages when a user directly depends on `A` and `B` but with a version requirement on `B` that is semver incompatible with `A`s version requirement on `B`. @@ -44,7 +46,7 @@ Excluded motivations: # Guide-level explanation [guide-level-explanation]: #guide-level-explanation -As a trivial example: +As a trivial, artificial example: ```toml [package] name = "diagnostic" @@ -73,12 +75,12 @@ impl std::str::FromStr for Diagnostic { ``` The dependencies `serde` and `serde_json` are both public dependencies, meaning their types are referenced in the public API. -This as the implication that a semver incompatible upgrade of these dependencies is a breaking change for this package. +This has the implication that a semver incompatible upgrade of these dependencies is a breaking change for this package. With this RFC, in pre-2024 editions, this will warn saying that `serde` and `serde_json` are private dependencies in a public API. In 2024+ editions, this will be an error. -To resolve this in a semver compatible way, they would need to declare both dependencies as public: +To resolve the warning in a semver compatible way, they would need to declare both dependencies as public: ```toml [package] name = "diagnostic" @@ -91,7 +93,7 @@ serde_json = { version = "1", pub = true } For edition migrations, `cargo fix` will look for the warning code and mark those dependencies as `pub`. However, for this example, it was an oversight in exposing `serde_json` in the public API. -Removing it from the public API is a semver incompatible change. +Note that removing it from the public API is a semver incompatible change. ```toml [package] name = "diagnostic" @@ -146,9 +148,18 @@ pub mod __derive_refs { pub use once_cell; } ``` -I say "last ditch" because in cases outside of `#[doc(hidden)]` items for macros, -a user would be better served by other features, -like `impl Trait` in type aliases if we had it. +A similar case is pub-in-private: +```rust +mod private { + #[allow(external_private_dependency)] + pub struct Foo { pub x: some_dependency::SomeType } +} +``` +Though this might be worked around by reducing the visibility to `pub(crate)`. + +I say "last ditch" because in most other cases, +a user would be better served by wrapping the API which would be helped with +features like `impl Trait` in type aliases if we had it. # Reference-level explanation [reference-level-explanation]: #reference-level-explanation @@ -156,10 +167,10 @@ like `impl Trait` in type aliases if we had it. ## rustc The main change to the compiler will be to accept a new modifier on the `--extern` flag that Cargo -supplies which is a list of public dependencies. -The mode will be called `priv` (e.g. `--extern priv:serde`). +supplies which marks it as a private dependency. +The modifier will be called `priv` (e.g. `--extern priv:serde`). The compiler then emits a lint if it encounters private -dependencies leaking to the public API of a crate. +dependencies exposed as `pub`. While unstable, this lint will be `warn` by default. If the presentatuion of the lint is what holds us back from stabilization, @@ -178,11 +189,11 @@ This most likely will also be necessary for the more complex relationship of ## cargo A new dependency field, `pub = ` will be added that defaults to `false`. -Cargo will use use the `priv` modifier with `--extern` for all private dependencies when building a `lib`. -What is private is what is left after recursively walking public dependencies. +When building a `lib`, Cargo will use the `priv` modifier with `--extern` for all private dependencies. +What is private is what is left after recursively walking public dependencies (`pub = true`). We'll ignore this for other build target kinds (e.g. `bin`) as that would create extra noise. -Old cargo version will emit a warning when this key is encountered but otherwise continue, +Old cargo versions will emit a warning when this key is encountered but otherwise continue, even if the feature is present but unstable. While it is unstable, `cargo publish` will strip the field. @@ -205,8 +216,10 @@ Crates.io should show public dependencies more prominently than private ones. The warning message might not be the clearest in how to resolve as its emitted by rustc but is resolved by changing information in the build system, generally, but not always, cargo. +As a last resort, we could put a hack in cargo to intercept the lint and emit a +new version of it that explains things in terms of cargo. -There are risks with the `cargo fix` approach is it requires us to take a non-machine applicable lint, +There are risks with the `cargo fix` approach as it requires us to take a non-machine applicable lint, parsing out the information we need to identify the corresponding `Cargo.toml`, and translate it into a change for `Cargo.toml`. @@ -223,18 +236,18 @@ You can't definitively lint when a `pub = true` is unused since it may depend on ## Misc -- `cargo add`: instead of `--pub ` it could be `--pub` / `--no-pub` like `--optional` or `--public` / `--private` -- `cargo add`: when adding a dependency, we could automatically add all of its `pub` dependencies. - - This was passed up as being too noisy, especially when dealing with facade crates, those that fully re-export their `pub = true` dependency +- `Cargo.toml`: instead of `pub` (named after the [Rust keyword](https://doc.rust-lang.org/reference/visibility-and-privacy.html), we could name the field `public` (like [RFC 1977]) or name the field `visibility = ""` + - The parallel with Rust seemed worth pursuing + - `pub` could be seen as ambiguous with `publish` + - While `visibility` would offer greater flexibility, it is unclear if we need that flexibility and if the friction of any feature leveraging it would be worth it - `Cargo.toml`: Instead of `pub = false` being the default and changing the warning level on an edition boundary, we could instead start with `pub = true` and change the default on an edition boundary. - This would require `cargo fix` marking all dependencies as `pub = true`, while using the warning means we can limit it to only those dependencies that need it. - `Cargo.toml`: Instead of `pub = false` being the default, we could have a "unchecked" / "unset" state - This would require `cargo fix` marking all dependencies as `pub = true`, while using the warning means we can limit it to only those dependencies that need it. - `Cargo.toml`: In the long term, we decided on the default being `pub = false` as that is the common case and gives us more information than supporting a `pub = "unchecked"` and having that be the long term solution. -- `Cargo.toml`: instead of `pub` (named after the [Rust keyword](https://doc.rust-lang.org/reference/visibility-and-privacy.html), we could name the field `public` (like [RFC 1977]) or name the field `visibility = ""` - - The parallel with Rust seemed worth pursuing - - `pub` could be seen as ambiguous with `publish` - - While `visibility` would offer greater flexibility, it is unclear if we need that flexibility and if the friction of any feature leveraging it would be worth it +- `cargo add`: instead of `--pub ` it could be `--pub` / `--no-pub` like `--optional` or `--public` / `--private` +- `cargo add`: when adding a dependency, we could automatically add all of its `pub` dependencies. + - This was passed up as being too noisy, especially when dealing with facade crates, those that fully re-export their `pub = true` dependency ## Minimal version resolution @@ -250,7 +263,7 @@ This is deferred to [Future possibilities](#future-possibilities) - More thought is needed as we found that making a dependency `pub = true` can be a breaking change if the caller also depends on it but with a different semver incompatible version - More thought is needed on what happens if you have multiple versions of a package that are public (via renaming like `tokio_03` and `tokio_1`) -Affects of deferring this out +Related problems potentially blocked on this - It is hoped that the resolver change would help with [cargo#9029](https://github.com/rust-lang/cargo/issues/9029) - If we allow duplication of private semver compatible dependencies, it would help with [cargo#10053](https://github.com/rust-lang/cargo/issues/10053) From 20a023b1edf8506c407d946a6cca8e964c717726 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 16 Oct 2023 13:51:10 -0500 Subject: [PATCH 16/43] fix: Call out the rollout plan as an unresolved question --- text/0000-public-private-dependencies.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index 87fb77962a2..51b7e21ed28 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -276,6 +276,14 @@ Within the cargo ecosystem: # Unresolved questions [unresolved]: #unresolved-questions +- Will the warning be too disruptive to the ecosystem to enable by default? + - Being automatically fixed with `cargo fix` (with cargo's reminder that you can run it) helps + - Not requiring an MSRV bump helps + - We could instead start it as `allow` + but in `rust-2024-compatibility` group, + still turning into an error in the 2024 edition, + and have the edition migration reduce the blast radius. + # Future possibilities [future-possibilities]: #future-possibilities From fba878287ce4e0acdc4c359e974b8dd157596b7f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 16 Oct 2023 13:54:30 -0500 Subject: [PATCH 17/43] fix: Typos --- text/0000-public-private-dependencies.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index 51b7e21ed28..fd7c7a07198 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -307,7 +307,7 @@ Some possible routes: ### Dependency visibility and the resolver -[RFC 1977] originall proposed handling this within the resolver +[RFC 1977] originally proposed handling this within the resolver Cargo will specifically reject graphs that contain two different versions of the same crate being publicly depended upon and reachable from each other. This will @@ -373,7 +373,7 @@ version should resolve to (clap 3.4 vs clap 4.0), then it is an error. Compared to the resolver doing this implicitly - It is unclear if this would be any more difficult to implement in the resolver - Changing a dependency from `pub = false` to `pub = true` is backwards compatible because it has no affect on existing callers. -- It is unclear how this would hanlde multiple versions of a package that are public +- It is unclear how this would handle multiple versions of a package that are public The downside is it feels like the declaration is backwards. If you have one core crate (e.g. `clap`) and many crates branching off (e.g. `clap_complete`, `clap_mangen`), From e43432fe9e6568eec87601a6c83382eb42e33366 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 16 Oct 2023 13:56:57 -0500 Subject: [PATCH 18/43] fix: Call out workspace inheritance --- text/0000-public-private-dependencies.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index fd7c7a07198..59fe781be4a 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -189,6 +189,7 @@ This most likely will also be necessary for the more complex relationship of ## cargo A new dependency field, `pub = ` will be added that defaults to `false`. +This field can be specified in `workspace.dependencies` and be overridden when `workspace = true` is in a dependency. When building a `lib`, Cargo will use the `priv` modifier with `--extern` for all private dependencies. What is private is what is left after recursively walking public dependencies (`pub = true`). We'll ignore this for other build target kinds (e.g. `bin`) as that would create extra noise. From 7e30a8172588c96e438c30813d84bed417d93e8a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 16 Oct 2023 13:59:09 -0500 Subject: [PATCH 19/43] fix: Link to Pre-RFC --- text/0000-public-private-dependencies.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/0000-public-private-dependencies.md b/text/0000-public-private-dependencies.md index 59fe781be4a..7b54247395a 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/0000-public-private-dependencies.md @@ -1,6 +1,7 @@ - Feature Name: `public_private_dependencies` - Start Date: 2023-10-13 - Prior RFC PR: [rust-lang/rfcs#1977](https://github.com/rust-lang/rfcs/pull/1977) +- Pre-RFC: [Pre-RFC: Superseding public/private dependencies](https://internals.rust-lang.org/t/pre-rfc-superseding-public-private-dependencies/19708) - RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) - Rust Issue: [rust-lang/rust#44663](https://github.com/rust-lang/rust/issues/44663) From eb678fb629251753ff5e1d4c25ec2343f97d625b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 16 Oct 2023 14:02:27 -0500 Subject: [PATCH 20/43] fix: Update RFC number --- ...vate-dependencies.md => 3516-public-private-dependencies.md} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename text/{0000-public-private-dependencies.md => 3516-public-private-dependencies.md} (99%) diff --git a/text/0000-public-private-dependencies.md b/text/3516-public-private-dependencies.md similarity index 99% rename from text/0000-public-private-dependencies.md rename to text/3516-public-private-dependencies.md index 7b54247395a..baca9dbc04b 100644 --- a/text/0000-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -2,7 +2,7 @@ - Start Date: 2023-10-13 - Prior RFC PR: [rust-lang/rfcs#1977](https://github.com/rust-lang/rfcs/pull/1977) - Pre-RFC: [Pre-RFC: Superseding public/private dependencies](https://internals.rust-lang.org/t/pre-rfc-superseding-public-private-dependencies/19708) -- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) +- RFC PR: [rust-lang/rfcs#3516](https://github.com/rust-lang/rfcs/pull/3516) - Rust Issue: [rust-lang/rust#44663](https://github.com/rust-lang/rust/issues/44663) # Summary From a3ee6c7a84ace351e15d86bbb54739645a847678 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 17 Oct 2023 08:11:23 -0500 Subject: [PATCH 21/43] fix: Typo --- text/3516-public-private-dependencies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index baca9dbc04b..d6f3e612c2b 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -32,7 +32,7 @@ crate `B` but some of the types from crate `B` are exposed through the API in cr - When running `cargo doc`, users may way to render [documentation for their accessible dependencies](https://github.com/rust-lang/cargo/issues/2025) [without the cost of their inaccessible dependencies](https://github.com/rust-lang/cargo/issues/4049) - When linting for semver compatibility [there isn't enough information to reason about dependencies](https://github.com/obi1kenobi/cargo-semver-checks/issues/121) -Related problems not with this scenario not handled by this RFC: +Related problems with this scenario not handled by this RFC: - Poor error messages when a user directly depends on `A` and `B` but with a version requirement on `B` that is semver incompatible with `A`s version requirement on `B`. From 5a73167aa15eff09b29d2f44281ff6241048aad3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Oct 2023 11:13:45 -0500 Subject: [PATCH 22/43] fix: Typo Co-authored-by: teor --- text/3516-public-private-dependencies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index d6f3e612c2b..8f25fb2d5c2 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -37,7 +37,7 @@ Related problems with this scenario not handled by this RFC: version requirement on `B` that is semver incompatible with `A`s version requirement on `B`. - See [Dependency visibility and the resolver](#rationale-and-alternatives) for why this is excluded. -- Allow mutually exclusiev features or overly-constrained version requirements +- Allow mutually exclusive features or overly-constrained version requirements by not requiring private dependencies to be unified. - Private dependencies are not sufficient on their own for this - There are likely better alternatives, like [Pre-RFC: Mutually-exclusive, global features](https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618) From 1841595d884fcfb25ada0e394eaa01dcf134dc29 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Oct 2023 11:13:58 -0500 Subject: [PATCH 23/43] fix: Typo Co-authored-by: teor --- text/3516-public-private-dependencies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index 8f25fb2d5c2..940eb68ef6d 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -174,7 +174,7 @@ The compiler then emits a lint if it encounters private dependencies exposed as `pub`. While unstable, this lint will be `warn` by default. -If the presentatuion of the lint is what holds us back from stabilization, +If the presentation of the lint is what holds us back from stabilization, one route to speed up the process is to change the level to `allow`. Once we feel comfortable with the presentation, we could then move it back towards `warn`. From 30fa936dd7a70760d699bcb3c494ed38c98ff93b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 19 Oct 2023 11:16:09 -0500 Subject: [PATCH 24/43] fix: Clarify drawback wording Co-authored-by: teor --- text/3516-public-private-dependencies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index 940eb68ef6d..11c6f7377e8 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -215,7 +215,7 @@ Crates.io should show public dependencies more prominently than private ones. # Drawbacks [drawbacks]: #drawbacks -The warning message might not be the clearest in how to resolve as its emitted +It might not be clear how to resolve the warning, as it's emitted by rustc but is resolved by changing information in the build system, generally, but not always, cargo. As a last resort, we could put a hack in cargo to intercept the lint and emit a From f4b6a2da83414528cea0bec64c25127e42dea1f3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 24 Oct 2023 10:21:38 -0500 Subject: [PATCH 25/43] fix: Include future ideas from cargo meeting --- text/3516-public-private-dependencies.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index 11c6f7377e8..91318ab5ad4 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -395,4 +395,14 @@ If packages start relying on it, it could coerce callers into abusing this mechanism, having a cascading effect in the ecosystem in the wrong direction. +## Warn about semver incompatible public dependency + +`cargo update` (or a manifest linter) could warn about public new incompatible +public dependencies that are available to help the ecosystem progress in +lockstep. + +## Warn about pre-1.0 public dependencies in 1.0+ packages + +See [rust-lang/cargo#6018](https://github.com/rust-lang/cargo/issues/6018) + [RFC 1977]: https://github.com/rust-lang/rfcs/pull/1977 From 6bab8b393881aeb277fa06b8f32bbf1f67d112d3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 24 Oct 2023 10:24:57 -0500 Subject: [PATCH 26/43] feat: Note the final decision on field name --- text/3516-public-private-dependencies.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index 91318ab5ad4..a19535ff403 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -242,6 +242,7 @@ You can't definitively lint when a `pub = true` is unused since it may depend on - The parallel with Rust seemed worth pursuing - `pub` could be seen as ambiguous with `publish` - While `visibility` would offer greater flexibility, it is unclear if we need that flexibility and if the friction of any feature leveraging it would be worth it + - In the end, we went with `pub` because `public` would prevent our "no MSRV bump" approach because cargo versions exist with `public` being reserved - `Cargo.toml`: Instead of `pub = false` being the default and changing the warning level on an edition boundary, we could instead start with `pub = true` and change the default on an edition boundary. - This would require `cargo fix` marking all dependencies as `pub = true`, while using the warning means we can limit it to only those dependencies that need it. - `Cargo.toml`: Instead of `pub = false` being the default, we could have a "unchecked" / "unset" state From dcd6ae2e32db099e0278b8abfbf6e037a5aec2cd Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 24 Oct 2023 10:32:14 -0500 Subject: [PATCH 27/43] feat: Demote lint from warn to allow --- text/3516-public-private-dependencies.md | 31 ++++++++++-------------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index a19535ff403..0528252967c 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -78,7 +78,10 @@ impl std::str::FromStr for Diagnostic { The dependencies `serde` and `serde_json` are both public dependencies, meaning their types are referenced in the public API. This has the implication that a semver incompatible upgrade of these dependencies is a breaking change for this package. -With this RFC, in pre-2024 editions, this will warn saying that `serde` and `serde_json` are private dependencies in a public API. +With this RFC, in pre-2024 editions, +you can enable add `lints.rust.external_private_dependency = "warn"` to your +manifest and rustc will warn saying that `serde` and `serde_json` are private +dependencies in a public API. In 2024+ editions, this will be an error. To resolve the warning in a semver compatible way, they would need to declare both dependencies as public: @@ -170,15 +173,12 @@ features like `impl Trait` in type aliases if we had it. The main change to the compiler will be to accept a new modifier on the `--extern` flag that Cargo supplies which marks it as a private dependency. The modifier will be called `priv` (e.g. `--extern priv:serde`). -The compiler then emits a lint if it encounters private +The compiler then emits the lint `external_private_dependency` if it encounters private dependencies exposed as `pub`. -While unstable, this lint will be `warn` by default. -If the presentation of the lint is what holds us back from stabilization, -one route to speed up the process is to change the level to `allow`. -Once we feel comfortable with the presentation, we could then move it back towards -`warn`. -In the 2024 edition, we would change this lint to `deny`. +`external_private_dependency` will be `allow` by default for pre-2024 editions. +It will be a member of the `rust-2024-compatibility` lint group so that it gets automatically picked up by `cargo fix --edition`. +In the 2024 edition, this lint will be `deny`. In some situations, it can be necessary to allow private dependencies to become part of the public API. In that case one can permit this with @@ -215,7 +215,7 @@ Crates.io should show public dependencies more prominently than private ones. # Drawbacks [drawbacks]: #drawbacks -It might not be clear how to resolve the warning, as it's emitted +It might not be clear how to resolve the warning/error, as it's emitted by rustc but is resolved by changing information in the build system, generally, but not always, cargo. As a last resort, we could put a hack in cargo to intercept the lint and emit a @@ -229,7 +229,7 @@ In the case where you depend on `foo = "300"`, there isn't a way to clarify that This doesn't cover the case where a dependency is public only if a feature is enabled. -The warning is emitted even when a `pub` item isn't accessible. +The warning/error is emitted even when a `pub` item isn't accessible. You can't definitively lint when a `pub = true` is unused since it may depend on which platform or features. @@ -243,6 +243,9 @@ You can't definitively lint when a `pub = true` is unused since it may depend on - `pub` could be seen as ambiguous with `publish` - While `visibility` would offer greater flexibility, it is unclear if we need that flexibility and if the friction of any feature leveraging it would be worth it - In the end, we went with `pub` because `public` would prevent our "no MSRV bump" approach because cargo versions exist with `public` being reserved +- `rustc`: Instead of `allow` by default for pre-2024 editions, we could warn by default + - More people would get the benefit of the feature now + - However, this would be extremely noisy and likely make people grumpy - `Cargo.toml`: Instead of `pub = false` being the default and changing the warning level on an edition boundary, we could instead start with `pub = true` and change the default on an edition boundary. - This would require `cargo fix` marking all dependencies as `pub = true`, while using the warning means we can limit it to only those dependencies that need it. - `Cargo.toml`: Instead of `pub = false` being the default, we could have a "unchecked" / "unset" state @@ -279,14 +282,6 @@ Within the cargo ecosystem: # Unresolved questions [unresolved]: #unresolved-questions -- Will the warning be too disruptive to the ecosystem to enable by default? - - Being automatically fixed with `cargo fix` (with cargo's reminder that you can run it) helps - - Not requiring an MSRV bump helps - - We could instead start it as `allow` - but in `rust-2024-compatibility` group, - still turning into an error in the 2024 edition, - and have the edition migration reduce the blast radius. - # Future possibilities [future-possibilities]: #future-possibilities From d1d2afa2e0cdc0a8c6d0922387017f51df9bacd6 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 24 Oct 2023 10:33:30 -0500 Subject: [PATCH 28/43] fix: Demo multiple version.from --- text/3516-public-private-dependencies.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index 0528252967c..e1a9cdda057 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -354,8 +354,9 @@ One possible approach for this: name = "some-cli" [dependencies] -clap = { version.from ["clap_complete"] } +clap = { version.from ["clap_complete", "clap_mangen"] } clap_complete = "3.4" +clap_mangen = "3.4" ``` When resolving the dependencies for `some-cli`, the resolver will not explicitly choose a version for `clap` but will continue resolving the graph. From a543c1448893447d39f2cbc3528fa6016a856d05 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 24 Oct 2023 10:38:02 -0500 Subject: [PATCH 29/43] feat: Talk about the Index --- text/3516-public-private-dependencies.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index e1a9cdda057..5e1a5159060 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -254,6 +254,11 @@ You can't definitively lint when a `pub = true` is unused since it may depend on - `cargo add`: instead of `--pub ` it could be `--pub` / `--no-pub` like `--optional` or `--public` / `--private` - `cargo add`: when adding a dependency, we could automatically add all of its `pub` dependencies. - This was passed up as being too noisy, especially when dealing with facade crates, those that fully re-export their `pub = true` dependency +- We leave whether `pub` is in the Index as unspecified + - It isn't strictly needed now + - It would make `cargo add` easier + - If we rely on `pub` in the resolver, we might need it but we can always backfill it + - Parts of the implementation are already there from the original RFC ## Minimal version resolution From 84c42bab0a810fc1a402ab7f6380f0c0a2653392 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 24 Oct 2023 10:52:44 -0500 Subject: [PATCH 30/43] fix: Make reduced warnings for bins clearer --- text/3516-public-private-dependencies.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index 5e1a5159060..6619353c789 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -193,7 +193,8 @@ A new dependency field, `pub = ` will be added that defaults to `false`. This field can be specified in `workspace.dependencies` and be overridden when `workspace = true` is in a dependency. When building a `lib`, Cargo will use the `priv` modifier with `--extern` for all private dependencies. What is private is what is left after recursively walking public dependencies (`pub = true`). -We'll ignore this for other build target kinds (e.g. `bin`) as that would create extra noise. +For other [`crate-type`s](https://doc.rust-lang.org/cargo/reference/cargo-targets.html#the-crate-type-field) (e.g. `bin`), +we'll tell rustc that all dependencies are public to reduce noise from inaccessible `pub` items. Old cargo versions will emit a warning when this key is encountered but otherwise continue, even if the feature is present but unstable. @@ -230,6 +231,7 @@ In the case where you depend on `foo = "300"`, there isn't a way to clarify that This doesn't cover the case where a dependency is public only if a feature is enabled. The warning/error is emitted even when a `pub` item isn't accessible. +We at least reduced the impact of this by not marking dependencies as private for `crate-type=["bin"]`. You can't definitively lint when a `pub = true` is unused since it may depend on which platform or features. From 072c5e720d21c91f2a860dd9ec9850eead3ee1f3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 24 Oct 2023 21:51:51 -0500 Subject: [PATCH 31/43] fix: Include another application of pub/private --- text/3516-public-private-dependencies.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index 6619353c789..fa8acd808b5 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -409,4 +409,12 @@ lockstep. See [rust-lang/cargo#6018](https://github.com/rust-lang/cargo/issues/6018) +## Flag for `cargo doc` to skip inaccessible dependencies + +When building documentation for local development, +a lot of times only the direct dependencies and their public dependencies are relevant but you can get stuck generating documentation for large dependencies +([rust-lang/cargo#4049](https://github.com/rust-lang/cargo/issues/4049)). +Instead, `cargo doc` could have a flag to skip any of the dependencies that aren't relevant to local development which are private transitive dependencies. +See [rust-lang/cargo#2025](https://github.com/rust-lang/cargo/issues/2025). + [RFC 1977]: https://github.com/rust-lang/rfcs/pull/1977 From 0f544d213e4621a3522539f8265e35e73f9e09e6 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 1 Nov 2023 08:15:57 -0500 Subject: [PATCH 32/43] fix: Include git with version.from --- text/3516-public-private-dependencies.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index fa8acd808b5..e4c1b6cc010 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -361,19 +361,19 @@ One possible approach for this: name = "some-cli" [dependencies] -clap = { version.from ["clap_complete", "clap_mangen"] } +clap = { from = ["clap_complete", "clap_mangen"] } clap_complete = "3.4" clap_mangen = "3.4" ``` When resolving the dependencies for `some-cli`, the resolver will not explicitly choose a version for `clap` but will continue resolving the graph. -Upon completion, it will look to see what version of `clap_complete` was +Upon completion, it will look to see what instance of `clap_complete` was resolved and act as if that was what was specified inside of the in-memory `clap` dependency. -The packakge using `version.from` must be a public dependency of the `from` package. +The packakge using `from` must be a public dependency of the `from` package. In this case, `clap` must be a public dependency of `clap_complete`. -If the different packages in `version.from` do not agree on what the package +If the different packages in `from` do not agree on what the package version should resolve to (clap 3.4 vs clap 4.0), then it is an error. Compared to the resolver doing this implicitly @@ -385,6 +385,9 @@ The downside is it feels like the declaration is backwards. If you have one core crate (e.g. `clap`) and many crates branching off (e.g. `clap_complete`, `clap_mangen`), it seems like those helper crates should have their version picked from `clap`. +Whether this should be specified across all sources (`from`) or per source (`registry.from`, `git.from`, `path.from`) will need to be worked out. +See [rust-lang/cargo#6921](https://github.com/rust-lang/cargo/issues/6921) for an example of using this for git dependencies. + ## Missing feature declaration check It is easy for packages to accidentally rely on a dependency enabling a feature for them. From a3ecfa6e2dce4a1a307f23008b26bf561a509205 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Nov 2023 11:42:25 -0600 Subject: [PATCH 33/43] fix: Extend version.from with the 'distribution' idea --- text/3516-public-private-dependencies.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index e4c1b6cc010..de7be5b7aa5 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -384,6 +384,10 @@ Compared to the resolver doing this implicitly The downside is it feels like the declaration is backwards. If you have one core crate (e.g. `clap`) and many crates branching off (e.g. `clap_complete`, `clap_mangen`), it seems like those helper crates should have their version picked from `clap`. +This can be worked around by publishing a `clap_distribution` package that has dependencies on every package. +Users would depend on `clap_distribution` through a never-matching target-dependency so it doesn't affect builds. +It exists so users would `version.from = ["clap_distribution"]` it, keeping the set in sync. +This only helps when the packages are managed by a single project. Whether this should be specified across all sources (`from`) or per source (`registry.from`, `git.from`, `path.from`) will need to be worked out. See [rust-lang/cargo#6921](https://github.com/rust-lang/cargo/issues/6921) for an example of using this for git dependencies. From 4896e9a6fc2fb505513a2270fdee974fe1ff15d4 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Nov 2023 11:50:23 -0600 Subject: [PATCH 34/43] fix: Switch from `pub` to `public` Since we switched from warn-by-default to allow-by-default, the need to set `pub` on dependencies with an unsupported MSRV is much lower. It'd help with people wanting to enable this now, especially for core packages that are more likely to have lower MSRVs (but also less likely to have dependencies). So if we drop the requirement for allowing `pub` on old MSRVs, we can re-visit using the name `public`. --- text/3516-public-private-dependencies.md | 62 ++++++++++++------------ 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index de7be5b7aa5..1861fcb54a8 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -91,10 +91,10 @@ name = "diagnostic" version = "1.0.0" [dependencies] -serde = { version = "1", features = ["derive"], pub = true } -serde_json = { version = "1", pub = true } +serde = { version = "1", features = ["derive"], public = true } +serde_json = { version = "1", public = true } ``` -For edition migrations, `cargo fix` will look for the warning code and mark those dependencies as `pub`. +For edition migrations, `cargo fix` will look for the warning code and mark those dependencies as `public`. However, for this example, it was an oversight in exposing `serde_json` in the public API. Note that removing it from the public API is a semver incompatible change. @@ -104,7 +104,7 @@ name = "diagnostic" version = "1.0.0" [dependencies] -serde = { version = "1", features = ["derive"], pub = true } +serde = { version = "1", features = ["derive"], public = true } serde_json = "1" ``` ```rust @@ -174,7 +174,7 @@ The main change to the compiler will be to accept a new modifier on the `--exter supplies which marks it as a private dependency. The modifier will be called `priv` (e.g. `--extern priv:serde`). The compiler then emits the lint `external_private_dependency` if it encounters private -dependencies exposed as `pub`. +dependencies exposed as `public`. `external_private_dependency` will be `allow` by default for pre-2024 editions. It will be a member of the `rust-2024-compatibility` lint group so that it gets automatically picked up by `cargo fix --edition`. @@ -189,24 +189,20 @@ This most likely will also be necessary for the more complex relationship of ## cargo -A new dependency field, `pub = ` will be added that defaults to `false`. +A new dependency field, `public = ` will be added that defaults to `false`. This field can be specified in `workspace.dependencies` and be overridden when `workspace = true` is in a dependency. When building a `lib`, Cargo will use the `priv` modifier with `--extern` for all private dependencies. -What is private is what is left after recursively walking public dependencies (`pub = true`). +What is private is what is left after recursively walking public dependencies (`public = true`). For other [`crate-type`s](https://doc.rust-lang.org/cargo/reference/cargo-targets.html#the-crate-type-field) (e.g. `bin`), -we'll tell rustc that all dependencies are public to reduce noise from inaccessible `pub` items. - -Old cargo versions will emit a warning when this key is encountered but otherwise continue, -even if the feature is present but unstable. -While it is unstable, `cargo publish` will strip the field. +we'll tell rustc that all dependencies are public to reduce noise from inaccessible `public` items. Cargo will not force a `rust-version` bump when using this feature as someone -building with an old version of cargo depending on packages that set `pub = +building with an old version of cargo depending on packages that set `public = true` will not start to fail when upgrading to new versions of cargo. -`cargo add` will gain `--pub ` flags to control this field. +`cargo add` will gain `--public ` flags to control this field. When adding a dependency today, the version requirement is reused from other dependency tables within your manifest. -With this RFC, that will be extended to also checking your dependencies for any `pub` dependencies, and reusing their version requirement. +With this RFC, that will be extended to also checking your dependencies for any `public` dependencies, and reusing their version requirement. This would be most easily done by having the field in the Index but `cargo add` could also read the `.crate` files as a fallback. ## crates.io @@ -233,33 +229,35 @@ This doesn't cover the case where a dependency is public only if a feature is en The warning/error is emitted even when a `pub` item isn't accessible. We at least reduced the impact of this by not marking dependencies as private for `crate-type=["bin"]`. -You can't definitively lint when a `pub = true` is unused since it may depend on which platform or features. +You can't definitively lint when a `public = true` is unused since it may depend on which platform or features. # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives ## Misc -- `Cargo.toml`: instead of `pub` (named after the [Rust keyword](https://doc.rust-lang.org/reference/visibility-and-privacy.html), we could name the field `public` (like [RFC 1977]) or name the field `visibility = ""` - - The parallel with Rust seemed worth pursuing +- `Cargo.toml`: instead of `public` (like [RFC 1977]), we could name the field `pub` (named after the [Rust keyword](https://doc.rust-lang.org/reference/visibility-and-privacy.html) or name the field `visibility = ""` + - `pub` has a nice parallel with Rust + - `pub`: Cargo doesn't use abbreviations as much as Rust (though some are used) - `pub` could be seen as ambiguous with `publish` + - `public` already is reserved and requires a `cargo_features`, meaning using it requires an MSRV bump - While `visibility` would offer greater flexibility, it is unclear if we need that flexibility and if the friction of any feature leveraging it would be worth it - - In the end, we went with `pub` because `public` would prevent our "no MSRV bump" approach because cargo versions exist with `public` being reserved - `rustc`: Instead of `allow` by default for pre-2024 editions, we could warn by default - More people would get the benefit of the feature now - However, this would be extremely noisy and likely make people grumpy -- `Cargo.toml`: Instead of `pub = false` being the default and changing the warning level on an edition boundary, we could instead start with `pub = true` and change the default on an edition boundary. - - This would require `cargo fix` marking all dependencies as `pub = true`, while using the warning means we can limit it to only those dependencies that need it. -- `Cargo.toml`: Instead of `pub = false` being the default, we could have a "unchecked" / "unset" state - - This would require `cargo fix` marking all dependencies as `pub = true`, while using the warning means we can limit it to only those dependencies that need it. -- `Cargo.toml`: In the long term, we decided on the default being `pub = false` as that is the common case and gives us more information than supporting a `pub = "unchecked"` and having that be the long term solution. -- `cargo add`: instead of `--pub ` it could be `--pub` / `--no-pub` like `--optional` or `--public` / `--private` -- `cargo add`: when adding a dependency, we could automatically add all of its `pub` dependencies. - - This was passed up as being too noisy, especially when dealing with facade crates, those that fully re-export their `pub = true` dependency -- We leave whether `pub` is in the Index as unspecified + - If we did this, we'd likely want to not require an MSRV bump so people can immediately silence the warning which would require using a key besides `public` (since its already reserved) and treating the field as an unused key when the `-Z` isn't enabled. +- `Cargo.toml`: Instead of `public = false` being the default and changing the warning level on an edition boundary, we could instead start with `public = true` and change the default on an edition boundary. + - This would require `cargo fix` marking all dependencies as `public = true`, while using the warning means we can limit it to only those dependencies that need it. +- `Cargo.toml`: Instead of `public = false` being the default, we could have a "unchecked" / "unset" state + - This would require `cargo fix` marking all dependencies as `public = true`, while using the warning means we can limit it to only those dependencies that need it. +- `Cargo.toml`: In the long term, we decided on the default being `public = false` as that is the common case and gives us more information than supporting a `public = "unchecked"` and having that be the long term solution. +- `cargo add`: instead of `--public ` it could be `--public` / `--no-public` like `--optional` or `--public` / `--private` +- `cargo add`: when adding a dependency, we could automatically add all of its `public` dependencies. + - This was passed up as being too noisy, especially when dealing with facade crates, those that fully re-export their `public = true` dependency +- We leave whether `public` is in the Index as unspecified - It isn't strictly needed now - It would make `cargo add` easier - - If we rely on `pub` in the resolver, we might need it but we can always backfill it + - If we rely on `public` in the resolver, we might need it but we can always backfill it - Parts of the implementation are already there from the original RFC ## Minimal version resolution @@ -273,7 +271,7 @@ This should be handled independent of this RFC. This is deferred to [Future possibilities](#future-possibilities) - This has been the main hang-up for stabilization over the last 6 years since the RFC was approved - For more on the complexity involved, see the thread starting at [this comment](https://github.com/rust-lang/rust/issues/44663#issuecomment-881965668) -- More thought is needed as we found that making a dependency `pub = true` can be a breaking change if the caller also depends on it but with a different semver incompatible version +- More thought is needed as we found that making a dependency `public = true` can be a breaking change if the caller also depends on it but with a different semver incompatible version - More thought is needed on what happens if you have multiple versions of a package that are public (via renaming like `tokio_03` and `tokio_1`) Related problems potentially blocked on this @@ -347,7 +345,7 @@ How this will work: If we want to go this route, some hurdles to overcome include: - Difficulties in working with cargo's resolver as this has been the main hang-up for stabilization over the last 6 years since the [RFC 1977] was approved - For more on the complexity involved, see the thread starting at [this comment](https://github.com/rust-lang/rust/issues/44663#issuecomment-881965668) -- More thought is needed as we found that making a dependency `pub = true` can be a breaking change if the caller also depends on it but with a different semver incompatible version +- More thought is needed as we found that making a dependency `public = true` can be a breaking change if the caller also depends on it but with a different semver incompatible version - More thought is needed on what happens if you have multiple versions of a package that are public (via renaming like `tokio_03` and `tokio_1`) ### Caller-declared relations @@ -378,7 +376,7 @@ version should resolve to (clap 3.4 vs clap 4.0), then it is an error. Compared to the resolver doing this implicitly - It is unclear if this would be any more difficult to implement in the resolver -- Changing a dependency from `pub = false` to `pub = true` is backwards compatible because it has no affect on existing callers. +- Changing a dependency from `public = false` to `public = true` is backwards compatible because it has no affect on existing callers. - It is unclear how this would handle multiple versions of a package that are public The downside is it feels like the declaration is backwards. From 0ad4b92dad3d5db0f9822e7f4d6f44cf06c56667 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 13 Nov 2023 13:14:33 -0600 Subject: [PATCH 35/43] fix: typos Co-authored-by: Eric Huss --- text/3516-public-private-dependencies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index 1861fcb54a8..073e7707827 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -29,7 +29,7 @@ crate `B` but some of the types from crate `B` are exposed through the API in cr - Brittle semver compatibility as `A` might not have intended to expose `B`, like when adding `impl From for AError` for convenience in using `?` in the implementation of `A`. - When self-hosting documentation, you may want to render documentation for all of your public dependencies as well -- When running `cargo doc`, users may way to render [documentation for their accessible dependencies](https://github.com/rust-lang/cargo/issues/2025) [without the cost of their inaccessible dependencies](https://github.com/rust-lang/cargo/issues/4049) +- When running `cargo doc`, users may want to render [documentation for their accessible dependencies](https://github.com/rust-lang/cargo/issues/2025) [without the cost of their inaccessible dependencies](https://github.com/rust-lang/cargo/issues/4049) - When linting for semver compatibility [there isn't enough information to reason about dependencies](https://github.com/obi1kenobi/cargo-semver-checks/issues/121) Related problems with this scenario not handled by this RFC: From e64a3babf1788e66fd64c751adb4d21ab332da53 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 13 Nov 2023 13:14:53 -0600 Subject: [PATCH 36/43] fix: typos Co-authored-by: Eric Huss --- text/3516-public-private-dependencies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index 073e7707827..29de5006c8b 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -296,7 +296,7 @@ When upgrading one dependency, you might need to upgrade another because you use it to interact with the first, like `clap` and `clap_complete`. The existing error messages are not great, along the lines of "expected `clap::Command`, found `clap::Command`". Ideally, you would be presented instead with a message saying "clap_complete -3.4 is not compatiblw with clap 4.0, try upgrading to clap_complete 4.0". +3.4 is not compatible with clap 4.0, try upgrading to clap_complete 4.0". Even better if we could help users do this upgrade automatically. As solving this, via the resolver, has been the main sticking point for [RFC 1997], From 111f5392aa15b62d4656419e61cf908835859311 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 13 Nov 2023 13:15:07 -0600 Subject: [PATCH 37/43] fix: typos Co-authored-by: Eric Huss --- text/3516-public-private-dependencies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index 29de5006c8b..f1c68a4762c 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -369,7 +369,7 @@ Upon completion, it will look to see what instance of `clap_complete` was resolved and act as if that was what was specified inside of the in-memory `clap` dependency. -The packakge using `from` must be a public dependency of the `from` package. +The package using `from` must be a public dependency of the `from` package. In this case, `clap` must be a public dependency of `clap_complete`. If the different packages in `from` do not agree on what the package version should resolve to (clap 3.4 vs clap 4.0), then it is an error. From 663d078bed928bc517a706dca0d373a03eb02fe9 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 13 Nov 2023 13:15:21 -0600 Subject: [PATCH 38/43] fix: typos Co-authored-by: Eric Huss --- text/3516-public-private-dependencies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index f1c68a4762c..f2f22e9939b 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -299,7 +299,7 @@ Ideally, you would be presented instead with a message saying "clap_complete 3.4 is not compatible with clap 4.0, try upgrading to clap_complete 4.0". Even better if we could help users do this upgrade automatically. -As solving this, via the resolver, has been the main sticking point for [RFC 1997], +As solving this, via the resolver, has been the main sticking point for [RFC 1977], this was deferred out to take smaller, more incremental steps, that open the From f3d0bb808fc2dc12a8ba03b67e793756b117e38e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 13 Nov 2023 13:33:18 -0600 Subject: [PATCH 39/43] fix: Update lint name to match implementation --- text/3516-public-private-dependencies.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index f2f22e9939b..331e068beac 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -79,7 +79,7 @@ The dependencies `serde` and `serde_json` are both public dependencies, meaning This has the implication that a semver incompatible upgrade of these dependencies is a breaking change for this package. With this RFC, in pre-2024 editions, -you can enable add `lints.rust.external_private_dependency = "warn"` to your +you can enable add `lints.rust.exported_private_dependencies = "warn"` to your manifest and rustc will warn saying that `serde` and `serde_json` are private dependencies in a public API. In 2024+ editions, this will be an error. @@ -145,7 +145,7 @@ Since the proc-macro can only guarantee that the namespace `clap` is accessible, As a last-ditch way of dealing with this, a user may allow the error: ```rust #[doc(hidden)] -#[allow(external_private_dependency)] +#[allow(exported_private_dependencies)] #[cfg(feature = "derive")] pub mod __derive_refs { #[doc(hidden)] @@ -155,7 +155,7 @@ pub mod __derive_refs { A similar case is pub-in-private: ```rust mod private { - #[allow(external_private_dependency)] + #[allow(exported_private_dependencies)] pub struct Foo { pub x: some_dependency::SomeType } } ``` @@ -173,16 +173,16 @@ features like `impl Trait` in type aliases if we had it. The main change to the compiler will be to accept a new modifier on the `--extern` flag that Cargo supplies which marks it as a private dependency. The modifier will be called `priv` (e.g. `--extern priv:serde`). -The compiler then emits the lint `external_private_dependency` if it encounters private +The compiler then emits the lint `exported-private-dependencies` if it encounters private dependencies exposed as `public`. -`external_private_dependency` will be `allow` by default for pre-2024 editions. +`exported-private-dependencies` will be `allow` by default for pre-2024 editions. It will be a member of the `rust-2024-compatibility` lint group so that it gets automatically picked up by `cargo fix --edition`. In the 2024 edition, this lint will be `deny`. In some situations, it can be necessary to allow private dependencies to become part of the public API. In that case one can permit this with -`#[allow(external_private_dependency)]`. This is particularly useful when +`#[allow(exported_private_dependencies)]`. This is particularly useful when paired with `#[doc(hidden)]` and other already existing hacks. This most likely will also be necessary for the more complex relationship of `libcore` and `libstd` in Rust itself. From 153b41361a08f48cf53769b024f1bc0bd1bb9a66 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 13 Nov 2023 14:42:35 -0600 Subject: [PATCH 40/43] fix: Typo Co-authored-by: Weihang Lo --- text/3516-public-private-dependencies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index 331e068beac..8aca6306064 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -41,7 +41,7 @@ Related problems with this scenario not handled by this RFC: by not requiring private dependencies to be unified. - Private dependencies are not sufficient on their own for this - There are likely better alternatives, like [Pre-RFC: Mutually-exclusive, global features](https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618) -- Help check for missing feature declarations by duplicating dependencies, rather than unifiying features +- Help check for missing feature declarations by duplicating dependencies, rather than unifying features - See [Missing feature declaration check](#future-possibilities) # Guide-level explanation From ec500c2cf51b3fa430ed7aae6a6b61d5dfb38891 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 13 Nov 2023 14:42:49 -0600 Subject: [PATCH 41/43] fix: Typo Co-authored-by: Weihang Lo --- text/3516-public-private-dependencies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index 8aca6306064..753fe1349d2 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -376,7 +376,7 @@ version should resolve to (clap 3.4 vs clap 4.0), then it is an error. Compared to the resolver doing this implicitly - It is unclear if this would be any more difficult to implement in the resolver -- Changing a dependency from `public = false` to `public = true` is backwards compatible because it has no affect on existing callers. +- Changing a dependency from `public = false` to `public = true` is backwards compatible because it has no effect on existing callers. - It is unclear how this would handle multiple versions of a package that are public The downside is it feels like the declaration is backwards. From 6cf4aa5d7a8b77d4a31fe656e4f4d96950350565 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 13 Nov 2023 14:42:59 -0600 Subject: [PATCH 42/43] fix: Typo Co-authored-by: Weihang Lo --- text/3516-public-private-dependencies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index 753fe1349d2..fb0f7ed751f 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -245,7 +245,7 @@ You can't definitively lint when a `public = true` is unused since it may depend - `rustc`: Instead of `allow` by default for pre-2024 editions, we could warn by default - More people would get the benefit of the feature now - However, this would be extremely noisy and likely make people grumpy - - If we did this, we'd likely want to not require an MSRV bump so people can immediately silence the warning which would require using a key besides `public` (since its already reserved) and treating the field as an unused key when the `-Z` isn't enabled. + - If we did this, we'd likely want to not require an MSRV bump so people can immediately silence the warning which would require using a key besides `public` (since it's already reserved) and treating the field as an unused key when the `-Z` isn't enabled. - `Cargo.toml`: Instead of `public = false` being the default and changing the warning level on an edition boundary, we could instead start with `public = true` and change the default on an edition boundary. - This would require `cargo fix` marking all dependencies as `public = true`, while using the warning means we can limit it to only those dependencies that need it. - `Cargo.toml`: Instead of `public = false` being the default, we could have a "unchecked" / "unset" state From c71a624c9729ec897d7d88b706abd360380a4318 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 13 Nov 2023 14:43:21 -0600 Subject: [PATCH 43/43] fix: Typo Co-authored-by: Weihang Lo --- text/3516-public-private-dependencies.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3516-public-private-dependencies.md b/text/3516-public-private-dependencies.md index fb0f7ed751f..ac1fb71ae35 100644 --- a/text/3516-public-private-dependencies.md +++ b/text/3516-public-private-dependencies.md @@ -236,7 +236,7 @@ You can't definitively lint when a `public = true` is unused since it may depend ## Misc -- `Cargo.toml`: instead of `public` (like [RFC 1977]), we could name the field `pub` (named after the [Rust keyword](https://doc.rust-lang.org/reference/visibility-and-privacy.html) or name the field `visibility = ""` +- `Cargo.toml`: instead of `public` (like [RFC 1977]), we could name the field `pub` (named after the [Rust keyword](https://doc.rust-lang.org/reference/visibility-and-privacy.html)) or name the field `visibility = ""` - `pub` has a nice parallel with Rust - `pub`: Cargo doesn't use abbreviations as much as Rust (though some are used) - `pub` could be seen as ambiguous with `publish`