Print the stack trace when formatting with %+v #28

Merged
merged 2 commits into from May 9, 2017

Conversation

Projects
None yet
5 participants
Contributor

mmikulicic commented Jan 19, 2017

Discovered in:
http://dave.cheney.net/2016/06/12/stack-traces-and-the-errors-package

Code based on:
https://github.com/pkg/errors

With a minor but hackish change to still allow %#v to print the structure fields, in order to not break the principle of least surprise. Let me know if there is a better way to strip a type of an interface or invoke the default %#v behaviour on a type that implements fmt.Formatter.


NOTE: this is the continuation of #26. I no longer work for cesanta and thus cannot address comments in the original PR.

#26 received the green light from @mjs, and had an open question by @howbazaar, whom I replied to.

@mjs mjs requested a review from howbazaar Apr 6, 2017

+// helper for Format
+type unformatter Err
+
+func (unformatter) Format() { /* break the fmt.Formatter interface */ }
@mjs

mjs Apr 6, 2017

Shouldn't this take (fmt.State, rune)?

@mmikulicic

mmikulicic Apr 13, 2017

Contributor

well, in that case it would fulfill the fmt.Formatter interface.
By adding a method with an incompatible type signature we're breaking the interface.

This allows us to do:

fmt.Fprintf(s, "%#v", (*unformatter)(e))

otherwise fmt.Fprintf will do a runtime type assertion e.(fmt.Formatter), which will succeed and cause infinite recursion here.

@mjs, do you know of a better way to preserve the default rendering that the object would have had with %#v?

@rogpeppe

rogpeppe May 9, 2017

Owner

There's no need to define a method - the unformatter type has no methods, because a newly defined named type in Go has no methods (other than those from embedded types).
I'd suggest defining the unformatter type inline inside Err.Format.

@mmikulicic

mmikulicic May 9, 2017

Contributor

yes indeed! thanks!
I believe I confused it with the embedding rules.

Contributor

mmikulicic commented Apr 21, 2017

@howbazaar, @mjs friendly ping

Owner

wallyworld commented May 9, 2017

$$merge$$

Contributor

jujubot commented May 9, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-errors

Contributor

jujubot commented May 9, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-errors/8

Looks reasonable, but this really needs some tests, and modulo a few comments and suggestions.

+ case s.Flag('#'):
+ // avoid infinite recursion by wrapping e into a type
+ // that doesn't implement Formatter.
+ fmt.Fprintf(s, "%#v", (*unformatter)(e))
@rogpeppe

rogpeppe May 9, 2017

Owner

This isn't going to produce quite what you want, I think - it'll show the "unformatter" type name, which will be quite unexpected.

It depends how useful we think the default %#v output is - personally, I think I'd write errors.Details(e) here, which means that you'd get reasonable default output when %#v is used (for example when gopkg.in/check.v1 prints a value).

@mmikulicic

mmikulicic May 9, 2017

Contributor

no strong opinions here; my expectations for %#v was to show the actual thing without any smart reformatting, i.e. as the doc says:

%#v	a Go-syntax representation of the value

So currently, with gopkg.in/check.v1 you see the guts of juju/errors:

	err := fmt.Errorf("foo")
	c.Check(err, Equals, 42)
	err = errors.Trace(err)
	c.Check(err, Equals, 42)

... go test:

foo_test.go:21:
    c.Check(err, Equals, 42)
... obtained *errors.errorString = &errors.errorString{s:"foo"} ("foo")
... expected int = 42

foo_test.go:23:
    c.Check(err, Equals, 42)
... obtained *errors.Err = &errors.Err{message:"", cause:(*errors.errorString)(0xc42000ef60), previous:(*errors.errorString)(0xc42000ef60), file:"pippo/foo_test.go", line:22} ("foo")
... expected int = 42

With this PR, the latter will become:

&errors.unformatter{message:"", cause:(*errors.errorString)(0xc42000ef60), previous:(*errors.errorString)(0xc42000ef60), file:"pippo/foo_test.go", line:22}

IMHO it's a bit ugly but at least it's not hiding anything.

@rogpeppe: Still prefer errors.Details(e) ?

@mmikulicic

mmikulicic May 9, 2017

Contributor

@rogpeppe: or what about this: mangle the output so that it outputs &errors.Err{ exactly as before

+ }
+ fallthrough
+ case 's':
+ fmt.Fprintf(s, "%s", e.Error())
@rogpeppe

rogpeppe May 9, 2017

Owner

s.Write([]byte(e.Error()))
?

@mmikulicic

mmikulicic May 9, 2017

Contributor

while we're at it, let's use io.WriteString

+ fallthrough
+ case 's':
+ fmt.Fprintf(s, "%s", e.Error())
+ }
@rogpeppe

rogpeppe May 9, 2017

Owner

Please add a default case so that we get some indication when someone uses the wrong verb.

@mmikulicic

mmikulicic May 9, 2017

Contributor

done

@mmikulicic

mmikulicic May 9, 2017

Contributor

also added support for %T

+// helper for Format
+type unformatter Err
+
+func (unformatter) Format() { /* break the fmt.Formatter interface */ }
@mjs

mjs Apr 6, 2017

Shouldn't this take (fmt.State, rune)?

@mmikulicic

mmikulicic Apr 13, 2017

Contributor

well, in that case it would fulfill the fmt.Formatter interface.
By adding a method with an incompatible type signature we're breaking the interface.

This allows us to do:

fmt.Fprintf(s, "%#v", (*unformatter)(e))

otherwise fmt.Fprintf will do a runtime type assertion e.(fmt.Formatter), which will succeed and cause infinite recursion here.

@mjs, do you know of a better way to preserve the default rendering that the object would have had with %#v?

@rogpeppe

rogpeppe May 9, 2017

Owner

There's no need to define a method - the unformatter type has no methods, because a newly defined named type in Go has no methods (other than those from embedded types).
I'd suggest defining the unformatter type inline inside Err.Format.

@mmikulicic

mmikulicic May 9, 2017

Contributor

yes indeed! thanks!
I believe I confused it with the embedding rules.

Contributor

mmikulicic commented May 9, 2017

Looks reasonable, but this really needs some tests ...

Yeah, makes sense. I'll add some test

@jujubot jujubot merged commit 8234c82 into juju:master May 9, 2017

Contributor

mmikulicic commented May 9, 2017

jujubot trigger happy :-)
I'll add tests in another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment