Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove all panics from standard library #3597

Closed
coolCucumber-cat opened this issue Mar 28, 2024 · 25 comments
Closed

Remove all panics from standard library #3597

coolCucumber-cat opened this issue Mar 28, 2024 · 25 comments

Comments

@coolCucumber-cat
Copy link

coolCucumber-cat commented Mar 28, 2024

I think the use of all panics in the standard library should be removed, with the exception of panic specific things like unwrap, expect and unimplemented.

Panics have all the same problems as exceptions, the only difference is that they are uncatchable. They change the control flow of a program in an unexpected way and are completely hidden unless you know yourself where they are, like documentation. I find them very un-Rust-like in the sense that they are implicit and you can never be sure exactly how your program works. It gives the same feeling as having to watch out for exceptions and null pointers in other languages where you are always on the edge of your seat and have to keep all these possible footguns in mind.

The example I dislike the most is division by 0 and overflows, which panics even in production. This is the kind of thing that Rust was supposed to avoid. There is not a mention of it unless you look at the source code. Despite being documented, said documentation can only be found by looking at the source code. Panicing should not be so widespread and hidden, especially not in the standard library.

Other operations like println! and adding ints fail silently, just expand that to all arithmetic (divison by 0 can return 0). Everything else can use Result and Option. If it does need to panic, then just use unwrap.

This is will break things, so I suggest adding it to the 2024 edition.

Edit: exception is for when it is impossible for the program to continue running, like being out of memory.

Edit 2: issue now no longer addresses indexing.

@BurntSushi
Copy link
Member

So to be clear, you're proposing that slice[i] be removed from Rust in the next edition? If so, this issue can be closed because it's a non-starter. It's never going to happen.

Editions aren't a license to do arbitrary breaking changes.

@coolCucumber-cat
Copy link
Author

@BurntSushi

No, it should just return an Option or Result instead. If you want it to panic, you can unwrap.

If I did mean that, then the rest of my proposal can still go through.

And it's also not an arbitrary change.

@coolCucumber-cat
Copy link
Author

@kennytm What's wrong with my suggestion? Feedback is appreciated.

@kennytm
Copy link
Member

kennytm commented Mar 28, 2024

@coolCucumber-cat Edition is only designed for change syntax, once a std library interface is stabilized it is there forever.

That is, since u64::div(u64) returns u64 in Edition 2015, it will always return u64, not Option<u64> nor Result<u64, DivError> in edition 2999 as long as it is still called "Rust 1.8554" not "Rust 2.something".

That is the "arbitrary breaking change" called out by BurntSushi above.

@kennytm
Copy link
Member

kennytm commented Mar 28, 2024

BTW you could force your binary to never able to panic by using https://docs.rs/no-panic/latest/no_panic/ which turns the panic call into a linker error.

@coolCucumber-cat
Copy link
Author

@kennytm Ok, I wasn't aware of not allowing changes. Where does it say that?

I never said arithmetic should change its return type, that's what the checked operations do. I said that it simply shouldn't panic and instead return 0. I think you meant to say that indexing shouldn't its return value, not arithmetic.

And I said it wasn't arbitrary, not that it wasn't a breaking change. I even said that it would break things.

So currently, the only problem is indexing, which I can remove from this RFC.

@coolCucumber-cat
Copy link
Author

coolCucumber-cat commented Mar 28, 2024

@kennytm @BurntSushi I removed indexing from this issue. Are there any other problems that need to be addressed?

Arithmetic not panicing is not a breaking change, but it would cause some programs to behave differently. That is allowed though, right?

@kennytm
Copy link
Member

kennytm commented Mar 28, 2024

@coolCucumber-cat

Ok, I wasn't aware of not allowing changes. Where does it say that?

https://github.com/rust-lang/rfcs/blob/master/text/2052-epochs.md#negative-examples-what-edition-opt-ins-cant-do

I said that it simply shouldn't panic and instead return 0.

