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: fallible-allocation #3140

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

maurer
Copy link
Contributor

@maurer maurer commented Jun 15, 2021

Move infallibly allocating alloc and std library functions behind a feature, as an extension/stabilization of no_global_oom_handler.

This RFC is specifically not looking to add any new fallible interfaces; those can be handled separately.

Rendered

CCing known interested parties:
@Ericson2314 @joshtriplett @Manishearth

@Manishearth Manishearth added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Jun 15, 2021
@Manishearth
Copy link
Member

cc @rust-lang/libs


## Making this feature accessible to Cargo
If sysroot crates (`std`, `core`, `compiler_builtins`, `alloc`) are specified as a dependency in the manifest, Cargo will attempt to locate a variant of that crate which has the minimum set of requested features to build the package.
Since features union as usual, having a crate with dependencies declared this way would be similar to a modern day `#![no_std]` crate: crates depending on it could still use the full `std` as usual with no issues.
Copy link

@guswynn guswynn Jun 15, 2021

Choose a reason for hiding this comment

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

Doesn't this mean that if you ever use a crate that doesn't specify default-features = false for alloc/std you may suddenly have default-features enabled?

I just tested this crate layout:

fn_under_default_feature:
src.rs:

[cfg(feature = hmm)]
pub fn hmm() {}

cargo.toml:

...
[features]
default = ["hmm"]
hmm = []

If I have a crate depend on this with default-features = false, I correctly can't use hmm, but if I add this crate to my downstream crate:
cargo.toml:

[dependencies]
fn_under_default = { path = "../fn_under_default" } # note no `default-features`

In my downstream crate, I can suddenly import and use the hmm fn

Does this mean that someone trying to use alloc = { default-features = false } can accidentally add a crate that doesn't specify turning off the default features and the compiler will longer protect them?

Copy link

Choose a reason for hiding this comment

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

In the [no_std] case you suddenly will try to link std in (I think) and fail to compile (at least in the embedded case, I think), but in this case, you suddenly can access api's right, which you get no warning for?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is how default features work. There is currently no way to tell cargo "turn this off and keep it off and i don't care if that makes the build fail i want it to stay off no matter what".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. You'll note that this is listed under potential future things to address. This behavior is actually beneficial, because it allow crates to state their compatibility with a restricted API while still being usable in more common environments.

If you're considering using this in an environment where you want to be sure the feature is off in the final product, you could consider removing the full std or full alloc from the sysroot, or using .cargo/config.toml and rustflags to set a custom sysroot containing only restricted libraries.

Copy link

Choose a reason for hiding this comment

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

Noting it seems reasonable! It's also possible that the kernel/other projects use cargo-guopy to check their deps for unwanted properties

@joshtriplett
Copy link
Member

The general concept of infallible_allocation seems sensible. However, I want to point out a forward-compatibility concern with the proposed way of handling this in cargo, which has also applied to previous proposals that suggest the same thing:

If crates start declaring dependencies on std with default-features = false, they'll break if we introduce a new enabled-by-default feature in the future. We'd likely find that we could never add a new default feature.

I think, instead, we'd need to have a syntax to say "default minus infallible_allocations", which would be forwards-compatible.

@Nemo157
Copy link
Member

Nemo157 commented Jun 16, 2021

If crates start declaring dependencies on std with default-features = false, they'll break if we introduce a new enabled-by-default feature in the future. We'd likely find that we could never add a new default feature.

The way this is solved in other crates is to move everything behind a feature and make that default on. You can then in the future subset that feature as necessary, and make it activate those new sub-features.

## Testing
In CI, `std` and `alloc`'s ability to build without the `infallible_allocation` feature only needs to be tested on a single platform (likely `x86_64-unknown-linux-gnu`) in order to prevent developers accidentally forgetting to tag new functions with the appropriate feature gate.

When cutting release, all Tier-2 platforms should perform a build with `infallible_allocation` disabled to ensure nothing architecture-specific slipped through (though this should be rare).
Copy link
Member

