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.

Sign up for GitHub

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

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

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8a883f9
Add initial feature-visibility RFC split from feature-metadata
tgross35 Sep 9, 2023
04ee34d
Update RFC number
tgross35 Sep 9, 2023
7247b2c
Misc cleanup
epage Sep 10, 2023
50fba0d
Add motivation and one alternative. Remove unused items, fix typos
tgross35 Sep 15, 2023
187afcf
Clarify index changes, add cfg diagnostic drawback
tgross35 Sep 15, 2023
778bdd7
Update text/3487-feature-visibility.md
tgross35 Sep 15, 2023
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
165 changes: 165 additions & 0 deletions text/3487-feature-visibility.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
- 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 •
edited
Loading

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

[Cargo features](link to features doc) are one of the main means to support conditional build and optional dependency
configuration in Rust crates. Often these are for configuration options that a
library user may want, but another common use case is hiding API that shouldn't
be available to downstream users. Examples include:

- Debugging, benchmarking or test-related features that expose unstable internal
API
- Intermediate features that are enabled by user-facing features but not meant
to be used on their own (e.g. a feature enabling dependency features)

A way to hide these features from user-facing configuration will make options
easier to understand and lowers the chance of library users accidentally using
unstable internal API.

# 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.


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 means that there will be no way
to `cfg` based on these features.
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

Comment on lines +95 to +98
Copy link

Choose a reason for hiding this comment

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

I think stripping features out on publish is a bad idea. I haven't thought through all the ramifications, but having different behaviour for the crates.io tarball and for the actual source code from git seems like a beartrap.

I'm not sure why any changes are needed to the infrastructure for initial support. The infrastructure would display the features and convey them to clients, and it would be up to clients to enforce them.

However, we do need something that a user of this feature can put in their crate that will prevent old versions of cargo from failing to honour the public flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI this is already being discussed some in #3487 (comment)

As for why infrastructure is needed is a "it depends". There are a lot of ways to slice and dice things to possibly make things work.

When dependency resolution happens, it works off of a Summary of the Cargo.toml that is stored inside the Index. If we want to prevent use when resolving dependencies, it must be present in the Summary. We could instead do a second pass on the dependency tree, checking for use of private features. This would likely break down quickly as we don't know why features are enabled (other features from within the package or the caller). Setting that aside, this would require downloading and extracting of .crate files which we normally do lazily. So we either lose the lazy approach or we only do the verification for what is currently being built which means the answer won't always be visible but you need to test every platform and feature combination.


Full support does not need to happen immediately, since it will require this
information be present in the index. The [`feature-deprecation`] RFC describes
a way to add attributes to features in a forward-compatible way under a
`features3` key, which would be suitible for any additional information needed
here.

# 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
- This adds confusion to the `cfg` diagnostics introduced in
<https://github.com/rust-lang/rust/pull/109005>

# Rationale and alternatives

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

- Currently, `docs.rs` will hide features from its autogenerated feature list
if they start with a leading underscore. This convention would work here, but
it would not be consistent with the Rust language (leading underscores indicate
unused variables, lang items are used to indicate visibility)

# 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?
- 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 (something such as `stable = 3.2` could be used to indicate when a
feature was stabilized). See also:
<https://github.com/rust-lang/cargo/issues/10882>
- 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
[`feature-deprecation`]: https://github.com/rust-lang/rfcs/pull/3486