Improve error handling #30

moosichu commented 2021年10月18日 22:04:16 +02:00 (Migrated from github.com)

This change in complete, currently in proof-of-concept stage.

Creates a new error set for non-program errors, i.e. errors caused by
dodgy input (malformed archives) as opposed to programming mistakes or
runtime issues.

Seperate-out printing to stderr (now done in main.zig) from producing
errors when parsing & formatting useful error strings.

The intent here is to make archive parsing more usable as an API, and
eventually capture & return more kinds of errors.

Top-level error handling for program errors (eg. OOM) to come after we
decide how to handle these kinds of errors.

This change in complete, currently in proof-of-concept stage. Creates a new error set for non-program errors, i.e. errors caused by dodgy input (malformed archives) as opposed to programming mistakes or runtime issues. Seperate-out printing to stderr (now done in main.zig) from producing errors when parsing & formatting useful error strings. The intent here is to make archive parsing more usable as an API, and eventually capture & return more kinds of errors. Top-level error handling for program errors (eg. OOM) to come after we decide how to handle these kinds of errors.

moosichu commented 2021年10月18日 22:04:48 +02:00 (Migrated from github.com)

@iddev5 it would be great to get your opinion on the overall direction of this PR. It's not yet ready to merge, but would like to hear your thoughts before pressing on and completing it.

@iddev5 it would be great to get your opinion on the overall direction of this PR. It's not yet ready to merge, but would like to hear your thoughts before pressing on and completing it.

alichraghi commented 2021年10月19日 07:21:08 +02:00 (Migrated from github.com)

i have an idea

fnprintError(err:Error,comptimemsg:[]constu8,print_args:anytype)Error{stdout.print(msg,print_args);returnerror;}...else=>{returnprintError(error.ParseError,"Parsing Error {s}",.{"Message"});},

i have an idea ```zig fn printError(err: Error, comptime msg: []const u8, print_args: anytype) Error { stdout.print(msg, print_args); return error; } ... else => { return printError(error.ParseError, "Parsing Error {s}", .{"Message"}); }, ```

iddev5 commented 2021年10月19日 12:31:00 +02:00 (Migrated from github.com)

Well, I wonder why are we doing error handling in this way in the first place...
The thing is zig and zld use something called std.log.scoped() to print out all such messages. Because it provides one single interface for debug messages, warnings, errors etc. I don't see what additional benefit we get from writing to a known stream (like stderr or stdout).

Well, I wonder why are we doing error handling in this way in the first place... The thing is zig and zld use something called std.log.scoped() to print out all such messages. Because it provides one single interface for debug messages, warnings, errors etc. I don't see what additional benefit we get from writing to a known stream (like stderr or stdout).

moosichu commented 2021年10月19日 20:01:54 +02:00 (Migrated from github.com)

Cheers, both that feedback and specificity with regards to how zar will be used is very helpful and clarifying. I'm still relatively inexperienced with zig, so learnt a lot. I've updated the PR a little then using scoped logs.

The only issue is that errors now are prefixed with the scope when printing, eg. error(archive): Malformed archive contents. which we don't want for user-facing errors when running the program as zig ar, so I'm going to look into how the compiler deals with that kind of thing as well (good source to reference generally I guess!).

Cheers, both that feedback and specificity with regards to how zar will be used is very helpful and clarifying. I'm still relatively inexperienced with zig, so learnt a lot. I've updated the PR a little then using scoped logs. The only issue is that errors now are prefixed with the scope when printing, `eg. error(archive): Malformed archive contents.` which we don't want for user-facing errors when running the program as `zig ar`, so I'm going to look into how the compiler deals with that kind of thing as well (good source to reference generally I guess!).

iddev5 commented 2021年10月19日 20:09:05 +02:00 (Migrated from github.com)

The only issue

Hmm yeah, thats something to look at. Compilers generally make their own diagnostics system to deal with this.
Everything else is looking good, this would now allow us to have arbitrary log outputs to check the program flow and such.

> The only issue Hmm yeah, thats something to look at. Compilers generally make their own diagnostics system to deal with this. Everything else is looking good, this would now allow us to have arbitrary log outputs to check the program flow and such.

moosichu commented 2021年10月20日 00:51:26 +02:00 (Migrated from github.com)

Compiler wasn't super helpful as that part of it is still implemented in C, but I think I've found a semi-reasonable solution (making use of a custom logger).

Compiler wasn't super helpful as that part of it is still implemented in C, but I think I've found a semi-reasonable solution (making use of a custom logger).

iddev5 commented 2021年10月20日 14:36:35 +02:00 (Migrated from github.com)

Why was the recent change made?

Why was the recent change made?

moosichu commented 2021年10月20日 19:34:46 +02:00 (Migrated from github.com)

Why was the recent change made?

So - it comes down to making sure that runtime errors (i/o issues mainly) are still handled and print an appropriate error message to the end-use when zar is used as a standalone program, whilst being usable as a library within the context of the zig compiler.

The reason I only applied the change in a handful of places was because I wanted your thoughts on it, as you might similarly have better solution ideas to this problem as you did for the previous set of changes.

> Why was the recent change made? So - it comes down to making sure that runtime errors (i/o issues mainly) are still handled and print an appropriate error message to the end-use when zar is used as a standalone program, whilst being usable as a library within the context of the zig compiler. The reason I only applied the change in a handful of places was because I wanted your thoughts on it, as you might similarly have better solution ideas to this problem as you did for the previous set of changes.

iddev5 commented 2021年10月20日 20:03:35 +02:00 (Migrated from github.com)

I do see your point, but no zig projects handle 100% of the errors, it's just not something considered necessary. Zig already provides quite a good error system on its own. I know you're concerned about showing a stacktrace to the end user but remember that no zig project ever handle all the errors. If we can't read a file, it's user's fault that fs perms are not correct, and such. For the seekTo errors, it's only possible when seektopos > filesize which is a malformed archive error.

I do see your point, but no zig projects handle 100% of the errors, it's just not something considered necessary. Zig already provides quite a good error system on its own. I know you're concerned about showing a stacktrace to the end user but remember that no zig project ever handle all the errors. If we can't read a file, it's user's fault that fs perms are not correct, and such. For the seekTo errors, it's only possible when seektopos > filesize which is a malformed archive error.

moosichu commented 2021年10月20日 21:28:36 +02:00 (Migrated from github.com)

If we can't read a file, it's user's fault that fs perms are not correct, and such

To me, this is precisely the kind of thing we should be printing a clear error message for. Explaining why the program failed to complete as clearly as possible.

I don't think it's desirable to just not handle that error and let it bubble all the way up the stack.

For the seekTo errors, it's only possible when seektopos > filesize which is a malformed archive error.

Yeah, we need make a seperate class of programmer errors as well. (i.e. things that should never happen based on our assumptions) so that we can print out that the error is a bug and ask the user to file a bug report.

> If we can't read a file, it's user's fault that fs perms are not correct, and such To me, this is precisely the kind of thing we should be printing a clear error message for. Explaining why the program failed to complete as clearly as possible. I don't think it's desirable to just not handle that error and let it bubble all the way up the stack. > For the seekTo errors, it's only possible when seektopos > filesize which is a malformed archive error. Yeah, we need make a seperate class of programmer errors as well. (i.e. things that should never happen based on our assumptions) so that we can print out that the error is a bug and ask the user to file a bug report.

moosichu commented 2021年10月21日 21:03:04 +02:00 (Migrated from github.com)

Even though error handling hasn't hit full-coverage yet (I think it's better to do that latter), this PR establishes a set of patterns I would be happy merging in.

@iddev5 are you convinced? I did have a brief exchange with @kubkon and the end goal with zig itself is to not present users with backtraces on errors etc. As zar itself is a much smaller program, aiming to have that kind of stuff in-place in-principle before merging upstream is a reasonable goal I think.

I've also made it so that it only has this behaviour in release builds, in debug builds errors will still show a stack-trace.

Even though error handling hasn't hit full-coverage yet (I think it's better to do that latter), this PR establishes a set of patterns I would be happy merging in. @iddev5 are you convinced? I did have a brief exchange with @kubkon and the end goal with zig itself is to not present users with backtraces on errors etc. As zar itself is a much smaller program, aiming to have that kind of stuff in-place in-principle before merging upstream is a reasonable goal I think. I've also made it so that it only has this behaviour in release builds, in debug builds errors will still show a stack-trace.

iddev5 commented 2021年10月22日 11:52:17 +02:00 (Migrated from github.com)

Oh okay, that's alright then. I did a brief rundown of the changes and it seems alright to me.

Oh okay, that's alright then. I did a brief rundown of the changes and it seems alright to me.