feat: add godoclint linter

[babakks]

This PR adds godoclint to available linters (godoc-lint/godoc-lint).

Godoc-Lint is a little opinionated linter for Go documentation practice.

About the linter

Godoc-Lint is a linter for godocs, applying the rules set by the Go team in Go Doc Comments blog post. I made Godoc-Lint as a small/encapsulated linter to enforce best practices around godocs. There might be other linters that support some of the Godoc-Lint rules, but I couldn't find one single linter encompassing them all. The linter is rather new, but is already used in a few projects, including googleapis/librarian, googleapis/google-cloud-rust, and a few others, where it's being used outside of the golang-lint suite. That's why I decided it makes sense for the linter to be integrated with golangci-lint as the current industry standard. More about the linter and its rules can be found in the repo's README.md file.

Integration

I made sure the linter satisfies all technical criteria on the checklist (e.g. no panics, CI tests, etc). The current latest version, v0.5.0 has been released for this purpose. Note that os.Exit has been used a couple of times in the cmd/main package of the linter. That package is the CLI binary entrypoint and is not reachable through the integration with golangci-lint.

Regarding the tests, there are exhaustive tests in the linter's repo, which seemed too much to be duplicated in here in golangci-lint. Instead I added a minimal set of test cases to assert certain aspects of the integration (e.g. default config behaviour, or configuration mapping). However, I couldn't find a decent, already implemented way of package-wide testing. There's this specific rule, named single-pkg-doc, that needs to check all files in a Go package to make sure only one of the package statements has godoc, if any. That specific rule is not covered by the tests in this PR.

Also, as pointed out in the checklist, the test files include an stdlib import. I'm not sure if it's the same reason, but during early days of developing the linter I noticed go/analysis iterates over all imported packages, and therefore I had to make the linter automatically exclude those packages to ensure it stays within the codebase.

Regarding the default configuration, the linter has a sensible set of defaults that makes it usable out of the box while bringing value.

[CLAassistant]

All committers have signed the CLA.

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[ldez]

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

• The CLA must be signed

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.
• It must use Go version <= 1.24.0
• The linter repository must have a CI and tests.
• It must use go/analysis.
• It must have a valid semver tag, ex: v1.0.0, v0.1.0 (0.0.x are not valid).
• It must not contain init().
• It must not contain panic().
• It must not contain log.Fatal(), os.Exit(), or similar.
• It must not modify the AST.
• 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 have integration tests without configuration (default).
• They must have integration tests with configuration (if the linter has a configuration).

.golangci.next.reference.yml

• The file .golangci.next.reference.yml must be updated.
• The file .golangci.reference.yml must NOT be edited.
• The linter must be added to the lists 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 lintersdb/builder_linter.go and .golangci.next.reference.yml.
• The load mode (WithLoadMode(...)):
  • if the linter uses goanalysis.LoadModeSyntax -> no WithLoadForGoAnalysis() in lintersdb/builder_linter.go
  • if the linter uses goanalysis.LoadModeTypesInfo, it requires WithLoadForGoAnalysis() in lintersdb/builder_linter.go
• The version in WithSince(...) must be the next minor version (v2.X.0) of golangci-lint.
• WithURL() must contain the URL of the repository.

Recommendations

• The file jsonschema/golangci.next.jsonschema.json should be updated.
• The file jsonschema/golangci.jsonschema.json must NOT be edited.
• The linter repository should have a readme and linting.
• The linter should be published as a binary (useful for diagnosing bug origins).
• The linter repository should have a .gitignore (IDE files, binaries, OS files, etc. should not be committed)
• A tag should never be recreated.
• use main as the default branch name.

---

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

The code review will start after the completion of those checkboxes (except for the specific items that the team will help to verify).

The reviews should be addressed as commits (no squash).

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]

The linter description claims to follow Go recommendations from this page, but several rules are not those recommendations (ex, line length).

