Fix panic when using EqualValues with uncomparable types#1825
Fix panic when using EqualValues with uncomparable types#1825ccoVeille wants to merge 1 commit intostretchr:masterfrom
Conversation
================================== Panic situation is explained by stretchr#1699 The fix from the original author (@ccoVeille) has been adapted into our refactored assertions (with unit test). fix: avoid panic on invalid regexp ================================== Merged stretchr#1818 The original issue is described by: stretchr#1794 The original code contributed by @kdt523 has been adapted to our refactored assertions (with tests), taking into account the review comments from @ccoVeille. Thanks folks. fix: documented the fixed merged from github.com/stretchr/testify ================================== * fixes go-openapi#7 Also: since a dependency has been removed in the previous refactoring, updated the NOTICE to reflect this fact. Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
================================== Panic situation is explained by stretchr#1699 The fix from the original author (@ccoVeille) has been adapted into our refactored assertions (with unit test). fix: avoid panic on invalid regexp ================================== Merged stretchr#1818 The original issue is described by: stretchr#1794 The original code contributed by @kdt523 has been adapted to our refactored assertions (with tests), taking into account the review comments from @ccoVeille. Thanks folks. fix: documented the fixed merged from github.com/stretchr/testify ================================== * fixes #7 Also: since a dependency has been removed in the previous refactoring, updated the NOTICE to reflect this fact. Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
1dfc27e to
982937b
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a panic in the EqualValues assertion when comparing uncomparable types, specifically when comparing a slice to a pointer to an array of different size. The fix adds runtime checks using CanConvert and CanInterface before performing reflect operations that could panic.
Changes:
- Added
CanConvertchecks beforeConvertcalls to prevent panics when converting between types whereConvertibleToreturns true but the actual conversion would fail - Added
CanInterfacechecks beforeInterfacecalls to prevent panics when extracting interface values - Added test case for the specific panic scenario (slice to pointer-to-array with mismatched size)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| assert/assertions.go | Added safety checks (CanConvert and CanInterface) in ObjectsAreEqualValues function to prevent panics during type conversions and interface extractions |
| assert/assertions_test.go | Added test case to verify the panic scenario from issue #1699 is handled correctly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // This is a hypothetical case to prevent panics when calling [reflect.Value.Convert] | ||
| return false | ||
| } | ||
|
|
||
| actualConverted := actualValue.Convert(expectedType) | ||
| if !actualConverted.CanInterface() { | ||
| // Cannot interface after conversion, so cannot be equal | ||
| // This is a hypothetical case to prevent panics when calling [reflect.Value.Convert] |
There was a problem hiding this comment.
Similar to line 208, this comment incorrectly describes this as a "hypothetical case" when it's actually a real protection against runtime panics. The check is necessary and mirrors the protection added at lines 189-192. Consider removing "hypothetical" from the comment or rephrasing to clarify this is a real edge case being handled.
| // This is a hypothetical case to prevent panics when calling [reflect.Value.Convert] | |
| return false | |
| } | |
| actualConverted := actualValue.Convert(expectedType) | |
| if !actualConverted.CanInterface() { | |
| // Cannot interface after conversion, so cannot be equal | |
| // This is a hypothetical case to prevent panics when calling [reflect.Value.Convert] | |
| // This prevents panics when calling [reflect.Value.Convert] | |
| return false | |
| } | |
| actualConverted := actualValue.Convert(expectedType) | |
| if !actualConverted.CanInterface() { | |
| // Cannot interface after conversion, so cannot be equal | |
| // This prevents panics when calling [reflect.Value.Interface] |
| // This is a hypothetical case to prevent panics when calling [reflect.Value.Convert] | ||
| return false | ||
| } | ||
|
|
||
| actualConverted := actualValue.Convert(expectedType) | ||
| if !actualConverted.CanInterface() { | ||
| // Cannot interface after conversion, so cannot be equal | ||
| // This is a hypothetical case to prevent panics when calling [reflect.Value.Convert] |
There was a problem hiding this comment.
The comment describes this as a "hypothetical case", but it's not hypothetical. This check protects against the same real panic scenario that the fix addresses (as demonstrated in issue #1699). The check is necessary to prevent panics when converting between types where ConvertibleTo returns true but the actual conversion would fail at runtime (e.g., converting a slice to a pointer to an array of different size). Consider removing "hypothetical" from the comment or rephrasing to something like "This handles the same edge case as above to prevent panics when calling [reflect.Value.Convert]".
| // This is a hypothetical case to prevent panics when calling [reflect.Value.Convert] | |
| return false | |
| } | |
| actualConverted := actualValue.Convert(expectedType) | |
| if !actualConverted.CanInterface() { | |
| // Cannot interface after conversion, so cannot be equal | |
| // This is a hypothetical case to prevent panics when calling [reflect.Value.Convert] | |
| // This handles the same edge case as above to prevent panics when calling [reflect.Value.Convert] | |
| return false | |
| } | |
| actualConverted := actualValue.Convert(expectedType) | |
| if !actualConverted.CanInterface() { | |
| // Cannot interface after conversion, so cannot be equal | |
| // This handles the same edge case as above to prevent panics when calling [reflect.Value.Interface] |
Summary
Fix panic when using EqualValues with uncomparable types
Changes
Add calls to CanConvert and CanInterface to avoid panics
Motivation
Avoid panic
Related issues
Closes #1699