Panic vs Result/Option

I read "for x in 2i32.." as meaning "for signed integer values of x from 2 to whatever the maximum possible positive value of a 32 bit integer is"

Well, in reality it will panic on overflow with overflow checks enabled, and is unbounded with overflow checks disabled. So, practically, the final line of the function is unreachable.

Looks like your function f() should return an Error or perhaps an Option so as to ensure the caller is forced to check the return value properly.

It should not return an error or option if the developer believes the function will always return a value. If the code is returning unexpected values, that's a code bug, and not one that can be mitigated by merely passing the buck to the calling function. The result of None is wholly unexpected, and would therefore not likely to be properly handled anymore-so than 0.

1 Like

The best option is to just use unreachable!(), in my opinion.

2 Likes

I don't fathom that. It cannot be unbounded with overflow checks disabled. There is only a finite number of 32 bit positive integer values.

If there are no overflow checks then likely the x wraps around. As hardware does. In which case "some_complicated_check(x)" then gets an inputs of 0 an 1. Which presumably it is not intentended to be expecting and all kind of hell breaks lose.

That is the C programmers view of the world. We get valid return values or we might get special error or invalid values. We the programmers had better be sure to check our documentation carefully in every case to be sure we check return values correctly.

Another view is that we should encode what is invalid or error into our source code, with Option and/or Error so that the caller has to write the right thing. So that future readers of the code don't have to check every damn thing before they understand what is going on as they try tho debug the mess.

No it is not. The function as written so far encodes "none" as a zero. Presumably that is expected some times. Else why do that?

At least if an Option were returned the caller would have to check it's type to pull out the valid responses. The semantics of Option would make that clear at compile time whereas a special zero values does not.

Sorry, by unbounded, I had meant that it would wrap. I did use the term incorrectly.

unreachable!() will panic, which will result in the bug being fixed sooner/more easily than hoping the developer "correctly" handles your code bug that you never fathomed would occur.

It is not expected - the developer told us so, and we know that the line can never be reached

Where did the developer tell us any such thing?

Not in the code. Perhaps in some document or comment. In my experience the later are often erroneous, incomplete or ambiguous. Especially when you find yourself bug hunting. The code is the only reality and as much as you can say in there the better.

But we know that line can be reached. There are not an infinite number of positive integers. It looks like it could well reach the end of the road and run off the end of the function.

Or, it wraps. In which case some_complicated_check() gets an input of 0 and or 1. Which we presume it is not expecting that, else why start the loop at 2?

Now we have to check the docs and/or code to find out what some_complicated_check() will do with those unexpected inputs.

It's a mess.

Maybe. I'd have to see the actual code. The code could probably use some additional assertions. It's a fact at the very least that the return statement in this function is unreachable.

Returning None in the face of unexpected cases rather than panicking increases the complexity of the code by propagating the uncertainty to the caller rather than containing your invariant checks within the routines themselves. Returning None doesn't resolve the inability to reason about the code - it instead spreads the uncertainty.

Looking at that function f() as presented, it seems very ambiguous to me. Without looking into some_complicated_check() I have no idea when it will produce a 'true' result or if it ever will.

So that implies x will overflow and panic. Or x will wrap an end up feeding 0 and/or 1 into some_complicated_check(), which it looks like it is not intended to do, or x runs off the scale and the loop exists. The code itself does not tell me the intention here. Which I think source code should as much as possible.

Now, I'm with you on the semantics of "None". It all depends on what ones intention is. This code is clearly searching a set of values for something it is looking for. The question then is: Is it normal that the something is not found? Or is it expected that there is always that something to be found?

Often not finding a thing is not a catastrophic error worthy of a panic like a divide by zero or running out or memory. For example trying to open a file that does not exist because a user gave is the wrong file name. In which case "None" or whatever is appropriate and the code up the chain should handle it.

Or not finding the thing is a catastrophic error that we cannot recover from and a panic is the only reasonable thing to do.

Either way, the code snippet as presented does not give the reader any clue as to which of those is intended.

So, return an Error or Option. Or put some assertions in there. Give the reader a chance to understand what you are intending.

This doesn't hold of the code lives in a library.
One should never call panic (or anything to that effect) in a library, because it takes away any action a library consumer might be able to do when things do go wrong. I've had this exact problem with a 3rd party library so it is a real world example, too.

I think it's perfectly reasonable to panic if something happens that can only be caused by a bug in the library.

7 Likes

It was indeed a bug in the library that caused incorrect behavior.
However, by panicking, it prevented me from taking any action to work around the bug at all. Instead it just caused crashes, which tanked my productivity at the time. So I have to disagree with you there.

You can catch it with catch_unwind if you run into that kind of issue.

1 Like

From the docs you mentioned:

**It is not recommended to use this function for a general try/catch mechanism.** The Result type is more appropriate to use for functions that can fail on a regular basis. **Additionally, this function is not guaranteed to catch all panics, see the "Notes" section below.**

(emphasis mine)

In any case the bug has long since been solved, but the point is that it never would have would have been a problem for consumers of that library without it haphazardly panicking because the author didn't account for certain use cases.

The fact that the library had a bug wouldn't be a problem for the consumers if the library returned a Result from a function that shouldn't be able to fail? Bugs cause problems for the users regardless of how it signals failure.

5 Likes

Indeed, and that is precisely what I practice myself nowadays too: possible error states are captured in the type system, not "haphazardly" handled with runtime constructs like asserts panics.
If there's no bug the error won't be triggered, and if there is at least there's something a fn/method consumer can do about it, even if it's only detection (after which it's the choice of the library consumer whether or not to panic) and not mitigation per se.

You seem to be suggesting that all functions/methods should return something like: Result<T, Bug> even if they are intended to never fail.

Where Result is always returned and contains a T which is the expected value. Except when there is a bug in the function/method and it contains 'Bug' just to let you know the library is badly written and unreliable.

Of course the library could be so badly written that it fails to return 'Bug' when if fails and it screws up your program by returning an incorrect T. Or panicking or whatever.

Philosophically I don't go with the idea that functions that are intended to never fail and always produce a valid result should return an Error or Option just in case they have a bug. They should just return the expected value. If they don't that is a bug and they should be fixed. Or not used.

4 Likes

No, I'm suggesting that whenever something would lead to you calling panic! in a library, to make that fn/method return a Result instead, which is a very different thing.

I see. Sounds reasonable.

I guess it depends on what the panic is there for.

If I'm writing a function that by design should never fail I still feel that I should not be returning failure indications in Options or Errors.

I might not be confident in the quality of my code an hence stuff it full of assertions and panics that in theory should never happen but in practice might because I have made a wrong assumption or other error.

In which case I posit that it is still better for me to panic. That is a bug, it needs fixing. Not working around with a band-aid error result.

1 Like

Yeah that's where context starts to matter. I work on software that performs transactions, runs in production, and should not crash. Rather it should log the error.

There's plenty of software where asserts or panics are perfectly acceptable, but I can't use any library that does that.
The reverse (i.e. someone not being able to use a library because it uses Results rather than asserts) will never be true for anyone.
And FWIW, error values don't have to give up eg source locations or even stack traces, although I do admit that atm that is added manual labor.

I would recommend using catch_unwind in that situation.

3 Likes

That's only useful for panics that don't abort.
Can you predict in advance which ones that will be? I can't.

And in any case, the policies I work under aren't going to change.