Choose a reason for hiding this comment

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

As std grows more feature flags this turns into an O(n!) operation, it might be worth thinking about how to mitigate that before adding feature support at all.

Copy link
Contributor

@Ericson2314 Ericson2314 Jun 16, 2021

Choose a reason for hiding this comment

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

The "portability" lint solves this. Features like this which just add items are closed to being able to turn into separate upstream crates (as proposed in the alternatives) which also solves the problem. splitting crates should be thought of as a special case of the portability lint in general, just as https://en.wikipedia.org/wiki/Horn-satisfiability is an easier special case of https://en.wikipedia.org/wiki/Boolean_satisfiability_problem

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 was unaware of the portability lint. After reading the portability lint's RFC, it seems like exactly what we'd want here. That would be superior to the extra builds I'm proposing, but as far as I can tell it's not implemented yet. Since the RFC is merged, hopefully we'll get it before an O(n!) feature explosion catches us.

@Ericson2314
Copy link
Contributor

@joshtriplett it would be useful to reform the concept of default features to mean "give me the default features as of x.y.z". That would solve the problem entirely, and I think corresponds to the way people actually use semver most of the time which is "I tested with x.y.z so give me that or any other compatible version".

@Lokathor
Copy link
Contributor

Lokathor commented Jun 16, 2021

That is much less forward compatible.

Removing a specific feature from the default for your own crate's usage is much better.

Also, a way to "forbid" a feature, at least from a binary, would be nice but not strictly necessary.

@Ericson2314
Copy link
Contributor

@Lokathor What is? How so?

@Lokathor
Copy link
Contributor

Lokathor commented Jun 16, 2021

Saying "default features for x.y.z" means developers can't add a new on by default feature and you get it automatically. Which would, for example, completely destroy the entire effort this RFC is trying to attempt.

Saying "default features but minus this one feature i know i don't want" lets you remove a specific offending feature without upsetting the ability for new on by default features in the library.

Annotation is manual, so we don't need a suppresion mechanism.
@Ericson2314
Copy link
Contributor

@Lokathor Let my clarify that when I say "default features for x.y.z", i mean not the literal feature set but the interface implied by that. so adding new features does work.

As @Nemo157 says

The way this is solved in other crates is to move everything behind a feature and make that default on. You can then in the future subset that feature as necessary, and make it activate those new sub-features.

My thing would be the equivalent of turning

