feat: add `gochecksumtype` linter #3671

alecthomas · 2023-03-12T09:56:10Z

gochecksumtype is a linter that checks that type switches on "sum types" in Go are exhaustive.

https://github.com/alecthomas/go-check-sumtype

This is based on BurntSushi/go-sumtype, but fixes, modernises and simplifies it.

fixes #402

CLAassistant · 2023-03-12T09:56:15Z

All committers have signed the CLA.

ldez · 2023-03-12T11:34:56Z

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

Pull Request Description

It must have a link to the linter repository.
It must provide a short description of the linter.

Linter

It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
It must have a valid license (AGPL is not allowed) and the file must contain the required information by the license, ex: author, year, etc.
The linter repository must have a CI and tests.
It must use go/analysis.
It must have a valid tag, ex: v1.0.0, v0.1.0.
It must not contain init().
It must not contain panic(), log.fatal(), os.exit(), or similar.
It must not have false positives/negatives. (the team will help to verify that)
It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

They must have at least one std lib import.
They must work with T=<name of the linter test file>.go make test_linters. (the team will help to verify that)

`.golangci.reference.yml`

The linter must be added to the list of available linters (alphabetical case-insensitive order).
- enable and disable options
If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
- The values must be different from the default ones.
- The default values must be defined in a comment.
- The option must have a short description.

Others Requirements

The files (tests and linter) inside golangci-lint must have the same name as the linter.
The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
The linters must be sorted in the alphabetical order (case-insensitive) in the Manager.GetAllSupportedLinterConfigs(...) and .golangci.reference.yml.
The load mode (WithLoadMode(...)):
- if the linter doesn't use types: goanalysis.LoadModeSyntax
- goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager.GetAllSupportedLinterConfigs(...)
The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.

Recommendations

The linter should not use SSA. (currently, SSA does not support generics)
The linter repository should have a readme and linting.
The linter should be published as a binary. (useful to diagnose bug origins)

The golangci-lint team will edit this comment to check the boxes before and during the review.

If the author of the PR is a member of the golangci-lint team, he should not edit this message.

This checklist does not imply that we will accept the linter.

ldez · 2023-03-12T11:40:25Z

Hello,

this linter seems really close to exhaustive maybe it can be an improvement of this lnter.
ping @nishanths

alecthomas · 2023-03-12T22:19:58Z

As I mentioned, this is just a fork of an existing linter that has been around for quite some time. There was only a minimal amount of work required to bring it up to date.

FWIW I did look at exhaustive and while it is similar it does not have this functionality. Given the choice between simply updating go-sumtype, or reimplementing all of the logic in exhaustive, I chose the former. I can understand if that's not the right choice for golangci-lint, but I personally don't have the time to reimplement this existing functional lexer in an entirely new codebase. If that's the path forward you'd prefer, I'll close this PR.

ldez · 2023-03-12T23:16:39Z

I will wait for feedback from @nishanths before taking a decision.

alecthomas · 2023-03-25T03:05:38Z

Ping!

ldez · 2023-03-25T17:57:40Z

still waiting for @nishanths feedback

ldez · 2023-04-14T02:43:57Z

I think we have waited enough time, so we will accept your linter.

@alecthomas Can you take a look at the checklist? #3671 (comment)

ldez · 2023-04-16T16:09:46Z

Since I added one sts import in the test case, the test fails:

