Skip to content

Reject crates.io uploads which declare a feature named no_std#1841

Closed
lambda-fairy wants to merge 3 commits into
rust-lang:masterfrom
lambda-fairy:no-no-std-standard
Closed

Reject crates.io uploads which declare a feature named no_std#1841
lambda-fairy wants to merge 3 commits into
rust-lang:masterfrom
lambda-fairy:no-no-std-standard

Conversation

@lambda-fairy

@lambda-fairy lambda-fairy commented Jan 3, 2017

Copy link
Copy Markdown
Contributor

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

@Ericson2314

Ericson2314 commented Jan 3, 2017

Copy link
Copy Markdown
Contributor

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 spin_no_std, I do regret using a name with bad implications, but since it only affects implementation not interface, there is no additivity issue either way.

@Ericson2314

Copy link
Copy Markdown
Contributor

We could perhaps disallow negating feature=... in cfgs on crate-public items. This would solve the problem in its entirety.

@emberian

emberian commented Jan 3, 2017

Copy link
Copy Markdown
Contributor

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

@lambda-fairy

Copy link
Copy Markdown
Contributor Author

@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?

@lambda-fairy

Copy link
Copy Markdown
Contributor Author

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

@emberian

emberian commented Jan 3, 2017

Copy link
Copy Markdown
Contributor

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

@emberian

emberian commented Jan 3, 2017

Copy link
Copy Markdown
Contributor

(Full disclosure: I implemented that)

@steveklabnik

Copy link
Copy Markdown
Contributor

In general, my initial thought here is that this is

  1. a big thing to the ecosystem
  2. kind of a footgun

So that said, in general, I am in favor of this kind of thing. I wouldn't want to suggest this for everything, but no-std as a feature is really, really important, and so might merit as special treatment as we did for *.

@steveklabnik

Copy link
Copy Markdown
Contributor

@cmr I find this idea very interesting; I didn't know that crates did this

@Ericson2314

Copy link
Copy Markdown
Contributor

@ifairy Ah good catch. I think your impl work around is better style anyways---helps catch those two signatures accidentally falling out of sync.

@sfackler

sfackler commented Jan 3, 2017

Copy link
Copy Markdown
Member

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.

@Ericson2314

Ericson2314 commented Jan 3, 2017

Copy link
Copy Markdown
Contributor

I few things from talking on IRC:

  • The "additivity" rule is really (well, in my opinion :)) a contravariance rule that

    ∀ fa, fb: Set<feature>. fa ⊂ fb ⇒ interface(crate-with(fa)) >: interface(crate-with(fb))
    

    or, imperatively in English, adding a feature always makes the crate's interface a subtype of what it was before. [This is contravariance because "subset-of" becomes "supertype-of".]

  • My proposal to disallow negation is sound, and I think complete for width subtyping of the crate's interface, i.e. subtyping from new items. So if you want to use features to extend an API with new items, my rule will certainly stop you from messing up, but also I think never block you by mistake provided you jump through hoops of @ifairy's impl trick.

  • Depth subtyping, i.e., a preexisting item is given a more-refined type with additional features, is harder to verify, however. This is both because multiple variants of a crate need to be compiled through type checking, not just name resolution, and worse, because some subtyping relationships are quite hard to prove.

  • For example, @cmr's examct example of https://github.com/serde-rs/serde/blob/master/serde/src/ser/mod.rs#L29 might be OK if we can show that Into<String> is super-bound of Into<&'static str> via an impl like impl<T, U: Into<T>, V: Into<U>> into<T> for V. But I don't think that impl can be written anyways right now.

Comment thread text/0000-no-no-std-standard.md Outdated

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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

0.4% 😉

Copy link
Copy Markdown
Contributor Author

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)

@nrc nrc added the T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. label Jan 4, 2017
@durka

durka commented Jan 4, 2017

Copy link
Copy Markdown
Contributor

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.

@Ericson2314

Copy link
Copy Markdown
Contributor

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

@bluss

bluss commented Jan 4, 2017

Copy link
Copy Markdown

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

@Ericson2314

Copy link
Copy Markdown
Contributor

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

@Ericson2314

Copy link
Copy Markdown
Contributor

Every optional dependency is implicitly a "feature" name.

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?

@bluss

bluss commented Jan 4, 2017

