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
add exhaustive linter #1166
Conversation
Hey, thank you for opening your first Pull Request ! |
@@ -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: |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.)
It looks like a test is failing because a prebuilt binary is being used in the test that doesn't know about the |
@@ -0,0 +1,17 @@ | |||
//args: -Eexhaustive |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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 |
Thank you. I've made it disabled by default. I can try to revisit after the next release. cc @jirfag |
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 |
It is problem of chicken and egg, because we are using github action, which is using a released version. |
Isn't it possible to just disable the official action and rely on |
@bombsimon I did a little more digging.
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? |
@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. |
Can we merge this? |
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. |
Great! |
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.