-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Reject crates.io uploads which declare a feature named no_std
#1841
Conversation
Additivity being a subtle thing that too few people know about is a serious and neglected issue. But, sorry, this strikes me as a very ad-hoc counter-measure. Also, as the creator of |
We could perhaps disallow negating |
Noooooooooo! That's the only way to conditionally change your API based on the feature set available. You need to know if you are "std" or "not std". |
@Ericson2314 I was going to say that would rule out patterns like #[cfg(not(feature = "fast_foo"))]
pub fn foo() { ... }
#[cfg(feature = "fast_foo")]
pub fn foo() { ... } but we can work around that: pub fn foo() {
foo_impl()
}
#[cfg(not(feature = "fast_foo"))] fn foo_impl() { ... }
#[cfg(feature = "fast_foo")] fn foo_impl() { ... } Not sure how we can preserve backwards compatibility though. Maybe have it as a warn-by-default lint? |
@cmr Is there a case where going from "not std" to "std" in fact changes or removes an API? I think that's what @Ericson2314 is trying to get at: that enabling a feature should always add something. And if it's implemented as a lint, then it can always be overridden. |
@lfairy for example, serde. Serde will change things to not use String/Vec if those aren't available, and will sometimes instead use &'static str. |
(Full disclosure: I implemented that) |
In general, my initial thought here is that this is
So that said, in general, I am in favor of this kind of thing. I wouldn't want to suggest this for everything, but |
@cmr I find this idea very interesting; I didn't know that crates did this |
@ifairy Ah good catch. I think your |
I am confused as to why it is worth going through the effort of making a change that will affect four thousandths of one percent of crates. It also seems pretty dubious to do this by blacklisting exactly one feature name. |
I few things from talking on IRC:
|
|
||
This can cause issues when one of these features is *negative*. In particular, a crate may provide a `no_std` feature, which restricts its API so that it works with just the `core` library. But because features can only be enabled, not disabled, any consumer which enables `no_std` here will also enable it for every other user of this crate. Most of the time, this is not what the user wants to do. | ||
|
||
As of this writing, there are [28 crates] on crates.io (0.004% of 7,426 total) with a feature that contains `no_` as a substring. Out of these, 21 of them have `no_std`. As for the remaining seven: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.4% 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a couple orders of magnitude, what's the big deal? 😉
(I'll fix it)
A broader thing than blacklisting one common feature name would be a crater-like service that compiles new crates with all possible feature combinations, to find out whether they really are additive. |
@durka that's basically what I'm proposing, except it needs to be a smart tool deeply leveraging incremental compilation to not be the most crazily exponential, crazily big constant factors, expensive thing. |
Some crates are not designed to be used with all possible feature combinations. Every optional dependency is implicitly a "feature" name. I document which features are intended to be enabled, can't support a random pick. (However additivity is of course obeyed.) |
@bluss that's valid, but actually doesn't get one out of a positivity requirement. I've long wanted Cargo packages giving a boolean expression of their features that must always be true---this is the best way to declare up front what feature combinations make sense. |
Are you saying Cargo automatically turns optional dependencies into features, or that users in practice will make sure every optional dependency is covered by at least one feature? |
Every optional dependency ( |
Woah. I'm not sure how I feel about that, but I guess it means with stdlib-aware cargo an explicit "std" feature isn't even necessary. |
Well, maybe it doesn't have to be a crater service. It could be part of
This is the reality, but is it at odds with the intended purpose of features? Do your crates where a random pick wouldn't work fall into categories of missing functionality that could be addressed by a more flexible feature system (e.g. rust-lang/cargo#2980, etc)? |
Btw for intuition on positivity and restricted feature sets, Take a look at this Hasse diagram: When following edges upword, the set of features (1s, not 0s) becomes larger, and the interface also becomes larger/richer (subtype). Deleting combinations removes nodes and edges from the diagram, but the same positivity restriction still applies to traversing the remaining edges. The fact that there are so many edges also illustrates why checking this the naive way is prohibitively expensive. |
I was thinking of for example: feature "accel" depends on crates "simd", "blas-sys", and "foo"; The intended usage is to enable the feature "accel" or not, not to pick either of the optional deps manually. In the regular case, all the internal code will simply cfg on the main feature variable, but since the other feature combinations are not supported, they may not ever have been compile-tested. Aha! But.. it is a goal for me to ensure a crate compiles with all supported features enabled or disabled. It's not a goal to ensure a crate compiles with nonsensical feature picks. (For example if I pick just features = ["blas-sys", "foo"], what do I get? The documented additions to the crate go off of "accel", so most likely you get nothing.) Real life example ndarray: Cargo.toml gives an unclear picture of which features are supported (Maybe because I didn't document it properly there). The README gives a better picture. |
So, the fact that you have to specify the supported feature combinations in English in the readme means that Cargo.toml isn't as expressive as we need. So that's why I said we likely need some more declarative possibilities before my broader check-everything idea could work. But we've strayed a bit from the title of this issue now. |
While we're derailing, I'd like to point out the checking feature additivity and checking semver compliance are all but identical processes 😉. |
Does one really have to forbid uploads of crates? Wouldn't it be less drastic to make cargo emit a warning at build time? |
@est31 As far as I know, there are no valid reasons to have a |
I don't fully understand all the implications of features or Hasse diagrams, but from what I do understand of this RFC, I think I agree with @alexcrichton that it would be more appropriate to improve documentation in this area, with examples of problematic features, and outreach to particular crate authors. |
@carols10cents BTW, have you used RFC bot yet? There's some basic docs here, and anyone on the tagged team can propose a "motion to FCP" at any point. (If you have questions about when that's appropriate, feel free to ping me on IRC to discuss.) |
@rfcbot fcp close |
Team member @carols10cents has proposed to close this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
@rfcbot reviewed |
Looks like serde has a completely broken no_std feature... serde-rs/serde#918 (comment) |
@whitequark it's not? It's working as intended; it's a defaulted "std" feature, which is how no_std support should be done. |
We have a |
OK, I misunderstood, sorry about that. |
Sheesh right at the heart of the ecosystem. The evidence is stark. Semver compatability, feature additivity, and portability all are simple too subtle, tedious, yet exceedingly important to ask users to police themselves. The same infrastructure can be used to enforce all three. |
It's not, serde did the right thing here, there was a misunderstanding 😄 |
Oops there goes my soapbox 😂 |
Not quite. Serde has a positive In general, the problem is a feature that breaks public api. It forces consumer crates to mirror those features just for the sake of version selection, lest they lock their users to a particular feature set for the dependency. |
Again, this is working as intended. If anyone would like to keep discussing Serde's std feature, please keep that on Serde's issue tracker or #serde IRC. |
I don't see what this has to do with Serde in particular. Non-additive features exist and they work counterintuitively. Whether or not you intended your particular feature to work that way is beside the point. In fact, I'd be interested to hear if anyone has an idea about how to make such features work without causing obscure build errors when you mix up incompatible feature sets. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period is now complete. |
Ok looks like not much new discussion happned during FCP, so I'm going to close. |
Ah and of course thanks regardless for the RFC @lfairy! |
Since Cargo features are additive, it is always better to expose a default-enabled
std
feature instead.Thanks to @cmr for the original idea, and @steveklabnik for suggesting that I write an RFC!
Rendered