Fixed NPE in Stringer() #528
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #528 +/- ##
==========================================
+ Coverage 97.06% 97.07% +0.01%
==========================================
Files 39 39
Lines 2279 2289 +10
==========================================
+ Hits 2212 2222 +10
Misses 59 59
Partials 8 8
Continue to review full report at Codecov.
|
| return Skip() | ||
| } | ||
|
|
||
| r := reflect.ValueOf(val) |
There was a problem hiding this comment.
I have a few concerns with this approach:
- we're adding a reflect call when creating a field, which could have a performance impact (we should measure the impact)
- we're changing the behaviour of
Stringer. It's possible for a type to implement aStringmethod that works fornil:
https://play.golang.org/p/EUNoR7Sz9Q
Now, we're changing the behaviour where it'll Skip the field.
This is also bad for nil slices for example, where we might want to output a empty sice rather than skipping the field.
There was a problem hiding this comment.
-
It's 288 ns/op vs 316 ns/op on my laptop, additional datapoints are welcome.
-
Yea, I'm aware of that possibility. Simplest way of maintaining compatibility is to introduce something like
StringerNilSafefield or bump the library version. If you have any ideas -- they are welcome.
I find the behavior of Error simple and useful: if argument is nil it's not rendered.
As for slices: there could be scenario where I want to distinguish nil-slice vs empty slice in logs.
There was a problem hiding this comment.
I don't mind adding the nil check above; it's cheap and always correct.
I'd prefer not to have the reflection-based checks below. If we get a non-nil interface with a nil value that also dereferences the receiver in String, it's completely reasonable to panic - in my book, that's definitely programmer error.
| return Skip() | ||
| } | ||
|
|
||
| r := reflect.ValueOf(val) |
There was a problem hiding this comment.
I don't mind adding the nil check above; it's cheap and always correct.
I'd prefer not to have the reflection-based checks below. If we get a non-nil interface with a nil value that also dereferences the receiver in String, it's completely reasonable to panic - in my book, that's definitely programmer error.
|
😶 Closing due to inactivity. 😶 This is just housekeeping - leave a comment and re-open if necessary! |
This commit ensures any panic raised while encoding zap.Stringer is catched and reported as an error of the form kError for a field k. The reported panic is prefixed with "PANIC=" to distinguish these errors from other encoding errors. Closes uber-go#495 Closes uber-go#528
Fixes #495