Satisfy errors.Is even if formatting is incorrect

[barrettj12]

If you create an error, but provide the wrong number of format arguments:

err := errors.NotValidf("missing username", username)

then, the resulting error doesn't satisfy errors.Is as you'd expect:

errors.Is(err, errors.NotValid) // false

Obviously this is a user error in this case, but nonetheless, we should still fulfill the contract of NotValidf and ensure the returned error satisfies errors.Is(_, errors.NotValid).

The issue was that the format string was being passed directly to fmt.Errorf, and we were relying on the %w functionality to correctly wrap the error type. If the error string is malformed, then the ConstError won't get wrapped properly - thus the returned error will fail to satisfy errors.Is.

This PR fixes the problem by explicitly wrapping the error using WithType before returning. This way, we can ensure the returned error always satisfies errors.Is, no matter what is returned from makeWrappedConstError.

I've also added regression tests for this bug.

[tlm]

Appreciate the regression tests done here.

Overall I think the implementation is a bit of a duck tape solution instead of patching the function with the problem.

Could we instead patch the problem with something like this to solve it?

func makeWrappedConstError(err error, format string, args ...interface{}) error {
    separator := " "
    if err.Error() == "" || errors.Is(err, &fmtNoop{}) {
        separator = ""
    }
    args = append(args, err)
    errPlacement := fmt.Sprintf("%%[%d]w", len(args))
    return fmt.Errorf(strings.Join([]string{format, errPlacement}, separator), args...)
}

[tlm]

Can we not include this in the PR? Project git ignore shouldn't be for ignoring your systems files made. There is a global git ignore for that. Also don't want to introduce different concepts in the PR.

[barrettj12]

global git ignore

Wow, first time I'm hearing about this

[hpidcock]

func makeWrappedConstError(err error, format string, args ...interface{}) error {
separator := " "
if err.Error() == "" || errors.Is(err, &fmtNoop{}) {
separator = ""
}
args = append(args, err)
errPlacement := fmt.Sprintf("%%[%d]w", len(args))
return fmt.Errorf(strings.Join([]string{format, errPlacement}, separator), args...)
}

I think we can just simplify this too:

func makeWrappedConstError(err error, format string, args ...interface{}) error {
    separator := " "
    if err.Error() == "" || errors.Is(err, &fmtNoop{}) {
        separator = ""
    }
    args = append(args, err)
    combinedFormat := fmt.Sprintf("%s%s%%[%d]w", format, separator, len(args))
    return fmt.Errorf(combinedFormat, args...)
}

[hpidcock]

go tool vet help printf
printf: check consistency of Printf format strings and arguments

Analyzer flags:

  -printf.funcs value
    	comma-separated list of print function names to check (default (*log.Logger).Fatal,(*log.Logger).Fatalf,(*log.Logger).Fatalln,(*log.Logger).Panic,(*log.Logger).Panicf,(*log.Logger).Panicln,(*log.Logger).Print,(*log.Logger).Printf,(*log.Logger).Println,(*testing.common).Error,(*testing.common).Errorf,(*testing.common).Fatal,(*testing.common).Fatalf,(*testing.common).Log,(*testing.common).Logf,(*testing.common).Skip,(*testing.common).Skipf,(testing.TB).Error,(testing.TB).Errorf,(testing.TB).Fatal,(testing.TB).Fatalf,(testing.TB).Log,(testing.TB).Logf,(testing.TB).Skip,(testing.TB).Skipf,fmt.Errorf,fmt.Fprint,fmt.Fprintf,fmt.Fprintln,fmt.Print,fmt.Printf,fmt.Println,fmt.Sprint,fmt.Sprintf,fmt.Sprintln,log.Fatal,log.Fatalf,log.Fatalln,log.Panic,log.Panicf,log.Panicln,log.Print,log.Printf,log.Println,runtime/trace.Logf)

The check applies to known functions (for example, those in package fmt)
as well as any detected wrappers of known functions.

A function that wants to avail itself of printf checking but is not
found by this analyzer's heuristics (for example, due to use of
dynamic calls) can insert a bogus call:

	if false {
		_ = fmt.Sprintf(format, args...) // enable printf checking
	}

The -funcs flag specifies a comma-separated list of names of additional
known formatting functions or methods. If the name contains a period,
it must denote a specific function using one of the following forms:

	dir/pkg.Function
	dir/pkg.Type.Method
	(*dir/pkg.Type).Method

Otherwise the name is interpreted as a case-insensitive unqualified
identifier such as "errorf". Either way, if a listed name ends in f, the
function is assumed to be Printf-like, taking a format string before the
argument list. Otherwise it is assumed to be Print-like, taking a list
of arguments with no format string.

[hpidcock]

Based on the docs from go tool vet we should do this:

func Timeoutf(format string, args ...interface{}) error {
	if false {
		_ = fmt.Sprintf(format, args...) // enable printf checking
	}
	return newLocationError(
		makeWrappedConstError(Timeout, format, args...),
		1,
	)
}

