Add nilness lint pass and fix its findings #61608
Merged
Conversation
This file contains 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
|
This change is |
stevendanna
force-pushed
the
ssd/nilness
branch
2 times, most recently
from
5b2e981 to
54e98e1
Compare
mgartner
reviewed
Mar 8, 2021
pkg/util/json/encoded.go
Outdated
| @@ -436,6 +436,8 @@ func (j *jsonEncoded) FetchValKey(key string) (JSON, error) { | |||
| } | |||
| return string(data) >= key | |||
| }) | |||
| // TODO(ssd): This err will always be nil. It looks | |||
| // like we intended to assign to it at 433 above. | |||
There was a problem hiding this comment.
Looks like this was added in #21676 and I agree it looks like the function passed to sort.Search is suppose to set the outer err variable.
There was a problem hiding this comment.
I've pushed a fix that collects the last non-nil error. I'm mildly nervous since from what I can see the unit test suite doesn't hit many of the error paths in this code. But I'm made less nervous by the fact that (I believe) an error here would imply that the caller has some incorrectly encoded json and this code already assumes a correct encoding in a number of places.
stevendanna
force-pushed
the
ssd/nilness
branch
2 times, most recently
from
34c21ef to
3998829
Compare
stevendanna
requested review from
a team and
miretskiy
and removed request for
a team
miretskiy
approved these changes
Mar 17, 2021
stevendanna
force-pushed
the
ssd/nilness
branch
from
3998829 to
2e55153
Compare
|
👏 👏 👏 👏 💯 |
stevendanna
force-pushed
the
ssd/nilness
branch
from
2e55153 to
b2e6115
Compare
This imports the nilness check from golang.org/x/tools at commit 3e1cb95235af786c5d1f0deffbd0b0646bf00024 with minor modifications to pass our linter. The purpose of importing this linter into the source tree rather than depending on it is so that we can make local modifications to it in future commits. Release note: None
These functions return nil if the first argument provided is nil. While this behaviour is often useful, passing an always-nil value to such a function is typically an indication of a bug. Release note: None
The "degenerate" cases that the nilness pass found covered a number of cases where the "tautological" check made the code a bit more readable or cases where a nil check might be tautological now, but could easily change in the future. Release note: None
These calls to errors.Wrap-style functions were passing errors that would always be nil, which means that they would be returning a nil error rather than the non-nil error that the author likely expected. Release note: None
These cases were found by the nilness linter. Most appear to be err checks that were simply copy and pasted but were no longer needed. Release note: None
The nilness linter checks for errant uses of provably nil values. The most useful of these checks so far as been its ability to find errors.Wrapf-style functions that are always being passed nil errors. Release note: None
stevendanna
force-pushed
the
ssd/nilness
branch
from
b2e6115 to
84fdb06
Compare
|
bors r=miretskiy |
|
Build succeeded: |
|
I think this should be back ported to at least 20.2 where we are still fixing bugs. |
craig bot
pushed a commit
that referenced
this pull request
May 25, 2021
65656: server: simplify an error case r=stevendanna a=knz Found while re-reviewing #61608 cc @cockroachdb/server for education `NewAssertionErrorWithWrappedErrf` already includes the text of the original error on the right of the wrapping message. No need to include it a second time. Release note: None Co-authored-by: Raphael 'kena' Poss <[email protected]>
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.
This adds a nilness lint pass that checks for errant uses of provably nil values.
This nilness analyzer was taken from
golang.org/x/toolsand then modifiedto add checks for various
errors.Wrapf-style functions where passing anilvaluecan lead to a nil error to be returned when the author likely expected a non-nil error.
While most of the errors fixed here were found by the nilness linter's check for
"degenerate" if conditions, I've turned off that check for now as there are still some cases
to fix.
Release note: None