Repository navigation
Commit 3d5ae77
authored
v2.2.1 - formatter fix (#23)
* Fix eight defects found by a contract test suite
doc.go states two things this package did not do: that it implements the Go2
error inspection design, and that every method works with any error type as well
as nil values. contract_test.go holds it to both, organised by obligation --
nothing panics, a message survives construction, the chain is walkable, Is and
As find what is there and only what is there, and the rendering paths neither
lose nor invent information. It found eight defects.
PANICS ON NIL. (*E).Error, (*E).Is and (*E).MarshalJSON dereferenced a nil
receiver. (*E).Is and the package-level Is also panicked whenever a frame
carried no annotation of its own, because reflect.TypeOf(nil) returns a NIL
reflect.Type and calling Comparable on that panics rather than answering false.
Both now share a comparableErrors helper that screens for it.
MESSAGE CORRUPTION. New and Wrap passed the message through fmt.Errorf, so any
message containing a percent sign was reinterpreted: New("100% complete")
produced "100%!c(MISSING)omplete" and "disk usage at 95%" produced
"...95%!(NOVERB)". Percent signs occur in URL-encoded values, percentages and
Windows paths -- ordinary text. A message is now stored verbatim; Errorf, and
Wrap when given arguments, still format.
MARSHAL PANIC. list() includes any error implementing Unwrap() error, so a
fmt.Errorf("%w") link in the chain reached an unguarded type assertion and a nil
field read. Marshalling an error must never panic: that path runs while
something is already failing, usually inside a logger.
TWO SEVERED CHAINS. Trace held the wrapped error in the annotation slot and left
prev nil, so Unwrap returned nothing and no sentinel below a trace was
reachable. Track built prev from a synthetic box whose own prev was nil, so for
anything that was not already an *E -- a fmt.Errorf("%w") wrapper, a joined
error, any foreign wrapping type -- everything beneath it was lost. A decorator
that adds caller data must not cost the caller their chain. Both now keep the
error on the Unwrap chain, and a frame with no message of its own renders
transparently rather than emitting a stray separator.
IS AND AS DISAGREED. WrapE's annotation is not on the Unwrap chain, and Is
special-cased it while As did not, so which of the two you reached for changed
the answer. (*E) now implements the standard As(interface{}) bool hook and the
package-level As searches the same slot.
The suite asserts agreement with the standard library as a PROPERTY across
sixteen chain shapes -- bdlm frames, fmt wrappers, foreign types, joined errors,
traced and tracked errors, and a foreign type whose Is hook answers false while
still wrapping a real cause -- rather than in hand-picked cases, since a
disagreement is exactly what leaves a caller unable to tell which function to
trust. It also covers depth 2000, concurrent reads under -race, and every
documented format verb.
Coverage 95.9%, go vet clean, race clean. Track's rendered message no longer
repeats the wrapped error's text, since the error it wraps renders immediately
after it.
Also corrects doc.go, which documented an errors.Has that does not exist -- Is
searches the whole chain, which is what that section described -- and showed
errors.Wrap(err, err2), which does not compile because Wrap takes a message
string.
* Match the standard library's As signature
As becomes As(err error, target interface{}) bool, replacing
As(err, test error) error. This is a breaking API change, hence v2.2.0.
The old form required the target to satisfy error, and that had two costs. A
caller could not ask for a bare interface type at all -- an interface pointer
does not itself implement error -- which is the most common real question to ask
of a chain: does anything in here implement this behaviour. And for concrete
types the target's receiver kind leaked into the call site: a type with a
pointer receiver could not be asked for, so callers chose value receivers to
make the call compile rather than because the type wanted one. This package's
own test helpers had done exactly that.
It also meant the central helper of a package that aims to implement the Go2
error inspection design did not match the standard library it mirrors, so code
moving between errors.As and bdlm/errors.As had to change shape as well as
import path.
Two behaviours now match the standard library exactly. As panics on a target
that is not a non-nil pointer to an error-implementing type or to an interface:
such a target can never match anything, so answering "not found" would hide a
call-site mistake in the paths hardest to observe. A nil ERROR remains an
ordinary false, which is the opposite case -- a runtime condition rather than a
programming error, and the standard library draws the same line.
The walk also no longer ends on a hook that answers false. It read
`if e, ok := err.(interface{ As(error) error }); ok { return e.As(test) }`, so
the first link with an As method decided the whole search -- the same defect
that was fixed in Is, in the same shape. It is now `ok && x.As(target)`.
Tests cover the panic contract across four unusable target shapes, interface
targets, pointer- and value-receiver concrete targets, and agreement with the
standard library on every chain shape using an identical call for both -- which
is itself the point of the change. Coverage 97.3%, vet clean, race clean.
The As(interface{}) bool hook on *E is unchanged and is what reaches the
annotation slot, so Is and As continue to agree about what the chain contains.
* Make %v carry the cause chain, like Error() and %s
Error() and %s returned "outer: middle: root cause". %v returned "outer". The
cause was preserved everywhere except the verb that fmt.Println uses and that
most loggers call, so a wrapped error was recorded in full and rendered
truncated -- which is the shape of the rollup failure that started this: the
ClickHouse cause was present on the error and absent from the log line.
fmt normally gives %v on an error the value of Error(), but implementing
Formatter overrides that, and Format's 'v' case took a separate path through the
per-frame message and stopped at the first frame. The 'default' case, which
serves %s and %q, called Error() and was correct all along. Both the standard
library and pkg/errors treat %v as the whole chain, so this package was the
outlier.
%v, %s, %-v and %#v now render the full message. %+v and %#+v are unchanged and
still print one line per frame carrying that frame's own message: the tail would
otherwise repeat on every line, and that separation is the whole reason the
trace forms exist.
This changes rendered output, hence v2.2.1 and a note in the changelog for
anyone asserting on log text. Eight examples encoded the old behaviour and are
updated. The four trace examples were unaffected, which is the evidence that the
change is scoped to the non-trace forms -- I briefly broke one by editing
expectations positionally rather than by function, and it failed immediately.
TESTS. The suite had a gap that let this through, and it is worth naming because
the gap was in the assertions rather than the coverage: TestFormatVerbs checked
that every verb produced something non-empty, contained no malformed directive,
and that %+v was longer than %v. All three pass while %v silently drops the
cause. Asserting SHAPE is not asserting BEHAVIOUR. TestPlainVerbsEqualError now
requires %v and %s to equal Error() across eight chain shapes, and
TestTraceVerbKeepsOneMessagePerFrame requires the trace form to state each
message exactly once, so neither form can drift into the other.
Coverage 96.9%, vet clean, race clean.1 parent 3da1e1c commit 3d5ae77
4 files changed
Lines changed: 95 additions & 14 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
9 | 16 | | |
10 | 17 | | |
11 | 18 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
348 | 348 | | |
349 | 349 | | |
350 | 350 | | |
351 | | - | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
| 354 | + | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
| 376 | + | |
| 377 | + | |
| 378 | + | |
| 379 | + | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
| 385 | + | |
| 386 | + | |
| 387 | + | |
| 388 | + | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
| 396 | + | |
| 397 | + | |
| 398 | + | |
| 399 | + | |
| 400 | + | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
352 | 405 | | |
353 | 406 | | |
354 | 407 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
20 | 20 | | |
21 | 21 | | |
22 | 22 | | |
23 | | - | |
| 23 | + | |
24 | 24 | | |
25 | 25 | | |
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
29 | | - | |
| 29 | + | |
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
33 | 33 | | |
34 | 34 | | |
35 | | - | |
| 35 | + | |
36 | 36 | | |
37 | 37 | | |
38 | 38 | | |
| |||
44 | 44 | | |
45 | 45 | | |
46 | 46 | | |
47 | | - | |
| 47 | + | |
48 | 48 | | |
49 | 49 | | |
50 | 50 | | |
| |||
58 | 58 | | |
59 | 59 | | |
60 | 60 | | |
61 | | - | |
| 61 | + | |
62 | 62 | | |
63 | 63 | | |
64 | 64 | | |
65 | 65 | | |
66 | 66 | | |
67 | 67 | | |
68 | 68 | | |
69 | | - | |
| 69 | + | |
70 | 70 | | |
71 | 71 | | |
72 | 72 | | |
73 | 73 | | |
74 | 74 | | |
75 | 75 | | |
76 | 76 | | |
77 | | - | |
| 77 | + | |
78 | 78 | | |
79 | 79 | | |
80 | 80 | | |
| |||
83 | 83 | | |
84 | 84 | | |
85 | 85 | | |
86 | | - | |
| 86 | + | |
87 | 87 | | |
88 | 88 | | |
89 | 89 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
120 | 120 | | |
121 | 121 | | |
122 | 122 | | |
123 | | - | |
124 | | - | |
| 123 | + | |
| 124 | + | |
125 | 125 | | |
126 | 126 | | |
127 | 127 | | |
128 | 128 | | |
129 | | - | |
130 | | - | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
131 | 132 | | |
132 | 133 | | |
133 | 134 | | |
134 | | - | |
| 135 | + | |
135 | 136 | | |
136 | 137 | | |
137 | 138 | | |
| |||
158 | 159 | | |
159 | 160 | | |
160 | 161 | | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
161 | 182 | | |
162 | 183 | | |
163 | 184 | | |
| |||
0 commit comments