For integers x / 0 returning any concrete number is simply wrong (integers don't have NaN or ±Infinity)

If you need to ensure divide-by-zero never happens make your denominator use the NonZero type.

@Diggsey
Copy link
Contributor

Diggsey commented Mar 28, 2024

I find them very un-Rust-like in the sense that they are implicit and you can never be sure exactly how your program works.
I never said arithmetic should change its return type, that's what the checked operations do. I said that it simply shouldn't panic and instead return 0.

Returning zero would be very un-rustlike. Rust prioritizes the following:

Correct program > Panicking program > Incorrect program > Undefined Behaviour

ie. a panicking program is better than an incorrect program, so making arithmetic operations incorrectly return 0 would make things worse.

@coolCucumber-cat
Copy link
Author

In my opinion, returning 0 isn't incorrect. NaN for floating points has similar behaviour, but isn't considered incorrect. Neither is an overflow. I just don't see how panicking is better than a silent failure or what could break by instead returning 0.

You can consider division by 0 being 0 because anything divided by 0 makes infinity, which overflows to make 0.

@coolCucumber-cat
Copy link
Author

Can you give an example where 0 would be worse than panicking?

@Diggsey
Copy link
Contributor

Diggsey commented Mar 28, 2024

You can consider division by 0 being 0 because anything divided by 0 makes infinity, which overflows to make 0.

And in debug builds Rust panics on overflow, because it's an error. It would do so in release to if it weren't for the performance cost of checking it.

Can you give an example where 0 would be worse than panicking?

In any situation where you actually want a correct answer? If you end up dividing by zero it means something has gone wrong earlier in the program. It's better to panic than allow the program to continue and potientially do something completely unexpected.

@kennytm
Copy link
Member

kennytm commented Mar 28, 2024

The only cases I know that x / 0 == 0 are

  1. The Pony language, which actually have 3 division operators (x / 0 == 0, x /? 0 == error, x /~ 0 == undefined behavior). However x / 0.0 == ±inf.
  2. Provers like Lean and Coq because they require the function to be total and divide-by-zero shouldn't happen in the domain the proof should be working on (they also sort-of defined 100 - 250 == 0 for unsigned numbers).

@coolCucumber-cat
Copy link
Author

I agree with what the Pony language has done: "So, in order to maintain a practical API and avoid undefined behaviour, normal division on integers in Pony is defined to be 0.".

When you divide by 0 with a floating point number, it produces NaN, which is not correct either. So if that's allowed then 0 can just be the equivalent of NaN but for ints.

In dev mode I think it should panic, as it does now, but in production I don't think it should because it doesn't even help you catch the error. Whoever is running your program will just get a random error that probably doesn't matter.

@kennytm
Copy link
Member

kennytm commented Mar 28, 2024

When you divide by 0 with a floating point number, it produces NaN, which is not correct either.

No you get +Infinity if x > 0, -Infinity if x < 0 and NaN otherwise.

This is correct in the context of float as in conforming to IEEE 754.

So if that's allowed then 0 can just be the equivalent of NaN but for ints.

Except that NaN is not 0 (nor ±∞) in float


In any case redefining x / 0 to return 0 today is a breaking change of runtime behavior according to #1105 since the contract to panic is documented already.

@coolCucumber-cat
Copy link
Author

When did I say that NaN is 0? I said we can use 0 as the int equivalent of NaN. I forgot about infinity, I really meant anything that isn't a real number (NaN, ±Infinity).

I know that it is correct according to IEE754, but if it's correct to have an unexpected value that isn't technically correct like Infinity (which is not a real number), then surely 0 can be allowed when dividing by 0.

What's the definition of breaking change here? Because nothing breaks, just that the behaviour changes.

@ChayimFriedman2
Copy link

Silent changes in behavior are exactly breaking changes.

@mohe2015
Copy link

When did I say that NaN is 0? I said we can use 0 as the int equivalent of NaN. I forgot about infinity, I really meant anything that isn't a real number (NaN, ±Infinity).

I know that it is correct according to IEE754, but if it's correct to have an unexpected value that isn't technically correct like Infinity (which is not a real number), then surely 0 can be allowed when dividing by 0.

What's the definition of breaking change here? Because nothing breaks, just that the behaviour changes.

The issue with that would be that 0 would then mean 0 or NaN or +-Infinity without being able to differentiate between them. For floating point numbers you can differentiate between them which is a major difference.

@senekor
Copy link

senekor commented Mar 28, 2024

@coolCucumber-cat maybe this clippy lint serves your needs: arithmetic_side_effects.

@coolCucumber-cat
Copy link
Author

@senekor is there a lint just for side effects that also panic? it's a helpful lint to avoid divisions by 0 but annoying for side effects that only overflow

@coolCucumber-cat
Copy link
Author

@ChayimFriedman2 There's clearly a misunderstanding here. I wouldn't consider a change like that to be breaking, what is a breaking change then? I can't find anything about it in the docs.

@senekor
Copy link

senekor commented Apr 4, 2024

is there a lint just for side effects that also panic?

I don't know, but the site I linked to has a search feature for all available clippy lints.

I wouldn't consider a change like that to be breaking, what is a breaking change then? I can't find anything about it in the docs.

I'm not aware of a Rust-specific documentation about backwards compatibility. To me it seems so obvious it doesn't have to be documented. If the thing doesn't do anymore what its documentation said it does, that's a breaking change. I would consider a silent breaking change like that to be much worse than a change in the API which the compiler will at least give me good messages about. A broken build is annoying, broken production is a catastrophe.

@coolCucumber-cat
Copy link
Author

@senekor So you mean anytime that a change would make the current documentation incorrect? Yeah sure I guess that seems reasonable.

@CraftSpider
Copy link

CraftSpider commented Apr 4, 2024

Rust backward-compatibility does in fact have fairly good documentation out there, though it's not trivial to find. This kind of situation is exactly why it's important to have policies - relying on the meaning of a thing being 'obvious' opens yourself up to different interpretations, as well as semantic drift.

Another note is simply - in its most strict form, a 'Breaking Change' is anything that turns working code into not-working code, for some abstract code that relies on the current behavior. This definition is why Rust distinguishes 'major' and 'minor' changes - just about any change could break some code, so we specify certain things that, while technically 'breaking', we have declared within our rights to change anyways. This change specifically is 'major' because it hits all of the following (not all major changes are all of the following, and this list isn't all the reasons a change could be major):

  • It changes user-visible behavior
  • It changes behavior that has been guaranteed by documentation
  • It would render many currently correct programs potentially incorrect, not just a tiny number of them

To quote RFC 1105, the API Evolution RFC, "But in some sense it is even more pernicious to make a change that allows downstream code to continue compiling, but causes its runtime behavior to break."

RFCs describing Rust's policy:
https://rust-lang.github.io/rfcs/1105-api-evolution.html
https://rust-lang.github.io/rfcs/1122-language-semver.html

Documentation of what constitutes a 'major' vs 'minor' change for external crates:
https://doc.rust-lang.org/cargo/reference/semver.html

@coolCucumber-cat
Copy link
Author

@CraftSpider Thanks, this would have been nice to know before. I wish it was easier to find though. I for sure agree it's a good idea to have policies, but it defeats the point of them to be hidden. Do you have any tips to make it easier to find things like this? Because I constantly cannot find things like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants