Retire VortexExpect

[AdamGS]

Summary

This PR builds on top of #6410.

This PR removes the VortexExpect trait, the way its used right now isn't very useful and adds visual noise in multiple places:

1. Because it ends up in a panic, we print out two backtraces, making it harder to parse where the issue is. The backtraces also end up with multiple layers of Vortex-specific content that just tracks the error's path through vortex-error.
2. vortex_expect (or create-specific expect functions) is just more verbose and not a common pattern in the ecosystem
3. It requires multiple variants of VortexError which are rarely used (and I would argue should never be used), some codepaths get around that by calling .ok() on the original error, which brings us back to the first point here.

API Changes

Removes the VortexExpect trait and a few VortexError variants (like TryFromInt).

Testing

[codspeed-hq]

Merging this PR will not alter performance

✅ 383 untouched benchmarks
⏩ 2020 skipped benchmarks¹

---

_{Comparing adamg/remove-vortex-expect (6811eb5) with develop (74c0bcf)}

Footnotes

1. 2020 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[connortsui20]

is the windows test failure expected here?

[AdamGS]

@connortsui20 the windows failure is from #6410, I think its now fixed there

[connortsui20]

This looks good to me. I was always wondering why we do this, but if @robert3005 has a reason then we can reconsider.

[a10y]

Yea, vortex_expect does not seem to do anything useful here. I thought the whole point was to force capturing a backtrace even if RUST_BACKTRACE isn't set, but it doesn't even seem to do that

[AdamGS]

as far as I can tell, it never did that. There is an API to force a backtrace capture even RUST_BACKTRACE=0, but I don't think we ever used it.

[robert3005]

I spent whole day thinking about this. We can remove VortexExpect BUT we cannot allow expect. It's not about force capturing a backtrace is to make sure that you cannot unwrap/expect errors without backtraces. If you would to allow nightly you could implement VortexExpect so it doesn't double capture but without it I don't have a solution. I spent a while playing on rust playground and the thing that's bad is something like https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=560e73d67f42262bbec6fb609f39b517

[robert3005]

This shouldn't sneak by here, it's orthogonal to the pr

[AdamGS]

it doesn't allow calling expect in functions that return a result

[robert3005]

Per my other comment, this should stay

[AdamGS]

the playground link you shared isn't a permalink, so no idea what you're looking at 😅

[AdamGS]

also removing VortexExpect and not allowing expect doesn't achieve much. If anything, it makes things more verbose.

[robert3005]

sorry I updated the link

[AdamGS]

Ok I see what you're saying, if that's a no go - this PR isn't relevant. I don't think there are many use cases of that, but if we don't trust people to use expect as assertions, the current design works IMO.

[robert3005]

We have added this after a painful debugging session where the source of the error was obscured because of expect in bad place. FWIW you can implement this properly if you had error_generic_member_access nightly feature. But right now it's impossible to know if error has a backtrace or not

[a10y]

Sorry, just to be concrete: the problem is that we don't capture the backtrace at the site where the TryFromIntError originates, so when we expect() on it we get an incomplete Backtrace?

But VortexExpect enforces this b/c it only allows us to expect on errors that are VortexError-shaped

If that's the case, I think there's a bit of a hole, which is that we currently let you .vortex_expect() on anything that is Into<VortexError>. TryFromIntError is in fact Into<VortexError>, which means that if you'd .vortex_expect() on it you'd get the same backtrace as you do here i think?

I guess in practice, this is generally not a problem b/c our functions always return VortexResult which means the backtrace should be captured immediately, but if we're really concerned about this then we should probably remove the blanket impl of `VortexExpect

[a10y]

Could we remove it for Option unwrapping? There are 487 usages of that (mostly in tests) and I don't think it helps you at all

[AdamGS]

mappings those to an error just to unwrap it seems really weird to me, and would actually provide a worse backtrace

[robert3005]

Basically I’m fine removing vortexexpect but also keeping deny for expect_used. Yes the problem is that you can use expect on an arbitrary error and that will not provide you with a backtrace.

[a10y]

It feels like what we actually want is to prevent propagating non-VortexError errors across function call boundaries? Like, if I do u32::try_from(x).vortex_expect("...") i get no valuable information than if i had just done .expect().

But if I return a Result<(), TryFromIntError> and then try and unwrap that 10 stack frames up, I lose a lot of useful information.

Can we accomplish this by adding std::result::Result as a disallowed type in clippy, forcing you to always return VortexResult ?

This would mean that you either propagate VortexError (which has backtrace), or you have some other error type and are immediately unwrapping it at its return site.

[robert3005]

I think forbidding std::result::Result could be a good solution

[a10y]

spent 5 minutes trying to do this ^ on #6575, i hit a snag with serde derived impls but otherwise it's not too bad