refactor: new error system

[alejandro-vaz]

this PR remakes error handling to be trait-based instead of relying on fn infallible

closes #622

[alejandro-vaz]

this diverges from Vec though I think

[alejandro-vaz]

I'm not sure I fully like this, I'll go back to working on it

[alejandro-vaz]

now it is cleaner

[bolshoytoster]

Is this worth the complication? As far as I can tell there are no functions that could benefit from a more precise error signature.

[alejandro-vaz]

it kills infallible, which already wasn't being inlined properly, and is more correct in terms of naming and conventions

honestly my stance is that something better is worth merging

and the trait machinery makes the internal API better

that's about it in terms of how much can I defend this

originally it was about making function signatures more precise by letting both kinds of errors exist independently, but it isn't even used anywhere

[bolshoytoster]

it kills infallible, which already wasn't being inlined properly

Do we want error handling code to be inlined? Allocation errors should always be the cold path, and we don't want to duplicate this code that will likely never be called.

and is more correct in terms of naming and conventions

Yeah, I was confused by the name when I first came to the repo. Do we need it to be a trait? Couldn't it just be a method on the error enum?

[alejandro-vaz]

it really can't be a method on the enum because we are implementing it on Result<Type, SmallVecError>, it has to be a trait because of the orphan rule

I think we want error handling to be inlined because it is not just error handling, it is matching the Result variants and if it is not Ok it then matches error variants

if it weren't inlined, there would be an additional jump always

but we could have something like "if it's okay then give the value, if not call this cold never inlined function which matches the error variant (a method)"

I think that's cleaner indeed

[bolshoytoster]

it really can't be a method on the enum because we are implementing it on Result<Type, SmallVecError>, it has to be a trait because of the orphan rule

Oh, I didn't see that. So it's either a trait or use something like .unwrap_or_else(handle).

[alejandro-vaz]

yeah but unwrap_or_else(handle) wouldn't work because the handle function needs access to the error, and it will cause MSRV conflicts with reachability, because it thinks that Infallible really exists and is returned

[bolshoytoster]

yeah but unwrap_or_else(handle) wouldn't work because the handle function needs access to the error

unwrap_or_else passes the error to the function

and it will cause MSRV conflicts with reachability, because it thinks that Infallible really exists and is returned

Does MSRV not allow arbitrary return types on functions that never return? Something like

fn hande<T>(e: CollectionAllocErr) -> T {
    panic!() or whatever
}

[alejandro-vaz]

your way is better

[bolshoytoster]

How come reserve wasn't just

pub fn reserve(&mut self, additional: usize) {
    infallibe(self.try_reserve(additional));
}

?

Same with {,try_}reserve_exact

That's probably out of scope for this PR though.

[alejandro-vaz]

I do want #644 merged soon because the nightly check takes ages to complete

[alejandro-vaz]

how the hell has it been going for 20 minutes

[jdm]

It appears that the final changes in this PR are:

• rename the error type
• change one variant away from an struct variant
• replace uses of the infallible function with unwrap_or_else

This ultimately feels more like idiomatic Rust, so I'm fine with that.