don't panic on unexported struct key in map #1816
Conversation
HaraldNordgren
force-pushed
the
unexported_key
branch
from
22deba0 to
07088d8
Compare
|
hi all, not showing actual diff, but no panic so it's looks good for me |
|
I feel like the test you could cover more use cases. Let me try yo understand and rephrase what this PR is trying to fix: If we refers to the issue, the code was panicking when the struct had no exported fields, there is a map with at least 2 values. So here we replace the go-spew diffs, by something else. But as reported this "something" is not a diff, but something else. the fact go-spew could panic is random. Here you identify an issue but there could be others. I wonder whether having a defer recover with error management to go-spew couldn't help. The fact the diff is broken shouldn't break the assertion lib. It should report the test is falling, and here you could try to render things with the solution you found. I'm not saying your way is wrong, not my solution is a good one. I'm asking for feedbacks. I'm a bit afraid we are replacing a diff that works in many cases except a few random ones. When one of them was clearly identified, while others may stay hidden in the darkness able to panic. Also, I'm a bit afraid, the code added could lead to a placeholder reporting that cannot compete with the diffs available in go-spew, when it might not fail in some edge cases we didn't identified yet. Again, I'm not asking for code changes, but for an open discussion. I have no problem in being wrong 😁 |
|
It's a valid question for sure and you have a good point that this is just one cause of potentially many others. But using |
|
@brackendawson do you remember the discussion we had about importing the code of go-spew, the same way we imported difflib ? The conclusion was not yet, because we don't know yet the "unforseen consequences" of importing the code of go-difflib, so we don't have enough information and experience on this. Maybe it's time to reconsider it. Importing the code of go-spew diff and and fixing the bug in the code could be considered. |
|
Based on go-spew code This is what's panic is the call to reflect.Value.Interface So the fix would be about calling reflect.Value.CanInterface Especially if we consider the comment
So the fix would be about adding calls to reflect.Value.CanInterface in this for loop before calling reflect.Value.Interface https://github.com/davecgh/go-spew/blob/8991bc29aa16c548c550c7ff78260e27b9ab7c73/spew/common.go#L315 |
| func isStructWithUnexportedMapField(t reflect.Type) bool { | ||
| if t.Kind() != reflect.Struct { | ||
| return false | ||
| } | ||
|
|
||
| for i := 0; i < t.NumField(); i++ { | ||
| field := t.Field(i) | ||
| if !field.IsExported() && | ||
| field.Type.Kind() == reflect.Map && | ||
| field.Type.Key().Kind() == reflect.Array { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
Based on the code the discussion that happened here
And based on the code of go-spew
There was a problem hiding this comment.
I feel like started writting something, but I posted before putting all together.
So here, I was spotting the code here limited to unexported field that are maps with 2 elements, but go-spew code would panic when dealing with Interface and here, maybe a better fix would be to use CanInterface and check nothing else.
I'm unsure what other code examples could panic, but maybe it could be considered as a better approach than the "recover" I suggested.
There was a problem hiding this comment.
I feel this might not be enough.
I checked at go-spew code while working on https://github.com/stretchr/testify/pull/1824/files
I feel like a code like this won't be caught by your fix
type Bar struct {
m map[[1]byte]*struct{}
}
type Foo struct {
Foo Bar
}When using a Foo, the problem is wider that simply recursively parsing the fields, as you need to also go through the whole stack, check for the pointer, the slices, maps, and arrays
So have something that goes as deep as go-spew.
You are right, considering catching the panic with recover and then try to apply the patch is not a good solution. I provided you some feedback and suggestion to your PR, please take a look at them. But I feel like the recover idea should be considered anyway. The fact the diff can panic is bad. But maybe we could also add a recover to invalidate the diff from error reporting when it panic, instead if breaking everything. |
|
@ccoVeille Thanks for the ideas, but I'm not exactly sure which of these ideas you want me to implement. All of them or only a few? I'm not exactly sure how to properly use |
Co-authored-by: ccoVeille <[email protected]>
HaraldNordgren
force-pushed
the
unexported_key
branch
from
e9ea773 to
4462478
Compare
I'm sorry about that. I should have been clearer. Many of the point I raised are open for debate with everyone, including you, and also other maintainers. We can discuss about them, some of them or all the ideas I raised might be rejected. I just shared my thoughts about the possible resolutions. If we decide to go in that direction new PR will be opened I think. Right now, I'm good with your changes. That's why I approve your PR.
The CanInterface ideas were for fixing the bug in go-spew code. |
|
@ccoVeille Got it! Are there any plans to fix go-spew? I guess it would need to be by forking (since @davecgh seems to have abandoned it), which then in practice introduces a new transitive dependency for all testify users. |
|
@ccoVeille Let's merge this? |
|
I was waiting reaction from other maintainers |
|
This only fixes a narrow set of cases where spew may try to derive a value from an un-exported field, right? I think we should either avoid calling spew in all such cases or we should look at fixing 'spew. Fixing spew might mean moving it into this repository. Not really a fan of just merging this for now, I believe it's an incomplete fix and is not urgent, this bug has been there for a long time. |
4 participants
Fixes #1813.