Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
RFC: fallible-allocation #3140
Changes from all commits
5dc6533
9551eb6
32e137e
2d0c4d9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
As
std
grows more feature flags this turns into anO(n!)
operation, it might be worth thinking about how to mitigate that before adding feature support at all.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.
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
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.
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.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.
Doesn't this mean that if you ever use a crate that doesn't specify
default-features = false
for alloc/std you may suddenly havedefault-features
enabled?I just tested this crate layout:
fn_under_default_feature
:src.rs:
cargo.toml:
If I have a crate depend on this with
default-features = false
, I correctly can't usehmm
, but if I add this crate to my downstream crate:cargo.toml:
In my downstream crate, I can suddenly import and use the
hmm
fnDoes 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?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.
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?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.
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".
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.
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 fullalloc
from the sysroot, or using.cargo/config.toml
andrustflags
to set a custom sysroot containing only restricted libraries.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.
Noting it seems reasonable! It's also possible that the kernel/other projects use cargo-guopy to check their deps for unwanted properties