golangci-lint: enable nakedret linter by thaJeztah · Pull Request #49493 · moby/moby

thaJeztah · 2025-02-19T09:26:55Z

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

robmry

LGTM

robmry · 2025-03-04T10:30:16Z

-	return
+	logDriver, err := container.StartLogger()
+	if err != nil {
+		return nil, false, err


In the original, created was set before trying to start the logger, and not cleared when it failed.

The equivalent here would be nil, true, err ... I'm not sure if that was a deliberate change - but there are only two callers, and they both ignore created if err != nil. So, it's fine.

Thanks! Hm.. not sure if it was entirely intentional, but I may have thought "what purpose does it have if it failed?", but maybe I should add it back.

Code flow in this area looks slightly dubious though, because StartLogger both creates (if needed) and starts the logger. But if anything fails in that process, it returns nil for the logger.

I wonder if there's resources leaked there if it was created successfully, but failed to start? 🤔

vvoland

LGTM

vvoland · 2025-03-04T10:42:50Z

+	if err == nil {
+		ipNet.IP = ip
 	}
-	return
+	return ipNet, err


nit: I think it would be more "idiomatic" to reverse the flow (handle err != nil first).

Good call; not sure why I didn't do so. I just had a quick peek at the Go code, and it always returns nil, nil, err if there's an error, so there's no case where ip would be set and ipNet not (or vice-versa), so we should just return nil on error; https://github.com/golang/go/blob/go1.23.6/src/net/ip.go#L519-L537

vvoland · 2025-03-04T10:46:57Z

+func getInodeFromStat(stat interface{}) (inode uint64) {
 	s, ok := stat.(*syscall.Stat_t)
-
-	if ok {
-		inode = s.Ino
+	if !ok {
+		return 0
 	}
-
-	return
+	return s.Ino


Looks like it should return a non-nil error if !ok. Perhaps let's just keep it for now and handle the bugfix in a follow up?

Hm; yes, I may have looked here and thought "what error to return?"; but looking at the getFileUIDGID code below, it looks like that had a similar issue, and there we just return our own error.

Let me adjust this patch;

keep the error return (for now) but always return nil

add a FIXME to look into returning an error

That way we don't have the code-churn on the caller side, which did handle the error before this (but of course it was always nil)

thaJeztah · 2025-03-04T12:16:20Z

Updated with those suggestions 👍

thaJeztah · 2025-03-04T12:54:11Z

Derp; missed updating a windows file somewhere

15.06 pkg/archive/archive_windows.go:53:9: not enough return values
15.06 	have (number)
15.06 	want (uint64, error)
56.63 + rm -f /go/src/github.com/docker/docker/go.mod
------

- use explicit returns - rename error-return that's used in a defer - move closing the tmpFile on error to a defer - add debug logs for cases where either closing the file, or failing to remove the temp-directory builder/dockerfile/copy.go:369:3: naked return in func `downloadSource` with 67 lines of code (nakedret) return ^ builder/dockerfile/copy.go:374:3: naked return in func `downloadSource` with 67 lines of code (nakedret) return ^ builder/dockerfile/copy.go:382:3: naked return in func `downloadSource` with 67 lines of code (nakedret) return ^ builder/dockerfile/copy.go:398:3: naked return in func `downloadSource` with 67 lines of code (nakedret) return ^ builder/dockerfile/copy.go:407:3: naked return in func `downloadSource` with 67 lines of code (nakedret) return ^ builder/dockerfile/copy.go:428:3: naked return in func `downloadSource` with 67 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

- remove/rename vars that shadowed - suppress some unhandled errors - remove naked return Signed-off-by: Sebastiaan van Stijn <[email protected]>

builder/remotecontext/detect.go:47:2: naked return in func `Detect` with 17 lines of code (nakedret) return ^ builder/remotecontext/archive.go:127:2: naked return in func `normalize` with 7 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

Remove output variables, and use explicit returns container/stream/bytespipe/bytespipe.go:165:2: naked return in func `Read` with 37 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

Remove output variables, and use explicit returns integration-cli/check_test.go:86:3: naked return in func `testRun` with 43 lines of code (nakedret) return ^ integration-cli/check_test.go:97:3: naked return in func `testRun` with 43 lines of code (nakedret) return ^ integration-cli/docker_cli_cp_utils_test.go:167:2: naked return in func `makeTestContainer` with 44 lines of code (nakedret) return ^ integration-cli/docker_api_attach_test.go:299:3: naked return in func `readTimeout` with 12 lines of code (nakedret) return ^ integration-cli/docker_cli_cp_utils_test.go:215:2: naked return in func `startContainerGetOutput` with 11 lines of code (nakedret) return ^ integration-cli/docker_cli_logs_test.go:276:4: naked return in func `ConsumeWithSpeed` with 18 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

internal/mod/mod.go:22:3: naked return in func `moduleVersion` with 38 lines of code (nakedret) return ^ internal/mod/mod.go:36:4: naked return in func `moduleVersion` with 38 lines of code (nakedret) return ^ internal/mod/mod.go:57:2: naked return in func `moduleVersion` with 38 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

In fairness, these could possibly be an exception to the rule, but adding explicit returns isn't too bad either, and allows running the nakedret linter without //nolint tags or exceptions in .golangci-lint.yaml Signed-off-by: Sebastiaan van Stijn <[email protected]>

layer/layer_store.go:524:4: naked return in func `CreateRWLayer` with 72 lines of code (nakedret) return ^ layer/layer_store.go:534:3: naked return in func `CreateRWLayer` with 72 lines of code (nakedret) return ^ layer/layer_store.go:537:3: naked return in func `CreateRWLayer` with 72 lines of code (nakedret) return ^ layer/migration.go:19:3: naked return in func `ChecksumForGraphID` with 28 lines of code (nakedret) return ^ layer/migration.go:25:3: naked return in func `ChecksumForGraphID` with 28 lines of code (nakedret) return ^ layer/migration.go:36:3: naked return in func `ChecksumForGraphID` with 28 lines of code (nakedret) return ^ layer/migration.go:40:3: naked return in func `ChecksumForGraphID` with 28 lines of code (nakedret) return ^ layer/migration.go:43:2: naked return in func `ChecksumForGraphID` with 28 lines of code (nakedret) return ^ layer/ro_layer.go:176:2: naked return in func `Read` with 13 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

libnetwork/cmd/networkdb-test/dbclient/ndbClient.go:251:5: naked return in func `checkTable` with 42 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

libnetwork/drivers/overlay/encryption.go:370:2: naked return in func `programSA` with 64 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

libnetwork/osl/namespace_linux.go:324:2: naked return in func `DisableARPForVIP` with 28 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

libnetwork/types/types.go:330:2: naked return in func `ParseCIDR` with 6 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

libnetwork/drivers/remote/driver_test.go:29:2: naked return in func `decodeToMap` with 3 lines of code (nakedret) return ^ libnetwork/ipams/remote/remote_test.go:23:2: naked return in func `decodeToMap` with 3 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

pkg/archive/archive_linux.go:65:2: naked return in func `ConvertWrite` with 43 lines of code (nakedret) return ^ pkg/archive/archive.go:265:2: naked return in func `Read` with 11 lines of code (nakedret) return ^ pkg/archive/copy.go:32:2: naked return in func `copyWithBuffer` with 5 lines of code (nakedret) return ^ pkg/archive/copy.go:114:3: naked return in func `TarResourceRebase` with 16 lines of code (nakedret) return ^ pkg/archive/copy.go:449:4: naked return in func `ResolveHostSourcePath` with 26 lines of code (nakedret) return ^ pkg/archive/copy.go:460:4: naked return in func `ResolveHostSourcePath` with 26 lines of code (nakedret) return ^ pkg/archive/wrap.go:58:2: naked return in func `parseStringPairs` with 11 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

pkg/archive/copy_unix_test.go:54:3: naked return in func `fileContentsEqual` with 35 lines of code (nakedret) return ^ pkg/archive/copy_unix_test.go:60:3: naked return in func `fileContentsEqual` with 35 lines of code (nakedret) return ^ pkg/archive/copy_unix_test.go:67:3: naked return in func `fileContentsEqual` with 35 lines of code (nakedret) return ^ pkg/archive/copy_unix_test.go:74:3: naked return in func `fileContentsEqual` with 35 lines of code (nakedret) return ^ pkg/archive/copy_unix_test.go:83:2: naked return in func `fileContentsEqual` with 35 lines of code (nakedret) return ^ pkg/archive/diff_test.go:314:3: naked return in func `makeTestLayer` with 35 lines of code (nakedret) return ^ pkg/archive/diff_test.go:326:5: naked return in func `makeTestLayer` with 35 lines of code (nakedret) return ^ pkg/archive/diff_test.go:330:5: naked return in func `makeTestLayer` with 35 lines of code (nakedret) return ^ pkg/archive/diff_test.go:336:3: naked return in func `makeTestLayer` with 35 lines of code (nakedret) return ^ pkg/archive/copy_unix_test.go:36:2: naked return in func `getTestTempDirs` with 10 lines of code (nakedret) return ^ pkg/stdcopy/stdcopy_test.go:93:3: naked return in func `getSrcBuffer` with 10 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>

pkg/pools/pools.go:83:2: naked return in func `Copy` with 5 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

pkg/stdcopy/stdcopy.go:68:2: naked return in func `Write` with 23 lines of code (nakedret) return ^ pkg/stdcopy/stdcopy_test.go:93:3: naked return in func `getSrcBuffer` with 10 lines of code (nakedret) return ^ pkg/stdcopy/stdcopy_test.go:97:2: naked return in func `getSrcBuffer` with 10 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

pkg/tarsum/versioning.go:150:2: naked return in func `v1TarHeaderSelect` with 35 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

plugin/backend_linux.go:722:3: naked return in func `CreateFromContext` with 112 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

registry/service.go:83:4: naked return in func `Auth` with 38 lines of code (nakedret) return ^ registry/search_session.go:91:2: naked return in func `Read` with 6 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

Remove output variables, and use explicit returns daemon/stats_unix.go:359:2: naked return in func `getSystemCPUUsage` with 38 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

daemon/daemon.go:1570:2: naked return in func `RemapContainerdNamespaces` with 21 lines of code (nakedret) return ^ daemon/daemon_linux.go:128:2: naked return in func `getCleanPatterns` with 14 lines of code (nakedret) return ^ daemon/logs.go:180:2: naked return in func `getLogger` with 11 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

daemon/cluster/cluster.go:423:2: naked return in func `managerStats` with 24 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

daemon/graphdriver/fsdiff.go:140:3: naked return in func `ApplyDiff` with 20 lines of code (nakedret) return ^ daemon/graphdriver/fsdiff.go:149:3: naked return in func `ApplyDiff` with 20 lines of code (nakedret) return ^ daemon/graphdriver/fsdiff.go:153:2: naked return in func `ApplyDiff` with 20 lines of code (nakedret) return ^ daemon/graphdriver/fsdiff.go:164:3: naked return in func `DiffSize` with 15 lines of code (nakedret) return ^ daemon/graphdriver/fsdiff.go:169:3: naked return in func `DiffSize` with 15 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

…ctor daemon/graphdriver/fuse-overlayfs/fuseoverlayfs.go:527:2: naked return in func `fusermountU` with 25 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

daemon/logger/proxy.go:35:3: naked return in func `StartLogging` with 17 lines of code (nakedret) return ^ daemon/logger/proxy.go:42:2: naked return in func `StartLogging` with 17 lines of code (nakedret) return ^ daemon/logger/proxy.go:61:3: naked return in func `StopLogging` with 16 lines of code (nakedret) return ^ daemon/logger/proxy.go:68:2: naked return in func `StopLogging` with 16 lines of code (nakedret) return ^ daemon/logger/proxy.go:80:3: naked return in func `Capabilities` with 14 lines of code (nakedret) return ^ daemon/logger/proxy.go:89:2: naked return in func `Capabilities` with 14 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

daemon/logger/awslogs/cloudwatchlogs.go:684:2: naked return in func `findValidSplit` with 10 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

image/tarexport/load.go:429:2: naked return in func `validatedParentLinks` with 12 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

api/server/router/build/build_routes.go:359:2: naked return in func `Write` with 5 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

api/types/registry/registry.go:60:2: naked return in func `UnmarshalJSON` with 9 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah added status/2-code-review area/testing kind/refactor PR's that refactor, or clean-up code labels Feb 19, 2025

thaJeztah self-assigned this Feb 19, 2025

thaJeztah marked this pull request as ready for review February 26, 2025 15:02

thaJeztah requested review from cpuguy83, samuelkarp and tonistiigi as code owners February 26, 2025 15:02

thaJeztah added this to the 28.0.2 milestone Mar 3, 2025

thaJeztah requested review from robmry and vvoland March 3, 2025 21:35

robmry approved these changes Mar 4, 2025

View reviewed changes

vvoland approved these changes Mar 4, 2025

View reviewed changes

thaJeztah force-pushed the enable_nakedret branch from 6f0535d to ba30a08 Compare March 4, 2025 12:16

thaJeztah added 14 commits March 4, 2025 13:55

builder/dockerfile: fix minor linting issues

0c1b37c

- remove/rename vars that shadowed - suppress some unhandled errors - remove naked return Signed-off-by: Sebastiaan van Stijn <[email protected]>

container/stream/bytespipe: fix naked returns, output vars

220b3c5

Remove output variables, and use explicit returns container/stream/bytespipe/bytespipe.go:165:2: naked return in func `Read` with 37 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

libnetwork/cmd/networkdb-test: fix naked return

94afddb

libnetwork/cmd/networkdb-test/dbclient/ndbClient.go:251:5: naked return in func `checkTable` with 42 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

libnetwork/drivers/overlay: fix naked returns, output variables

02b4c7c

libnetwork/drivers/overlay/encryption.go:370:2: naked return in func `programSA` with 64 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

libnetwork/osl: fix naked returns

619e8f8

libnetwork/osl/namespace_linux.go:324:2: naked return in func `DisableARPForVIP` with 28 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

libnetwork/types: fix naked returns

8978b30

libnetwork/types/types.go:330:2: naked return in func `ParseCIDR` with 6 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah added 18 commits March 4, 2025 13:56

pkg/chrootarchive: remove unused named return

52b8298

Signed-off-by: Sebastiaan van Stijn <[email protected]>

pkg/pools: fix naked returns

d3d84bd

pkg/pools/pools.go:83:2: naked return in func `Copy` with 5 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

pkg/tarsum: fix naked returns

faa9cb1

pkg/tarsum/versioning.go:150:2: naked return in func `v1TarHeaderSelect` with 35 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

plugin: fix naked returns, output vars

17448ef

plugin/backend_linux.go:722:3: naked return in func `CreateFromContext` with 112 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

daemon: getSystemCPUUsage(): fix naked returns, output vars

85c8fb7

Remove output variables, and use explicit returns daemon/stats_unix.go:359:2: naked return in func `getSystemCPUUsage` with 38 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

daemon/cluster: fix naked returns

964413c

daemon/cluster/cluster.go:423:2: naked return in func `managerStats` with 24 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

daemon/graohdriver/fuse-overlayfs/: fix naked returns and slight refa…

eeb5651

…ctor daemon/graphdriver/fuse-overlayfs/fuseoverlayfs.go:527:2: naked return in func `fusermountU` with 25 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

daemon/logger/awslogs: remove unused named return

99b6012

daemon/logger/awslogs/cloudwatchlogs.go:684:2: naked return in func `findValidSplit` with 10 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

image/tarexport: fix naked returns and slight refactor

4aecdd5

image/tarexport/load.go:429:2: naked return in func `validatedParentLinks` with 12 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

api/server/router/build: fix naked returns

b1c008c

api/server/router/build/build_routes.go:359:2: naked return in func `Write` with 5 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

api/types/registry: fix naked returns

c359cc6

api/types/registry/registry.go:60:2: naked return in func `UnmarshalJSON` with 9 lines of code (nakedret) return ^ Signed-off-by: Sebastiaan van Stijn <[email protected]>

golangci-lint: enable nakedret linter

a817861

Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the enable_nakedret branch from ba30a08 to a817861 Compare March 4, 2025 12:56

thaJeztah merged commit 113c296 into moby:master Mar 4, 2025

thaJeztah deleted the enable_nakedret branch March 4, 2025 23:38

corhere mentioned this pull request Jul 28, 2025

[25.0] libnetwork/overlay: backport all the fixes #50551

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

golangci-lint: enable nakedret linter#49493

golangci-lint: enable nakedret linter#49493
thaJeztah merged 32 commits intomoby:masterfrom
thaJeztah:enable_nakedret

thaJeztah commented Feb 19, 2025

Uh oh!

robmry left a comment

Uh oh!

robmry Mar 4, 2025

Uh oh!

thaJeztah Mar 4, 2025

Uh oh!

vvoland left a comment

Uh oh!

vvoland Mar 4, 2025

Uh oh!

thaJeztah Mar 4, 2025

Uh oh!

vvoland Mar 4, 2025

Uh oh!

thaJeztah Mar 4, 2025

Uh oh!

thaJeztah commented Mar 4, 2025

Uh oh!

thaJeztah commented Mar 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

thaJeztah commented Feb 19, 2025

Uh oh!

robmry left a comment

Choose a reason for hiding this comment

Uh oh!

robmry Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

vvoland Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

vvoland Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 4, 2025

Uh oh!

thaJeztah commented Mar 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants