gocritic->appendAssign - Detects suspicious append result assignments. Should we enable it?

[zak-pawel]

Description

This issue starts a discussion about enabling:

• linter: gocritic - Highly extensible Go source code linter providing checks currently missing from other linters.
• group: diagnostic - Diagnostics try to find programming errors in the code. They also detect code that may be correct, but looks suspicious.
• checker: appendAssign - Detects suspicious append result assignments.

Example

Before:

p.positives = append(p.negatives, x)
p.negatives = append(p.negatives, y)

After:

p.positives = append(p.positives, x)
p.negatives = append(p.negatives, y)

Expected output

Decision about enabling or not enabling this checker.

Findings

For this checker, the following findings were found in the current codebase:

cmd/telegraf/main.go:130:16                                               gocritic  appendAssign: append result not assigned to the same slice
models/running_output_test.go:355:14                                      gocritic  appendAssign: append result not assigned to the same slice
models/running_output_test.go:416:14                                      gocritic  appendAssign: append result not assigned to the same slice
plugins/inputs/fail2ban/fail2ban.go:79:10                                 gocritic  appendAssign: append result not assigned to the same slice
plugins/inputs/fail2ban/fail2ban.go:101:11                                gocritic  appendAssign: append result not assigned to the same slice
plugins/inputs/gnmi/utils.go:102:18                                       gocritic  appendAssign: append result not assigned to the same slice
plugins/inputs/minecraft/client.go:138:11                                 gocritic  appendAssign: append result not assigned to the same slice
plugins/inputs/ping/ping_test.go:216:14                                   gocritic  appendAssign: append result not assigned to the same slice
plugins/inputs/x509_cert/x509_cert.go:98:19                               gocritic  appendAssign: append result not assigned to the same slice
plugins/inputs/x509_cert/x509_cert.go:497:9                               gocritic  appendAssign: append result not assigned to the same slice
plugins/inputs/x509_cert/x509_cert_test.go:560:9                          gocritic  appendAssign: append result not assigned to the same slice
plugins/outputs/azure_monitor/azure_monitor.go:269:41                     gocritic  appendAssign: append result not assigned to the same slice
plugins/outputs/yandex_cloud_monitoring/yandex_cloud_monitoring.go:155:9  gocritic  appendAssign: append result not assigned to the same slice
plugins/parsers/json_v2/parser.go:564:13                                  gocritic  appendAssign: append result not assigned to the same slice

Additional configuration

For this checker, no additional configuration can be provided.

[zak-pawel]

I would enable this checker.

[powersj]

What is your plan with the current findings then? Are these all going to become additional ignore statements?

Take for example, one of x509 examples:

    san := append(cert.DNSNames, cert.EmailAddresses...)
    for _, ip := range cert.IPAddresses {
        san = append(san, ip.String())
    }
    for _, uri := range cert.URIs {
        san = append(san, uri.String())
    }
    tags["san"] = strings.Join(san, ",")

The append in this case is used to combine multiple string slices and additional strings into one to create a tag.

I see the purpose of this one to catch typos during development, but once merged I would expect every instance of this to be a valid usage and turn into yet another ignore.

[zak-pawel]

@powersj I believe that all of them should be fixed. Not only to satisfy linter ;) but also to be sure that there is no common underlying array: https://freshman.tech/snippets/go/concatenate-slices/

[powersj]

The linter is finding when a slice is not assigned to the same slice in the first place. So let me ask again, how are you planning on resolving this linter.

edit: for example, are you planning on creating a new slice for every instance that all the other dump into?

[srebhan]

Having looked at the first (main) and fail2ban case as well as the x509 above, I come to the conclusion that at least those occurrences are false-positives. having something like myTempParams := append(someCommonParameterSet , someUseCaseSpecifcParameterSet...) IS a valid use-case.

[powersj]

I'm closing this as something we won't take.

The primary thing this linter seems to look for is that the resulting slice is not the same as the first argument. After going through the above instances of this linter, they are generally false positives. In the tests for example, we combine two different arrays into an "expected" array, or want a specific order of merging two arrays into one new array.

Keeping the more verbose, new value names, adds clarity and intention to what the code is meant to do.