default = [ "foo, "bar" ]

into

default_x_z_y = [ "foo, "bar" ]
default = [ "default_x_z_y" ]

and then later versions can define default_x_z_y as need be. The trick is the user doesn't need to worry about default_x_z_y but just worries about no default features. We can do similar things for default-features = false too, i.e how the old empty set becomes the new non-empty set. The trick is to realize users refer to names with respect to a fixed version, no with respect to whatever future compatible version the Cargo solver picks.


Stepping back a bit, feature sets form a partial order, and when packages are compatible, there should be a partial order homomorphism from the old package's feature sets to the new ones. This accounts for all the scenarios we were imagining. The trick is simply to make defining the partial order relationship as easy as possible because users won't want to think about the math.


Stepping back again, none of these issues are unique to the standard library and the RFC at hand; the same problems can block crates.io. I sometimes feel holding Cargo to a higher standard for stdlib crates is holding us back, we should other make Cargo better for everyone, or not let this stuff get in the way of making Cargo and the standard library rely on each other more.

@Amanieu
Copy link
Member

Amanieu commented Jun 17, 2021

I feel that this should be implemented as a lint rather than messing with Cargo features. Fundamentally this is about enforcing a form of code style ("don't use infallible allocation") rather than working around a missing OS feature ("my target doesn't have a memory allocator").

@Manishearth
Copy link
Member

@Amanieu

I feel that this should be implemented as a lint rather than messing with Cargo features. Fundamentally this is about enforcing a form of code style ("don't use infallible allocation") rather than working around a missing OS feature ("my target doesn't have a memory allocator").

Is it? I can imagine, over time, some stdlib features getting dual support for alloc and no-alloc, where they still work without allocations but perhaps not as efficiently.

@programmerjake
Copy link
Member

@Amanieu

I feel that this should be implemented as a lint rather than messing with Cargo features. Fundamentally this is about enforcing a form of code style ("don't use infallible allocation") rather than working around a missing OS feature ("my target doesn't have a memory allocator").

Is it? I can imagine, over time, some stdlib features getting dual support for alloc and no-alloc, where they still work without allocations but perhaps not as efficiently.

sorting would be a good example --- some algorithms need extra memory (e.g. merge sort), others can work in-place (e.g. quick sort).

@Amanieu
Copy link
Member

Amanieu commented Jun 17, 2021

We already have a split for alloc/no-alloc: everything that requires allocation is in alloc, while everything that doesn't is in core.

@Manishearth
Copy link
Member

We already have a split for alloc/no-alloc: everything that requires allocation is in alloc, while everything that doesn't is in core.

No, I'm talking about things that can work with a split at an implementation level. libstd has many things which also need allocations, but I wouldn't be surprised if some of it could be implemented less efficiently without alloc.

This RFC is for libstd, not just core.

@matklad
Copy link
Member

matklad commented Jun 18, 2021

It's unclear to me which parts of std would actually be usable without infallible_allocation. I have a suspicion that almost everything useful in std actually requires allocation? For example, every syscall which uses Path needs to allocate to convert OsString to something the OS can use (appending \0 on Linux, utf-16 encoding on windows). But maybe we can hack around that with adding an io::ErrorKind::BadAlloc?

To be clear, I don't want to get into specifics in this thread, but it would be useful if RFC contained some kind of a table which, for each std module, classified it as "just works", "needs minor API additions", "needs major API additions/redesign". That way, the readers can imagine what std with fallible alloc looks like.

@maurer
Copy link
Contributor Author

maurer commented Jun 18, 2021

I don't think a ton of std would remain available in the immediate aftermath of this feature. Part of the point is to highlight small gotchas (e.g. Path to OsString conversion) which could be remedied without much work. In cases where a io::Result is already returned, adding an io::ErrorKind::BadAlloc seems like it could improve things even in the normally-infallible case, and since io::ErrorKind is non-exhaustive, no APIs would actually be broken.

If you think it would be useful to go through and annotate I can do that. I would have to do that eventually anyways to actually implement the feature split, so I can try doing the feature split and add to the RFC a summary of what would land on what side of the split, and how easy it would be to move things to out from under the feature.

@jstarks
Copy link

jstarks commented Jun 21, 2021

If you start returning BadAlloc from something that previously could only fail on a precondition violation, then you have introduced a breaking change to the API contract: previously, callers could assume that if the preconditions held, then the function succeeded.

Callers that use unwrap on the result in this case will not see a behavior change (at least if they are using panic=abort...), but callers that are ignoring the result will get silent behavior changes under low resource conditions. This could introduce security bugs into what is today correct code.

See related discussion around rust-lang/rust#84612, e.g. rust-lang/rust#84612 (comment).

A possible counterargument is that (on most OSes?), most syscalls that take strings are fallible, so at least for the PathBuf -> CString case, callers that ignored the result of such calls were already broken.

I suppose one option would be to only return BadAlloc when infallable_allocations was opted out. Then if your crate opts out of this feature, you must not only use only fallible versions of allocating APIs, but you must ensure your code treats all APIs returning io::Error as possibly fallible. This might also provide a solution for the above linked issue.

@Manishearth
Copy link
Member

I suppose one option would be to only return BadAlloc when infallable_allocations was opted out

I believe that is indeed the proposal

@yaahc yaahc added the A-error-handling Proposals relating to error handling. label Jun 21, 2021
@maurer
Copy link
Contributor Author

maurer commented Jun 21, 2021

For functions returning io::Result, I was in fact suggesting we could change these functions to return BadAlloc in the general case. These functions are not ones where you can assume the success arm of the Result because you have provided "good" arguments - IO can fail in a variety of ways outside your program that you don't have control over.

I agree that it is not necessarily the case that already returning a bool/Option/Result implies that BadAlloc can be safely added as an error condition, especially in the case described above where a user might have been using a documented statement on how the function can fail.

However, I think this should be a separate discussion, not for this RFC. This RFC only adds the distinction between infalliable_allocations and non-infallible interfaces. I'm intentionally leaving the details of how the new interfaces should look to future discussion, because that is also a complex issue that deserves discussion, and may differ for things which today return io::Result vs other situations.

@yaahc
Copy link
Member

yaahc commented Jun 21, 2021

I've nominated this RFC for discussion in the next @rust-lang/libs-api meeting. Here are my notes and questions from my review of the RFC / comments so far:

  • Is there any reason to not expose this configuration via feature flagging?
    • Amanieu: Can this instead be implemented as a lint?
      • Manishearth: Doing it as a lint will preclude creating alternative versions for the same functions that work less efficiently but don't allocate in order to preserve more of the API when infallible allocation is disabled.
        • Do we want alternative impls of the same functions for with/without fallible allocation? Why not just create an alternative version that exposes the allocation failure rather than silently introducing a less efficient version of the same function?
  • How badly will users of this configuration need the ability to ban specific features from being enabled at all?
    • guswynn: probably not urgent, can be enforced with cargo-guppy
  • Nemo157: Does this mean we need to implement the portability lint more urgently?
    • Does this RFC's implementation need to be blocked on the portability lint existing? Likely not, this will only become an issue once we've got a large number of features.
  • While this is unstable, is there any reason to block implementing this RFC on the forward compatibility concerns around default features?

How do we want to resolve the enabled by default feature forward compatibility issue? Proposals:

  • Nemo157: feature flag all of std and subset it in the future when we need to add new features
  • Erikson2314: Create alias features for sets of defaults.
  • joshtriplett: Create a syntax for disabling individual default features but leaving the rest of the defaults on.

Do the latter two solutions even solve the issue around not being able to add new default features? Why can't you still use default-features = false and end up with the same issues around not being able to move any existing APIs behind a new enabled by default feature flags? It seems like the latter two solutions also implicitly ban the usage of default-features = false for sysroot crates.

@jstarks
Copy link

jstarks commented Jun 21, 2021

However, I think this should be a separate discussion, not for this RFC. This RFC only adds the distinction between infalliable_allocations and non-infallible interfaces. I'm intentionally leaving the details of how the new interfaces should look to future discussion, because that is also a complex issue that deserves discussion, and may differ for things which today return io::Result vs other situations.

I agree, the question of the precise interfaces is separable. But I think there's a question to be answered now, too--what's the process for marking APIs as in the safe-for-fallible set? Is it opt-in or opt-out? I believe currently whether an API in the alloc or std crates actually allocates is an implementation detail. The existence of this feature will make it contractual--APIs available without the infallible_allocations feature must either:

  • Not allocate
  • Allocate but have fallback behavior when allocation fails
  • Report allocation failures via Result

And they must preserve this behavior for all time. This may not always be desirable, even if a function does not currently allocate.

Similar to const fn, perhaps is something that would need to be considered and stabilized on a per-function basis. And by default all functions would need to be considered out of scope for infallible_allocations.

@Ericson2314
Copy link
Contributor

How do we want to resolve the enabled by default feature forward compatibility issue?

I am thinking of opening a Cargo RFC about that problem. I figure having two separate technical discussions is better since the deficiency with Cargo features is in no way specific to this RFC or the standard library. The final policy decision of course will depend on both, but I still think separate threads will probably be more manageable.

@Manishearth
Copy link
Member

@maurer Out of curiosity, how well does the portability lint address your use case? I do feel like feature flagging is the better option here, but it's probably worth comparing the two.

Co-authored-by: Jane Lusby <jlusby42@gmail.com>
@Ericson2314
Copy link
Contributor

@Manishearth I don't see why they would be mutually exclusive.

  • The portability lint ought to work on arbitrary cfg formulae, so it is useful to catch errors without 2^n rebuilds whether or not we use ad-hoc cfg tokens or Cargo features.
  • Cargo features are useful to mediate feature needs between dependencies. ad-hoc cfg tokens simply don't have any mechanism for that, so it would be very hard to foster a "no global OOM handing" ecosystem the way we fostered a "no std" ecosystems.

It thus seems clear to me that one wants both. The real alternative is cfg vs separate crate. Because separate crate both avoids the 2^n problem (crate boundaries are enforced like the portability lint would) and supported by Cargo (basic deps, simpler than features).

@Manishearth
Copy link
Member

Manishearth commented Jun 21, 2021

@Ericson2314 Yeah, that's fair! To be clear; I wasn't 100% sure on how the portability lint fits in here, but also I hadn't seen it addressed in the thread so I asked. That's useful context, thanks!

@joshtriplett
Copy link
Member

joshtriplett commented Jun 23, 2021

We discussed this in today's @rust-lang/libs-api meeting.

We didn't have any objections in principle to the concept of this.

Our biggest concern was the stability guarantees around these feature flags; we're very concerned about ending up with unexpected breakage when trying to add or modify feature flags over time. If we can address that, such as by ensuring that this can only be used on a nightly toolchain, then we'd be in favor of enabling experimentation here.

We need to see a plan for how that could potentially work. (There are several such proposals in this thread; we just need to see a concrete proposal to use one of them that's capable of meeting our stability requirements, and that for now would keep this exclusive to nightly.)

@Lokathor
Copy link
Contributor

build-std is itself unstable, so this would automatically also be unstable until at least then, right?

@the8472
Copy link
Member

the8472 commented Jun 23, 2021

build-std is itself unstable, so this would automatically also be unstable until at least then, right?

The issue is that build-std may be stabilized while a big feature like fallible allocation still needs time to bake as unstable.

@Lokathor
Copy link
Contributor

Right, it's not a permanent solution, just a stop-gap.

@Manishearth
Copy link
Member

Hmm, I'm actually hitting a similar issue in -Zbuild-std where we need a way for users of -Zbuild-std to globally opt in to a Rust feature flag imposed by libstd: rust-lang/wg-cargo-std-aware#69

In that situation, -Zbuild-std on certain platforms requires a feature flag for every use of std::. I think we might want something similarly shaped here, but as in that situation, provide a global opt in so that you don't have to patch your dependencies. Perhaps -Zstd-cfg-fallible-allocations or something

@inquisitivecrystal
Copy link
Contributor

inquisitivecrystal commented Jun 25, 2021

The general concept of infallible_allocation seems sensible. However, I want to point out a forward-compatibility concern with the proposed way of handling this in cargo, which has also applied to previous proposals that suggest the same thing:

If crates start declaring dependencies on std with default-features = false, they'll break if we introduce a new enabled-by-default feature in the future. We'd likely find that we could never add a new default feature.

I think, instead, we'd need to have a syntax to say "default minus infallible_allocations", which would be forwards-compatible.

Okay, possibly silly comment: why don't we make a general syntax to say "don't enable this specific feature, even if it's part of default"? It's always felt like a big hole to me that to opt out of even one default feature, you have to opt out of all of them, absent the library being specifically designed to deal with the situation. I know you can always work around that by manually enabling them, but then you bump into the same problem we do here.

@yaahc
Copy link
Member

yaahc commented Jun 25, 2021

Okay, possibly silly comment: why don't we make a general syntax to say "don't enable this specific feature, even if it's part of default"? It's always felt like a big hole to me that to opt out of even one default feature, you have to opt out of all of them, absent the library being specifically designed to deal with the situation. I know you can always work around that by manually enabling them, but then you bump into the same problem we do here.

Not a silly comment. I think this is definitely a possible option we could go for and should be considered as an option in the RFC/Plan for how we can resolve the forward compatibility concern around std feature flags.

@programmerjake
Copy link
Member

how about features = ["!fallible-allocation"] as the new syntax for opting-out of features? There could also be a option in std's Cargo.toml that disallows using default-features = false, requiring users to explicitly list all features they wish to opt-out of.

@Ericson2314
Copy link
Contributor

Opting out is no solution. Now if someone else adds a regular optional feature, well that's getting dragged in to. And say that new feature depend on the one you opted out of!

@programmerjake
Copy link
Member

Opting out is no solution. Now if someone else adds a regular optional feature, well that's getting dragged in to. And say that new feature depend on the one you opted out of!

opting out of features could be transitive, where if feature a depends on feature b, opting out of feature b would also opt out of feature a. cargo would error if you explicitly opt-in for a feature that was directly or transitively opted-out of.

@Ericson2314
Copy link
Contributor

@programmerjake that would mean it becomes a breaking change to add dependencies between existing features, because that means opt-outs can turn off more features than before.

Thinking about comparability is hard enough without allowing negative reasoning.

@programmerjake
Copy link
Member

@programmerjake that would mean it becomes a breaking change to add dependencies between existing features, because that means opt-outs can turn off more features than before.

but adding dependencies is already a breaking change if you have feature a which is changed to depend on feature b and you want feature a but require feature b to not be enabled (e.g. feature b could depend on std on a no_std target).

making it so opting out of feature b automatically disables feature a isn't that different -- you would still not be able to use feature a without feature b after the breaking change.

@Ericson2314
Copy link
Contributor

@programmerjake It's really important to distinguish between:

  • Cargo failing to find a solution with the new version where it did before
  • Cargo finding a solution with the version that fails to build

Only the latter is a real breaking change. The former is still unfortunate, but allows the solver to do it's job keeping the user's build working.

Today it is impossible to force off a feature in dependency specifications, and I even believe it is disallowed in the workspace root, so you would just get a std feature which would fail to build. But if we could express that the std feature isn't buildable without some platform functionality, then it would squarely be the second case: a solver failure saying something like foo -> std -> cfg(any(target_os = "linux", target_os = "windows", target_os = "macos")), the last of which is not met.

With your thing, however, would loose features you always had before, which would trigger a build failure, the second case.

@programmerjake
Copy link
Member

@programmerjake It's really important to distinguish between:

  • Cargo failing to find a solution with the new version where it did before
  • Cargo finding a solution with the version that fails to build

Only the latter is a real breaking change. The former is still unfortunate, but allows the solver to do it's job keeping the user's build working.

I disagree, both are breaking changes:

so are you saying cargo will automatically move to an older version of the package if it fails to find a solution with the newer version? that's news to me.

If cargo doesn't automatically move to an older version, it's still a breaking change since if cargo can't solve the features it will fail to build, breaking previously working code -- just failing before running rustc instead of while running rustc.

If cargo automatically moves to an older version, I'd still consider it a breaking change since cargo silently ignores the new version, which may fix critical bugs, even if the version number is compatible, making it very user-unfriendly.

Today it is impossible to force off a feature in dependency specifications, and I even believe it is disallowed in the workspace root, so you would just get a std feature which would fail to build. But if we could express that the std feature isn't buildable without some platform functionality, then it would squarely be the second case: a solver failure saying something like foo -> std -> cfg(any(target_os = "linux", target_os = "windows", target_os = "macos")), the last of which is not met.

With your thing, however, would loose features you always had before, which would trigger a build failure, the second case.

no, if you declare the features you depend on then the solver will error after that broken crate made a breaking change. they only get silently disabled if you were depending on the feature being enabled by default, rather than explicitly naming it or some feature that transitively depends on it.

In any case, the problem is caused by the crate making the breaking change but not correctly reflecting that in the version number.

@Ericson2314
Copy link
Contributor

Ericson2314 commented Jul 1, 2021

I am going to finish writing my counter-proposal soon and I think it would be better to discuss that there. It is rather off topic here.

@Ericson2314
Copy link
Contributor

My proposal is up: #3146

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-error-handling Proposals relating to error handling. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.