gocritic->sloppyReassign - Detects suspicious/confusing re-assignments. Should we enable it?

zak-pawel commented 1 year ago

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: sloppyReassign - Detects suspicious/confusing re-assignments.

Example

Before:

if err = f(); err != nil { return err }

After:

if err := f(); err != nil { return err }

Expected output

Decision about enabling or not enabling this checker.

Findings

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

internal/rotate/file_writer.go:84:5                                  gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := w.rotateIfNeeded()`
internal/rotate/file_writer.go:147:5                                 gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := w.current.Close()`
internal/rotate/file_writer.go:154:5                                 gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := os.Rename(w.filename, rotatedFilename)`
internal/rotate/file_writer.go:177:7                                 gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := os.Remove(filename)`
plugins/common/cookie/cookie.go:36:5                                 gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := c.initializeClient(client)`
plugins/common/starlark/field_dict.go:246:7                          gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := dict.SetKey(sKey, sValue)`
plugins/inputs/azure_monitor/azure_monitor_test.go:44:5              gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := json.Unmarshal(file, &genericResourcesExpanded)`
plugins/inputs/azure_monitor/azure_monitor_test.go:72:5              gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := json.Unmarshal(file, &genericResourcesExpanded)`
plugins/inputs/azure_monitor/azure_monitor_test.go:120:5             gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := json.Unmarshal(file, &metricDefinitions)`
plugins/inputs/azure_monitor/azure_monitor_test.go:167:5             gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := json.Unmarshal(file, &metricResponses)`
plugins/inputs/dpdk/dpdk.go:78:5                                     gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := isSocket(dpdk.SocketPath)`
plugins/inputs/dpdk/dpdk.go:85:5                                     gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := dpdk.validateCommands()`
plugins/inputs/gnmi/gnmi.go:124:6                                    gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := subscription.buildFullPath(c)`
plugins/inputs/gnmi/gnmi.go:129:6                                    gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := c.TagSubscriptions[idx].buildFullPath(c)`
plugins/inputs/intel_dlb/intel_dlb.go:88:5                           gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := validateEventdevCommands(d.EventdevCommands)`
plugins/inputs/intel_rdt/intel_rdt.go:126:5                          gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := validateInterval(r.SamplingInterval)`
plugins/inputs/jenkins/jenkins.go:73:6                               gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := j.initialize(client)`
plugins/inputs/kibana/kibana.go:241:5                                gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := json.NewDecoder(response.Body).Decode(v)`
plugins/inputs/libvirt/libvirt.go:155:5                              gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := l.utils.EnsureConnected(l.LibvirtURI)`
plugins/inputs/mailchimp/chimp_api.go:164:5                          gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := chimpErrorCheck(body)`
plugins/inputs/mysql/mysql.go:644:6                                  gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := rows.Scan(valPtrs...)`
plugins/inputs/mysql/mysql.go:766:6                                  gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := rows.Scan(&key, &val)`
plugins/inputs/opensearch_query/opensearch_query_test.go:630:5       gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := indexer.Close(context.Background())`
plugins/inputs/phpfpm/fcgi.go:123:5                                  gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := binary.Read(r, binary.BigEndian, &rec.h)`
plugins/inputs/phpfpm/fcgi_client.go:51:6                            gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := c.writeRecord(typeStdin, reqID, []byte(requestData))`
plugins/inputs/postgresql_extensible/postgresql_extensible.go:190:5  gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := row.Scan(columnVars...)`
plugins/inputs/powerdns_recursor/protocol_v3.go:37:5                 gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := writeNativeUIntToConn(conn, uint(len(command)))`
plugins/inputs/salesforce/salesforce.go:120:6                        gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := s.login()`
plugins/inputs/sflow/packetdecoder.go:66:5                           gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := read(r, &addressIPType, "address ip type")`
plugins/inputs/sflow/packetdecoder.go:79:5                           gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := read(r, &p.AgentAddress.IP, "Agent Address IP")`
plugins/inputs/sflow/packetdecoder.go:82:5                           gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := read(r, &p.SubAgentID, "SubAgentID")`
plugins/inputs/sflow/packetdecoder.go:85:5                           gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := read(r, &p.SequenceNumber, "SequenceNumber")`
plugins/inputs/sflow/packetdecoder.go:88:5                           gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := read(r, &p.Uptime, "Uptime")`
plugins/inputs/statsd/statsd.go:412:8                                gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := conn.SetKeepAlive(true)`
plugins/inputs/statsd/statsd.go:417:9                                gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := conn.SetKeepAlivePeriod(time.Duration(*s.TCPKeepAlivePeriod))`
plugins/inputs/teamspeak/teamspeak.go:43:7                           gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := ts.client.Use(vserver)`
plugins/inputs/teamspeak/teamspeak.go:46:7                           gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := ts.client.SetNick(ts.Nickname)`
plugins/inputs/zipkin/codec/thrift/thrift.go:32:6                    gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := zs.Read(context.Background(), transport)`
plugins/inputs/zipkin/codec/thrift/thrift.go:38:5                    gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := transport.ReadListEnd(context.Background())`
plugins/outputs/sumologic/sumologic.go:172:5                         gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := gz.Close()`
plugins/outputs/syslog/syslog.go:108:6                               gocritic  sloppyReassign: re-assignment to `err` can be replaced with `err := s.Connect()`

Additional configuration

For this checker, no additional configuration can be provided.

zak-pawel commented 1 year ago

I'm not a big fan of this checker.

Hipska commented 1 year ago

I understand why it is suggested. The code will look/be more clean.

powersj commented 1 year ago

I'm not a big fan of this checker.

Any particular reason?

zak-pawel commented 1 year ago

Any particular reason?

Just don't like declaration shadowing.

srebhan commented 1 year ago

@zak-pawel I agree in general but like it for err as this is almost always a throw-away variable used solely locally.

influxdata / telegraf