From 2c70f907df0394e2fc6c63b04fdfd5890f5a260d Mon Sep 17 00:00:00 2001 From: Dominik Honnef Date: Sun, 28 Jul 2019 22:40:14 +0200 Subject: [PATCH] stylecheck: validate docstrings on exported identifiers --- config/config.go | 2 +- stylecheck/analysis.go | 12 ++ stylecheck/doc.go | 51 ++++++ stylecheck/lint.go | 154 ++++++++++++++++++ stylecheck/lint_test.go | 3 + .../CheckExportedFunctionDocs.go | 21 +++ .../src/CheckExportedFunctionDocs/foo_test.go | 6 + .../CheckExportedTypeDocs.go | 35 ++++ .../CheckExportedVarDocs.go | 19 +++ 9 files changed, 302 insertions(+), 1 deletion(-) create mode 100644 stylecheck/testdata/src/CheckExportedFunctionDocs/CheckExportedFunctionDocs.go create mode 100644 stylecheck/testdata/src/CheckExportedFunctionDocs/foo_test.go create mode 100644 stylecheck/testdata/src/CheckExportedTypeDocs/CheckExportedTypeDocs.go create mode 100644 stylecheck/testdata/src/CheckExportedVarDocs/CheckExportedVarDocs.go diff --git a/config/config.go b/config/config.go index fa17d90f0..a4bb92948 100644 --- a/config/config.go +++ b/config/config.go @@ -136,7 +136,7 @@ func (c Config) String() string { } var DefaultConfig = Config{ - Checks: []string{"all", "-ST1000", "-ST1003", "-ST1016"}, + Checks: []string{"all", "-ST1000", "-ST1003", "-ST1016", "-ST1020", "-ST1021", "-ST1022"}, Initialisms: []string{ "ACL", "API", "ASCII", "CPU", "CSS", "DNS", "EOF", "GUID", "HTML", "HTTP", "HTTPS", "ID", diff --git a/stylecheck/analysis.go b/stylecheck/analysis.go index 9b4ee4299..9b774db41 100644 --- a/stylecheck/analysis.go +++ b/stylecheck/analysis.go @@ -64,4 +64,16 @@ var Analyzers = lintutil.InitializeAnalyzers(Docs, map[string]*analysis.Analyzer Run: CheckDuplicatedImports, Requires: []*analysis.Analyzer{facts.Generated, config.Analyzer}, }, + "ST1020": { + Run: CheckExportedFunctionDocs, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + }, + "ST1021": { + Run: CheckExportedTypeDocs, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + }, + "ST1022": { + Run: CheckExportedVarDocs, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + }, }) diff --git a/stylecheck/doc.go b/stylecheck/doc.go index 83ab78b4e..67aee05b1 100644 --- a/stylecheck/doc.go +++ b/stylecheck/doc.go @@ -156,4 +156,55 @@ bug, we prefer the more idiomatic 'if x == 42'.`, Title: `Importing the same package multiple times`, Since: "Unreleased", }, + + "ST1020": &lint.Documentation{ + Title: "The documentation of an exported function should start with the function's name", + Text: `Doc comments work best as complete sentences, which +allow a wide variety of automated presentations. The first sentence +should be a one-sentence summary that starts with the name being +declared. + +If every doc comment begins with the name of the item it describes, +you can use the doc subcommand of the go tool and run the output +through grep. + +See https://golang.org/doc/effective_go.html#commentary for more +information on how to write good documentation.`, + Since: "Unreleased", + NonDefault: true, + }, + + "ST1021": &lint.Documentation{ + Title: "The documentation of an exported type should start with type's name", + Text: `Doc comments work best as complete sentences, which +allow a wide variety of automated presentations. The first sentence +should be a one-sentence summary that starts with the name being +declared. + +If every doc comment begins with the name of the item it describes, +you can use the doc subcommand of the go tool and run the output +through grep. + +See https://golang.org/doc/effective_go.html#commentary for more +information on how to write good documentation.`, + Since: "Unreleased", + NonDefault: true, + }, + + "ST1022": &lint.Documentation{ + Title: "The documentation of an exported variable or constant should start with variable's name", + Text: `Doc comments work best as complete sentences, which +allow a wide variety of automated presentations. The first sentence +should be a one-sentence summary that starts with the name being +declared. + +If every doc comment begins with the name of the item it describes, +you can use the doc subcommand of the go tool and run the output +through grep. + +See https://golang.org/doc/effective_go.html#commentary for more +information on how to write good documentation.`, + Since: "Unreleased", + NonDefault: true, + }, } diff --git a/stylecheck/lint.go b/stylecheck/lint.go index 5dae2ccf4..400019a69 100644 --- a/stylecheck/lint.go +++ b/stylecheck/lint.go @@ -664,3 +664,157 @@ func CheckInvisibleCharacters(pass *analysis.Pass) (interface{}, error) { pass.ResultOf[inspect.Analyzer].(*inspector.Inspector).Preorder([]ast.Node{(*ast.BasicLit)(nil)}, fn) return nil, nil } + +func CheckExportedFunctionDocs(pass *analysis.Pass) (interface{}, error) { + fn := func(node ast.Node) { + if IsInTest(pass, node) { + return + } + + decl := node.(*ast.FuncDecl) + if decl.Doc == nil { + return + } + if !ast.IsExported(decl.Name.Name) { + return + } + kind := "function" + if decl.Recv != nil { + kind = "method" + switch T := decl.Recv.List[0].Type.(type) { + case *ast.StarExpr: + if !ast.IsExported(T.X.(*ast.Ident).Name) { + return + } + case *ast.Ident: + if !ast.IsExported(T.Name) { + return + } + default: + ExhaustiveTypeSwitch(T) + } + } + prefix := decl.Name.Name + " " + if !strings.HasPrefix(decl.Doc.Text(), prefix) { + ReportNodef(pass, decl.Doc, `comment on exported %s %s should be of the form "%s..."`, kind, decl.Name.Name, prefix) + } + } + + pass.ResultOf[inspect.Analyzer].(*inspector.Inspector).Preorder([]ast.Node{(*ast.FuncDecl)(nil)}, fn) + return nil, nil +} + +func CheckExportedTypeDocs(pass *analysis.Pass) (interface{}, error) { + var genDecl *ast.GenDecl + fn := func(node ast.Node, push bool) bool { + if !push { + genDecl = nil + return false + } + if IsInTest(pass, node) { + return false + } + + switch node := node.(type) { + case *ast.GenDecl: + if node.Tok == token.IMPORT { + return false + } + genDecl = node + return true + case *ast.TypeSpec: + if !ast.IsExported(node.Name.Name) { + return false + } + + doc := node.Doc + if doc == nil { + if len(genDecl.Specs) != 1 { + // more than one spec in the GenDecl, don't validate the + // docstring + return false + } + if genDecl.Lparen.IsValid() { + // 'type ( T )' is weird, don't guess the user's intention + return false + } + doc = genDecl.Doc + if doc == nil { + return false + } + } + + s := doc.Text() + articles := [...]string{"A", "An", "The"} + for _, a := range articles { + if strings.HasPrefix(s, a+" ") { + s = s[len(a)+1:] + break + } + } + if !strings.HasPrefix(s, node.Name.Name+" ") { + ReportNodef(pass, doc, `comment on exported type %s should be of the form "%s ..." (with optional leading article)`, node.Name.Name, node.Name.Name) + } + return false + case *ast.FuncLit, *ast.FuncDecl: + return false + default: + ExhaustiveTypeSwitch(node) + return false + } + } + + pass.ResultOf[inspect.Analyzer].(*inspector.Inspector).Nodes([]ast.Node{(*ast.GenDecl)(nil), (*ast.TypeSpec)(nil), (*ast.FuncLit)(nil), (*ast.FuncDecl)(nil)}, fn) + return nil, nil +} + +func CheckExportedVarDocs(pass *analysis.Pass) (interface{}, error) { + var genDecl *ast.GenDecl + fn := func(node ast.Node, push bool) bool { + if !push { + genDecl = nil + return false + } + if IsInTest(pass, node) { + return false + } + + switch node := node.(type) { + case *ast.GenDecl: + if node.Tok == token.IMPORT { + return false + } + genDecl = node + return true + case *ast.ValueSpec: + if genDecl.Lparen.IsValid() || len(node.Names) > 1 { + // Don't try to guess the user's intention + return false + } + name := node.Names[0].Name + if !ast.IsExported(name) { + return false + } + if genDecl.Doc == nil { + return false + } + prefix := name + " " + if !strings.HasPrefix(genDecl.Doc.Text(), prefix) { + kind := "var" + if genDecl.Tok == token.CONST { + kind = "const" + } + ReportNodef(pass, genDecl.Doc, `comment on exported %s %s should be of the form "%s..."`, kind, name, prefix) + } + return false + case *ast.FuncLit, *ast.FuncDecl: + return false + default: + ExhaustiveTypeSwitch(node) + return false + } + } + + pass.ResultOf[inspect.Analyzer].(*inspector.Inspector).Nodes([]ast.Node{(*ast.GenDecl)(nil), (*ast.ValueSpec)(nil), (*ast.FuncLit)(nil), (*ast.FuncDecl)(nil)}, fn) + return nil, nil +} diff --git a/stylecheck/lint_test.go b/stylecheck/lint_test.go index 1697d01e5..bb24cdd8d 100644 --- a/stylecheck/lint_test.go +++ b/stylecheck/lint_test.go @@ -22,6 +22,9 @@ func TestAll(t *testing.T) { "ST1017": {{Dir: "CheckYodaConditions"}}, "ST1018": {{Dir: "CheckInvisibleCharacters"}}, "ST1019": {{Dir: "CheckDuplicatedImports"}}, + "ST1020": {{Dir: "CheckExportedFunctionDocs"}}, + "ST1021": {{Dir: "CheckExportedTypeDocs"}}, + "ST1022": {{Dir: "CheckExportedVarDocs"}}, } testutil.Run(t, Analyzers, checks) diff --git a/stylecheck/testdata/src/CheckExportedFunctionDocs/CheckExportedFunctionDocs.go b/stylecheck/testdata/src/CheckExportedFunctionDocs/CheckExportedFunctionDocs.go new file mode 100644 index 000000000..f711c1b10 --- /dev/null +++ b/stylecheck/testdata/src/CheckExportedFunctionDocs/CheckExportedFunctionDocs.go @@ -0,0 +1,21 @@ +package pkg + +// whatever +func foo() {} + +// Foo is amazing +func Foo() {} + +// Whatever // want `comment on exported function` +func Bar() {} + +type T struct{} + +// Whatever +func (T) foo() {} + +// Foo is amazing +func (T) Foo() {} + +// Whatever // want `comment on exported method` +func (T) Bar() {} diff --git a/stylecheck/testdata/src/CheckExportedFunctionDocs/foo_test.go b/stylecheck/testdata/src/CheckExportedFunctionDocs/foo_test.go new file mode 100644 index 000000000..5fd392415 --- /dev/null +++ b/stylecheck/testdata/src/CheckExportedFunctionDocs/foo_test.go @@ -0,0 +1,6 @@ +package pkg + +import "testing" + +// This is a test +func TestFoo(t *testing.T) {} diff --git a/stylecheck/testdata/src/CheckExportedTypeDocs/CheckExportedTypeDocs.go b/stylecheck/testdata/src/CheckExportedTypeDocs/CheckExportedTypeDocs.go new file mode 100644 index 000000000..2ddc354d2 --- /dev/null +++ b/stylecheck/testdata/src/CheckExportedTypeDocs/CheckExportedTypeDocs.go @@ -0,0 +1,35 @@ +package pkg + +// Some type +type t1 struct{} + +// Some type // want `comment on exported type` +type T2 struct{} + +// T3 is amazing +type T3 struct{} + +type ( + // Some type // want `comment on exported type` + T4 struct{} + // The T5 type is amazing + T5 struct{} + // Some type + t6 struct{} +) + +// Some types +type ( + T7 struct{} + T8 struct{} +) + +// Some types +type ( + T9 struct{} +) + +func fn() { + // Some type + type T1 struct{} +} diff --git a/stylecheck/testdata/src/CheckExportedVarDocs/CheckExportedVarDocs.go b/stylecheck/testdata/src/CheckExportedVarDocs/CheckExportedVarDocs.go new file mode 100644 index 000000000..416b42ffb --- /dev/null +++ b/stylecheck/testdata/src/CheckExportedVarDocs/CheckExportedVarDocs.go @@ -0,0 +1,19 @@ +package pkg + +// Whatever +var a int + +// Whatever // want `should be of the form` +var B int + +// Whatever +var ( + // Whatever + C int +) + +func fn() { + // Whatever + var D int + _ = D +}