commit c9b30991decd307c47f3fb18e7e8278adb080b4c
author Ryan Harrison <rharrison@chromium.org> Mon Sep 08 13:22:14 2025 -0700
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Sep 08 13:22:14 2025 -0700
tree 74a7ab0a9abe398ef7d8b9bd4ef751f427fb56a6
parent adbe88f76afb7b3844e1f2b4a5cb4107c4c723ae

Cleanup of various lint issues across the code base

Fixing a variety of issues discovered by running staticcheck locally,
along with other linting issues found via golangci-lint in my IDE.

This does NOT eliminate all of the findings that staticcheck has in
its default configuration, just significantly reduces them. Some
issues require more extensive changes/investigation than I was willing
to put into this CL, or probably should just be suppressed since it is
unlikely we will fix them.

Types of issues identified by staticcheck and fixed:
- should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...)) (S1028)
- var <name> is unused (U1000)
- func <name> is unused (U1000)
- type <name> is unused (U1000)
- this value of err is never used (SA4006)
- package <name> is being imported more than once (ST1019)
- unnecessary assignment to the blank identifier (S1005)
- Various "io/ioutil" has been deprecated since Go 1.19
- reflect.PtrTo has been deprecated since Go 1.22
- should use time.Since instead of time.Now().Sub (S1012)
- should use copy(to, from) instead of a loop (S1001)

Other types of issues fixed:
- Various pkg/var name collisions ('auth', 'git', 'gerrit', etc)
- Various direct comparison against errors instead of using errors.Is()
- Redundant conversions, e.g. string("literal")
- Struct <name> has methods on both value and pointer receivers,
  i.e. mixing (c Cmd) and (c *Cmd), there seemed to be lots of false
  positives for this, not clear why
- Ineffectual assignments, e.g. val := "", then immediately assigning
  to val, sometimes these assignments hid unused variables
- Instances where types could be omitted
- Snake case naming

I intentionally did not fix the following findings:
- error strings should not be capitalized (ST1005), since a lot of the
  existing tests are doing string comparison for error checking, so
  would need to be updated also
- strings.Title being deprecated, since it requires a new library call
  to fix, which has different semantics
- Fixing slices being initialized with empty literal instead of nil,
  e.g. var := []int{}. Technically nil for a slice can be appended to
  like an empty slice and some other usages and potentially save an
  allocation if not used, but in other cases, like working with JSON
  marshaling/unmarshaling nil vs an empty slice have semantic
  differences. I think the nil-behaviour of slices was a mistake,
  since it is a bit of a gotcha and really only exists for a niche
  optimization.
- Unused params, which could be fixed via '_'. There are hundreds of
  these in the code base and it is debatable if they should be fixed.
- Warnings about documentation, broadly ignored, since often they were
  related to having a '// TODO...' with no other documentation. Fixed
  examples that just needed moving text around instead of adding
  documentation
- Unhand-led errors, since these are often just being used in defer or
  from a printing statement, and wrapping them to be explicitly
  ignored is verbose
- Some examples of unused vars in tests, because they smelt of
  incomplete tests, i.e. there should have cases that used
  them.
- Some name collisions, especially with 'len' and 'delete', because
  they smelt of overly smart meta-programming going on, and I didn't
  feel like sussing out what was going on

Bug: 439814281

Change-Id: Ib828330bfcffb1290fc15909218fb1eb432c45b5
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/258134
Commit-Queue: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: Brian Sheedy <bsheedy@google.com>
Commit-Queue: Brian Sheedy <bsheedy@google.com>
Auto-Submit: Ryan Harrison <rharrison@chromium.org>