Copy link
Copy Markdown

Every optional dependency (xyz) is something you can use as --feature = "xyz", so for cargo they are the same thing.

@Ericson2314

Ericson2314 commented Jan 4, 2017

Copy link
Copy Markdown
Contributor

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.

@durka

durka commented Jan 4, 2017

Copy link
Copy Markdown
Contributor

@Ericson2314

it needs to be a smart tool deeply leveraging incremental compilation

Well, maybe it doesn't have to be a crater service. It could be part of cargo publish.

@bluss

Some crates are not designed to be used with all possible feature combinations.

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)?

@Ericson2314

Ericson2314 commented Jan 4, 2017

Copy link
Copy Markdown
Contributor

Btw for intuition on positivity and restricted feature sets, Take a look at this Hasse diagram: 4-feature 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.

@bluss

bluss commented Jan 4, 2017

Copy link
Copy Markdown

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.

@durka

durka commented Jan 4, 2017

Copy link
Copy Markdown
Contributor

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.

@Ericson2314

Ericson2314 commented Jan 4, 2017

Copy link
Copy Markdown
Contributor

While we're derailing, I'd like to point out the checking feature additivity and checking semver compliance are all but identical processes 😉.

@est31

est31 commented Jan 5, 2017

Copy link
Copy Markdown
Member

Does one really have to forbid uploads of crates? Wouldn't it be less drastic to make cargo emit a warning at build time?

@lambda-fairy

Copy link
Copy Markdown
Contributor Author

@est31 As far as I know, there are no valid reasons to have a no_std feature flag. And we have banned wildcard dependencies already, so we have precedent for enforcing rules this way.

@carols10cents

Copy link
Copy Markdown
Member

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.

@aturon

aturon commented May 1, 2017

Copy link
Copy Markdown
Contributor

@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.)

@carols10cents

Copy link
Copy Markdown
Member

@rfcbot fcp close

@rfcbot

rfcbot commented May 2, 2017

Copy link
Copy Markdown

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.

@vadimcn

vadimcn commented May 3, 2017

Copy link
Copy Markdown
Contributor

@rfcbot reviewed

2 similar comments
@japaric

japaric commented May 5, 2017

Copy link
Copy Markdown
Contributor

@rfcbot reviewed

@michaelwoerister

Copy link
Copy Markdown
Member

@rfcbot reviewed

@whitequark

Copy link
Copy Markdown

Looks like serde has a completely broken no_std feature... serde-rs/serde#918 (comment)

@Manishearth

Copy link
Copy Markdown
Member

@whitequark it's not? It's working as intended; it's a defaulted "std" feature, which is how no_std support should be done.

@dtolnay

dtolnay commented May 9, 2017

Copy link
Copy Markdown
Member

We have a std feature (not a no_std feature) that is working as intended.

@whitequark

Copy link
Copy Markdown

OK, I misunderstood, sorry about that.

@Ericson2314

Ericson2314 commented May 9, 2017

Copy link
Copy Markdown
Contributor

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.

@Manishearth

Copy link
Copy Markdown
Member

Sheesh right at the heart of the ecosystem.

It's not, serde did the right thing here, there was a misunderstanding 😄

@Ericson2314

Copy link
Copy Markdown
Contributor

Oops there goes my soapbox 😂

@le-jzr

le-jzr commented May 9, 2017

Copy link
Copy Markdown

Not quite. Serde has a positive std feature, but it's still non-additive, so incurs the same problems as a negative no_std feature would.

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.

@dtolnay

dtolnay commented May 9, 2017

Copy link
Copy Markdown
Member

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.

@le-jzr

le-jzr commented May 9, 2017

Copy link
Copy Markdown

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.

@rfcbot

rfcbot commented May 11, 2017

Copy link
Copy Markdown

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

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label May 11, 2017
@rfcbot

rfcbot commented May 21, 2017

Copy link
Copy Markdown

The final comment period is now complete.

@alexcrichton

Copy link
Copy Markdown
Member

Ok looks like not much new discussion happned during FCP, so I'm going to close.

@alexcrichton

Copy link
Copy Markdown
Member

Ah and of course thanks regardless for the RFC @lfairy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-dev-tools Relevant to the development tools team, which will review and decide on the RFC.

Projects

None yet

Development

Successfully merging this pull request may close these issues.