[barrettj12]

We can do one better, and put this check inside makeWrappedConstError.

[hpidcock]

only reason I put it at the exported level was to save the linter from recursing further than it needs.

[barrettj12]

@hpidcock I agree, the vet check is a really nice thing to add in.

However, we still have the same problem if the format string is not a constant. Even a case as simple as this still fails:

fstring := "extra arg %v"
err := errors.NotValidf(fstring, 3, 2)
errors.Is(err, errors.NotValid) // false

@tlm you have a fair point about this being a "duct-tape" solution. However, I personally would feel much more comfortable if these MyErrorTypef functions return a type where the Unwrap() value is explicitly set to MyErrorType.

I don't think we should be relying on fmt.Errorf to give us the correct wrapping behaviour (it's already shown us that it won't). And modifying the format string feels like a very hacky solution to me. Not to mention that "%%[%d]w" or "%s%s%%[%d]w" are extremely hard to understand, hence hard to maintain.

I'll come back with a more well-thought-out and well-designed solution.

[hpidcock]

I don't think we should be relying on fmt.Errorf to give us the correct wrapping behaviour (it's already shown us that it won't). And modifying the format string feels like a very hacky solution to me. Not to mention that "%%[%d]w" or "%s%s%%[%d]w" are extremely hard to understand, hence hard to maintain.

I honestly don't think these are hacky solutions, they are just not readable. That is ok. Needs a comment and its a sufficient and well formed solution.

[tlm]

@barrettj12,

I agree with @hpidcock that the go vet check should be added. It's a static analysis check so it will only ever be performed on const strings as you just have no way of performing the check on what is dynamic runtime data.

If you take the following code:

formatString := "hello %v"
fmt.Printf(formatString, 3, 4)

This suffers the same problem. It's just the fact of life that we live with. Taking this on as a runtime check would be both expensive and introduce a new error handling path into the code. I did some quick number counting over the code base to see how many of our format calls to the error library don't contain a constant string and it's bugger all. Was a bit hard to tell fully as newline's make it hard but we don't have a lot compared to how many of these calls we do have.

As for the other solution being too complicated to read I am not sure I agree with that. But there are methods to make it more composed so it's build on pieces that are easier to digest and we can also add comments to the function to describe the intent.

Your original PR still failed to address on the functions that needs to be maintained. Most of these *f() funcs are used because they offer very specific error formatting with the error type. For example "hazza not found". We have a ton of tests in Juju that rely on very specific error message formatting still. The solution I have proposed maintains this contract which is part of the reason we left these functions in place the first time.

I personally would feel much more comfortable if these MyErrorTypef functions return a type where the Unwrap() value is explicitly set to MyErrorType.

How are they not doing that now. %w produces a wrapped error where by if you call Unwrap you end up with the same const error. I fail to see the point here if any.

My advice. We have identified a very small problem here that has affected us a grand total of once thus far and hasn't made it to committed code. Both solutions proposed here fix the issue well. Keep it stupid simple and don't overcomplicate what is a simple problem and simple fix.

[barrettj12]

The issue is that we're breaking the contract of these methods:

// NotValidf returns an error which satisfies Is(err, NotValid) and the
// Locationer interface.

This is incorrect for the current code, and a more correct version would read:

// NotValidf returns an error which satisfies Is(err, NotValid), *IF* the provided
// format string and arguments are consistent. Otherwise, it will *NOT* return an
// error which satisfies Is(err, NotValid).

The problem I have is that we're feeding things into this black box called fmt.Errorf, and expecting it to output something with the correct properties. This is extremely fragile and easy to break. Instead we should return a custom error type, so we can guarantee that the result of Unwrap is the expected ConstError.

This suffers the same problem.

Except it doesn't, because fmt.Printf doesn't make any promises about the properties of the obtained string.

Your original PR still failed to address on the functions that needs to be maintained. Most of these *f() funcs are used because they offer very specific error formatting with the error type. For example "hazza not found". We have a ton of tests in Juju that rely on very specific error message formatting still.

This PR has maintained that contract. You can run the unit tests locally to verify that none of the error messages have changed.

%w produces a wrapped error where by if you call Unwrap you end up with the same const error.

Huuuge caveat: IF the format string is correct. Otherwise it will not have the correct Unwrap behaviour. That is the whole point here.

don't overcomplicate

Does "%s%s%%[%d]w" not strike you as overcomplicated?

[barrettj12]

@hpidcock @tlm just pushed a new version of this which patches the MyErrorTypef methods to return a correctly typed error. As a bonus:

• the format string/args are passed directly (unmodified) to fmt.Sprintf, which will enable go vet checking on these methods;
• the suffix will display correctly even if the format string is wrong:

  f := "hello %v %v"
  errors.NotValidf(f, "foo") // hello foo %!v(MISSING) not valid