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

Bug: Errors aren't created with %w, breaking errors.Is #503

Open
rkennedy opened this issue Apr 30, 2024 · 0 comments · May be fixed by #504
Open

Bug: Errors aren't created with %w, breaking errors.Is #503

rkennedy opened this issue Apr 30, 2024 · 0 comments · May be fixed by #504
Labels

Comments

@rkennedy
Copy link

rkennedy commented Apr 30, 2024

Bug Description

In one of my build targets, I use sh.Output to check the output of a command. For example:

output, err := sh.Output("command", "--version")

But in my case, it's completely normal for "command" not to exist sometimes, so I want to handle that error differently from other failures that sh.Output might return. When the command doesn't exist, the underlying os/exec code will return an fs.PathError, and the idiomatic way to check for that is to use errors.Is:

if err != nil {
    if !errors.Is(err, fs.ErrNotExist) {
        // This is really an error.
        return err
    }
    // File-doesn't-exist is an expected condition that we can handle.
}

But errors.Is returns false for errors returned by sh.Output.

What did you do?

As described above, I tried to use errors.Is to inspect errors returned by sh.Output and other command-executing functions from sh.

What did you expect to happen?

I expected errors.Is to recognize errors from sh functions as any of various common system errors, particularly fs.ErrNotExist.

What actually happened?

errors.Is fails to acknowledge that any sh error is like any other known error.

Environment

  • Mage Version: v1.15.0
  • OS: Linux

Additional context

This happens because errors in sh are constructed using fmt.Errorf("...%v", err). That inserts the value of err.String() but does not actually wrap the error, so any additional information about err is discarded. Changing it to fmt.Errorf("...%w", err) would solve it without affecting any other behavior. Error wrapping was introduced in Go 1.13.

My workaround for this issue is to inspect the text of the error message instead:

if err != nil {
    if !strings.Contains(err.Error(), "no such file or directory") {
        // This is really an error
        return err
    }
    // File-doesn't-exist is an expected condition that we can handle.
}
rkennedy added a commit to rkennedy/mage that referenced this issue May 1, 2024
Wrapping errors instead of merely embedding error.messages allows
calling code to use `errors.Is` and `errors.As` to more precisely check
the reasons for various errors instead of having to rely only on
substring searches.

Fixes magefile#503
rkennedy added a commit to rkennedy/mage that referenced this issue May 1, 2024
Wrapping errors instead of merely embedding error messages allows
calling code to use `errors.Is` and `errors.As` to more precisely check
the reasons for various errors instead of having to rely only on
substring searches.

Fixes magefile#503
@rkennedy rkennedy linked a pull request May 1, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant