Nobody Panic (please).

[michaelkirk]

There are different ideas about when it's considered acceptable to panic. A lot of that depends on the life cycle of the software. For example, I use todo! all the time while I'm building out a feature in a branch, or I'll lob an overly cocky unwrap into main for some of my personal / prototype projects.

I feel that georust/geo has moved well beyond "prototype" status at this point, but I realized that we never actually talked about what our expectations are around panic'ing, so as a first step, I want to share my own thoughts in hopes of building a shared understanding.

tldr; Outside of documented #Panic usage, geo code should never panic, and we should not accept a PR that panics.

To clarify a couple things:

• using unwrap (or preferably expect) in cases which truly can never happen can simplify code — this is a good usage of panic.
• assert! and debug_assert! are great tools and we should use them (in line with the rest of this rant).
• When (not if) one of these "should not happens" does in fact happen, well, darn. Mistakes do happen, and that's not what I'm principally trying to talk about right now.

What I'm talking about is the expectations of completion for contribution.

For example, let's say I open a PR to merge in my new feature:

/// Add a topping to your pizza
///
/// # Panics
/// Will panic if given a pizza with no "dough"
fn add_pizza_topping(arg: &'static str, pizza: &mut Vec<&'static str>) {
  if !pizza.contains("dough") {
    // case 1: this panic condition was documented
    panic!("can only add toppings once you have dough")
  }

  if arg == "sauce" {
    // case 2: yay happy path, we handle this 😎
    pizza.push(arg)
  } else if arg == "pineapple" {
    // case 3:  less common, but a thing that can and will happen
    todo!("I (or someone else) will hopefully finish this 'real soon'.")
  } else if arg == "banana curry" {
    // case 4: super unlikely edge case (not actually `unreachable!` 🇸🇪)
    unreachable!("I don't have the time to build out this crazy edge case handling, and it should be super rare in practice.")
  }

  // case 5: programmer has verified that this `expect` *can not* panic. 😎
  let dough_index = pizza.first("dough).expect("already verified that pizza has dough - see case 1");
  handle_pizza_dough(dough_index, pizza)
}

Firstly, I know the example is dumb. I'm sorry. But I think the "cases" are emblematic.

My own opinion is that only documented panics (case 1) and truly unreachable code (case 5) should use panic.

Case 3 should not be considered for merging. It's a todo! that belongs only in a development branch, not a PR that is "ready for review".

Case 4 is almost surely not OK. I might personally consider it reasonable depending on the exact circumstance, but probably not. In any case it would need to be called out and documented to the user under a # Panics heading.

No other panics should be considered for merging - please do not open pull requests that panic without explanation of the panic.

The reason I'm saying this is because I end up doing a lot of code reviews for this library, and I don't know where the other person is coming from. Especially because a lot of people only contribute once or twice, so I never get a chance to learn their habits. But, to be fair, we have never set explicit expectations about this before.

I know that sometimes people open PR's just to get some feedback before they complete their programming work. That's also ok! I'm not trying to be an overly pedantic process butthead. But recognize that it's impossible to distinguish the case of "I'll fix this before I merge" from when someone is trying to merge "broken" code unless they say so.

For example I once merged a case 3 thinking it was a case 5 precisely because the code said "this does not happen." and the submitter didn't call it out. So I figured, great, "this does not happen". Well it did happen and it crashed for people, and I'm still salty about it.

I'm not sad because the code had a bug (hey, it happens!), I'm sad because I think the person actually knew it was a case 3. I don't think they were trying to sneak it by me or anything, they just had a different idea about what the quality expectation was for the library. They helped move a feature along that worked 80% of the way and crashed on the yet to be handled case. They moved the ball down the field, and I'm grateful for that.

If I had understood that at the time, I would have left the PR open, or finished it myself rather than merging code which knowingly crashed our downstream users code. Crashing makes downstream users not trust geo, and makes me not comfortable recommending that people use it.

I realize that some of this concern can feel really over blown if you are only ever using georust for prototypes or a one off command line analysis or something, but georust is also being used (by me and others!) in remote production deployments like mobile apps, which can't be trivially patched. If a bug is discovered after deployment, I can fix my source code, but I have to go through a build, upload, and approval process and then wait for all my clients to update whenever they get around to it. In the meanwhile, my software is just broken for them.

And also, I want to be able to be proud of this thing that we've all worked on, and that's easier to do when we're on the same page about what the expected limitations are.

I'd just like to reiterate one last time: I'm not ranting about errors in geo. I actually I think we're generally doing a pretty good job on that front. I just want to see if we're mostly already on the same page, or willing to be on the same page, about what our expectations are for code coming into geo/geo-types, and I guess now geo-traits in terms of quality / completion for code that we accept into the project.

Sorry for the long winded essay.

[urschrei]

I didn't see this at the time. Sorry! I agree with this completely. But apart from enforcing this among regular committers in a friendly way, how should we communicate this expectation to new / occasional contributors?

I already mentioned activating the todo lint for Clippy, which would catch some oversights, but that's not a form of communication – it's a safety net if and when our communication strategy fails (which is to be expected at least some of the time). Similarly, we could embark on a long, horrible journey of converting all our extant unwrap calls into expect calls, and then forbid unwrap (could we do this on a module-by-module level? Maybe…) too. Again, this is not an ideal way to communicate our intent here. So do we try to write a short style guide and link to it from the PR template or use something like https://github.com/marketplace/actions/first-contribution to show it to them? Would that help?

[michaelkirk]

I already mentioned activating the todo lint for Clippy

👍

Similarly, we could embark on a long, horrible journey of converting all our extant unwrap calls into expect calls, and then forbid unwrap (could we do this on a module-by-module level? Maybe…) too.

I wouldn't stop someone from making progress on this, but probably would not personally devote much time to it. To some extent, existing panics have been fuzz tested to an extent and the worse ones dealt with. I'm most interested on setting some shared minimum expectations for contributions going forward.

So do try to write a short style guide and link to it from the PR template or use something like https://github.com/marketplace/actions/first-contribution to show it to them? Would that help?

That sounds interesting.

On the "cheap and cheerful" side of the spectrum, we could add a dumb check-box to PR template:

- [ ] This code cannot panic, or if it does, I've explained and documented the conditions.

[mthh]

But apart from enforcing this among regular committers in a friendly way, how should we communicate this expectation to new / occasional contributors?

Maybe the CONTRIBUTING.md file can be an appropriate place to indicate such general guidelines ? (some projects, such as geopandas, have a lengthy section in their documentation on contribution and the rules to be followed: https://geopandas.org/en/stable/community/contributing.html, but as long as geo doesn't have a Web page of this type, the CONTRIBUTING.md file could be a good start?).
As an occasional contributor I'll always take a look at this file and I'd be happy to find these kinds of guidelines in it.

Also, maybe the georust/geo README (in its Contributing section) should link more clearly to this file if it contains useful code style information ?

This doesn't prevent from having a github action like the one you propose as a reminder for first time contributors (but I think that learning it as soon as you read the CONTRIBUTING.md file might be a good idea!)

[urschrei]

On the "cheap and cheerful" side of the spectrum, we could add a dumb check-box to PR template:

- [ ] This code cannot panic, or if it does, I've explained and documented the conditions.

I'm not wedded to using the action, but I know that the pr check boxes are often blindly ticked, whereas a comment in-thread might elicit more attention. Which is not to say we shouldn't do it.

[lnicola]

To add a bit of nuance, since I was against this in #1233 (comment): I'm wary of permanently adding an error variant saying "this failed because of an invalid geometry (but not all invalid inputs will fail), an implementation limitation or bug, or maybe because of catastrophic loss of precision, so try with f64 instead". Any code that uses assert! might panic in case of a bug (or wrong assumptions in the code), and I don't think this means that assert! should not be used, that every function indirectly using it should be documented as possibly panicking, or should be made fallible.

Of course, for long-standing bugs that users keep hitting, either documenting the panic, or returning Result is better than doing nothing.

[michaelkirk]

As far as I know, no one was advocating documenting every function that uses an assert.

So, in case I wasn't clear, I think assert!/debug_assert! are great ways to document that certain states are not reachable. (And build evidence over time that it's true). If you are triggering an assert, then there is a bug - either in the assert or in the code leading up to it.

Of course none of this applies to programmer errors, but I don't really see how we can write rules about that. As you say, there's no sense in having a disclaimer on every single piece of code that effectively says "If this code has a bug, it might do something crazy". I think that's implied.

Of course, for long-standing bugs that users keep hitting, either documenting the panic, or returning Result is better than doing nothing.

👍

[lnicola]

Right, you said they're great, but how does this apply to the booleanops panics, which IIRC were caused by a debug_assert! and an expect? How would you expose a perma-buggy or incomplete implementation?

As prior art, PostGIS raises an error when GEOS doesn't like a geometry, which is pretty annoying TBF.