The linter applies the rules to non-exposed elements, but those elements are not a part of the godoc render (ex, a non-exported constant doesn't need to have a doc beginning with its own name), and so they don't need to follow the same rules as exposed elements.
Also, this is, in some ways, opposed to the recommendations themselves:

Every exported (capitalized) name should have a doc comment.

This can be seen as false positives, IMO, this will be the main criterion for users to not use this linter.

Even the examples of "good" from the linter README don't pass the rules of the linter:

// Check [docs] here. // (Good)
//
// [docs]: https://foo.com/docs
const foo = 0

[babakks]

Thanks for taking the time reviewing the linter, @ldez! 🙏

Let me first provide a categorised table of the rules so that I can refer to them later:

Kind	Rules	Notes
Basic (default)	pkg-doc, single-pkg-doc, start-with-name	recommended by Go Doc Comments, and low-effort
Strict	require-doc, require-pkg-doc	recommended by Go Doc Comments, and high-effort
Extra	max-len, no-unused-link	extra but compatible with Go Doc Comments

By default, the linter only applies the Basic rules. They're low-effort because they don't require user to add more godocs. Rather, they just check if the existing godocs match the recommended formatting.

The Strict rules are costly to enable. Although they're recommended by Go Doc Comments, but the linter do not apply them by default, because it will be too much, and in some cases unnecessary. So, that's up to the user to enable them based on their situation.

And finally the Extra rules which are not part of the blog post, are additional rules to help improve godocs. They're not breaking/opposing the recommended rules. These are also not enabled by default.

Now, back to your points:

The linter description claims to follow Go recommendations from this page, but several rules are not those recommendations (ex, line length).

True. max-len and no-unused-link are Extra rules, to help further improve godocs. Note that they're opt-in and not enabled by default.

The linter applies the rules to non-exposed elements, but those elements are not a part of the godoc render (ex, a non-exported constant doesn't need to have a doc beginning with its own name), and so they don't need to follow the same rules as exposed elements. Also, this is, in some ways, opposed to the recommendations themselves:

Every exported (capitalized) name should have a doc comment.

This can be seen as false positives, IMO, this will be the main criterion for users to not use this linter.

Good point, and that was one of my initial challenges. Note that godocs are not only used with the renderer or on pkg.go.dev. The Go language server (gopls) uses them to improve the IDE experience. So, godocs on unexported symbols are still visible to developers and it makes sense to apply the Basic rules to them. That's one of the reasons why I called the linter a little opinionated in the README.

That said, I think I'm open to exclude unexported symbols from the default behaviour, of course, in favour of a configuration parameter to enable the rule for them. Please let me know your thoughts.

Even the examples of "good" from the linter README don't pass the rules of the linter:

// Check [docs] here. // (Good)
//
// [docs]: https://foo.com/docs
const foo = 0

Makes sense, and thanks for the feedback. I'll update those examples to avoid confusion for the reader.

[ldez]

That said, I think I'm open to exclude unexported symbols from the default behaviour, of course, in favour of a configuration parameter to enable the rule for them. Please let me know your thoughts.

I think it should be an option of start-with-name (and false by default)

[ldez]

Note: golangci-lint will not follow your configuration naming style with /.
The options will be on structures.

[babakks]

I think it should be an option of start-with-name (and false by default)

Makes sense. I'll push the changes for this and README good/bad examples, and will then rebase the PR.

Note: golangci-lint will not follow your configuration naming style with /. The options will be on structures.

Just to be clear, you saying this format is not acceptable, right?

linters:
  settings:
    godoclint:
      options:
        max-len/length: 127

And if yes, how about using dashes like this?

linters:
  settings:
    godoclint:
      options:
        max-len-length: 127

Or even dots?

linters:
  settings:
    godoclint:
      options:
        max-len.length: 127

Any particular recommendation?

[ldez]

linters:
  settings:
    godoclint:
      options:
        max-len:
          length: 127

[babakks]

linters:
  settings:
    godoclint:
      options:
        max-len:
          length: 127

I'll push the changes for this as well.

[babakks]

I made the following changes and published v0.6.1 of the linter:

• The start-with-name rule skips unexported symbol by default.
  • A new option added, named start-with-name/include-unexported (default false), to override the behaviour.
• The examples in the linter README are improved so that "good" cases are actually okay with the linter.
• The same categorised table I mentioned in feat: add godoclint linter #6062 (comment) is now added to the linter's README to proivde a quick overview of different types of rules.
• A few typos are fixed.

And here are the changes I (force) pushed to this PR branch:

• The fine tuning options are now nested structs (rather than being forward-slash separated).
  • The next version JSON schema and config reference are both updated accordingly.
• The test files are renamed with a godoclint_ prefix.

[ldez]

As an improvement of the linter, I think some messages can be more precise.

ex:

// Foo is a thing.
type Bar struct {}

Current message:

foo.go:14:2: godoc should start with symbol name (pattern "((A|a|An|an|THE|The|the) )?%") (godoclint)
// Foo is a thing.
^

Maybe something like that:

foo.go:14:2: godoc should start with symbol name ('Bar') (godoclint)
// Foo is a thing.
^

[babakks]

Good point, and I like the nicer message. I'll make a release for it and rebase this PR. Thanks for the feedback. 🙏

[ldez]

Please don't rebase.

The reviews should be addressed as commits (no squash).
#6062 (comment)

[ldez]

From my experience, when users don't understand a report:

1. In most cases, they disabled the linter or don't enable it.
2. A few of them will try to understand, and will understand, but because they are working with a team, and teams don't like confusion, they will disable the linter.
3. A few of the few will understand and keep the linter.

Report messages are as critical as good defaults for the adoption of a linter.

I recommend reviewing the report messages to ensure they are easily understandable by Go newcomers and non-native English speakers.
I think this can be frustrating for native English speakers, but this is important.

example:

godoc exceeds max length

This is confusing because the problem is related to a line inside the godoc and not the godoc by itself.

[babakks]

Valuable insight, @ldez! With that I'll review the report messages and will come back with a new release. I was leaning toward short messages, but the cost of losing adoption due to this is a real thing that I would never have thought. Thanks again! ✌️

[Antonboom]

Finally someone did it!

Related to

• x/tools/gopls: add analyzer that reports broken doc links golang/go#57963
• proposal: x/tools/go/analysis: doc comment analyzer golang/go#63929
• proposal: cmd/vet: verify that doc links are valid golang/go#73056
• brokenDocLink: verify that doc links are valid go-critic/go-critic#1479

[ldez]

It doesn't verify links, but it verifies that links are used.
And only element with the form [link]: URL

[babakks]

@ldez is right. But checking for valid links is on my list of todos.

[babakks]

I just pushed the changes to issue messages. You can see godoc-lint/godoc-lint#33 for an overvall look.

[ldez]

I don't have the answers, but I wonder if a godoc starting with // Deprecated: should be skipped.

https://go.dev/doc/comment#deprecations

The paragraph does not have to be the last paragraph in the doc comment.

// My constants.
const (
	A = "A"
	B = "B"
	C = "C"

	// Deprecated: use A instead.
	D = "D"
)

Same thing on notes (at least on non-exported symbols):
https://go.dev/doc/comment#notes

// TODO(user1): refactor to use standard library context
// BUG(user2): not cleaned up
var ctx context.Context

This is not blocking from the POV of golangci-lint, this is only some questions.

[babakks]

The // Deprecated: thing is already on my radar (recently noticed it as a valid use case when I saw kubernetes/kubernetes#133566). Notes can also be handled, I think. The good thing about the notes on unexported symbols is that now they don't need to start with the symbol name by default.

Anyway, definitely both use cases are on my list, and yeah, they're not seem like blockers.

[ldez]

Maybe you can create one or several issues to expose what is on your list.
It may encourage contributions or at least avoid having issues on already known topics.

[babakks]

Good point. I'll create one issue for each for now to cover the ideas so far.

[ldez]

The new message of start-with-name when using a explicit pattern is unexpected.

// Foo is a thing.
type Bar struct {}

        start-with-name:
          pattern: "GODOC %"

main.go:8:1: godoc should start with symbol name ("Bar") (godoclint)
// Foo is a thing.
^

I think the message is right in 99% of cases, but the problem is maybe the option pattern.

[ldez]

I found the previous problem, because I dug a bit more about // Deprecated: comment, and it seems to be a common pattern.

I wanted to quickly hack around the topic by modifying the pattern (%|Deprecated:), and the reports were confusing.
And the pattern doesn't seem to work. 🤔

[babakks]

I think I already have a fix for the // Deprecated: case. Just need to add a couple of tests to maje sure ot's the right change.

Regarding the pattern thing, I think I can modify the report message to be different based on the configured pattern.

I'll push a commit when I'm done with the fix. Probably tomorrow.

[ldez]

Regarding the pattern thing, I think I can modify the report message to be different based on the configured pattern.

I think the pattern option is not useful; maybe it can be removed.
Do you have some real cases where this option is useful?

[babakks]

A pattern is needed, because as of the godoc reference godocs should start with a complete senetence, which is okay to start with an article, for example. But I'm not sure there's a point in making the pattern configurable. 🤔