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

try_* methods for containers #39

Open
Ericson2314 opened this issue Feb 12, 2020 · 10 comments
Open

try_* methods for containers #39

Ericson2314 opened this issue Feb 12, 2020 · 10 comments

Comments

@Ericson2314
Copy link

Ericson2314 commented Feb 12, 2020

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
Copy link
Contributor

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: rust-lang/rust#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
Copy link
Author

Ericson2314 commented Feb 12, 2020

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

@TimDiekmann TimDiekmann mentioned this issue Aug 5, 2020
18 tasks
@Ericson2314 Ericson2314 changed the title try_ methods for containers try_* methods for containers Nov 17, 2020
@kornelski
Copy link

kornelski commented Feb 18, 2021

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
Copy link
Author

Ericson2314 commented Apr 22, 2021

See rust-lang/rust#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
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Author

Ericson2314 commented May 6, 2021

Now that rust-lang/rust#84266 is merged (!), I suppose we start adding more unstable try_ methods.

@safinaskar
Copy link

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

@SimonSapin
Copy link
Contributor

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
Copy link

FYI, rust-lang/rust#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().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants