Merge pull request juju#18822 from jack-w-shaw/add_new_linters

juju#18822

Most gocritic linters are too opinionated or specific to really be useful. However, there are a few I think are worth enabling:

- badLock: This detects bad lock usage. I.e. if you lock, and then immediately unlock a mutex without doing anything. In this case, it is likely a developer forgot a defer.

- boolExprSimpligy: This applies some simple boolean arithmetic rules to out bool expressions, and suggests simplifications. I was a little suspicious of this, but the cases it highlighted were pretty egregious, so I thought worth including

- captLocal: This fails if any local variables begin with capitals. None were found, but I have seen this a little in Juju and other go code, so worth including I think.

- caseOrder: This fails if a switch-case statement is ordered in such a way to make some cases unreachable. This is a pretty sure bug, which is why I have included this linters

- deprecatedComment: This detects invalidly formatted deprecation docstrings. There were a few throughout Juju that I have fixed.

- docStub. This fails if a docString is a stub. Write a more detailed docString!

- equalFold: use EqualFold to compare strings in a case insensitive way. This is the recommended way of doing this.

- evalOrder: catches potential depedency on the order of evaluation of return values. This can cause bugs

## QA steps

static analysis passes

.github/golangci-lint.config.yaml

@@ -33,7 +33,15 @@ linters-settings:
   gocritic:
     disable-all: true
     enabled-checks:
+      - badLock
+      - boolExprSimplify
+      - captLocal
+      - caseOrder
+      - deprecatedComment
       - dupImport
+      - docStub
+      - equalFold
+      - evalOrder
   revive:
     rules:
       - name: unexported-naming

apiserver/authentication/user_test.go

@@ -415,7 +415,8 @@ func (s *mockBakeryService) NewMacaroon(ctx context.Context, version bakery.Vers
 
 func (s *mockBakeryService) ExpireStorageAfter(t time.Duration) (authentication.ExpirableStorageBakery, error) {
 	s.MethodCall(s, "ExpireStorageAfter", t)
-	return s, s.NextErr()
+	err := s.NextErr()
+	return s, err
 }
 
 type mockAuthorizer struct{}

apiserver/common/storagecommon/volumes.go

@@ -85,7 +85,8 @@ func StoragePoolConfig(ctx context.Context, name string, storagePoolGetter Stora
 	} else if err != nil {
 		return "", nil, errors.Annotatef(err, "getting pool %q", name)
 	}
-	return pool.Provider(), pool, nil
+	provider := pool.Provider()
+	return provider, pool, nil
 }
 
 // VolumesToState converts a slice of params.Volume to a mapping

apiserver/export_test.go

@@ -161,7 +161,8 @@ func TestingAPIHandler(c *gc.C, pool *state.StatePool, st *state.State, sf servi
 	)
 	c.Assert(err, jc.ErrorIsNil)
 
-	return h, h.Resources()
+	resources := h.Resources()
+	return h, resources
 }
 
 type StubDomainServicesGetter struct{}

apiserver/facades/client/modelmanager/modelinfo_test.go

@@ -1149,7 +1149,8 @@ func (st *mockState) AllModelUUIDs() ([]string, error) {
 
 func (st *mockState) GetBackend(modelUUID string) (common.ModelManagerBackend, func() bool, error) {
 	st.MethodCall(st, "GetBackend", modelUUID)
-	return st, func() bool { return true }, st.NextErr()
+	err := st.NextErr()
+	return st, func() bool { return true }, err
 }
 
 func (st *mockState) GetModel(modelUUID string) (common.Model, func() bool, error) {
@@ -1175,7 +1176,8 @@ func (st *mockState) AllFilesystems() ([]state.Filesystem, error) {
 func (st *mockState) NewModel(args state.ModelArgs) (common.Model, common.ModelManagerBackend, error) {
 	st.MethodCall(st, "NewModel", args)
 	st.model.tag = names.NewModelTag(args.Config.UUID())
-	return st.model, st, st.NextErr()
+	err := st.NextErr()
+	return st.model, st, err
 }
 
 func (st *mockState) ControllerTag() names.ControllerTag {

caas/kubernetes/cloud/cloud.go

@@ -116,7 +116,8 @@ func CloudFromKubeConfigContext(
 		return newCloud, errors.NotFoundf("kubernetes cluster %q associated with context %q",
 			context.Cluster, ctxName)
 	}
-	return newCloud, buildCloudFromCluster(&newCloud, cluster)
+	err := buildCloudFromCluster(&newCloud, cluster)
+	return newCloud, err
 }
 
 // CloudFromKubeConfigContextReader constructs a Juju cloud object using the
@@ -156,8 +157,8 @@ func CloudFromKubeConfigCluster(
 	if !exists {
 		return newCloud, errors.NotFoundf("kubernetes cluster %q not found", clusterName)
 	}
-
-	return newCloud, buildCloudFromCluster(&newCloud, cluster)
+	err := buildCloudFromCluster(&newCloud, cluster)
+	return newCloud, err
 }
 
 // CloudFromKubeConfigClusterReader attempts to construct a Juju cloud object

cmd/juju/cloud/addcredential_test.go

@@ -82,7 +82,8 @@ func (s *addCredentialSuite) runCmd(c *gc.C, stdin io.Reader, args ...string) (*
 	}
 	ctx := cmdtesting.Context(c)
 	ctx.Stdin = stdin
-	return ctx, addCmd, addCmd.Run(ctx)
+	err = addCmd.Run(ctx)
+	return ctx, addCmd, err
 }
 
 func (s *addCredentialSuite) run(c *gc.C, stdin io.Reader, args ...string) (*cmd.Context, error) {

cmd/juju/crossmodel/show_test.go

@@ -74,7 +74,7 @@ func (s *showSuite) TestShowNameOnly(c *gc.C) {
 func (s *showSuite) TestShowNameAndEnvvarOnly(c *gc.C) {
 	// Ensure envvar (fred/model) overrides CurrentModel (fred/test)
 	os.Setenv(osenv.JujuModelEnvKey, "fred/model")
-	defer func() { os.Unsetenv(osenv.JujuModelEnvKey) }()
+	defer func() { _ = os.Unsetenv(osenv.JujuModelEnvKey) }()
 	s.assertShowYaml(c, "db2")
 }
 

cmd/juju/storage/attach_test.go

@@ -94,7 +94,8 @@ type fakeEntityAttacher struct {
 
 func (f *fakeEntityAttacher) new(ctx context.Context) (storage.EntityAttacherCloser, error) {
 	f.MethodCall(f, "NewEntityAttacherCloser")
-	return f, f.NextErr()
+	err := f.NextErr()
+	return f, err
 }
 
 func (f *fakeEntityAttacher) Close() error {

cmd/juju/storage/detach_test.go

@@ -85,7 +85,8 @@ type fakeEntityDetacher struct {
 
 func (f *fakeEntityDetacher) new(ctx context.Context) (storage.EntityDetacherCloser, error) {
 	f.MethodCall(f, "NewEntityDetacherCloser")
-	return f, f.NextErr()
+	err := f.NextErr()
+	return f, err
 }
 
 func (f *fakeEntityDetacher) Close() error {

cmd/juju/storage/remove_test.go

@@ -145,7 +145,8 @@ type fakeStorageRemover struct {
 
 func (f *fakeStorageRemover) new(ctx context.Context) (storage.StorageRemoverCloser, error) {
 	f.MethodCall(f, "NewStorageRemoverCloser")
-	return f, f.NextErr()
+	err := f.NextErr()
+	return f, err
 }
 
 func (f *fakeStorageRemover) Close() error {

domain/network/service/interface.go

@@ -67,7 +67,7 @@ type SubnetState interface {
 	// GetSubnet returns the subnet by UUID.
 	GetSubnet(ctx context.Context, uuid string) (*network.SubnetInfo, error)
 	// GetSubnetsByCIDR returns the subnets by CIDR.
-	// Deprecated, this method should be removed when we re-work the API
+	// Deprecated: this method should be removed when we re-work the API
 	// for moving subnets.
 	GetSubnetsByCIDR(ctx context.Context, cidrs ...string) (network.SubnetInfos, error)
 	// UpdateSubnet updates the subnet identified by the passed uuid.

domain/network/state/subnet.go

@@ -333,7 +333,8 @@ WHERE  subnet_uuid = $M.id;`
 }
 
 // GetSubnetsByCIDR returns the subnets by CIDR.
-// Deprecated, this method should be removed when we re-work the API for moving
+//
+// Deprecated: this method should be removed when we re-work the API for moving
 // subnets.
 func (st *State) GetSubnetsByCIDR(
 	ctx context.Context,

internal/cmd/cmd.go

@@ -187,7 +187,7 @@ func (ctx *Context) Errorf(format string, params ...interface{}) {
 // WriteError will output the formatted text to the writer with
 // a colored ERROR like the logging would.
 //
-// DEPRECATED: Use ctx.Errorf instead
+// Deprecated: Use ctx.Errorf instead
 func WriteError(writer io.Writer, err error) {
 	w := ansiterm.NewWriter(writer)
 	ansiterm.Foreground(ansiterm.BrightRed).Fprintf(w, "ERROR")
@@ -315,7 +315,7 @@ func (i *Info) HelpWithSuperFlags(superF *gnuflag.FlagSet, f *gnuflag.FlagSet) [
 		filteredSuperF := gnuflag.NewFlagSetWithFlagKnownAs("", gnuflag.ContinueOnError, superF.FlagKnownAs)
 		contains := func(one string) bool {
 			for _, a := range i.ShowSuperFlags {
-				if strings.ToLower(one) == strings.ToLower(a) {
+				if strings.EqualFold(one, a) {
 					return true
 				}
 			}

internal/cmd/supercommand.go

@@ -648,9 +648,9 @@ func (c *SuperCommand) FindClosestSubCommand(name string) (string, Command, bool
 	matchedName := matches[0].Name
 	matchedValue := matches[0].Value
 
-	// If the matched value is less than the length+1 of the string, fail the
+	// If the matched value is less or equal to the length of the string, fail the
 	// match.
-	if _, ok := c.subcmds[matchedName]; ok && matchedName != "" && matchedValue < len(matchedName)+1 {
+	if _, ok := c.subcmds[matchedName]; ok && matchedName != "" && matchedValue <= len(matchedName) {
 		return matchedName, c.subcmds[matchedName].command, true
 	}
 	return "", nil, false

internal/errors/std.go

@@ -59,7 +59,8 @@ func As(err error, target any) bool {
 //	}
 func AsType[T error](err error) (T, bool) {
 	var zero T
-	return zero, As(err, &zero)
+	as := As(err, &zero)
+	return zero, as
 }
 
 // Errorf implements a straight through proxy for [pkg/fmt.Errorf]. The one

internal/provider/lxd/environ_broker_test.go

@@ -85,7 +85,7 @@ func (s *environBrokerSuite) TestStartInstanceDefaultNIC(c *gc.C) {
 		if spec.Config[containerlxd.NetworkConfigKey] != "" {
 			return false
 		}
-		return !(len(spec.Devices) > 0)
+		return len(spec.Devices) == 0
 	}
 
 	exp := svr.EXPECT()

internal/worker/uniter/relation/export_test.go

@@ -44,8 +44,8 @@ func NewStateTrackerForTest(c *gc.C, cfg StateTrackerForTestConfig) (RelationSta
 		logger:          loggertesting.WrapCheckLogWithLevel(c, logger.DEBUG),
 		newRelationer:   cfg.NewRelationerFunc,
 	}
-
-	return rst, rst.loadInitialState(stdcontext.Background())
+	err := rst.loadInitialState(stdcontext.Background())
+	return rst, err
 }
 
 func NewStateTrackerForSyncScopesTest(c *gc.C, cfg StateTrackerForTestConfig) (RelationStateTracker, error) {

internal/worker/uniter/relation/statemanager.go

@@ -21,7 +21,8 @@ import (
 // NewStateManager creates a new StateManager instance.
 func NewStateManager(ctx context.Context, rw UnitStateReadWriter, logger logger.Logger) (StateManager, error) {
 	mgr := &stateManager{unitStateRW: rw, logger: logger}
-	return mgr, mgr.initialize(ctx)
+	err := mgr.initialize(ctx)
+	return mgr, err
 }
 
 type stateManager struct {

internal/worker/uniter/util_test.go

@@ -2032,7 +2032,7 @@ type provisionStorage struct{}
 
 func (s provisionStorage) step(c *gc.C, ctx *testContext) {
 	ctx.stateMu.Lock()
-	ctx.stateMu.Unlock()
+	defer ctx.stateMu.Unlock()
 	for si := range ctx.storage {
 		ctx.storage[si].attached = true
 	}

juju/testing/apiserver.go

@@ -152,7 +152,8 @@ type ApiServerSuite struct {
 	ControllerUUID string
 
 	// InstancePrechecker is used to validate instance creation.
-	// DEPRECATED: This will be removed in the future.
+	//
+	// Deprecated: This will be removed in the future.
 	InstancePrechecker func(*gc.C, *state.State) environs.InstancePrechecker
 }