Repository navigation
Replace the remaining useful parts of golint which aren't already in staticcheck #542
Description
I use golint in addition to staticcheck, but over time staticcheck has incorporated a lot of the useful checks from golint. Additionally, golint is poorly maintained and isn't as nice to use. I'd love to completely stop using golint, but there are some checks I would miss.
I went through each of the checks golint has and verified whether they are in staticcheck. First, here are the ones which staticcheck already has:
-
Package comments (ST1000)
- Package comments must be of the form "Package foo ...".
- Packages must have a package comment. (Golint is file-oriented so its implementation is basically useless.)
-
Imports
- Should not use dot imports. (ST1001)
-
Ranges
- Should use
for range ...instead offor _, _ = range ...(S1005) - Should use
for i := range ...instead offor i, _ = range ...(S1005)
- Should use
-
Errorf
- Should use
fmt.Errorf(...)instead oferrors.New(fmt.Sprintf(...)). (S1028)
- Should use
-
Errors
- Error globals should have a name of the form errFoo. (ST1012)
-
Error strings (ST1005)
- Error strings should not be capitalized
- Error strings should not end with punctuation
-
Names (ST1003)
- Package names should not contain underscores.
- Package names should not use caps
- Names shouldn't be in ALL_CAPS; use CamelCase
- Names shouldn't use underscores
- Well-known initialisms should be capitalized (URL, etc)
-
Receiver names
- Receiver names should be consistent (ST1016)
- Receiver name should not be an underscore, omit the name if it is unused (ST1006)
- Receivers should not be named "self" or "this" (ST1006)
-
Error return
- Functions which return multiple values should have the error as the last one. (ST1008)
Here are the checks golint has which are not provided by staticcheck, along with my own notes about whether I think they're useful.
-
Blank imports:
- Blank imports in non-main packages should have justifying comments. [This seems like a good check, but not too important.]
-
Exported names:
- Exported names should have doc comments. (Golint has a whitelist for certain methods like "Read".) [This one is good, but can be too noisy. Might need to be disabled by default in staticcheck.]
- Doc comments on exported names should be of the form "Foo ..." (or "A foo ..." and similar for types). Take into account comments on decl groups. [Very useful]
- Exported names should not stutter with the package name. [Nice check]
-
Names:
- Names shouldn't have leading 'k' [I guess this comes from other programming
languages? I've never come across this.]
- Names shouldn't have leading 'k' [I guess this comes from other programming
-
VarDecls:
- The LHS of a decl shouldn't have a redundant type that can be inferred from the RHS. [Useful and comes up in practice.]
-
Elses:
- Shouldn't use an
elseif theifends in a return. [I think staticcheck got rid of this check? I don't really agree with it as a blanket rule.]
- Shouldn't use an
-
Inc/Dec:
- Statements that increment/decrement a variable should use x++/x--. [Good check]
-
Unexported return:
- Exported functions should not return unexported types (which can be annoying to use). [Good check]
-
Time names:
- Variables of type time.Duration or *time.Duration should not be named with suffixes like like "Sec", "secs", "Milli", etc. [I've never seen this, but it's plausible. Low priority.]
-
Context: [These both seem worth having.]
- Call expressions to context.WithValue should not use a basic type for the key
argument. (See https://golang.org/issue/17293.) - Functions which take context.Contexts should accept the context as the first
argument.
- Call expressions to context.WithValue should not use a basic type for the key
To summarize, I propose adding the following checks to staticcheck:
ST1???:
- Blank imports in non-main packages should have justifying comments.
- All exported names should have doc comments.
- Doc comments on exported names should be of the normal form ("Foo ...", etc.)
- Exported names should not stutter with the package name.
- Statements that increment/decrement a variable should use x++/x--.
- Exported functions should not return unexported types.
- Functions which take context.Contexts should accept them as the first argument.
SA4???:
- The LHS of a decl shouldn't have a redundant type that can be inferred from the RHS.
SA1???:
- Calls to context.WithValue should not use a basic type for the key.
Perhaps if we can agree on a set of checks you think are worth adding, I can open issues for each of those and close this issue.
Activity
dominikh commentedon Jul 24, 2019
This is #484
I would need a lot of persuading to implement this check, even as a non-default one. It's my least favourite check in golint, and IMO it causes more harm than good. Any whitelist will always be incomplete, and a machine is really not the best judge as to whether an identifier needs documentation or not. Even if I made the check non-default, I don't want someone to run
-checks alland be bombarded with thousands of "please add a comment" findings.We can experiment with that idea. Edit: this is ST1020, ST1021, and ST1022 – our implementation is more lenient than golint, only flagging instances that are obviously wrong.
This tends to have a number of false positives. We can experiment with the idea, but odds are it won't make it into stylecheck.
I've only seen this check cause false positives, not true positives. Its intention is to catch the use of something similar to Hungarian notation but limited to constants. Google's C++ style guide requires the leading k, so this check was likely written for Googlers.
Can do, but we should avoid the false positives that golint has.
I don't remember if we ever had this check, but I don't like it. It would certainly flag some code I have written.
This is #483
This is #482 – my current opinion is that it's too noisy.
This is ST1011
This is #248.This is SA1029This is #481.
cespare commentedon Jul 29, 2019
@dominikh thanks! I think we're pretty much there. I commented on a few of the linked issues.
Re: exported names should have doc comments:
OK, I'm fine with that. We mostly ignore this check today, though I do use it for particular packages when I care about the API. Just looking through godoc covers this as well (and catches other documentation issues too).
Re: doc comments shouldn't be malformed:
Thanks!
Re: name stutter:
I'm fine with abandoning it. I do see this issue come up in real code semi-regularly, but the volume is pretty low (you generally have to create a new package) so it doesn't seem very important.
Re: redundant types in LHS of a decl:
I filed #551.
Thanks for the quick response. There are open issues now for all the remaining checks that I care about, so I'll go ahead and close this.