From e23ea68ff01b76a64dfa9b308dd1752ec2794a1b Mon Sep 17 00:00:00 2001 From: Michael Whittaker Date: Wed, 16 Aug 2023 10:28:55 -0700 Subject: [PATCH] Switched to using staticcheck linter. Previously, we used golangci-lint, but the Go team recommended we use staticcheck instead. --- .github/workflows/go.yml | 6 +++--- .golangci.yaml | 28 ------------------------- cmd/weaver/main.go | 4 ++-- dev/build_and_test | 10 ++++----- examples/collatz/main.go | 2 -- internal/benchmarks/benchmarks_test.go | 5 ----- internal/envelope/conn/envelope_conn.go | 4 ++-- internal/envelope/conn/weavelet_conn.go | 1 - internal/status/dashboard.go | 6 +++--- internal/status/server.go | 2 +- internal/tool/generate/example/main.go | 12 +++++------ internal/tool/multi/deployer.go | 2 +- internal/tool/single/deploy.go | 2 +- internal/weaver/remoteweavelet.go | 2 +- runtime/bin/bin.go | 2 +- runtime/bin/testprogram/main.go | 12 +++++------ runtime/codegen/registry_test.go | 2 +- runtime/envelope/envelope.go | 2 +- runtime/metrics/labels_test.go | 2 +- runtime/prometheus/prometheus.go | 2 +- runtime/traces/db.go | 2 +- staticcheck.conf | 17 +++++++++++++++ weaver.go | 8 ++++--- weavertest/deployer.go | 2 +- weavertest/internal/simple/simple.go | 1 - website/blog/deployers/multi/main.go | 10 ++++----- website/blog/deployers/single/main.go | 4 ++-- 27 files changed, 67 insertions(+), 85 deletions(-) delete mode 100644 .golangci.yaml create mode 100644 staticcheck.conf diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index a4d257835..dc2cf1570 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -41,12 +41,12 @@ jobs: - name: Cache linter uses: actions/cache@v3 with: - path: ~/go/bin/golangci-lint - key: golangci-lint-v1.51.2 + path: ~/go/bin/staticcheck + key: staticcheck-v0.4.3 if: ${{ matrix.command == 'lint' }} - name: Install linter - run: go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.51.2 + run: go install honnef.co/go/tools/cmd/staticcheck@v0.4.3 if: ${{ matrix.command == 'lint' }} - name: Build the weaver binary diff --git a/.golangci.yaml b/.golangci.yaml deleted file mode 100644 index 23ec9ec83..000000000 --- a/.golangci.yaml +++ /dev/null @@ -1,28 +0,0 @@ -# Copyright 2022 Google LLC -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -linters: - enable: - - nolintlint - - misspell -issues: - exclude-rules: - - path: '_test\.go' - linters: [errcheck] # Currently too many places are ignoring errors -linters-settings: - staticcheck: - checks: - - all - - '-SA9003' # Flags empty else{}, which is sometimes useful for a comment. - - '-SA1019' # disable noisy deprecated API checks; TODO: fix and re-enable diff --git a/cmd/weaver/main.go b/cmd/weaver/main.go index 020234ef5..edd59f60e 100644 --- a/cmd/weaver/main.go +++ b/cmd/weaver/main.go @@ -77,7 +77,7 @@ func main() { generateFlags.Usage = func() { fmt.Fprintln(os.Stderr, generate.Usage) } - generateFlags.Parse(flag.Args()[1:]) //nolint:errcheck // does os.Exit on error + generateFlags.Parse(flag.Args()[1:]) if err := generate.Generate(".", flag.Args()[1:], generate.Options{}); err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(1) @@ -109,7 +109,7 @@ Description: [2]: https://github.blog/2022-02-14-include-diagrams-markdown-files-mermaid/` flags := flag.NewFlagSet("callgraph", flag.ExitOnError) flags.Usage = func() { fmt.Fprintln(os.Stderr, usage) } - flags.Parse(flag.Args()[1:]) //nolint:errcheck + flags.Parse(flag.Args()[1:]) if flags.NArg() == 0 { fmt.Fprintln(os.Stderr, "ERROR: no binary provided.") } diff --git a/dev/build_and_test b/dev/build_and_test index 84f09cb0d..7d8c435ce 100755 --- a/dev/build_and_test +++ b/dev/build_and_test @@ -10,7 +10,7 @@ # generate -- Generates code # build -- Builds the module # vet -- Runs go vet -# lint -- Runs golangci-lint +# lint -- Runs staticcheck # test -- Tests the module # testrace -- Tests the module for data races # @@ -53,16 +53,16 @@ function cmd_vet() { } function cmd_lint() { - if ! exists golangci-lint; then - printf "golangci-lint not found; install via\ngo install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.51.2\n" >&2 + if ! exists staticcheck; then + printf "staticcheck not found; install via\ngo install honnef.co/go/tools/cmd/staticcheck@0.4.3\n" >&2 exit 1 fi - golangci-lint --timeout=10m run ./... + staticcheck ./... # Run unused check while skipping uses found in tests. This helps us # identify non-test code that is only used from tests. - golangci-lint --timeout=10m run --enable unused --tests=false ./... + staticcheck -tests=false ./... } function cmd_test() { diff --git a/examples/collatz/main.go b/examples/collatz/main.go index 9a3f940ab..002ca0e1a 100644 --- a/examples/collatz/main.go +++ b/examples/collatz/main.go @@ -55,8 +55,6 @@ import ( //go:generate ../../cmd/weaver/weaver generate -var localAddr = flag.String("local_addr", "localhost:9000", "Collatz server local address") - func main() { flag.Parse() if err := weaver.Run(context.Background(), serve); err != nil { diff --git a/internal/benchmarks/benchmarks_test.go b/internal/benchmarks/benchmarks_test.go index 6af75bddf..900eba861 100644 --- a/internal/benchmarks/benchmarks_test.go +++ b/internal/benchmarks/benchmarks_test.go @@ -26,7 +26,6 @@ import ( "io" "math/rand" "testing" - "time" "github.com/ServiceWeaver/weaver/runtime/codegen" "github.com/ServiceWeaver/weaver/weavertest" @@ -362,10 +361,6 @@ func BenchmarkPing(b *testing.B) { } } -func init() { - rand.Seed(time.Now().UnixNano()) -} - // TestBenchmark is a test to ensure that BenchmarkPing works correctly. func TestBenchmark(t *testing.T) { // Test plan: Send a ping request from Component1 to Component10. Verify that diff --git a/internal/envelope/conn/envelope_conn.go b/internal/envelope/conn/envelope_conn.go index fdf9e1d40..734f7461f 100644 --- a/internal/envelope/conn/envelope_conn.go +++ b/internal/envelope/conn/envelope_conn.go @@ -178,7 +178,7 @@ func (e *EnvelopeConn) Serve(h EnvelopeHandler) error { } }) - e.running.Wait() //nolint:errcheck // supplanted by e.err + e.running.Wait() return e.err } @@ -381,7 +381,7 @@ func checkVersion(v *protos.SemVer) error { } got := version.SemVer{Major: int(v.Major), Minor: int(v.Minor), Patch: int(v.Patch)} if got != version.DeployerVersion { - return fmt.Errorf("version mismatch: deployer's deployer API version %s is incompatible with app' deployer API version %s.", version.DeployerVersion, got) + return fmt.Errorf("version mismatch: deployer's deployer API version %s is incompatible with app' deployer API version %s", version.DeployerVersion, got) } return nil } diff --git a/internal/envelope/conn/weavelet_conn.go b/internal/envelope/conn/weavelet_conn.go index c333560b8..f5e50a892 100644 --- a/internal/envelope/conn/weavelet_conn.go +++ b/internal/envelope/conn/weavelet_conn.go @@ -185,7 +185,6 @@ func (w *WeaveletConn) handleMessage(handler WeaveletHandler, msg *protos.Envelo go func() { data, err := Profile(req) // Reply with profile data. - //nolint:errcheck //errMsg will be returned on next send w.conn.send(&protos.WeaveletMsg{ Id: -id, Error: errstring(err), diff --git a/internal/status/dashboard.go b/internal/status/dashboard.go index dc4d17e43..737c75f5e 100644 --- a/internal/status/dashboard.go +++ b/internal/status/dashboard.go @@ -143,7 +143,7 @@ Flags: url := "http://" + lis.Addr().String() fmt.Fprintln(os.Stderr, "Dashboard available at:", url) - go browser.OpenURL(url) //nolint:errcheck // browser open is optional + go browser.OpenURL(url) return http.Serve(lis, nil) }, } @@ -312,7 +312,7 @@ func (d *dashboard) handleMetrics(w http.ResponseWriter, r *http.Request) { var b bytes.Buffer imetrics.TranslateMetricsToPrometheusTextFormat(&b, snapshots, reg.Addr, prometheusEndpoint) - w.Write(b.Bytes()) //nolint:errcheck // response write error + w.Write(b.Bytes()) } // handleTraces handles requests to /traces?id= @@ -403,5 +403,5 @@ func (d *dashboard) handleTraceFetch(w http.ResponseWriter, r *http.Request) { http.Error(w, fmt.Sprintf("cannot encode spans: %v", err), http.StatusInternalServerError) return } - w.Write(data) //nolint:errcheck // response write error + w.Write(data) } diff --git a/internal/status/server.go b/internal/status/server.go index cd52691f7..a517f438f 100644 --- a/internal/status/server.go +++ b/internal/status/server.go @@ -63,6 +63,6 @@ func RegisterServer(mux *http.ServeMux, server Server, logger *slog.Logger) { } var b bytes.Buffer imetrics.TranslateMetricsToPrometheusTextFormat(&b, snapshots, r.Host, prometheusEndpoint) - w.Write(b.Bytes()) //nolint:errcheck // response write error + w.Write(b.Bytes()) }) } diff --git a/internal/tool/generate/example/main.go b/internal/tool/generate/example/main.go index a29f26119..aedcffb8c 100644 --- a/internal/tool/generate/example/main.go +++ b/internal/tool/generate/example/main.go @@ -61,18 +61,18 @@ type B interface { type a struct { weaver.Implements[A] - b weaver.Ref[B] //nolint:unused - lis1 weaver.Listener `weaver:"renamed_listener"` //nolint:unused - lis2 weaver.Listener //nolint:unused + b weaver.Ref[B] //lint:ignore U1000 Present for code generation. + lis1 weaver.Listener `weaver:"renamed_listener"` //lint:ignore U1000 Present for code generation. + lis2 weaver.Listener //lint:ignore U1000 Present for code generation. weaver.WithConfig[config] weaver.WithRouter[router] } type b struct { weaver.Implements[B] - a weaver.Ref[A] //nolint:unused - lis1 weaver.Listener `weaver:"renamed_listener"` //nolint:unused - lis2 weaver.Listener //nolint:unused + a weaver.Ref[A] //lint:ignore U1000 Present for code generation. + lis1 weaver.Listener `weaver:"renamed_listener"` //lint:ignore U1000 Present for code generation. + lis2 weaver.Listener //lint:ignore U1000 Present for code generation. weaver.WithConfig[config] weaver.WithRouter[router] } diff --git a/internal/tool/multi/deployer.go b/internal/tool/multi/deployer.go index 96b275832..9f635beb5 100644 --- a/internal/tool/multi/deployer.go +++ b/internal/tool/multi/deployer.go @@ -272,7 +272,7 @@ func (d *deployer) computeGroups() error { // // REQUIRES: d.mu is NOT held. func (d *deployer) wait() error { - d.running.Wait() //nolint:errcheck // supplanted by b.err + d.running.Wait() d.mu.Lock() defer d.mu.Unlock() return d.err diff --git a/internal/tool/single/deploy.go b/internal/tool/single/deploy.go index 6d54b38be..d118a3c22 100644 --- a/internal/tool/single/deploy.go +++ b/internal/tool/single/deploy.go @@ -91,7 +91,7 @@ func deploy(ctx context.Context, args []string) error { go func() { <-killed if cmd.Process != nil { - cmd.Process.Kill() //nolint:errcheck + cmd.Process.Kill() } os.Exit(1) }() diff --git a/internal/weaver/remoteweavelet.go b/internal/weaver/remoteweavelet.go index 0dcd01680..5e823ff01 100644 --- a/internal/weaver/remoteweavelet.go +++ b/internal/weaver/remoteweavelet.go @@ -533,7 +533,7 @@ func (w *RemoteWeavelet) logger(name string, attrs ...string) *slog.Logger { }, Write: func(entry *protos.LogEntry) { // TODO(mwhittaker): Propagate error. - w.conn.SendLogEntry(entry) //nolint:errcheck + w.conn.SendLogEntry(entry) }, }) } diff --git a/runtime/bin/bin.go b/runtime/bin/bin.go index f76fd705d..823631615 100644 --- a/runtime/bin/bin.go +++ b/runtime/bin/bin.go @@ -33,7 +33,7 @@ import ( // version into a Service Weaver binary. We split declaring and assigning // versionData to prevent the compiler from erasing it. // -//nolint:unused +//lint:ignore U1000 See comment above. var versionData string func init() { diff --git a/runtime/bin/testprogram/main.go b/runtime/bin/testprogram/main.go index d5e20ed37..535f53626 100644 --- a/runtime/bin/testprogram/main.go +++ b/runtime/bin/testprogram/main.go @@ -29,18 +29,18 @@ type C interface{} type app struct { weaver.Implements[weaver.Main] - a weaver.Ref[A] //nolint:unused // intentionally declared but not used - appLis weaver.Listener //nolint:unused // intentionally declared but not used + a weaver.Ref[A] //lint:ignore U1000 intentionally declared but not used + appLis weaver.Listener //lint:ignore U1000 intentionally declared but not used } func (*app) Main(context.Context) error { return nil } type a struct { weaver.Implements[A] - b weaver.Ref[B] //nolint:unused // intentionally declared but not used - c weaver.Ref[C] //nolint:unused // intentionally declared but not used - aLis1, aLis2 weaver.Listener //nolint:unused // intentionally declared but not used - unused weaver.Listener `weaver:"aLis3"` //nolint:unused // intentionally declared but not used + b weaver.Ref[B] //lint:ignore U1000 intentionally declared but not used + c weaver.Ref[C] //lint:ignore U1000 intentionally declared but not used + aLis1, aLis2 weaver.Listener //lint:ignore U1000 intentionally declared but not used + unused weaver.Listener `weaver:"aLis3"` //lint:ignore U1000 intentionally declared but not used } type b struct { diff --git a/runtime/codegen/registry_test.go b/runtime/codegen/registry_test.go index ea4da445c..a0f349fe2 100644 --- a/runtime/codegen/registry_test.go +++ b/runtime/codegen/registry_test.go @@ -110,7 +110,7 @@ type B interface{} type aimpl struct { weaver.Implements[A] - b weaver.Ref[B] //nolint:nolintlint,unused // present just for call graph extraction + b weaver.Ref[B] //lint:ignore U1000 present just for call graph extraction } type bimpl struct { diff --git a/runtime/envelope/envelope.go b/runtime/envelope/envelope.go index c358d7cea..9a2bebc47 100644 --- a/runtime/envelope/envelope.go +++ b/runtime/envelope/envelope.go @@ -236,7 +236,7 @@ func (e *Envelope) Serve(h EnvelopeHandler) error { return err }) - running.Wait() //nolint:errcheck // supplanted by stopErr + running.Wait() // Wait for the weavelet command to finish. This needs to be done after // we're done reading from stdout/stderr pipes, per comments on diff --git a/runtime/metrics/labels_test.go b/runtime/metrics/labels_test.go index 1224e53da..11ff1a487 100644 --- a/runtime/metrics/labels_test.go +++ b/runtime/metrics/labels_test.go @@ -103,7 +103,7 @@ func TestValidLabels(t *testing.T) { func TestInvalidLabels(t *testing.T) { type t1 = string // not a struct - type t2 = struct{ x string } //nolint:unused // unexported field + type t2 = struct{ x string } //lint:ignore U1000 unexported field type t3 = struct { // duplicate field X string `weaver:"Z"` Y string `weaver:"Z"` diff --git a/runtime/prometheus/prometheus.go b/runtime/prometheus/prometheus.go index a9aa9e14e..7583396b8 100644 --- a/runtime/prometheus/prometheus.go +++ b/runtime/prometheus/prometheus.go @@ -213,7 +213,7 @@ func writeLabels(w *bytes.Buffer, labels map[string]string, separator := "{" for _, l := range sortedLabels { w.WriteString(separator + l + `="`) - escaper.WriteString(w, labels[l]) //nolint:errcheck // bytes.Buffer.Write does not error + escaper.WriteString(w, labels[l]) w.WriteByte('"') separator = "," } diff --git a/runtime/traces/db.go b/runtime/traces/db.go index 2b99ef6b8..4d1e85549 100644 --- a/runtime/traces/db.go +++ b/runtime/traces/db.go @@ -130,7 +130,7 @@ func (d *DB) Store(ctx context.Context, app, version string, spans *protos.Trace if err != nil { return err } - defer tx.Rollback() //nolint:errcheck // supplanted by errs below + defer tx.Rollback() var errs []error for _, span := range spans.Span { if isRootSpan(span) { diff --git a/staticcheck.conf b/staticcheck.conf new file mode 100644 index 000000000..9ff09a55f --- /dev/null +++ b/staticcheck.conf @@ -0,0 +1,17 @@ +# In addition to the checks disabled by default [1], we also disable the +# following style checks: +# +# - ST1005 [2]. This check enforces that error messages do not begin with a +# capital letter or end with punctuation. However, we have many error +# messages that begin with exported names or proper nouns (e.g., +# NewEnvelopeConn, Service Weaver). +# - ST1012 [3]. This check enforces that exported errors are prefixed with Err +# (e.g., ErrFoo). weaver.RemoteCallError violates this. +# +# [1]: https://staticcheck.dev/docs/configuration/options/#checks. +# [2]: https://staticcheck.dev/docs/checks/#ST1005 +# [3]: https://staticcheck.dev/docs/checks/#ST1012 +# +# TODO(mwhittaker): Think about enabling some of these checks and updating our +# code accordingly. +checks = ["all", "-ST1000", "-ST1003", "-ST1005", "-ST1012", "-ST1016", "-ST1020", "-ST1021", "-ST1022"] diff --git a/weaver.go b/weaver.go index f896de7c1..97ac1d545 100644 --- a/weaver.go +++ b/weaver.go @@ -232,7 +232,9 @@ type Implements[T any] struct { // The component_interface_type field exists to make it possible. // // [1]: https://github.com/golang/go/issues/54393. - component_interface_type T //nolint:unused + // + //lint:ignore U1000 See comment above. + component_interface_type T // We embed implementsImpl so that component implementation structs // implement the Unrouted interface by default but implement the @@ -263,7 +265,7 @@ func (i *Implements[T]) setLogger(logger *slog.Logger) { // package. It exists so that a component struct that embeds Implements[T] // implements the InstanceOf[T] interface. // -//nolint:unused +//lint:ignore U1000 implements is used by InstanceOf. func (Implements[T]) implements(T) {} // InstanceOf[T] is the interface implemented by a struct that embeds @@ -477,7 +479,7 @@ type WithRouter[T any] struct{} // routedBy(T) implements the RoutedBy[T] interface. // -//nolint:unused +//lint:ignore U1000 routedBy is used by RoutedBy and Unrouted. func (WithRouter[T]) routedBy(T) {} // RoutedBy[T] is the interface implemented by a struct that embeds diff --git a/weavertest/deployer.go b/weavertest/deployer.go index b2e652f96..dae252df8 100644 --- a/weavertest/deployer.go +++ b/weavertest/deployer.go @@ -220,7 +220,7 @@ func (d *deployer) stopLocked(err error) { // cleanup cleans up all of the running envelopes' state. func (d *deployer) cleanup() error { d.ctxCancel() - d.running.Wait() //nolint:errcheck // supplanted by b.err + d.running.Wait() d.mu.Lock() defer d.mu.Unlock() return d.err diff --git a/weavertest/internal/simple/simple.go b/weavertest/internal/simple/simple.go index 1948e6776..0085964a2 100644 --- a/weavertest/internal/simple/simple.go +++ b/weavertest/internal/simple/simple.go @@ -122,7 +122,6 @@ type server struct { } func (s *server) Init(ctx context.Context) error { - //nolint:nolintlint,typecheck // golangci-lint false positive on Go tip s.addr = s.hello.String() s.proxy = s.hello.ProxyAddr() diff --git a/website/blog/deployers/multi/main.go b/website/blog/deployers/multi/main.go index 0fb8f037a..84b355077 100644 --- a/website/blog/deployers/multi/main.go +++ b/website/blog/deployers/multi/main.go @@ -56,8 +56,8 @@ var deploymentId = uuid.New().String() func main() { flag.Parse() d := &deployer{handlers: map[string]*handler{}} - d.spawn(runtime.Main) //nolint:errcheck // omitted for brevity - select {} // block forever + d.spawn(runtime.Main) + select {} // block forever } // spawn spawns a weavelet to host the provided component (if one hasn't @@ -97,10 +97,10 @@ func (d *deployer) spawn(component string) (*handler, error) { go func() { // Inform the weavelet of the component it should host. - envelope.UpdateComponents([]string{component}) //nolint:errcheck // omitted for brevity + envelope.UpdateComponents([]string{component}) // Handle messages from the weavelet. - envelope.Serve(h) //nolint:errcheck // omitted for brevity + envelope.Serve(h) }() // Return the handler. @@ -118,7 +118,7 @@ func (h *handler) ActivateComponent(_ context.Context, req *protos.ActivateCompo } // Tell the weavelet the address of the requested component. - h.envelope.UpdateRoutingInfo(&protos.RoutingInfo{ //nolint:errcheck // omitted for brevity + h.envelope.UpdateRoutingInfo(&protos.RoutingInfo{ Component: req.Component, Replicas: []string{spawned.address}, }) diff --git a/website/blog/deployers/single/main.go b/website/blog/deployers/single/main.go index 1369122d0..12f5a209b 100644 --- a/website/blog/deployers/single/main.go +++ b/website/blog/deployers/single/main.go @@ -75,10 +75,10 @@ func (d *deployer) ActivateComponent(_ context.Context, req *protos.ActivateComp d.components = append(d.components, req.Component) // Tell the weavelet to run the component. - d.envelope.UpdateComponents(d.components) //nolint:errcheck // omitted for brevity + d.envelope.UpdateComponents(d.components) // Tell the weavelet to route requests to the component locally. - d.envelope.UpdateRoutingInfo(&protos.RoutingInfo{ //nolint:errcheck // omitted for brevity + d.envelope.UpdateRoutingInfo(&protos.RoutingInfo{ Component: req.Component, Local: true, })