Remove panic! instances

[urschrei]

• [x ] I agree to follow the project's code of conduct.
• I added an entry to rstar/CHANGELOG.md if knowledge of this change could be valuable to users.

---

While tidying the doc links etc earlier, I came across a couple of instances of panic!. While these are clearly intended to cover unlikely corner cases, this is…not good? I certainly didn't realise that we potentially had to catch panics when e.g. calculating euclidean distances in geo. So far, I haven't even defined an actual error type yet, but am obviously happy to do so if people feel that it's warranted.

[michaelkirk]

I get the instinctual reaction that "crashing our client's app is bad", but I personally don't think that all errors should be translated to run-time errors.

In particular, I think that it doesn't make sense to capture logic-errors in this crate as run-time errors to the user, since there's really nothing the downstream user can do with them.

[adamreichold]

In particular, I think that it doesn't make sense to capture logic-errors in this crate as run-time errors to the user, since there's really nothing the downstream user can do with them.

I'd like to second that. Result is best used when there are actual runtime conditions that make a given code path fail some of the time but not of all it. If a code path fails unconditionally because it has a logic error a panic and a bug fix are a sensible reaction IMHO. "Handling" this as a runtime code is limited to logging in any case.

(Things might look different in high assurance environments when a safe system state needs to be ensured even in the presence of logic errors. But even there, this might happen by catching panics top-level and resetting the system.)

Looking at the diff here, the original panic message

panic!("Unexpected reinsert. This is a bug in rstar.")

does suggest that there are indeed no runtime conditions that make this valid and hence a panic seems appropriate.

[adamreichold]

https://rust-lang.github.io/rust-clippy/master/index.html#try_err

[adamreichold]

(Actually, I think running cargo fmt --all -- --check and cargo clippy --all -- --deny warnings as part of the CI would be nice.)

[Stoeoef]

To give a bit more background about why this panic should never be hit: The algorithm will from time to time (when a node overflows) attempt to reinsert (remove and insert again) a point to improve the tree's internal structure. This is what InsertionResult::Reinsert(_,_) means - it tells the top level function (the beginning of the recursion) that some point should be reinserted.
Since inserting that point can lead to another reinsertion (and possibly even an endless loop), reinsertion is only attempted once. That's where forced_insertion differs - it will never allow re-inserting an element.

I think the best way to get rid of the panic would be to modify the return type of forced_insert to a new ForcedInsertionResult enum which doesn't contain a reinsert entry. Then this branch would not appear in the first place.

[adamreichold]

I think this line also good to compare to the panic invocation below. This indexing into node.children could panic and would do so for the same reason the explicitly visible panic would be triggered, i.e. because the internal function choose_subtree gave an incorrect result.

[urschrei]

Thanks both, I think you're absolutely correct w/r/t "if this panics it's a bug, and we should figure out what's gone wrong".

Do you feel that there's any need to note that a panic can occur (though I would prefer to understand the conditions that could produce this first and document those)

Regarding formatting / clippy in CI, we've discussed it for other georust repos before: I'd prefer to warn rather than fail, and I don't think we came up with a good way to do that, but am very happy to investigate suggestions.

Finally, I have no idea why this was a PR and not an issue. Sorry!

[adamreichold]

Do you feel that there's any need to note that a panic can occur (though I would prefer to understand the conditions that could produce this first and document those)

If a panic is part of the contract of an API than this should be noted in a # Panics section in the API documentation, e.g. indexing into a slice will panic if the index is out of bounds. In this particular case, I fear the contract would be a rather unhelpful "will panic if there is in an internal inconsistency". Meaning I would only document it if there is well-defined way for the caller of API to trigger this and we do not consider this a bug in itself.

I would also like to repeat pointing out the slice indexing two lines above the panic invocation: Just because there are two explicit panic! calls in the code does not mean these are the only places were it could panic. So if we really want panic safety, we would need to audit the whole code base thoroughly from e.g. any standard library code we call that could panic. And document where we call user code that could, e.g. Point::nth usually has a panicking branch if the index is out of bounds too which could happen if Point::DIMENSIONS is inconsistent.

Regarding formatting / clippy in CI, we've discussed it for other georust repos before: I'd prefer to warn rather than fail, and I don't think we came up with a good way to do that, but am very happy to investigate suggestions.

Personally, warnings are what I want when working locally. I think errors are fine in the CI as I would not want to merge PR with faulty formatting or unhandled warnings, even if it means suppressing some. And one can always wrangle commits manually if it is an emergency of some sort.