Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: deprecate varcheck, deadcode, and structcheck #3125

Merged
merged 2 commits into from Aug 21, 2022

Conversation

ldez
Copy link
Member

@ldez ldez commented Aug 21, 2022

also, remove them from the default linters.

They can be replaced by unused.

varcheck and structcheck have not changed for 3 years. https://github.com/opennota/check / https://gitlab.com/opennota/check
deadcode have not changed for 6 years. https://github.com/remyoudompheng/go-misc

All those linters are using the deprecated golang.org/x/tools/go/loader API that is deprecated for more than 3 years (this API does not have support for Go modules).

https://github.com/golang/tools/blob/587a15310bddfc939d37cfaa8be8ea4c3808c3f1/go/loader/doc.go#L10-L11

Fixes #1841

issue related to structcheck:
Close #1517
Close #826

@ldez ldez added the linter: update Update the linter implementation inside golangci-lint label Aug 21, 2022
@ldez ldez marked this pull request as draft August 21, 2022 15:28
@ldez ldez marked this pull request as ready for review August 21, 2022 16:12
Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change but curious about the reasoning for modifying default linters not being a breaking change. Breaking default behavior is usually a reason to reject suggestions and push them to v2 (f.e.x modifying or changing exclude use defaults) so this feels a bit inconsistent.

Won't //nolint directives for non enabled linters yield a warning by nolintlint?

@ldez
Copy link
Member Author

ldez commented Aug 21, 2022

In fact, unused already handles the same things as varcheck and deadcode.
As unused is enabled by default, it's not really a breaking change because the feature is still here, just not handled by the same linters.

Another point, as they are in the default linters, deprecation warnings will appear even if you are not using any configuration.

yes nolint directives will yield some warning, I don't know if this will be a bigger problem than linter changes.

@bombsimon
Copy link
Member

In fact, unused already handles the same things as varcheck and deadcode. As unused is enabled by default, it's not really a breaking change because the feature is still here, just not handled by the same linters.

Yeah I get that the analysis result might be the same but the change can still result in changes that breaks CI or other workflows.

Another point, as they are in the default linters, deprecation warnings will appear even if you are not using any configuration.

Yup, and this is a great reason to disable them by default if deprecating, if not that would be a pretty bad user experience.

yes nolint directives will yield some warning, I don't know if this will be a bigger problem than linter changes.

Yeah me neither. Someone running without configuration and golangci-lint run -Enolintlint with //nolint:varcheck directives will start to get errors by just upgrading version. That to me is a change that I don't feel is in line with previous discussions where this project tend to be pretty restrictive to avoid breaking changes.

Maybe nolintlint isn't a big enough problem to block this kind of change, I just wanted to hear your reasoning around it. I feel I want to leave that decision up to you, personally I don't have a big problem to go less restrictive since bumping version always been a pretty controlled change for me where I've worked.

Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change is good, resonable deprecation. As discussed might introduce breaking changes but I trust your judgment around this change!

@ldez ldez merged commit 37d3aa4 into golangci:master Aug 21, 2022
@ldez ldez deleted the feat/deprecate-linters branch August 21, 2022 19:37
lucacome added a commit to nginxinc/kubernetes-ingress that referenced this pull request Aug 23, 2022
The linters have not been updated in 3-6 years and have been deprecated.
golangci/golangci-lint#3125
lucacome added a commit to nginxinc/nginx-prometheus-exporter that referenced this pull request Aug 23, 2022
The linters have not been updated in 3-6 years and have been deprecated.
golangci/golangci-lint#3125
lucacome added a commit to nginxinc/nginx-plus-go-client that referenced this pull request Aug 23, 2022
The linters have not been updated in 3-6 years and have been deprecated.
golangci/golangci-lint#3125
lucacome added a commit to nginxinc/nginx-asg-sync that referenced this pull request Aug 24, 2022
The linters have not been updated in 3-6 years and have been deprecated.
golangci/golangci-lint#3125
lucacome added a commit to nginxinc/kubernetes-ingress that referenced this pull request Aug 24, 2022
The linters have not been updated in 3-6 years and have been deprecated.
golangci/golangci-lint#3125
lucacome added a commit to nginxinc/nginx-prometheus-exporter that referenced this pull request Aug 24, 2022
The linters have not been updated in 3-6 years and have been deprecated.
golangci/golangci-lint#3125
lucacome added a commit to nginxinc/nginx-asg-sync that referenced this pull request Aug 24, 2022
The linters have not been updated in 3-6 years and have been deprecated.
golangci/golangci-lint#3125
lucacome added a commit to nginxinc/nginx-plus-go-client that referenced this pull request Aug 25, 2022
The linters have not been updated in 3-6 years and have been deprecated.
golangci/golangci-lint#3125
adamdecaf added a commit to moov-io/infra that referenced this pull request Aug 26, 2022
haywoodsh pushed a commit to nginxinc/kubernetes-ingress that referenced this pull request Aug 30, 2022
The linters have not been updated in 3-6 years and have been deprecated.
golangci/golangci-lint#3125
@ejain
Copy link

ejain commented Sep 2, 2022

unused detects an unused const x = 42, but, unlike deadcode, won't flag const X = 42.

@ldez
Copy link
Member Author

ldez commented Sep 2, 2022

varcheck and structcheck have not changed for 3 years. https://github.com/opennota/check / https://gitlab.com/opennota/check

So the project is abandoned.

serprex pushed a commit to wal-g/wal-g that referenced this pull request Nov 14, 2022
Remove deprecated lints: golangci/golangci-lint#3125
Re-enable gocritic features which previously didn't support generics
usernamedt pushed a commit to wal-g/wal-g that referenced this pull request Dec 20, 2022
* Update golangci-lint version to 1.50.1

Remove deprecated lints: golangci/golangci-lint#3125
Re-enable gocritic features which previously didn't support generics

* gofmt

* Pass PrevBackupInfo by reference as it is 232 bytes
SeigeC pushed a commit to SeigeC/golangci-lint that referenced this pull request Apr 4, 2023
johnboyes added a commit to agilepathway/label-checker that referenced this pull request Feb 8, 2024
johnboyes added a commit to agilepathway/label-checker that referenced this pull request Feb 10, 2024
* Bump reviewdog golangci-lint action major version

This is a necessary precursor to bumping the Go version from 1.21 to
1.22, which will be done in separate PR.

* Remove deprecated deadcode linter

See golangci/golangci-lint#3125

* Add missing period at end of comment

As warned by golangci-lint.

* Remove unnecessary trailing whitespace

As per golangci-lint warning.

* Change case of variable to mixed not snake

As per golangci-lint warning.

* Fix cuddled statements warned by golangci-lint

* Fix return with no blank line before lint warnings

* Fix unchecked error return value linting warnings

* Exclude line from line length linter warning

* Format files with gofumpt to fix linting warnings

* Exclude file permissions line from gosec linting

The [gosec linter][1] warns by default on
[file permissions above 0600][2]. We need the permissions to be 0644 for
this line (because it has to be written to), so we exclude it from
linting.

[1]: https://github.com/securego/gosec
[2]: securego/gosec#107

* Remove depguard from Go linting

Created #430 to consider reinstating it in the future.

* Fix magic number warning by extracting constant

* Fix cuddling linter warnings

* Add required comment to exported function

* Do not use deprecated ioutil package

* Remove unused function
@ldez ldez added this to the v1.49 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter: update Update the linter implementation inside golangci-lint
Projects
None yet
3 participants