$ T=gochecksumtype.go make test_linters         
GL_TEST_RUN=1 go test -v ./test -count 1 -run TestSourcesFromTestdata/gochecksumtype.go
=== RUN   TestSourcesFromTestdata
    linters_test.go:21: testdata/*.go
=== RUN   TestSourcesFromTestdata/gochecksumtype.go
=== PAUSE TestSourcesFromTestdata/gochecksumtype.go
=== CONT  TestSourcesFromTestdata/gochecksumtype.go
level=info msg="[test] ran [/home/ldez/sources/go/src/github.com/golangci/golangci-lint/golangci-lint run --internal-cmd-test --no-config --allow-parallel-runners --disable-all --out-format=json --max-same-issues=100 -Egochecksumtype gochecksumtype.go] in 213.428115ms"
    analysis.go:66: gochecksumtype.go:26: no diagnostic was reported matching `exhaustiveness check failed for sum type.*SumType.*missing cases for Two`
level=info msg="[test] ran [/home/ldez/sources/go/src/github.com/golangci/golangci-lint/golangci-lint run --internal-cmd-test --no-config --allow-parallel-runners --disable-all --out-format=json --max-same-issues=100 -Etypecheck -Egochecksumtype gochecksumtype.go] in 240.006321ms"
    analysis.go:66: gochecksumtype.go:26: no diagnostic was reported matching `exhaustiveness check failed for sum type.*SumType.*missing cases for Two`
--- FAIL: TestSourcesFromTestdata (2.04s)
    --- FAIL: TestSourcesFromTestdata/gochecksumtype.go (0.45s)
=== RUN   TestSourcesFromTestdataSubDir
--- PASS: TestSourcesFromTestdataSubDir (0.00s)
FAIL
FAIL    github.com/golangci/golangci-lint/test  2.528s
FAIL
make: *** [Makefile:48: test_linters] Error 1

ldez · 2023-04-16T16:15:53Z

I move the logger and the test works, maybe there is something that I don't understand.

gnuletik · 2023-06-06T13:42:34Z

Hi ! Thanks for this PR ! I was looking for a similar feature! Is there anything blocking here?

ldez · 2023-06-06T13:46:43Z

If you can explain why this change produces a different behavior.

gnuletik · 2023-06-06T16:33:56Z

@ldez thanks for the fast feedback!

It seems that this behavior is intended by the linter's author.

https://github.com/alecthomas/go-check-sumtype/blob/803c965876eec8ffc4d0f26a65af8fa4d2520423/check.go#L64-L65

This behavior comes from upstream:

https://github.com/alecthomas/go-check-sumtype/blob/803c965876eec8ffc4d0f26a65af8fa4d2520423/check.go#L64-L65

NB: The comment is not exact because it seems that the linter only check if the default case is only containing a panic.

`gochecksumtype` is a linter that checks that type switches on "sum types" in Go are exhaustive. https://github.com/alecthomas/go-check-sumtype This is based on BurntSushi/go-sumtype, but fixes, modernises and simplifies it.

maratori · 2023-10-24T15:38:42Z

pkg/golinters/gochecksumtype.go

+	}
+
+	var unknownError error
+	errors := gochecksumtype.Run([]*packages.Package{pkg})


This implementation doesn't allow declaring a sum type in package A and checking a type switch in package B. That's because each package is checked separately.

maybe to open separate issue?
I feel like this comment will get lost

ldez added the linter: new Support new linter label Mar 12, 2023

ldez added the feedback required Requires additional feedback label Mar 12, 2023

ldez self-requested a review March 30, 2023 20:04

ldez force-pushed the master branch from ad91b59 to 45afa57 Compare April 16, 2023 16:07

ldez force-pushed the master branch from a209ce6 to c2bc6c2 Compare April 16, 2023 16:22

ldez removed the feedback required Requires additional feedback label Jun 6, 2023

ldez force-pushed the master branch from c2bc6c2 to ab2c59f Compare June 7, 2023 12:46

ldez force-pushed the master branch from ab2c59f to 1844ac9 Compare July 31, 2023 16:45

ldez added the enhancement New feature or improvement label Jul 31, 2023

golangci deleted a comment from Antonboom Oct 1, 2023

alecthomas and others added 6 commits October 9, 2023 17:18

feat: add gochecksumtype linter

97ce207

`gochecksumtype` is a linter that checks that type switches on "sum types" in Go are exhaustive. https://github.com/alecthomas/go-check-sumtype This is based on BurntSushi/go-sumtype, but fixes, modernises and simplifies it.

review

42e9e3e

review: add one std lib import

627d68e

review

13ad645

review

dff0320

review: update Since attribute

90c35d6

ldez force-pushed the master branch from 1844ac9 to 90c35d6 Compare October 9, 2023 15:19

ldez approved these changes Oct 9, 2023

View reviewed changes

ldez merged commit 69d6cc9 into golangci:master Oct 9, 2023

maratori reviewed Oct 24, 2023

View reviewed changes

maratori mentioned this pull request Oct 24, 2023

gochecksumtype: works only inside one package #4158

Open

5 tasks

marians mentioned this pull request Oct 31, 2023

Upgrade golangci-lint and nancy giantswarm/architect#843

Closed

ldez modified the milestone: v1.55 Mar 4, 2024

alexandear mentioned this pull request Mar 15, 2024

docs fix broken links alexandear/golangci-lint#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `gochecksumtype` linter #3671

feat: add `gochecksumtype` linter #3671

alecthomas commented Mar 12, 2023 •

edited by ldez

Loading

CLAassistant commented Mar 12, 2023 •

edited

Loading

ldez commented Mar 12, 2023 •

edited

Loading

ldez commented Mar 12, 2023 •

edited

Loading

alecthomas commented Mar 12, 2023

ldez commented Mar 12, 2023

alecthomas commented Mar 25, 2023

ldez commented Mar 25, 2023

ldez commented Apr 14, 2023

ldez commented Apr 16, 2023

ldez commented Apr 16, 2023

gnuletik commented Jun 6, 2023

ldez commented Jun 6, 2023

gnuletik commented Jun 6, 2023

maratori Oct 24, 2023 •

edited

Loading

Antonboom Oct 24, 2023

maratori Oct 24, 2023

feat: add gochecksumtype linter #3671

feat: add gochecksumtype linter #3671

Conversation

alecthomas commented Mar 12, 2023 • edited by ldez Loading

CLAassistant commented Mar 12, 2023 • edited Loading

ldez commented Mar 12, 2023 • edited Loading

Pull Request Description

Linter

The Linter Tests Inside Golangci-lint

.golangci.reference.yml

Others Requirements

Recommendations

ldez commented Mar 12, 2023 • edited Loading

alecthomas commented Mar 12, 2023

ldez commented Mar 12, 2023

alecthomas commented Mar 25, 2023

ldez commented Mar 25, 2023

ldez commented Apr 14, 2023

ldez commented Apr 16, 2023

ldez commented Apr 16, 2023

gnuletik commented Jun 6, 2023

ldez commented Jun 6, 2023

gnuletik commented Jun 6, 2023

maratori Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Antonboom Oct 24, 2023

Choose a reason for hiding this comment

maratori Oct 24, 2023

Choose a reason for hiding this comment

feat: add `gochecksumtype` linter #3671

feat: add `gochecksumtype` linter #3671

alecthomas commented Mar 12, 2023 •

edited by ldez

Loading

CLAassistant commented Mar 12, 2023 •

edited

Loading

ldez commented Mar 12, 2023 •

edited

Loading

`.golangci.reference.yml`

ldez commented Mar 12, 2023 •

edited

Loading

maratori Oct 24, 2023 •

edited

Loading