Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Cargo feature visibility (private/public) #3487

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 160 additions & 0 deletions text/3487-feature-visibility.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
- Feature Name: feature-metadata
Copy link
Contributor

Choose a reason for hiding this comment

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

feature name should be updated

- Start Date: 2023-09-08
- RFC PR: [rust-lang/rfcs#3487](https://github.com/rust-lang/rfcs/pull/3487)
- Rust Issue:
[rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000)

# Summary

[summary]: #summary

This RFC describes a new key under `features` in `Cargo.toml` to indicate that a
feature is private.

Please see the parent meta RFC for background information: [`feature-metadata`].
Copy link
Member

Choose a reason for hiding this comment

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

the link is broken

Copy link
Contributor Author

@tgross35 tgross35 Sep 15, 2023

Choose a reason for hiding this comment

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

Thanks, a lot of links got reshuffled when I split the RFCs. I will fix this

Copy link
Contributor

Choose a reason for hiding this comment

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

This RFC is blocked on feature-metadata


# Motivation

[motivation]: #motivation

WIP

No way to hide unstable API such as testing-only features or internally enabled
features.
tgross35 marked this conversation as resolved.
Show resolved Hide resolved

# Guide-level explanation

[guide-level-explanation]: #guide-level-explanation

There will be a new flag allowed within `[features]`: `public`. This is boolean
flag defaulting to `true` that indicates whether or not downstream crates should
be allowed to use this feature.

```toml
[features]
foo = { enables = [], public = false}
```

Attempting to use a private feature on a downstream crate will result in
messages like the following:

```
error: feature `baz` on crate `mycrate` is private and cannot be used by
downstream crates
```

# Reference-level explanation

[reference-level-explanation]: #reference-level-explanation

`public` is a boolean value that defaults to `true`. It can be thought of as
`pub` in Rust source files, with the exception of being true by default. If set
to `false`, Cargo should forbid its use with an error message on any downstream
crates.

The default `true` is not consistent with [`public_private_dependencies`] or
Rust's `pub`, but is a reasonable default to be consistent with the current
behavior. This means that either `feature = []` or
`feature = { "enables" = [] }` will result in the same configuration.

The name `public` was chosen in favor of `pub` to be consistent with the
[`public_private_dependencies`] RFC, and to match the existing style of using
non-truncated words as keys.
epage marked this conversation as resolved.
Show resolved Hide resolved

In general, marking a feature `public = false` should make tooling treat the
feature as non-public API. This is described as the following:

- The feature is always usable within the same crate:
- Enabled by other features, e.g.
`foo = { enables = [some-private-feature] }`, is allowed
- Referenced in `[[bench]]` and `[[test]]` target `required-features`
tgross35 marked this conversation as resolved.
Show resolved Hide resolved
- Using the feature on the command-line is allowed
Comment on lines +79 to +80
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we expand this to all required-features? That way we can do things like have private bins for testing purposes

- Users may explicitly specifying the private features for their dependencies
on the command-line (e.g. `--features somecrate/private-feature`) which would
otherwise be forbidden
- The feature should not be accepted by `cargo add --features`
- The feature should not be reported from `cargo add`'s feature output report
- A future tool like `cargo info` shouldn't display information about these
features
- Once `rustdoc` is able to consume feature metadata, `rustdoc` should not
document these features unless `--document-private-items` is specified

Attempting to use a private feature in any of the forbidden cases should result
in an error. Exact details of how features work will likely be refined during
implementation and experimentation.
Comment on lines +91 to +93
Copy link
Contributor

Choose a reason for hiding this comment

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

What about

  • cargo install? Is that more like a [dependencies] entry and forbidden or like cargo check --features and allowed?
    • Does the answer change if we are doing the install from registery vs git vs path?
  • What about path dependencies? Should we allow workspace members to have access to private details?
    • If not, what about a path = "." dependency? These are used in dev-dependencies to enable features specific to testing

The answer to these isn't necessarily "yes" or "no" but can also be "not yet, we'll error for now and re-evaluate in the future" at which point it should be in the future possibilities.


Two sample use cases for `public = false` include:

- `docs.rs` having a way to know which features should be hidden
- Features that are included in feature chains (feature `a` enables feature `b`)
but not meant for public consumption could be marked not public
tgross35 marked this conversation as resolved.
Show resolved Hide resolved

This feature requires adjustments to the index for full support. This RFC
proposes that it would be acceptable for the first implementation to simply
strip private features from the manifest; this meanss that there will be no way
tgross35 marked this conversation as resolved.
Show resolved Hide resolved
to `cfg` based on these features.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we strip them, that affects a couple use cases

  • When helping with managing the feature graph, you might want to cfg based on this
  • Activating debug features in clap or winnow via --features winnow/debug

Copy link
Contributor Author

@tgross35 tgross35 Sep 15, 2023

Choose a reason for hiding this comment

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

In that case, do you think it is worth including the index change as part of the MVP?

Copy link
Contributor

Choose a reason for hiding this comment

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

The index change can be fairly minor

  • Add a new private-features = [<name>, ...] entry
    • private because those should be in the minority and keeps the index smaller
  • Only allow them to be used when MSRV is high enough
    • Question is what to do when MSRV is unset.
    • It might be of interest for us to add other MSRV fields, like published-rust-version to record what version of rust was cargo publish done in and a calculated-rust-version that isn't as precise as rust-version but is a minimum value based on things like setting the edition or using private features
    • Alternatively, we could just make it a warning: "Hey, your rust-version is unset, if you publish with private features, older versions of cargo can use them and then will. break on upgrade"

Copy link
Member

Choose a reason for hiding this comment

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

It's not clear what cfg means here. I assumed it is the cfg attribute and the cfg macro. Could you expand and link to their doc respectively?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kind of forgot the exact context here, this comes from Ed's comment at #3416 (comment) and the two following comments. Also #3416 (comment).

Do you have any ideas for how to improve the wording?

Thank you for taking a look by the way

Copy link
Contributor

Choose a reason for hiding this comment

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

If we strip the private features then when cargo does a build, it won't tell rustc about private features and if you do #[cfg(feature = "private-foo")] then it will never evaluate to true


Full support does not need to happen immediately, since it will require this
information be present in the index. [Index changes] describes how this can take
tgross35 marked this conversation as resolved.
Show resolved Hide resolved
place.

# Drawbacks

[drawbacks]: #drawbacks
tgross35 marked this conversation as resolved.
Show resolved Hide resolved

- Added complexity to Cargo. Parsing is trivial, but exact implementation
details do add test surface area
- Added Cargo arguments if escape hatches for `public` are created
- `public` uses may not be common enough to be worth including

# Rationale and alternatives

[rationale-and-alternatives]: #rationale-and-alternatives

TODO
epage marked this conversation as resolved.
Show resolved Hide resolved

# Prior art

[prior-art]: #prior-art

- `docs.rs` treats features with a leading `_` as private / hidden
- Ivy has a [visibility attribute] for its configuration (mentioned in
[cargo #10882])
- Discussion on stable/unstable/nightly-only features
<https://github.com/rust-lang/cargo/issues/10881>

# Unresolved questions

[unresolved-questions]: #unresolved-questions

- Are the semantics of `public` proposed in this RFC suitable? Should private
features be usable in examples or integration tests without a `--features`
argument?
- Should there be a simple `--allow-private-features` flag that allows using all
features, such as for crater runs? This can be decided during implementation.
tgross35 marked this conversation as resolved.
Show resolved Hide resolved
- Does `public` need to be in the index?

# Future possibilities

[future-possibilities]: #future-possibilities

- A `stable` field can be set false to indicate API-unstable or nightly-only
features (somethign such as `stable = 3.2` could be used to indicate when a
tgross35 marked this conversation as resolved.
Show resolved Hide resolved
feature was stabilized). See also:
<https://github.com/rust-lang/cargo/issues/10882>
- A `rust-version` field that could indicate e.g. `rust-version = "nightly"` or
`rust-version = "1.65"` to specify a MSRV for that feature. See:
<https://github.com/rust-lang/rfcs/pull/3416#discussion_r1174478461>
epage marked this conversation as resolved.
Show resolved Hide resolved
- The `public` option could be used to allow optional dev dependencies. See:
<https://github.com/rust-lang/cargo/issues/1596>

[cargo #12335]: https://github.com/rust-lang/cargo/issues/12235
[cargo #10882]: https://github.com/rust-lang/cargo/issues/10882
[`cargo-info`]: https://github.com/rust-lang/cargo/issues/948
[`deprecated`]: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute
[`deprecated-suggestions`]: https://github.com/rust-lang/rust/issues/94785
[discussion on since]: https://github.com/rust-lang/rfcs/pull/3416#discussion_r1172895497
[`public_private_dependencies`]: https://rust-lang.github.io/rfcs/1977-public-private-dependencies.html
[`rustdoc-cargo-configuration`]: https://github.com/rust-lang/rfcs/pull/3421
[`tokio`]: https://docs.rs/crate/tokio/latest/features
[visibility attribute]: https://ant.apache.org/ivy/history/latest-milestone/ivyfile/conf.html