rust-lang / wg-allocators

Home of the Allocators working group: Paving a path for a standard set of allocator traits to be used in collections!
http://bit.ly/hello-wg-allocators
205 stars 9 forks source link

try_* methods for containers #39

Open Ericson2314 opened 4 years ago

Ericson2314 commented 4 years ago

Given that #23 is closed, the API looks something like:

// OnOomError is a variable, `try_*` is always available
fn try_push(&mut self: Vec<T, A: AllocRef, OnOomError>, t: T) -> Result<(), (AllocError, T)> { .. }

// NoAbort is a data type, used as a sentinel indicating the consumer is prohibited from using the aborting methods.
struct NoAbort();

// Abort is a data type, used as a sentinel indicating the consumer is allowed to use the aborting methods.
struct Abort();

fn push(&mut self: Vec<T, A: AllocRef, Abort>, t: T) -> T {
   match self.push(t) {
     OK(v) => v,
     Err(e) => handle_alloc_error(e),
  }
}

Per https://rust-lang.zulipchat.com/#narrow/stream/197181-t-libs.2Fwg-allocators/topic/Design.20of.20.60AbortAlloc.60.20not.20compatible.20with.20.60try_reserve.60

SimonSapin commented 4 years ago

Prior art for this is https://rust-lang.github.io/rfcs/2116-alloc-me-maybe.html, implemented as try_reserve on various containers (tracking issue: https://github.com/rust-lang/rust/issues/48043). It does not involve encoding error handling in the type system. It’s up to callers to choose to call try_reserve v.s. reserve.

Ericson2314 commented 4 years ago

This policy param is that it greatly help audit code to make sure that OOM is always handled by the library consumer. @TimDiekmann and I both refactored https://github.com/TimDiekmann/alloc-wg a few times and its much easier to do correctly if the type system helps to make sure that one only uses try_ within try_.

kornelski commented 3 years ago

Is io::Read going to be generic over OnOomError? Do you plan to have into and as_ref methods that convert fallibility of the Vec?

I've tried switching my codebases to FallibleVec, and found that incompatibility with standard Vec was really painful. New incompatible type was "viral" and required changing more than I could handle, including being incompatible with std and dependencies.

OTOH trying to use type system to enforce this was ineffective, because allocations within functions that don't escape them via return types or shared structs could not be enforced. I've had to go through a whack-a-mole of finding all vec![], etc. throughout the codebase.

For enforcement of abort-safe code I would rather have some scope-based attribute that I could put on functions or modules, or even crate-wide switch.

I've written more about this here

Ericson2314 commented 3 years ago

See https://github.com/rust-lang/rust/pull/84266 where we just globally disable the non try_ ones everywhere. This will (currently) break most of the ecosystem, but it's the sort of thing that the world could adapt to. It's also very simple. I say start with this, and then worry worry about more complicated things like "some portions are allowed to implicitly use the global OOM handler but not others".

SimonSapin commented 3 years ago

This will (currently) break most of the ecosystem, but it's the sort of thing that the world could adapt to.

I assume you’re either joking or expressing yourself in a way you did not intend. Pushing the ecosystem through a breaking change of this magnitude is obviously a non-starter. Disabling existing stable method must be conditional on some opt-in mechanism.

Ericson2314 commented 3 years ago

@SimonSapin The latter. I meant that just as "std" and "alloc" features slowly made there way through the ecosystem, "global-oom-handling" features would to, that's all.

Ericson2314 commented 3 years ago

Now that https://github.com/rust-lang/rust/pull/84266 is merged (!), I suppose we start adding more unstable try_ methods.

safinaskar commented 2 years ago

Important news! HashMap got method try_insert ( https://github.com/rust-lang/rust/issues/82766 ), where try_ means something completely different to Box::try_new. Please, prevent somehow this try_insert from becoming stable

SimonSapin commented 2 years ago

This isn’t the first API to use a try_ prefix for fallibility other than memory allocation failures. For example RefCell::try_borrow is stable.

If you still think try_insert should be changed, please file a dedicated issue about it (probably on rust-lang/rust) is it’s only indirectly related to this issue.

dpaoliello commented 2 years ago

FYI, https://github.com/rust-lang/rust/pull/95051 adds a bunch of try_ methods to Vec using a technique that @Ericson2314 helped me out with to have one implementation per method, but avoiding the overhead of simply having the infallible methods calling the try_ methods + unwrap().