-
Notifications
You must be signed in to change notification settings - Fork 553
Fix panic in ExtractIntoStructPtr #2873
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for submitting your first PR! Be sure that we will be looking at it but keep in mind
this sometimes takes a while.
Please let the maintainers know if your PR has not got enough attention after a few days.
If any doubt, please consult our PR tutorial.
The comment to the function you are modifying specifically mentions that it's "for internal use only". However I realise that since it's exported, it's integral part of the API surface. I think your change request has merit and it's correctly written. Can you please do the same for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobuhiro11 thanks for the PR. I think it's also worth adding a check, whether interface is not a pointer. And an error message should be more specific, this should help during the development.
I have a last-minute doubt about this PR. see #2872 (comment) |
Passing nil as an argument to 'ExtractIntoStructPtr' or 'ExtractIntoSlicePtr' causes "panic: runtime error: invalid memory address or nil pointer dereference". This is an invalid argument and should be corrected to return an error. For reference, standard package functions such as 'json.Unmarshal' return an error instead of panic. Signed-off-by: Nobuhiro MIKI <nob@bobuhiro11.net> Co-authored-by: pýrus <kayrus@users.noreply.github.com>
6ce8f91
to
571d3f5
Compare
@pierreprinetti @kayrus
Of course. I added the same change for
I appied your suggestions. |
@pierreprinetti @kayrus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's merge this. I was hesitant, but if the standard library goes out of its way to match a literal nil, maybe we can too.
@kayrus are you fine with the changes you requested? |
@kayrus ping? |
@kayrus Could you please check? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
I don't understand why we can't merge it. |
Thank you for your review. |
Passing nil as an argument to 'ExtractIntoStructPtr' causes "panic: runtime error: invalid memory address or nil pointer dereference". This is an invalid argument and should be corrected to return an error. For reference, standard package functions such as 'json.Unmarshal' return an error instead of panic.
Fixes #2872