Expect cleanup

[peter-shen-dev]

Remove a few expects.

• One of them could be removed entirely by refactoring two Some that assumed they were always Some/None together. It should've been impossible for it to happen, and now it is.
• I left the expect in history_get because similarly, if it fails, it's an internal bug. Not that those bugs won't happen, but I think we should probably use something like human_panic or another panic handler/catch (maybe one that lets us continue running). std is full of panics, and they're gonna happen, so it'd be more robust to take that route.

[lthoerner]

I really like your idea about using human_panic. I'd love to see potentially a small overhaul to error handling that uses the panic handler to deal with all internal bugs, rather than just those with a remaining .expect() or .unwrap() call. What do you think?

[peter-shen-dev]

Yeah, I agree. I'll add human_panic as part of another PR. It's not really an overhaul - just adding a dependency and a single line :P

There is the alternative of catching the panic and trying to continue execution - human_panic does still crash - but that's probably a bad idea, as it may leave us in an inconsistent state and lead to cascading errors or vulnerabilities.

[lthoerner]

So is the panic handler the same for all panics? Like would the message be the same?

[peter-shen-dev]

There's only one panic handler, but you get panic info, e.g.

#[panic_handler]
fn panic(info: &PanicInfo) -> ! {
    println!("{}", info);
    exit(1);
}

So you can't say "I'm only going to handle panics from except" - they're all just panics either way. But when you display the panic, you get specific info, e.g. a backtrace.

human-panic has example output.

[lthoerner]

Can you change the conversion method you wrote for BuiltinError to a From<io::Error> implementation?

[peter-shen-dev]

BuiltinError::read_file? The pattern I'm trying to going for here is one where there's a unique conversion per use-case so we can have nicer errors. Rust only exposes generic errors like PermissionDenied/NotFound/etc, but with the additional context of read_file, we can say "no read permissions" or "file not found". Other possible conversions might be: write_file, open_dir, etc. But if we make a single From<io::Error> implementation, it implies that it's the canonical conversion for all use cases.

[lthoerner]

I might be dumb but does this PR still need to be merged? I am confused on whether this just turned into a discussion or if it actually is still a PR.

[peter-shen-dev]

Of the two changes, one fixes a bug where cat / panics, so I'd say it should still be merged. The other is effectively a refactoring. I get the discussion-y vibes so it's a pretty sensible thing to be confused about tho.

[lthoerner]

I'll take a look at this later today.

[lthoerner]

BuiltinError::read_file? The pattern I'm trying to going for here is one where there's a unique conversion per use-case so we can have nicer errors. Rust only exposes generic errors like PermissionDenied/NotFound/etc, but with the additional context of read_file, we can say "no read permissions" or "file not found". Other possible conversions might be: write_file, open_dir, etc. But if we make a single From<io::Error> implementation, it implies that it's the canonical conversion for all use cases.

Looking back at this, was your point essentially that these error types have different meanings in-context, but that std::io does not provide said context, so we must have different mappings for different contexts (which is not possible to do with a single From impl)?

[peter-shen-dev]

Yup, exactly.

[lthoerner]

Yup, exactly.

In that case, can you make the method names clearer? For instance, read_file could be something more along the lines of convert_when_read_file. Or - and I actually like this idea slightly better - you could make an enum called FileOperationContext or something similar, and a general-purpose method called convert_with_context.

[peter-shen-dev]

Not quite sure it's what you had in mind, but it's what I found to work best, though it's a bit at odds with having all custom errors for everything - that would require more verbosity and some trait hackery. I think this should be fine as far as users getting the right information goes.

Also, I went with x_context instead of convert_with_x_context following anyhow's .context() convention - also with_context implies that it takes a function imo.

[lthoerner]

I like the direction you're going with this, but I don't think the implementation fits in with the existing codebase. This is more of a reflection on the oversimplification of error handling in the existing code than it is a critique of your contribution. However, I would like to have a more thorough discussion on the exact ways we are planning to handle errors going forward - and we will change this PR appropriately after that occurs.

P.S. I am aware that my comments may seem a bit pedantic; they were not intended to be so, I just would like to be thorough in my feedback, rather than just saying "fix this" etc.

[lthoerner]

This needs to have a clearer name and a description.

[peter-shen-dev]

Does IoErrorContextExt seem fine? I tried some other stuff like ConvertIoErrorWithContextExt but that seems to be getting a bit too long imo.

[lthoerner]

Do you think this would be better as FileRead or OnFileRead or something? That one's up to you - just wondering what would allow for the most consistency.

[peter-shen-dev]

I think it depends on whether the convention is to qualify it or not - if it's imported with a glob, FileRead would be better, but if it's qualified, FileContext::Reading is better. I think importing with a glob makes sense here, so I'll go with that.

[lthoerner]

If this is not specific to executables or builtins, it may be worth thinking about either:

1. Making a separate error type called FileSystemError
2. Making an upstream error enum (CommandError or something) that has a File variant

[peter-shen-dev]

Another thing I forgot to delete 😅

Right now it's basically just spitting out a string, but those concrete errors types are never used. I agree the work in this PR should probably be lifted somewhere higher, but I'm gonna leave that alone pending further discussion.

[peter-shen-dev]

I like the direction you're going with this, but I don't think the implementation fits in with the existing codebase. This is more of a reflection on the oversimplification of error handling in the existing code than it is a critique of your contribution. However, I would like to have a more thorough discussion on the exact ways we are planning to handle errors going forward - and we will change this PR appropriately after that occurs.

That makes sense - let's put this on hold until then - I'll respond to your comments then we can leave this alone for a while.

P.S. I am aware that my comments may seem a bit pedantic; they were not intended to be so, I just would like to be thorough in my feedback, rather than just saying "fix this" etc.

They didn't come across as pedantic - I think it's a pretty normal part of code review.