Joshua Warner (Dec 24 2024 at 15:15): Joshua Warner (Dec 24 2024 at 15:15):There's currently a CI job that checks for any increase in the number of panic/unwrap/expect calls that I keep tripping over.
I'm concerned that this CI check is encouraging unhelpful behavior.
In the most recent instance, I added the following line:
let last = fields.pop().expect("must have at least one tuple field");
Some things to note:
fields is parsed in the same place, so we fully control what it contains.if branch of the function, we're guaranteed to have parsed at least one element already. Furthermore, a pop here is always followed by a push immediately after.first element in order to guarantee that we have at least one item, but that would make the code much more complicated (needlessly, IMO).There are a few things I could do at this juncture:
fields[fields.len() - 1] and then pop() - which will have the same effect (including preserving the panic!)internal_error!() macros - e.g. let Some(last) = fields.pop() else { internal_error!(...) }; (or similar)My main concern is that there's so much code out there that's implicitly doing bounds checks + panics on arrays (i.e. effectively that second option), that this whole effort is a boondoggle. Rust doesn't have any good tools to control that, unlike (for example) wuffs. (which I would not want to write a full compiler in!)
I have two concerns with internal_error!():
I would like to propose:
expect() and panic!() (but only if the panic has an explanatory string), in order to force developers to actually explain themselves and make tracking down problems easierinternal_error in the main compiler binary, compile that with panic=abort and install a global panic handler that prints the same message and exits.internal_error!() calls and replace them with panic!()sThoughts?
Brendan Hansknecht (Dec 24 2024 at 19:44):Does a global panic handle work with panic=abort?
Brendan Hansknecht (Dec 24 2024 at 19:45):Also, I don't understand why internal_error! is any worse than panic! for testing. Doesn't it just call panic! under the hood? If not, couldn't it?
Joshua Warner (Dec 24 2024 at 19:46):Does a global panic handle work with
panic=abort?
Yes, this works fine (we ship this way at work)
Brendan Hansknecht (Dec 24 2024 at 19:46):I do think there are a lot of errors that should be using user_error! and eventually have nice error messages wired through the compiler as a whole.
Brendan Hansknecht (Dec 24 2024 at 19:48):I think it is only a bit more verbose than expect. There is .unwrap_or(|| internal_error!(...)) instead of .expect(...)
Joshua Warner (Dec 24 2024 at 19:50):Doesn't it just call
panic!under the hood? If not, couldn't it?
Yes and no. It used to always be an explicit call to exit(). Now if you call set_panic_not_exit(true); it'll panic instead of exiting. Which is... fine, except it's really easy to forget to do that in tests - in which case you end up with no stack trace (and no more tests run, and no printed output).
Joshua Warner (Dec 24 2024 at 19:50):There is
.unwrap_or(|| internal_error!(...))
True, that's not terrible on the verbosity front.
Joshua Warner (Dec 24 2024 at 19:51):I still maintain that no code that's reachable from tests should explicitly be calling exit or abort
Joshua Warner (Dec 24 2024 at 19:52):100% agree on eventually converting things to nice error messages when appropriate
Brendan Hansknecht (Dec 24 2024 at 19:52):The original goal here was to try and record more intent. If hit, is it:
Brendan Hansknecht (Dec 24 2024 at 19:53):But the compiler has progressed a lot since it was added. Maybe it is less relevant nowadays
Joshua Warner (Dec 24 2024 at 19:54):Yep, and my intent would be for expect / panic / etc to _only_ be used for (2)
Richard Feldman (Dec 24 2024 at 19:55):a problem is that if we have that as a convention, it's really easy for new contributors to see a classic Rust expect/panic/etc and not know this
Richard Feldman (Dec 24 2024 at 19:57):separately, I've actually been trying out a "do not panic, ever, even for things that seem unreachable; instead, push a Problem onto a vec to be reported later, and recover in whatever graceful way I can find" (e.g. returning some node that will code gen to a crash)
Richard Feldman (Dec 24 2024 at 19:58):and it's a bit more work, but it gives me way more peace of mind that the compiler won't actually crash even if there's a bug I didn't anticipate :big_smile:
Joshua Warner (Dec 24 2024 at 19:58):Hmm, in my experience Rust heavily encourages use of Result for category 1. I don't think rust developers who are new to Roc's codebase will make that particular mistake
Richard Feldman (Dec 24 2024 at 19:58):maybe, but in practice our current code base has been doing that, and we definitely have a problem with end users encountering panics :sweat_smile:
Joshua Warner (Dec 24 2024 at 19:59):The parser & formatter is pretty good about only panicking for case (2), and we're driving that to zero pretty fast, with fuzzing :stuck_out_tongue:
Joshua Warner (Dec 24 2024 at 20:00):it gives me way more peace of mind that the compiler won't actually crash even if there's a bug I didn't anticipate
True, but then it also makes the code more complicated (possibly introducing/hiding bugs!)
Also keep in mind you're not going to get away from things like implicit bounds checks on arrays
Richard Feldman (Dec 24 2024 at 20:00):sure
Joshua Warner (Dec 24 2024 at 20:00):IMO "panic-free" is unrealistic without some fancier language features (e.g. dependent-types, in-language-proofs, etc)
Richard Feldman (Dec 24 2024 at 20:01):well we do have a good bit of like "unwrap or panic" in the soa crate
Richard Feldman (Dec 24 2024 at 20:02):anyway, I think the main thing is to make it work reliably; we can always go back and adjust styles later if desired!
Joshua Warner (Dec 24 2024 at 20:03):I'm not sure being persnickety about introducing new calls to panic/unwrap/expect actually helps with reliability in practice
Joshua Warner (Dec 24 2024 at 20:03):It just squeezes the problem elsewhere, often into an area that will be harder to understand/inspect
Joshua Warner (Dec 24 2024 at 20:04):What would have been a compiler crash will turn into very hard-to-diagnose miscompilations, etc.
Richard Feldman (Dec 24 2024 at 21:17):yeah, I guess given that I'm ok with removing the ci check, but I'd like to hear from others (some of whom won't be around in the next week or so due to holidays!) before we actually make the change
Brendan Hansknecht (Dec 24 2024 at 21:40):Personally, I like the enforcement, otherwise, we should just not have internal_error!. People won't learn to use it unless they get the CI error and we tell them to change it to .unwrap_or(|| internal_error!(...))
Brendan Hansknecht (Dec 24 2024 at 21:43):Also, I think if we change the CI job, we should still ban unwrap.
Joshua Warner (Dec 24 2024 at 23:20):Agreed on continuing to ban unwrap
Joshua Warner (Dec 24 2024 at 23:21):otherwise, we should just not have
internal_error!
I actually think that would be a good change
Joshua Warner (Dec 24 2024 at 23:21):Internal errors are "any panic that's not a user_error"
Brendan Hansknecht (Dec 25 2024 at 01:45):Yeah
Brendan Hansknecht (Dec 25 2024 at 01:47):Internal errors are "any panic that's not a user_error"
Yeah, I think removing internal_error!, banning unwrap, and catching all panics so we can add on a message to file a bug sounds reasonable.
Brendan Hansknecht (Dec 25 2024 at 01:47):Still long term pushing towards less panics and more proper error logging
Brendan Hansknecht (Dec 25 2024 at 01:48):user_error! only comes up like 10 times in the codebase. So clearly we aren't using it to label error that need to be upgraded to proper Problems
Brendan Hansknecht (Dec 25 2024 at 01:49):And if we only have internal errors, there really is no gain over .expect()
Brendan Hansknecht (Dec 25 2024 at 01:49):So I guess I'm for this change
Anton (Dec 27 2024 at 13:27):The main reason why this check was introduced is because panics crash the repl and then all state is lost, which is annoying for the user. If we want to support e.g. data science workflows that use the repl through a notebook (like Jupyter) this potential for loss of state would be a significant drawback.
Ayaz Hafiz (Dec 27 2024 at 16:46):A wrong result or a message that the compiler crashed isn’t much better, I think. In either case we still lose the intermediate state. And they are all bugs that must be resolved.
Anton (Dec 27 2024 at 16:55):Hmm, couldn't we keep our previous state if the step function here never panicked?
Ayaz Hafiz (Dec 27 2024 at 17:08):You’re probably right-but if that’s the case, that problem is much easier to solve right? Can that be wrapped in catch_unwind?
Anton (Dec 27 2024 at 17:12):I hadn't thought of that, that could work, I'll leave some time for others to weigh in
Ayaz Hafiz (Jan 03 2025 at 16:08):patch to catch panics in repl session and print backtrace, request bug report without ending repl session: https://github.com/roc-lang/roc/pull/7459
Ayaz Hafiz (Jan 03 2025 at 16:36):i think we should remove the panic checker with this, since it solves the main issue
Anton (Jan 03 2025 at 16:42):i think we should remove the panic checker with this, since it solves the main issue
Maybe we should alter it, when people just use unwrap the error message is terrible for the user
Anton (Jan 03 2025 at 16:50):But we can remove it and re-introduce another check in the future, the panic checker has been annoying people
Brendan Hansknecht (Jan 03 2025 at 20:03):Yeah, I like the idea of keeping an unwrap checker at a minimum
Brendan Hansknecht (Jan 03 2025 at 20:03):That and eventually removing internal_error! And instead adding the info via the catch unwind
Last updated: Jul 06 2025 at 12:14 UTC