cleanup: add new linters and fix bugs and alerts #983
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
mtardy
force-pushed
the
pr/mtardy/add-linters
branch
from
f0a71d5 to
c349950
Compare
mtardy
added
the
kind/cleanup
mtardy
force-pushed
the
pr/mtardy/add-linters
branch
2 times, most recently
from
0cd3409 to
814e9ad
Compare
mtardy
changed the title
mtardy
force-pushed
the
pr/mtardy/add-linters
branch
2 times, most recently
from
c8b5367 to
3c10546
Compare
willfindlay
approved these changes
May 11, 2023
Comment on lines
-298
to
-301
| if key == "" { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
Doesn't this change the behaviour of the loop?
There was a problem hiding this comment.
It was actually a fix for a bug because I think the author did not understand why they were empty keys in the slice. See this:
f021b5d#diff-468002e48832ac8408184ef20e818fa41c0f35b2a7562d8bc20b902182860bfdL47-R47.
See this commit message f021b5d.
There was a problem hiding this comment.
Can we change the 'continue' to 'return error' instead? (As per my comment on #1832?
|
Btw I'm also not a fan of that "BUG in SensorCtl" thing. Maybe we can refactor that slightly in the future. |
waw thanks a lot for that fast and good review! :) |
mtardy
force-pushed
the
pr/mtardy/add-linters
branch
from
3c10546 to
61a910a
Compare
Simplify multiple situations: - omit comparison to bool constants - return comparison instead of using if statements - simplify useless usage of fmt.Printf family functions - remove redundant return statement - remove unnecessary assignment to the blank identifier - remove useless usage of select - simplify type assertion switch case - merge variable declaration with assignment Signed-off-by: Mahe Tardy <[email protected]>
Signed-off-by: Mahe Tardy <[email protected]>
Makezero is a linter to ensure that slices are not initialized with non-zero length. Since in Go, we often use append on the slice, initializing it with non-zero length can tend to create a slice full of zero that will clobber the beginning of the slice. In our bug, we created a list with 300+ zeros and then append after them, only to skip the zero in the calling method. Usually with want to prealloc the slice with a capacity of len but a length of 0, i.e. replace make([]int, len(nums)) with make([]int, 0, len(nums)). See more info here https://github.com/ashanbrown/makezero. Signed-off-by: Mahe Tardy <[email protected]>
Loggercheck checks for odd number of key and value pairs for common logger libraries such as klog. Fix three minor log bugs in which we did not use klog correctly. Signed-off-by: Mahe Tardy <[email protected]>
Exporting pointers for loop variables can create very hard to debug bug. Contrary to what can think, the `:=` assignment in the range loop does not create a new local variable on each iteration, see the following Go Language specification extract. "The iteration variables may be declared by the “range” clause using a form of short variable declaration (:=). In this case their types are set to the types of the respective iteration values and their scope is the block of the "for" statement; they are re-used in each iteration. If the iteration variables are declared outside the "for" statement, after execution their values will be those of the last iteration." Creating a local variable fix this issue and allocate new memory each time making it possible to export the pointer. When immediately using break after, it's okay but linter is not capable of detecting that because of perf slowdown, see kyoh86/exportloopref#14. See more info: - https://github.com/kyoh86/exportloopref - https://medium.com/swlh/use-pointer-of-for-range-loop-variable-in-go-3d3481f7ffc9 Signed-off-by: Mahe Tardy <[email protected]>
This is a minor one but that detects effective typos which can be autofixed with the `--fix` flag of golangci-lint run. Signed-off-by: Mahe Tardy <[email protected]>
Linter gofmt verifies the code has been properly gofmt-ed and also run by default with -s option to check for code simplification. Signed-off-by: Mahe Tardy <[email protected]>
A naming convention exists for Go errors and this linter help to enforce it. It's mostly a style linters but I think this convention is nice to respect. See more https://github.com/golang/go/wiki/Errors#naming and https://github.com/Antonboom/errname. Signed-off-by: Mahe Tardy <[email protected]>
Gocritic is sometimes an opinionated linter, explaining why I removed some of the checks. However it provides valuable checks on differents areas. See https://github.com/go-critic/go-critic. This commit fixes: - badCall: suspicious Join on 1 argument - unslice: could simplify slice[:] to slice - unlambda: unecesseray lambda - wrapperFunc: use strings.ReplaceAll instead of replace with -1 - captLocal: local var should not be capitalized - exitAfterDefer: replace some log.Fatal to permit cleanup using defer Signed-off-by: Mahe Tardy <[email protected]>
Unparam is doing a bit better than revive, finding return values that are unused or always nil (for error it can be useful). See linter for more information: https://github.com/mvdan/unparam Signed-off-by: Mahe Tardy <[email protected]>
Signed-off-by: Mahe Tardy <[email protected]>
Signed-off-by: Mahe Tardy <[email protected]>
mtardy
force-pushed
the
pr/mtardy/add-linters
branch
from
61a910a to
967292e
Compare
michi-covalent
added
the
needs-rebase
This was referenced Nov 29, 2023
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 is a follow-up of #976 to enable new linters to increase the code quality, remove bugs and improve readability in a sustainable way.
Add the following linters and fix their alerts:
I would love to enable these ones about errors, but those will be a followup cleanup PR since this is a lot of work:
We still don't catch situations where we use the return value before checking the error.
Added this issue for follow-up #989.
Just for info the pattern used here, that defaults to error and rely on the function to return nil and override is a bit weird and led to an issue while removing useless "always nil" return value from functions:
tetragon/pkg/sensors/manager.go
Line 60 in 557a6c9