Change https://go.dev/cl/498475 mentions this issue: go/analysis: add a new analyzer for check missing values after append

Change https://go.dev/cl/498416 mentions this issue: cmd/vet: add a new analyzer for check missing values after append

Related #30040.

ISTM, instead of a vet check, just make it illegal if go.mod version is greater than Go 1.22 or whatever.

A large amount of code in the previous version also needs a way to easily circumvent this problem

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.

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:

Vet is run every day by many programmers, often as part of every compilation or
submission. The cost in execution time is considerable, especially in aggregate,
so checks must be likely enough to find real problems that they are worth the
overhead of the added check. A new check that finds only a handful of problems
across all existing programs, even if the problem is significant, is not worth
adding to the suite everyone runs daily.

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"}))?

I looked at a bunch of examples. My thoughts:

An append of one argument is always a no-op, harmless on its own. gofmt -s should 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:

for len(e.Sigs) <= round {
    e.Sigs = append(e.Sigs)
}

This loops forever if it loops at all, because the length of e.Sigs never 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.

Interesting. 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 to s1 = 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 of s2.

But it may be that none of these are common enough to worry about.

I strongly agree, but in the end I have a slightly different opinion. Open source projects, especially with a lot of stars, are often have higher quality than projects running within the company.

I have seen many Go beginners have this problem intentionally or unintentionally. Even a very senior person may have this problem due to negligence.

If adding compile-time detection is too costly, it might be a good idea to add an analyzer to vet.

Just my personal opinion, happy to accept the final decision of the community.

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.

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.

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

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?

Another question: What prompt should be given when this situation is detected? The current possible prompt message is "called without values to append", maybe there is a better prompt.

Welcome to discuss!

"append with no values" seems fine

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Celebrate!

I will complete the implementation of the code as soon as possible.

The code has been merged. Thanks all.