WIP: Allocator- and fallibility-polymorphic collections #60703
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This was referenced May 10, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
r? @sfackler |
|
|
lachlansneff
suggested changes
May 10, 2019
| /// A hack so the default impl can condition on the associated type. This `Err` | ||
| /// type ought to just live in the `Alloc` trait. | ||
| #[unstable(feature = "allocator_api", issue = "32838")] | ||
| pub trait AllocHelper { |
There was a problem hiding this comment.
I'm fairly certain this trait needn't exist. Since Alloc is still experimental, we can make breaking changes.
There was a problem hiding this comment.
See #52396 this was the only way to make it type check without removing the default methods. Happy to get rid of it once bug is fixed.
There was a problem hiding this comment.
Doesn't matter if it's a breaking change though.
There was a problem hiding this comment.
I think we need to have some default methods; not having that constrains the design a lot. It should be a simple bug to fix; this isn't the spooky specialization-breaking part of specialization.
There was a problem hiding this comment.
Maybe I'm misunderstanding the issue. There are plenty of traits with associated types and default methods in the standard library. Iterator is an example. Don't use specialization, and it should be fine.
There was a problem hiding this comment.
The issue is when the default method is only valid for certain associated types. You can't write that constraint today in a way that works.
There was a problem hiding this comment.
If we remove the {alloc|dealloc}_array and {alloc|dealloc}_one from the Alloc trait like this issue suggests, we don't need any fancy specialization or the helper trait.
There was a problem hiding this comment.
That's true. I still want that bug fixed though! :D That fact that one can write Alloc<Err = !> means that AllocHelper should't need to be referenced (almost?) anywhere. We have a little runway because we need a compiler change for unstable type parameters anyway.
There was a problem hiding this comment.
It's still inelegant to have the helper trait, so I'd like to avoid it if possible.
| @@ -659,7 +670,7 @@ pub unsafe trait GlobalAlloc { | |||
| /// Note that this list may get tweaked over time as clarifications are made in | |||
| /// the future. | |||
| #[unstable(feature = "allocator_api", issue = "32838")] | |||
| pub unsafe trait Alloc { | |||
| pub unsafe trait Alloc: AllocHelper { | |||
src/liballoc/str.rs
Outdated
| @@ -578,6 +579,6 @@ impl str { | |||
| /// ``` | |||
| #[stable(feature = "str_box_extras", since = "1.20.0")] | |||
| #[inline] | |||
| pub unsafe fn from_boxed_utf8_unchecked(v: Box<[u8]>) -> Box<str> { | |||
| Box::from_raw(Box::into_raw(v) as *mut str) | |||
| pub unsafe fn from_boxed_utf8_unchecked<A: Alloc + AllocHelper>(v: Box<[u8], A>) -> Box<str, A> { | |||
There was a problem hiding this comment.
This is a breaking change, I'm pretty sure.
There was a problem hiding this comment.
Actually, I'm not sure. I feel like there might be edgecases where it's not backwards compatible.
There was a problem hiding this comment.
This is an example of something similar where it's not backward compatible because type-inference isn't smart enough.
There was a problem hiding this comment.
I think that's acceptable. It's basically impossible to do anything with a higher order function whose arguments and return type are not constrained by the type checker.
|
|
||
| unsafe impl<A: Alloc> Alloc for AbortAdapter<A> { | ||
| unsafe fn alloc(&mut self, layout: Layout) -> Result<NonNull<u8>, Self::Err> { | ||
| self.0.alloc(layout).or_else(|_| handle_alloc_error(layout)) |
There was a problem hiding this comment.
I feel like we should handle this in a way that includes the actual error that occured. Maybe it'd be best to bound the Err associated type on Debug or Display?
There was a problem hiding this comment.
Sure, but I figure we can hash out what handle_alloc_error should take later?
There was a problem hiding this comment.
handle_alloc_error is stable, so it can't be changed. But yeah, this is less important for the moment.
| /// The type of any errors thrown by the allocator, customarily | ||
| /// either `AllocErr`, for when error recovery is allowed, or `!` | ||
| /// to signify that all errors will result in . | ||
| type Err = AllocErr; |
There was a problem hiding this comment.
Also, why does it have a default error type?
There was a problem hiding this comment.
Optional backwards compat with existing instances.
There was a problem hiding this comment.
We don't need backwards compatibility.
fbstj
reviewed
May 12, 2019
| @@ -610,8 +670,10 @@ impl<T> LinkedList<T> { | |||
| /// assert_eq!(dl.front().unwrap(), &1); | |||
| /// ``` | |||
| #[stable(feature = "rust1", since = "1.0.0")] | |||
| pub fn push_front(&mut self, elt: T) { | |||
| self.push_front_node(box Node::new(elt)); | |||
| pub fn push_front_alloc(&mut self, elt: T) -> Result<(), A::Err> { | |||
There was a problem hiding this comment.
change to a stable method? insta-stabilising a method?
There was a problem hiding this comment.
👍 Right the docs and annotations should stay with the push_front, which is now in a different impl.
fbstj
reviewed
May 12, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
SimonSapin
added
the
S-blocked
|
Landing this is blocked on either:
|
|
@SimonSapin Indeed it is, amended OP to say as much and link to your post. I have no problem with that. You know my generally wanting to slam on the breaks with stabilization, and my own PR is no exception! |
|
@Ericson2314 i tried to experiment a bit with this PR, but it didn't compile for me (see compile output below). Is this a known issue? Compile Output |
|
@phil-opp Most of those yes. I just tried building by running cargo manually, which does work. I need to fix up all the |
|
@Ericson2314 Thanks for clarifying! |
|
☔ The latest upstream changes (presumably #60986) made this pull request unmergeable. Please resolve the merge conflicts. |
phil-opp
reviewed
May 21, 2019
| } | ||
|
|
||
| #[unstable(feature = "allocator_api", issue = "32838")] | ||
| default unsafe impl<A: AllocHelper<Err = AllocErr>> Alloc for A { |
There was a problem hiding this comment.
This seems to cause a "conflicting implementations of trait core::alloc::Alloc" error for allocator crates without #![feature(specialization)]. The full error is:
error[E0119]: conflicting implementations of trait `core::alloc::Alloc` for type `Heap`:
--> /…/linked-list-allocator/src/lib.rs:136:1
|
136 | unsafe impl Alloc for Heap {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: conflicting implementation in crate `core`:
- impl<A> core::alloc::Alloc for A
where <A as core::alloc::AllocHelper>::Err == core::alloc::AllocErr, A: core::alloc::AllocHelper, A: core::alloc::Alloc;
With the feature added to the crate, it compiles fine.
There was a problem hiding this comment.
@phil-opp sorry to get back to you so late. Firstly, there is a proposal to remove the offending methods for which I need this AllocHelper business in the first place. If they aren't however, I'd hope we could use something lighter than specialization for this. They key property is if the choice if default method can be made without regard to the specific parameter, there is no parametric violation, and thus no interesting "specialization". Haskell has had https://ghc.gitlab.haskell.org/ghc/doc/users_guide/glasgow_exts.html#extension-DefaultSignatures along these lines with no global interactions for ages. We should add this limited form of default impls to Rust, and fix #52396 while we are at it.
This turns `Box<T>` into `Box<T, A = Global>`, with a `A: Alloc` bound for impls. Ideally, inherent methods like `Box::new` would be applied to `Box<T, A: Alloc + Default>`, but as of now, that would be backwards incompatible because it would require type annotations in places where they currently aren't required. `impl FromIterator` is not covered because it relies on `Vec`, which would need allocator awareness. `DispatchFromDyn` is left out or being generic over `A` because there is no bound that would make it work currently. `FnBox` is left out because it's related to `DispatchFromDyn`.
5 tasks
We will use this for fallibility polymorphism
Ericson2314
force-pushed
the
allocator-error
branch
from
5b94a6d
to
835df4c
Compare
|
@glandium rebased, fixing conflicts and removing the stage0 |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
See its documentation for why its useful
Ericson2314
force-pushed
the
allocator-error
branch
from
835df4c
to
66b01b6
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #61421) made this pull request unmergeable. Please resolve the merge conflicts. |
Dylan-DPC-zz
removed
the
S-waiting-on-review
|
Closing for being blocked on WG decision, as set in description. |
hdhoang
added
S-blocked-closed
and removed
S-blocked
hdhoang
added
S-blocked
|
Done, thanks for thefeedback. Please add a note to the description too so that triagers can skip this. |
|
Thanks! |
|
Closing this. Thanks for contributing |
|
Yeah it's fine now as the current wg design has evolved a bit. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Reopened #52420. Cannot be merged until WG approves design, but good to keep open so still have CI.
This picks up where #58457 will leave off, adding allocator parameters to the other collections. It also adds and associated error type for allocation, so as to enable fallibility-polymorphic code and support the few impls and other functions which require "infallible" allocation.
When done, should convert all the collections for #42774
Unfortunately, as documented in #52396, I had to make an AllocHelper trait in order to get my default impl to work.
Like #58457 which this continues, this blocked on some decision about stability and the new type parameters. See #60703 (comment) for details.