v1.53.x invisibly breaks configs that were valid in v1.52.x if they used depguard

[natefinch]

Welcome

• Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
• Yes, I've searched similar issues on GitHub and didn't find any.
• Yes, I've included all information below (version, config, etc.).
• Yes, I've tried with the standalone linter if available (e.g., gocritic, go vet, etc.). (https://golangci-lint.run/usage/linters/)

Description of the problem

GitHub's current depguard config looks like this:

[linters-settings.depguard]
    # ban some modules with replacements
    list-type = "blacklist"
    include-go-root = true
    packages = [ 
        # we shouldn't use pkg/error anymore
        "github.com/pkg/error",
    ]

	[[linters-settings.depguard.packages-with-error-message]]
	    "github.com/pkg/error" = "Please use stdlib errors module"

In v1.52, this worked fine.

In v1.53, it is ignored, because the config for depguard changed completely, and instead the default config is used. So now we get depguard errors in code that didn't have them previously.

Solution Discussion

golangci-lint really needs to prioritize backwards compatibility with existing configurations. Updating the binary shouldn't invalidate an existing config in almost any case. I personally spent dozens of hours tweaking GitHub's golangci-lint configuration file. It really undermines the trust we put in this product to know that it could break without warning whenever anyone installs a new version of golangci-lint.

I think there needs to be a policy around backwards compatibility in golangci-lint configurations, so that large groups of developers know how to manage their configurations and golangci-lint updates.

Ideally, every new version of golangci-lint would be backwards compatible with existing configurations for basically forever. and by compatible I mean, if all I do is update golangci-lint, the linters I have turned on and the configurations for them should stay the same and produce the same errors, plus or minus linters that fixed bugs to find problems they previously missed.

In circumstances where it is impossible to maintain backwards compatibility, such as when an old version of a linter is discontinued and the new version has completely different configuration parameters, there should be a transition period to warn people that they need to upgrade before their config breaks.

My suggestion would be to have a few months of releases where the old config and the new config both work. During that time, using the old config should print out a warning, so that people know they need to update, and the new config should default to off, so it doesn't break if not set. Only after people have had plenty of time to update do you then turn off the old config and require the new config.

The cleanest way to do that, I think, is to have different linter names for the old version and the new version. so, like, keep the old config under depguard and make the new one under depguard_v2 or something.

Obviously there could be other solutions, but I do think there needs to be careful thought about upgrade paths when the configuration is going to change.

Version of golangci-lint

% golangci-lint --version 
golangci-lint has version 1.53.2 built with go1.20.4 from 59a7aaf7 on 2023-06-03T15:04:31Z

Configuration file

relevant part below

```toml [linters-settings.depguard] # ban some modules with replacements list-type = "blacklist" include-go-root = true packages = [ # we shouldn't use pkg/error anymore "github.com/pkg/error", ]

[[linters-settings.depguard.packages-with-error-message]]
    "github.com/pkg/error" = "Please use stdlib errors module"

</details>

### Go environment

<details>

```console
% go version && go env
go version go1.20 darwin/amd64
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/natefinch/Library/Caches/go-build"
GOENV="/Users/natefinch/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/natefinch/pkg/mod"
GONOPROXY=""
GONOSUMDB="github.com/github/*"
GOOS="darwin"
GOPATH="/Users/natefinch"
GOPRIVATE=""
GOPROXY="https://goproxy.githubapp.com/mod,https://proxy.golang.org/,direct"
GOROOT="/Users/natefinch/sdk/go1.20"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/natefinch/sdk/go1.20/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.20"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/natefinch/src/github.com/github/go-linter/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/_k/vt7y4zw92_l6b89wxdgr7mxh0000gn/T/go-build3115426105=/tmp/go-build -gno-record-gcc-switches -fno-common"

Verbose output of running

% golangci-lint run
cmd/chatops-test/main.go:11:2: import 'github.com/github/go-chatops/v2' is not allowed from list 'Main' (depguard)
        chatops "github.com/github/go-chatops/v2"
        ^
cmd/chatops/main.go:7:2: import 'github.com/github/copilot-platform-api/pkg/chatops' is not allowed from list 'Main' (depguard)
        "github.com/github/copilot-platform-api/pkg/chatops"

.... etc

Code example or link to a public repository

Looks like literally any import in a package main is a failure.

[ldez]

Hello,

the topic was discussed here with the author of depguard: #3727

I did my best to keep compatibility but it was not simple.

I already created compatibility bridges for some linters but it was impossible for depguard.

I communicated on the topic:

• https://twitter.com/golangci/status/1664374779808284677
• https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md#v1530 (maybe it is not enough clear in the changelog)

We can't warranty the compatibility of linters: the authors are free to break things (even to not follow semver)

Golangci-lint is a runner/wrapper, even if we do our best it's impossible to "be backward compatible with existing configurations for basically forever".

[natefinch]

I can understand that the old config doesn't work with the new version of depguard, and I appreciate the communication, but I feel like there should be an in-application warning ahead of a breaking change. It would at least give users another chance of seeing the warning before having to update their code.

I would also like to see it called out in the changelog more clearly. Anything that requires a config change would ideally be right at the top and highlighted.

I'm not asking you to warranty against changes from the linters themselves. Clearly if they change what they output for the same inputs, you can't be expected to know or detect that. But this was a configuration-related change that was known ahead of time. Golangci-lint does control the interpretation of its own configuration.

[ldez]

Even with configuration, golangci-lint doesn't have control: we depend on the API of linters.

I understand your point of view, the only commitment I can make is to better communicate when there are breaking changes in a linter configuration (changelog, warn logs, etc.).

And be sure that I always try to not break if I can.

Related to #1987

[Nivl]

This one is hurting the company I work at pretty badly too since updating would be breaking a large amount of repos.

Maybe we could have depguard being 2 different linters under gci?

• depguard: same version as in 1.52, but we mark it as deprecated.
• depguard2: the new version of depguard

[maqdev]

It seems that this issue is being addressed here OpenPeeDeeP/depguard#70

[maqdev]

Please include the depguard v2.2.0 to resolve this issue with the next release?

[ldez]

The tag was done 6 hours ago, our bot will handle the dependency automatically soon.