proposal: x/tools/go/analysis/passes/appends: check for incorrect slice length initialization

[cuishuang]

Proposal Details

The following code exists in many projects, and developers actually want [0 1 2], but due to the initialization error of slice, the final result is [0 0 0 0 1 2]

package main

import "fmt"

func main() {

	sli := make([]int, 3)

	for i := range 3 {
		sli = append(sli, i)
	}

	fmt.Println(sli) // the result is [0 0 0 0 1 2]

}

The online demo: https://go.dev/play/p/q1BcVCmvidW

Over the past few months, I have conducted extensive research and analysis, and also submitted pull requests to fix issues in many well-known Go projects such as prometheus, zap, vitess. Below are some pull requests submitted by me and others related to this problem.

Due to limitations in search skills and time, I only checked records of such issues in the past few months. The history of more such issues has not been traced back. But I think it's already enough

I would like to propose adding a new analyzer to go vet that can detect such situations, thereby avoiding these issues in the future.

Now I have already completed an initial version of the code, and if the proposal is approved, I would be happy to refine it and add the necessary test cases.

The merged pr:

prometheus/prometheus#14702 (comment)

uber-go/zap#1461

cadence-workflow/cadence#6293

prometheus/prometheus#15026

vitessio/vitess#16674

kedacore/keda#6179

external-secrets/external-secrets#3964

brianvoe/gofakeit#365

fission/fission#3018

DataDog/datadog-agent#29744

https://github.com/superseriousbusiness/gotosocial/pull/3382

ccfos/nightingale#2169

gookit/color#97

vdaas/vald#2672

supabase/auth#1788

pufferpanel/pufferpanel#1367

juju/juju#18176

go-spatial/tegola@0f3131f

lxc/incus#1285

yunionio/cloudpods#21346

taubyte/tau#253

fleetdm/fleet#22608

antrea-io/antrea#6715

tdewolff/canvas#315

Consensys/gnark#1288

superfly/flyctl#3982

bazel-contrib/rules_go#4133

zitadel/oidc#658

jhump/protoreflect#629

apache/rocketmq-client-go#1171

edgexfoundry/edgex-go#4938

dolthub/doltgresql#812

apache/trafficcontrol#8091

pingcap/tidb-operator#5755

botlabs-gg/yagpdb#1734

Altinity/clickhouse-backup#1019

openshift/installer#9072

GoogleCloudPlatform/magic-modules#11919

openmeterio/openmeter#1615

target/goalert#4090

kubeovn/kube-ovn#4579

syyongx/php2go#49

fluid-cloudnative/fluid#4335

akuity/kargo#2648

kubernetes/kubernetes#127785

apache/dubbo-go#2734

letsencrypt/boulder#7725

cortexproject/cortex#6237

kubeedge/kubeedge#5895

grafana/mimir#9449

rocboss/paopao-ce#581

authelia/authelia#7720

cilium/cilium#35164

git-lfs/git-lfs#5874

https://github.com/hashicorp/nomad/pull/24109/files

cosmos/ibc-go#6444

minio/minio#19567

VictoriaMetrics/VictoriaMetrics#6897

hyperledger/fabric#4956

grafana/pyroscope#3600

cosmos/cosmos-sdk#21494 (review)

anchore/grype#2133

https://github.com/ethereum-optimism/optimism/pull/11542/files

https://github.com/libp2p/go-libp2p/pull/2938/files

stashapp/stash#5327

trufflesecurity/trufflehog#3293

c9s/bbgo#1724 (comment)

cosmos/cosmos-sdk#22006

FerretDB/FerretDB#4598

dagger/dagger#8612

letsencrypt/boulder#7731

Layr-Labs/eigenda#767

wal-g/wal-g#1800

VictoriaMetrics/VictoriaMetrics#7161

harmony-one/harmony#4767

stackrox/stackrox#13028

stefanprodan/timoni#430

Altinity/clickhouse-operator#1523

iotexproject/iotex-core#4412

ane more in review process.

[gabyhelp]

Related Issues and Documentation

• fmt: fix formatter init.
• runtime: fix slice init length

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[gopherbot]

Change https://go.dev/cl/619915 mentions this issue: go/analysis: add a new analyzer to check for incorrect slice length initialization

[gopherbot]

Change https://go.dev/cl/619935 mentions this issue: cmd: add a new analyzer to check for incorrect slice length initialization

[gopherbot]

Change https://go.dev/cl/618875 mentions this issue: runtime: fix slice init length

[timothy-king]

What is the proposed criteria to decide when vet should issue a report? It is hard to decide whether to accept a vet proposal without this being specified. We need to assess the likely false positive and negatives rates of the criteria. Thanks.

(You can try to turn the included CL into natural language.)

[zigo101]

I'm surprised that there are so many misuses of the append function.

[adonovan]

Guessing from logic of the CL I just reviewed, the criterion is:

Vet should report a diagnostic when appending to a slice s if there exists an assignment s = make(T, n) where n is not a constant zero and there are no expressions of the form s[i] or copy(s, ...).

[cuishuang]

What is the proposed criteria to decide when vet should issue a report? It is hard to decide whether to accept a vet proposal without this being specified. We need to assess the likely false positive and negatives rates of the criteria. Thanks.

(You can try to turn the included CL into natural language.)

Yes, this is the issue I initially wanted to discuss on the CL page. Thanks to @adonovan for the detailed summary.

Additionally, through extensive practice, I have encountered several challenges and gained some insights: How do we balance false positives and omissions? Should we aim to comprehensively identify similar issues and accept a high rate of false positives, or should we focus on precision, which might lead to some omissions? This balance certainly warrants further discussion.

Specifically regarding the code, I’ve noticed that when slice are processed with binary.LittleEndian.PutUint16, they often lead to false positives. Moreover, if a slice is initialized in function A but only appended to after several layers of calls in function Z, should we address this situation? Or should we only consider initialization and appending that occur within the same method?

Last year, with your guidance, I added the "appends" analyzer to go vet, so I have some experience in this area.

However, it’s clear that this situation is more complex than last year, and there are more details to discuss. I look forward to your suggestions.

Thank you all!

[cuishuang]

I'm surprised that there are so many misuses of the append function.

Yes, if using sli[i] for assignment, there is no such problem, but many people like me are used to using append. So I think it can also be considered as an initialization issue with the slice.

Based on my limited investigation and analysis of high star libraries related to the Go ecosystem, it seems worthwhile to add such an analyzer.