Model json.Unmarshal and xml.Unmarshal as guaranteeing non-nil output parameter#399
Open
nickita-khylkouski wants to merge 1 commit intouber-go:mainfrom
Open
Model json.Unmarshal and xml.Unmarshal as guaranteeing non-nil output parameter#399nickita-khylkouski wants to merge 1 commit intouber-go:mainfrom
nickita-khylkouski wants to merge 1 commit intouber-go:mainfrom
Conversation
… parameter on nil error Fixes uber-go#342 The json.Unmarshal and xml.Unmarshal functions allocate maps/slices when the output parameter is nil. Following the pattern of errors.As, we now model this contract by replacing the call expression with: json.Unmarshal(data, &v) && v != nil This adds an implicit nil check that makes NilAway understand the output parameter is non-nil after a successful unmarshal (error == nil), eliminating a common false positive.
Contributor
|
Thanks for the contributions! This doesn't seem to be correct, errors.As returns a bool such that we can replace it with errors.As && target != nil. These are equivalent. However, here the json.Marshal or xml.Marshal actually returns an error, and such replacement does not really make sense. We should actually replace the error check err != nil with err != nil && v ! = nil. This could be a bit tricky since the error check usually wouldn't be at the same node. With this PR, we would basically always consider v to be non-nil, right? Even if err == nil. Perhaps I missed something, happy to discuss further. |
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.
Fixes #342
Problem
A common pattern when using
json.Unmarshalwith a nil map/slice was being flagged as a false positive:When
json.Unmarshalreturns a nil error, the output parameter is guaranteed to be non-nil because the function allocates maps/slices when they're nil. This contract was not being modeled by NilAway.Solution
Following the pattern of
errors.As, we now model this contract using theReplaceConditionalhook system. The call expression is replaced with:This adds an implicit nil check that makes NilAway understand the output parameter is non-nil after a successful unmarshal (error == nil).
The same pattern applies to
xml.Unmarshal, which has identical behavior.Changes
jsonUnmarshalActionfunction inhook/replace_conditional.goReplaceConditionalmap to handleencoding/json.Unmarshalandencoding/xml.Unmarshaltestdata/src/go.uber.org/trustedfunc/json_unmarshal.goTesting
Alternative Approaches Considered
The
ReplaceConditionalapproach is the simplest and most appropriate solution, following the established pattern fromerrors.As.