fix: pass original value to ParseTypeError in ArrayParameter and MapParameter#3486
Closed
he-yufeng wants to merge 1 commit into
Closed
fix: pass original value to ParseTypeError in ArrayParameter and MapParameter#3486he-yufeng wants to merge 1 commit into
he-yufeng wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in ArrayParameter.Parse and MapParameter.Parse where a type assertion failure resulted in ParseTypeError reporting a nil value instead of the original invalid input value. The fix correctly passes the original input v to ParseTypeError. Additionally, a new unit test TestParseTypeErrorIncludesValue has been added to verify this behavior. I have no feedback to provide.
…arameter After a failed two-result type assertion in Go, the LHS variable is set to the zero value for its type (nil for slices and maps). Both ArrayParameter.Parse and MapParameter.Parse were passing this zero value to ParseTypeError instead of the original `v`, causing error messages to always report the zero value rather than the actual bad input. All five other Parse methods in the same file (String, Int, Float, Bool, and the integer sub-case) correctly pass `v`. This commit aligns ArrayParameter and MapParameter with that pattern and adds regression tests that verify the original value appears in the error text.
7ed6139 to
855708e
Compare
Contributor
Author
|
Closing in favor of #3512, which lands the same fix (passing the original |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In Go, a two-result type assertion like
x, ok := v.(T)setsxto the zero value ofTwhen the assertion fails —nilfor slice and map types. BothArrayParameter.ParseandMapParameter.Parsewere passing this zero value toParseTypeErrorinstead of the original argumentv, so a caller would see an error like:instead of something useful that identifies what was actually passed in.
The five other
Parsemethods in the same file —StringParameter,IntParameter,FloatParameter,BoolParameter, and the integer sub-case — all correctly passvtoParseTypeError. This PR bringsArrayParameterandMapParameterin line with that pattern.Changed lines:
Two regression tests are added in
TestParseTypeErrorIncludesValue: one forArrayParameter.Parse("not-an-array")and one forMapParameter.Parse("bad"), each asserting the error message contains the original input value. Both tests fail before this fix and pass after.