go: cmd/vet: add a new analyzer for check missing values after append
package main
func main() {
sli1 := []string{"a", "b", "c"}
sli2 := make([]string, 0)
for _, val := range sli1 {
print(val)
sli2 = append(sli2)
}
}
When I execute go vet for static detection, no information is output.
I was expecting to be able to tell that I didn’t append variables for sli2.
such as : called without values to append
About this issue
- Original URL
- State: closed
- Created a year ago
- Reactions: 8
- Comments: 20 (18 by maintainers)
Commits related to this issue
- go/analysis: add a new analyzer for check missing values after append Currently, if there is no second parameter added during append, there will be no prompt when executing go vet. Add an analyzer to... — committed to golang/tools by cuishuang a year ago
- cmd: add a new analyzer for check missing values after append If there is no second parameter added during append, there will be no prompt when executing go vet. Add an analyzer to detect this situat... — committed to golang/go by cuishuang a year ago
No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group
It seems like this is worth doing - the linked examples seem to be real bugs, at least some of them, and none of them look like “innocent” code. At best they are confusing code.
Have all concerns about this proposal been addressed?
This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group
Related https://github.com/golang/go/issues/30040.
The code has been merged. Thanks all.
Celebrate!
I will complete the implementation of the code as soon as possible.
Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group
“append with no values” seems fine
Do note that https://staticcheck.io/docs/checks/#SA4021 exists, so even if it’s not in vet, it already gets flagged for a large portion of Go devs, and buggy code likely doesn’t get committed in the first place.
Here are some results on the precision of the checker. We looked at 20 findings. Four of them seem to be a correctness issue. Please let me know if you see something wrong in the table below.
We took the total 727 findings and sorted them based on how often their modules are used, preferring more popular modules. We then made sure a single random finding was picked for a package. Finally, we took the top 20 findings in the resulting list and analyzed them.
append([]T{...}); no-opappend(s); no-opappend(append(s)); inner append does the workappend(append(s)); inner append does the works=append(s); actual append done in a separate function; likely refactoring residues=append(s); likely bug,sigsis not usedappend([]T{...}); test; doesn’t seem like anything is missingappend(s)append(s)s1=append(f()); no-op; tests1=append(s2);baseTemplatesis never updateds1=append(s2); looks like an artifact of refactoring/code editingappend([]T{...}); no-op; has similar findings near this locationappend([]T{...}); no-op; similar code exists above that does not use appendappend([]T{...}); no-opappend([]T{...}); no-ops=append(s); masks self-assignments=append(s); other case branches appenda.tape.Tape[a.off]todstappend([]T{...})s=append(s); it seemsexistNodeshould be appendedInteresting. I just want to note that while
s = append(s)is clearly buggy in that example, it’s normally just a no-op. On the other hand,s1 = append(s2), which is equivalent tos1 = s2, seems to me to be more likely to indicate a bug, as the person writing that code may think that they are making a copy ofs2.But it may be that none of these are common enough to worry about.
I looked at a bunch of examples. My thoughts:
An append of one argument is always a no-op, harmless on its own.
gofmt -sshould remove it.Sometimes, although it is harmless on its own, one-arg append masks a bug. In the cases I’ve seen, this always happens when assigning back to the same slice. For example:
This loops forever if it loops at all, because the length of
e.Sigsnever changes.If we remove the append, the Go compiler catches this: “self-assignment of e.Sigs to e.Sigs”. So I think this compiler check should be changed to consider one-arg appends, if that’s not too expensive. If the compiler can’t be changed, then a vet check for
X = append(X)might be appropriate, although we’d have to weigh the benefit.It is worth keeping in mind that there is no hard threshold for # of instances that must be found in the wild. This is a judgement call and a balancing act. The vet “frequency” requirement:
https://github.com/golang/go/blob/master/src/cmd/vet/README#L18-L23
I do agree with @zpavlinovic that the current data suggests this issue is somewhat infrequent and suggests against inclusion. 727 reports may be more than a “handful of problems” though. I think it will be helpful to understand of whether the check is meeting vet’s correctness criteria. Are the existing reports potentially underlying bugs or instances of bad ‘style’ (e.g.
append([]string{"a", "b", "c"}))?We used the proposed change and ran it on open source Go projects.
On the benchmark suite of ~276k Go projects (all projects minus those that have certain build issues), the proposed checker reports 727 issues in 650 projects. This suggests the proposed checker does not satisfy the vet frequency criteria.
We’ll also soon share results on the true positive/false positive ratio on a random sample.