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

add exhaustive linter #1166

Merged
merged 12 commits into from May 29, 2020
Merged

add exhaustive linter #1166

merged 12 commits into from May 29, 2020

Conversation

nishanths
Copy link
Contributor

This adds a linter for the exhaustive static analysis program found at https://github.com/nishanths/exhaustive. The program checks for exhaustiveness of enum switch statements.

I've disabled the linter in .golangci.yml. (I'm happy to change this behavior if necessary.)

Thanks for taking a look.

more

add new files

run command fixes

more
@boring-cyborg
Copy link

boring-cyborg bot commented May 25, 2020

Hey, thank you for opening your first Pull Request !

test/testdata/exhaustive/exhaustive.go Outdated Show resolved Hide resolved
@@ -935,7 +935,8 @@ func sizeOfReflectValueTreeBytes(rv reflect.Value, visitedPtrs map[uintptr]struc
return rv.Len()
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64,
reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64,
reflect.Uintptr, reflect.Bool, reflect.Float32, reflect.Float64, reflect.UnsafePointer:
reflect.Uintptr, reflect.Bool, reflect.Float32, reflect.Float64,
reflect.Complex64, reflect.Complex128, reflect.Func, reflect.UnsafePointer:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These missing cases

reflect.Complex64, reflect.Complex128, reflect.Func

were reported by the exhaustive linter when I temporarily enabled it for a run.

Copy link
Member

@jirfag jirfag left a comment

Choose a reason for hiding this comment

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

thank you, only 1 comment in review

.golangci.yml Outdated
@@ -104,6 +106,7 @@ linters:

# don't enable:
# - asciicheck
# - exhaustive
Copy link
Member

@jirfag jirfag May 25, 2020

Choose a reason for hiding this comment

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

why not enable it? I think dogfooding linters is important

Copy link
Contributor Author

@nishanths nishanths May 25, 2020

Choose a reason for hiding this comment

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

Sounds good, I can enable it.

I did not enable it originally because I was unsure if users would find the default behavior of the analyzer to be too strict. (The default behavior of the analyzer is to point out missing enum members in the switch even if a default case is present. That might actually be useful though — otherwise we wouldn't have discovered the missing reflect.Complex64 etc. cases above.)

@nishanths
Copy link
Contributor Author

It looks like a test is failing because a prebuilt binary is being used in the test that doesn't know about the exhaustive analyzer? Any suggestion appreciated.

@@ -0,0 +1,17 @@
//args: -Eexhaustive

Choose a reason for hiding this comment

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

Should another test case including enabling DefaultSignifiesExhaustive be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would help. I'll add one.

@mcristina422
Copy link

@nishanths The failing test is because it's only running with the last released version. Maybe it should only be added as a default after it is released?

https://github.com/golangci/golangci-lint/blob/master/.github/workflows/pr.yml#L21

@nishanths
Copy link
Contributor Author

@nishanths The failing test is because it's only running with the last released version. Maybe it should only be added as a default after it is released?

Thank you. I've made it disabled by default. I can try to revisit after the next release. cc @jirfag

@nishanths
Copy link
Contributor Author

@nishanths The failing test is because it's only running with the last released version. Maybe it should only be added as a default after it is released?

On second thought, this appears to be a bottleneck to dogfooding new linters. What is the reason the CI tests use a released version instead of HEAD? I'm happy to help improve this process in any way.

@ernado
Copy link
Member

ernado commented May 26, 2020

What is the reason the CI tests use a released version instead of HEAD?

It is problem of chicken and egg, because we are using github action, which is using a released version.

@bombsimon
Copy link
Member

Isn't it possible to just disable the official action and rely on make test which will build and run golangci-lint from HEAD in the PR?

@nishanths
Copy link
Contributor Author

nishanths commented May 27, 2020

@bombsimon I did a little more digging. pr.yml has a comment mentioning that testing the latest stable release is intentional, in addition to testing using make test.

 # We already run the current golangci-lint in tests, but here we test
 # our GitHub action with the latest stable golangci-lint.

https://github.com/golangci/golangci-lint/blob/master/.github/workflows/pr.yml#L11-L12

But I wonder what the reason is for testing new PRs on an already released version. Is this common practice?

@bombsimon
Copy link
Member

@nishanths Oh, nice findings. I thought about something similar but came to the conclusion that it wasn't needed. I guess I was wrong. 😸

I kind of see the reason, to ensure that any bugs in the action will be detected as soon as possible, but it puts this project in a weird state with sometimes hackish workarounds to be able to release changes.

Maybe we could use different configurations for the action and the project itself. I guess it will be a mess to maintain but it might be better than the alternatives. I had a similar issue in #1148 after a linter introduced a false positive in a minor release.

@ernado
Copy link
Member

ernado commented May 29, 2020

Can we merge this?

@nishanths
Copy link
Contributor Author

Yes, from me for merging. :) I feel the rest of the discussion is separate from this PR. Maybe we track it in a separate issue if necessary.

@ernado
Copy link
Member

ernado commented May 29, 2020

Great!

@ernado ernado merged commit f3376ca into golangci:master May 29, 2020
@nishanths nishanths deleted the exhaustive branch August 24, 2020 10:34
@ldez ldez added the linter: new Support new linter label Dec 7, 2020
@ldez ldez added this to the v1.28 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter: new Support new linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants