Skip to content

Replace the remaining useful parts of golint which aren't already in staticcheck #542

@cespare

Description

@cespare

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 of for _, _ = range ... (S1005)
    • Should use for i := range ... instead of for i, _ = range ... (S1005)
  • Errorf

    • Should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...)). (S1028)
  • 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.]
  • 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 else if the if ends in a return. [I think staticcheck got rid of this check? I don't really agree with it as a blanket rule.]
  • 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.

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions