gocritic->filepathJoin - Detects problems in filepath.Join() function calls. Should we enable it?

[zak-pawel]

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: filepathJoin - Detects problems in filepath.Join() function calls.

Example

Before:

filepath.Join("dir/", filename)

After:

filepath.Join("dir", filename)

Expected output

Decision about enabling or not enabling this checker.

Findings

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

config/config_test.go:1078:49                                gocritic  filepathJoin: "./testdata/processor_order" contains a path separator
internal/globpath/globpath_test.go:45:37                     gocritic  filepathJoin: "log\\[!*" contains a path separator
internal/globpath/globpath_test.go:47:37                     gocritic  filepathJoin: "log\\[^*" contains a path separator
plugins/inputs/diskio/diskio_linux.go:118:54                 gocritic  filepathJoin: "/dev" contains a path separator
plugins/inputs/file/file_test.go:30:37                       gocritic  filepathJoin: "dev/testfiles/**.log" contains a path separator
plugins/inputs/file/file_test.go:45:39                       gocritic  filepathJoin: "dev/testfiles/json_a.log" contains a path separator
plugins/inputs/file/file_test.go:70:37                       gocritic  filepathJoin: "dev/testfiles/json_a.log" contains a path separator
plugins/inputs/file/file_test.go:89:37                       gocritic  filepathJoin: "dev/testfiles/grok_a.log" contains a path separator
plugins/inputs/powerdns_recursor/powerdns_recursor.go:47:31  gocritic  filepathJoin: "/" contains a path separator
plugins/inputs/prometheus/kubernetes.go:66:42                gocritic  filepathJoin: ".kube/config" contains a path separator
plugins/inputs/system/ps.go:175:27                           gocritic  filepathJoin: "/" contains a path separator
testutil/socket.go:22:25                                     gocritic  filepathJoin: "/tmp" contains a path separator

Additional configuration

For this checker, no additional configuration can be provided.

[zak-pawel]

I would review current findings and enable this checker.

[Hipska]

The findings seem correct uses to me?

[powersj]

-1 the findings all seem to line up with valid use cases

[srebhan]

I think that

• testutil/socket.go:22:25 should use os.MkdirTemp
• plugins/inputs/system/ps.go:175:27 should maybe use os.PathSeparator as it might fail on Windows otherwise
• plugins/inputs/prometheus/kubernetes.go:66:42 is a clear true-positive
• plugins/inputs/powerdns_recursor/powerdns_recursor.go:47:31 should maybe just have a fixed /var/run string
• plugins/inputs/diskio/diskio_linux.go:118:54 can just do string concatenation as it's only built for Linux

Also for the test-findings, it might not hurt to construct the paths using filepath.Join instead of doing this. I even don't know why this works on Windows machines...

With all this being said, I think there is a point in enabling this linter!