-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add new static analyzer: ineffassign #7413
Merged
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
Codecov Report
@@ Coverage Diff @@
## master #7413 +/- ##
==========================================
- Coverage 60.48% 60.47% -0.01%
==========================================
Files 419 419
Lines 30618 30618
==========================================
- Hits 18519 18516 -3
- Misses 9070 9072 +2
- Partials 3029 3030 +1 |
prestonvanloon
approved these changes
Oct 2, 2020
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.
What type of PR is this?
What does this PR do? Why is it needed?
filterAttestationsForBlockInclusion
#7398 and Fix ineffectual assignments #7403). @terencechain suggested that we can benefit from static code analyzer that enforces effectual assignments only. This PR introduces such an analyzer.Which issues(s) does this PR fix?
N/A
Other notes for review
❗ Merge after Enable cryptorand analyzer in slasher #7417
Here is how output of this analyzer looks like:
The only exceptions are the following constructs:
Although
ctx
after re-assigning fromtrace.StartSpan
is not used in a func (making in ineffectual assignment), we seem to be ok withctx
argument not used within funcs (we have many places wherectx
argument is defined but not used). Normally, non-used arguments are immediately removed, but withctx
we are lenient in our current code.As such, replacing
ctx, span := trace.StartSpan(ctx, "detection.detectIncomingAttestations")
with_, span := trace.StartSpan(ctx, "detection.detectIncomingAttestations")
will require next developer to be careful enough, to make surectx
is overwritten again.NB: Overall, I think even
ctx
variable shouldn't be used as placeholder, and if func doesn't rely on it, that argument should be removed and only reintroduced when necessary. If we decide to proceed with that, updating this analyzer will be trivial (just delete two lines of code), moreover, after the deletion this analyzer will help detect cases which need to be updated.