Replace direct panics with Errors #432
Conversation
Even though there are still `.unwrap()`s and `.expects()` left, this change should cover the most common error code paths. If we find a new case of an error that we want to propagate, it is generally straightforward to deprecate the old function and add a new function that the old function calls and `.unwrap()`.
|
I almost forgot to mention; here is how this PR impacts the syntect API compared to master: |
|
To give one example where this PR helps: bat will not panic anymore when encountering sharkdp/bat#915. Instead, an error will be propagated. So users will get instead of (The above example is with bat end-users, but the point is of course that an error is propagated from syntect now. Also note that I ran the test with #427 reverted since otherwise no error occurs) |
|
Another example is broot. |
Update CHANGELOG.md to include the changes made in Replace direct panics with Errors trishume#432
|
@trishume Hello! Just a heads-up that this is close to being in review for a month, so my intention is to merge this a week from now or so. Let me know if you want more time to review, that would be no problem at all. I am comfortable to merge this without review though, because I see it as a natural and straightforward continuation of previous PRs of the same type. If this is merged, #409 can also be merged as is, because it has been prepared to include this PR in the 5.0.0 release notes. And once that is merged, we can release master as 5.0.0 as far as I know. See #409 (comment). Since this is the first release in the 5.x.x series, I don't think much can go wrong this time 🤞 |
|
Yup thanks for the heads up! I've been extra busy lately and indeed may not get to this by then, if not go ahead and merge and don't wait more for me. Sorry! |
Even though there are still
.unwrap()s and.expect()s left, this change should cover the most common error code paths.If we find a new case of an error that we want to propagate, it is generally straightforward to deprecate the old function and add a new function that calls the old function and
.unwrap(). This can be done in a minor release. In many cases this can hopefully be done without API changes since many parts of the API have been changed already.On my low-end desktop, this PR does seem to have a slight (1-2%-ish) impact on performance. But it is hard to make a final judgement, because I get that kind of performance difference between benchmarks of the same commit on master. Either way, I think it is reasonable and expected to pay a slight performance penalty for (checking for) error propagation from hot code paths. But I am open to hearing other opinions.
Here is an example run of
cargo benchwith master as baseline (click to expand):cargo bench -- --baseline master-6b211f9-4
Some words on the details of this PR:
.unwrap().expect("#[cfg(test)]")to make the diff easy to review.ok()?instead of propagating the error, but I feel like propagating the error is too big of a change at this point.ScopeErrorenum due to theparsingfeature and the current structure of the code. It is possible that a new big restructuring of the code could avoid the need for that, but honestly I think we need to try to get 5.0.0 out at this point..any()and.map()I do.unwrap()to not have to refactor the code. That can be fixed in 5.0.1 or 5.1.0 if necessary.panic!()since it seems to be too much of an edge-case to propagate. I added a comment in the code.This PR is for #98.
FWIW, here is a CI run of bat with all regression tests passing when using the code in this PR: Enselic/bat#85