cmd/vet: add a new analyzer for check missing values after append

[cuishuang]

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

[gopherbot]

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

[gopherbot]

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

[timothy-king]

Related https://github.com/golang/go/issues/30040.

[earthboundkid]

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

[cuishuang]

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

[zpavlinovic]

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.

[timothy-king]

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

[jba]

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.

[ianlancetaylor]

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.

[cuishuang]

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.

[zpavlinovic]

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.

Package@version	Source	Correctness issue?	Notes
github.com/armon/go-metrics/prometheus@v0.4.1	link	no	append([]T{...}); no-op
github.com/theupdateframework/notary/client@v0.7.0	link	no	append(s); no-op
github.com/klaytn/klaytn/cmd/utils/nodecmd@v1.10.2	link	no	append(append(s)); inner append does the work
github.com/klaytn/klaytn/storage/database@v1.10.2	link	no	append(append(s)); inner append does the work
github.com/FactomProject/factomd/controlPanel_test@v1.13.0	link	no	s=append(s); actual append done in a separate function; likely refactoring residue
github.com/FactomProject/factomd/elections@v1.13.0	link	yes	s=append(s); likely bug, sigs is not used
github.com/m3db/m3/src/aggregator/client@ v1.5.0	link	no	append([]T{...}); test; doesn't seem like anything is missing
github.com/operator-framework/api/pkg/validation/internal@v0.17.5	link	no	append(s)
github.com/solo-io/skv2/codegen/collector@v0.30.1	link	no	append(s)
github.com/turingchain2020/turingchain/system/mempool@ v1.1.21	link	no	s1=append(f()); no-op; test
github.com/lovoo/goka/web/index@v1.1.8	link	no	s1=append(s2); baseTemplates is never updated
github.com/drone/go-scm/scm/driver/harness@v1.29.1	link	no	s1=append(s2); looks like an artifact of refactoring/code editing
github.com/micro/micro/v2/service/auth@v2.9.3	link	no	append([]T{...}); no-op; has similar findings near this location
github.com/go-kratos/kratos/tool/testgen@v1.0.1	link	no	append([]T{...}); no-op; similar code exists above that does not use append
github.com/aximchain/axc-cosmos-sdk/types/fees@v0.1.9	link	no	append([]T{...}); no-op
github.com/aerospike/aerospike-client-go/v6@v6.12.0	link	no	append([]T{...}); no-op
github.com/couchbase/cbgt@v1.3.4	link	yes	s=append(s); masks self-assignment
github.com/minio/simdjson-go@v0.4.5	link	yes	s=append(s); other case branches append a.tape.Tape[a.off] to dst
github.com/getamis/sirius/metrics@v1.1.16	link	no	append([]T{...})
github.com/aacfactory/fns/service@v1.0.32	link	yes	s=append(s); it seems existNode should be appended

[dominikh]

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.

[rsc]

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

[rsc]

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?

[cuishuang]

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!

[rsc]

"append with no values" seems fine

[rsc]

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

[rsc]

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

[cuishuang]

Celebrate!

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

[cuishuang]

The code has been merged. Thanks all.