From 69b0ecf8ca591ee3001106bbd23d196d676fd07b Mon Sep 17 00:00:00 2001 From: fregin Date: Tue, 8 Aug 2023 11:45:24 +0000 Subject: [PATCH 1/8] [var-naming] handle private uppercased const --- rule/var-naming.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rule/var-naming.go b/rule/var-naming.go index 2aa5ae2fd..cc4900d1f 100644 --- a/rule/var-naming.go +++ b/rule/var-naming.go @@ -13,8 +13,8 @@ import ( var anyCapsRE = regexp.MustCompile(`[A-Z]`) -// regexp for constant names like `SOME_CONST`, `SOME_CONST_2`, `X123_3` (#851) -var upperCaseConstRE = regexp.MustCompile(`^[A-Z][A-Z\d]*(_[A-Z\d]+)*$`) +// regexp for constant names like `SOME_CONST`, `SOME_CONST_2`, `X123_3`, `_SOME_PRIVATE_CONST` (#851) +var upperCaseConstRE = regexp.MustCompile(`^_?[A-Z][A-Z\d]*(_[A-Z\d]+)*$`) // VarNamingRule lints given else constructs. type VarNamingRule struct { From 47f336cfa063adeff4935e170545d37a6a91bc9b Mon Sep 17 00:00:00 2001 From: comdiv Date: Fri, 28 Jun 2024 15:01:26 +0500 Subject: [PATCH 2/8] FEATURE #1002 - "checkPublicInterface" option for "exported" rule - to check public interface method comments --- rule/exported.go | 62 +++++++++++++++++++++++++++++++-- test/exported_test.go | 7 ++++ testdata/exported-issue-1002.go | 20 +++++++++++ 3 files changed, 86 insertions(+), 3 deletions(-) create mode 100644 testdata/exported-issue-1002.go diff --git a/rule/exported.go b/rule/exported.go index b8663c48c..5a3d961e0 100644 --- a/rule/exported.go +++ b/rule/exported.go @@ -18,6 +18,7 @@ type ExportedRule struct { configured bool checkPrivateReceivers bool disableStutteringCheck bool + checkPublicInterface bool stuttersMsg string sync.Mutex } @@ -26,7 +27,12 @@ func (r *ExportedRule) configure(arguments lint.Arguments) { r.Lock() if !r.configured { var sayRepetitiveInsteadOfStutters bool - r.checkPrivateReceivers, r.disableStutteringCheck, sayRepetitiveInsteadOfStutters = r.getConf(arguments) + + r.checkPrivateReceivers, + r.disableStutteringCheck, + sayRepetitiveInsteadOfStutters, + r.checkPublicInterface = r.getConf(arguments) + r.stuttersMsg = "stutters" if sayRepetitiveInsteadOfStutters { r.stuttersMsg = "is repetitive" @@ -57,7 +63,9 @@ func (r *ExportedRule) Apply(file *lint.File, args lint.Arguments) []lint.Failur genDeclMissingComments: make(map[*ast.GenDecl]bool), checkPrivateReceivers: r.checkPrivateReceivers, disableStutteringCheck: r.disableStutteringCheck, + checkPublicInterface: r.checkPublicInterface, stuttersMsg: r.stuttersMsg, + } ast.Walk(&walker, fileAst) @@ -70,7 +78,12 @@ func (*ExportedRule) Name() string { return "exported" } -func (r *ExportedRule) getConf(args lint.Arguments) (checkPrivateReceivers, disableStutteringCheck, sayRepetitiveInsteadOfStutters bool) { +func (r *ExportedRule) getConf(args lint.Arguments) ( + checkPrivateReceivers, + disableStutteringCheck, + sayRepetitiveInsteadOfStutters bool, + checkPublicInterface bool, +) { // if any, we expect a slice of strings as configuration if len(args) < 1 { return @@ -88,6 +101,8 @@ func (r *ExportedRule) getConf(args lint.Arguments) (checkPrivateReceivers, disa disableStutteringCheck = true case "sayRepetitiveInsteadOfStutters": sayRepetitiveInsteadOfStutters = true + case "checkPublicInterface": + checkPublicInterface = true default: panic(fmt.Sprintf("Unknown configuration flag %s for %s rule", flagStr, r.Name())) } @@ -104,6 +119,7 @@ type lintExported struct { onFailure func(lint.Failure) checkPrivateReceivers bool disableStutteringCheck bool + checkPublicInterface bool stuttersMsg string } @@ -330,7 +346,15 @@ func (w *lintExported) Visit(n ast.Node) ast.Visitor { } w.lintTypeDoc(v, doc) w.checkStutter(v.Name, "type") - // Don't proceed inside types. + + if w.checkPublicInterface { + if iface, ok := v.Type.(*ast.InterfaceType); ok { + if ast.IsExported(v.Name.Name) { + w.doCheckPublicInterface(v.Name.Name, iface) + } + } + } + return nil case *ast.ValueSpec: w.lintValueSpecDoc(v, w.lastGen, w.genDeclMissingComments) @@ -338,3 +362,35 @@ func (w *lintExported) Visit(n ast.Node) ast.Visitor { } return w } + + +func (w *lintExported) doCheckPublicInterface(typeName string, iface *ast.InterfaceType) { + for _, m := range iface.Methods.List { + if ast.IsExported(m.Names[0].Name) { + w.lintInterfaceMethod(typeName, m) + } + } +} + +func (w *lintExported) lintInterfaceMethod(typeName string, m *ast.Field) { + name := m.Names[0].Name + if m.Doc == nil { + w.onFailure(lint.Failure{ + Node: m, + Confidence: 1, + Category: "comments", + Failure: fmt.Sprintf("public interface method %s.%s should be commented", typeName, name), + }) + return + } + s := normalizeText(m.Doc.Text()) + prefix := m.Names[0].Name + " " + if !strings.HasPrefix(s, prefix) { + w.onFailure(lint.Failure{ + Node: m.Doc, + Confidence: 0.8, + Category: "comments", + Failure: fmt.Sprintf(`comment on exported interface method %s.%s should be of the form "%s..."`, typeName, name, prefix), + }) + } +} \ No newline at end of file diff --git a/test/exported_test.go b/test/exported_test.go index 841ab4a01..f6c7784bf 100644 --- a/test/exported_test.go +++ b/test/exported_test.go @@ -24,3 +24,10 @@ func TestExportedReplacingStuttersByRepetitive(t *testing.T) { testRule(t, "exported-issue-519", &rule.ExportedRule{}, &lint.RuleConfig{Arguments: args}) } + + +func TestCheckPublicInterfaceOption(t *testing.T) { + args := []any{"checkPublicInterface"} + + testRule(t, "exported-issue-1002", &rule.ExportedRule{}, &lint.RuleConfig{Arguments: args}) +} \ No newline at end of file diff --git a/testdata/exported-issue-1002.go b/testdata/exported-issue-1002.go new file mode 100644 index 000000000..f88da11b4 --- /dev/null +++ b/testdata/exported-issue-1002.go @@ -0,0 +1,20 @@ +// Package golint comment +package golint + +// by default code bellow is valid, +// but if checkPublicInterface is switched on - it should check documentation in interfaces + +// Some - some interface +type Some interface { + // Correct - should do all correct + Correct() + // MATCH /comment on exported interface method Some.SemiCorrect should be of the form "SemiCorrect ..."/ + SemiCorrect() + NonCorrect() // MATCH /public interface method Some.NonCorrect should be commented/ +} + +// for private interfaces it doesn't check docs anyway + +type somePrivate interface { + AllGood() +} \ No newline at end of file From 98eabe8728800c2eb3f9ca61017e427c451ed361 Mon Sep 17 00:00:00 2001 From: comdiv Date: Fri, 28 Jun 2024 15:10:26 +0500 Subject: [PATCH 3/8] fix exported #1002 for ast.Ident --- rule/exported.go | 3 +++ testdata/exported-issue-1002.go | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/rule/exported.go b/rule/exported.go index 5a3d961e0..e08f01195 100644 --- a/rule/exported.go +++ b/rule/exported.go @@ -366,6 +366,9 @@ func (w *lintExported) Visit(n ast.Node) ast.Visitor { func (w *lintExported) doCheckPublicInterface(typeName string, iface *ast.InterfaceType) { for _, m := range iface.Methods.List { + if _, ok := m.Type.(*ast.Ident); ok { + continue + } if ast.IsExported(m.Names[0].Name) { w.lintInterfaceMethod(typeName, m) } diff --git a/testdata/exported-issue-1002.go b/testdata/exported-issue-1002.go index f88da11b4..3610fb6fe 100644 --- a/testdata/exported-issue-1002.go +++ b/testdata/exported-issue-1002.go @@ -6,6 +6,7 @@ package golint // Some - some interface type Some interface { + Other // should not fail // Correct - should do all correct Correct() // MATCH /comment on exported interface method Some.SemiCorrect should be of the form "SemiCorrect ..."/ @@ -13,6 +14,9 @@ type Some interface { NonCorrect() // MATCH /public interface method Some.NonCorrect should be commented/ } +// Other - just to check names compatibility +type Other interface {} + // for private interfaces it doesn't check docs anyway type somePrivate interface { From d7fc8627384eec8ed0e315c5e78cc2879c992418 Mon Sep 17 00:00:00 2001 From: comdiv Date: Fri, 28 Jun 2024 15:12:32 +0500 Subject: [PATCH 4/8] fix exported #1002 for ast.Ident 2 --- rule/exported.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rule/exported.go b/rule/exported.go index e08f01195..614f1536d 100644 --- a/rule/exported.go +++ b/rule/exported.go @@ -366,7 +366,8 @@ func (w *lintExported) Visit(n ast.Node) ast.Visitor { func (w *lintExported) doCheckPublicInterface(typeName string, iface *ast.InterfaceType) { for _, m := range iface.Methods.List { - if _, ok := m.Type.(*ast.Ident); ok { + // case of ast.Ident and other implicit fields + if len(m.Names) == 0 { continue } if ast.IsExported(m.Names[0].Name) { From ecda17b34651fd64a6c9eece828be47fcf386575 Mon Sep 17 00:00:00 2001 From: comdiv Date: Fri, 28 Jun 2024 15:18:29 +0500 Subject: [PATCH 5/8] go fmt applyed --- rule/exported.go | 24 +++++++++++------------- test/exported_test.go | 3 +-- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/rule/exported.go b/rule/exported.go index 614f1536d..0f884a3d1 100644 --- a/rule/exported.go +++ b/rule/exported.go @@ -27,11 +27,11 @@ func (r *ExportedRule) configure(arguments lint.Arguments) { r.Lock() if !r.configured { var sayRepetitiveInsteadOfStutters bool - - r.checkPrivateReceivers, - r.disableStutteringCheck, - sayRepetitiveInsteadOfStutters, - r.checkPublicInterface = r.getConf(arguments) + + r.checkPrivateReceivers, + r.disableStutteringCheck, + sayRepetitiveInsteadOfStutters, + r.checkPublicInterface = r.getConf(arguments) r.stuttersMsg = "stutters" if sayRepetitiveInsteadOfStutters { @@ -63,9 +63,8 @@ func (r *ExportedRule) Apply(file *lint.File, args lint.Arguments) []lint.Failur genDeclMissingComments: make(map[*ast.GenDecl]bool), checkPrivateReceivers: r.checkPrivateReceivers, disableStutteringCheck: r.disableStutteringCheck, - checkPublicInterface: r.checkPublicInterface, + checkPublicInterface: r.checkPublicInterface, stuttersMsg: r.stuttersMsg, - } ast.Walk(&walker, fileAst) @@ -79,8 +78,8 @@ func (*ExportedRule) Name() string { } func (r *ExportedRule) getConf(args lint.Arguments) ( - checkPrivateReceivers, - disableStutteringCheck, + checkPrivateReceivers, + disableStutteringCheck, sayRepetitiveInsteadOfStutters bool, checkPublicInterface bool, ) { @@ -119,7 +118,7 @@ type lintExported struct { onFailure func(lint.Failure) checkPrivateReceivers bool disableStutteringCheck bool - checkPublicInterface bool + checkPublicInterface bool stuttersMsg string } @@ -346,7 +345,7 @@ func (w *lintExported) Visit(n ast.Node) ast.Visitor { } w.lintTypeDoc(v, doc) w.checkStutter(v.Name, "type") - + if w.checkPublicInterface { if iface, ok := v.Type.(*ast.InterfaceType); ok { if ast.IsExported(v.Name.Name) { @@ -363,7 +362,6 @@ func (w *lintExported) Visit(n ast.Node) ast.Visitor { return w } - func (w *lintExported) doCheckPublicInterface(typeName string, iface *ast.InterfaceType) { for _, m := range iface.Methods.List { // case of ast.Ident and other implicit fields @@ -397,4 +395,4 @@ func (w *lintExported) lintInterfaceMethod(typeName string, m *ast.Field) { Failure: fmt.Sprintf(`comment on exported interface method %s.%s should be of the form "%s..."`, typeName, name, prefix), }) } -} \ No newline at end of file +} diff --git a/test/exported_test.go b/test/exported_test.go index f6c7784bf..da298c588 100644 --- a/test/exported_test.go +++ b/test/exported_test.go @@ -25,9 +25,8 @@ func TestExportedReplacingStuttersByRepetitive(t *testing.T) { testRule(t, "exported-issue-519", &rule.ExportedRule{}, &lint.RuleConfig{Arguments: args}) } - func TestCheckPublicInterfaceOption(t *testing.T) { args := []any{"checkPublicInterface"} testRule(t, "exported-issue-1002", &rule.ExportedRule{}, &lint.RuleConfig{Arguments: args}) -} \ No newline at end of file +} From 8f7a95aa4e47c8d42c2cc26a3b4196d332d030d2 Mon Sep 17 00:00:00 2001 From: comdiv Date: Fri, 28 Jun 2024 15:20:06 +0500 Subject: [PATCH 6/8] #1002 update documentation on `exported` rule --- RULES_DESCRIPTIONS.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index b0fa9fd28..be7256052 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -14,7 +14,7 @@ List of all available rules. - [call-to-gc](#call-to-gc) - [cognitive-complexity](#cognitive-complexity) - [comment-spacings](#comment-spacings) - - [comments-density](#comment-spacings) + - [comments-density](#comments-density) - [confusing-naming](#confusing-naming) - [confusing-results](#confusing-results) - [constant-logical-expr](#constant-logical-expr) @@ -474,12 +474,13 @@ Available flags are: * _checkPrivateReceivers_ enables checking public methods of private types * _disableStutteringCheck_ disables checking for method names that stutter with the package name (i.e. avoid failure messages of the form _type name will be used as x.XY by other packages, and that stutters; consider calling this Y_) * _sayRepetitiveInsteadOfStutters_ replaces the use of the term _stutters_ by _repetitive_ in failure messages +* _checkPublicInterface_ enabled checking public method definitions in public interface types Example: ```toml [rule.exported] - arguments = ["checkPrivateReceivers", "disableStutteringCheck"] + arguments = ["checkPrivateReceivers", "disableStutteringCheck", "checkPublicInterface"] ``` ## file-header From 2f9bc257ad7a8ac320e833bab7333d80632783f2 Mon Sep 17 00:00:00 2001 From: comdiv Date: Wed, 3 Jul 2024 18:05:40 +0500 Subject: [PATCH 7/8] refactor `exported` rule configuration logic --- rule/exported.go | 64 ++++++++++++++---------------------------------- 1 file changed, 19 insertions(+), 45 deletions(-) diff --git a/rule/exported.go b/rule/exported.go index 0f884a3d1..08283f646 100644 --- a/rule/exported.go +++ b/rule/exported.go @@ -25,22 +25,29 @@ type ExportedRule struct { func (r *ExportedRule) configure(arguments lint.Arguments) { r.Lock() + defer r.Unlock() if !r.configured { - var sayRepetitiveInsteadOfStutters bool - - r.checkPrivateReceivers, - r.disableStutteringCheck, - sayRepetitiveInsteadOfStutters, - r.checkPublicInterface = r.getConf(arguments) - - r.stuttersMsg = "stutters" - if sayRepetitiveInsteadOfStutters { - r.stuttersMsg = "is repetitive" + for _, flag := range arguments { + flagStr, ok := flag.(string) + if !ok { + panic(fmt.Sprintf("Invalid argument for the %s rule: expecting a string, got %T", r.Name(), flag)) + } + r.stuttersMsg = "stutters" + switch flagStr { + case "checkPrivateReceivers": + r.checkPrivateReceivers = true + case "disableStutteringCheck": + r.disableStutteringCheck = true + case "sayRepetitiveInsteadOfStutters": + r.stuttersMsg = "is repetitive" + case "checkPublicInterface": + r.checkPublicInterface = true + default: + panic(fmt.Sprintf("Unknown configuration flag %s for %s rule", flagStr, r.Name())) + } } - r.configured = true } - r.Unlock() } // Apply applies the rule to given file. @@ -77,39 +84,6 @@ func (*ExportedRule) Name() string { return "exported" } -func (r *ExportedRule) getConf(args lint.Arguments) ( - checkPrivateReceivers, - disableStutteringCheck, - sayRepetitiveInsteadOfStutters bool, - checkPublicInterface bool, -) { - // if any, we expect a slice of strings as configuration - if len(args) < 1 { - return - } - for _, flag := range args { - flagStr, ok := flag.(string) - if !ok { - panic(fmt.Sprintf("Invalid argument for the %s rule: expecting a string, got %T", r.Name(), flag)) - } - - switch flagStr { - case "checkPrivateReceivers": - checkPrivateReceivers = true - case "disableStutteringCheck": - disableStutteringCheck = true - case "sayRepetitiveInsteadOfStutters": - sayRepetitiveInsteadOfStutters = true - case "checkPublicInterface": - checkPublicInterface = true - default: - panic(fmt.Sprintf("Unknown configuration flag %s for %s rule", flagStr, r.Name())) - } - } - - return -} - type lintExported struct { file *lint.File fileAst *ast.File From 058cc567291e1e7399f733c7c370346b4a208d80 Mon Sep 17 00:00:00 2001 From: comdiv Date: Mon, 22 Jul 2024 11:06:21 +0500 Subject: [PATCH 8/8] test and review fixes --- rule/exported.go | 42 ++++++++++++++++++++----------------- testdata/golint/exported.go | 7 +++++++ 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/rule/exported.go b/rule/exported.go index 08283f646..a1566b9a3 100644 --- a/rule/exported.go +++ b/rule/exported.go @@ -27,12 +27,12 @@ func (r *ExportedRule) configure(arguments lint.Arguments) { r.Lock() defer r.Unlock() if !r.configured { + r.stuttersMsg = "stutters" for _, flag := range arguments { flagStr, ok := flag.(string) if !ok { panic(fmt.Sprintf("Invalid argument for the %s rule: expecting a string, got %T", r.Name(), flag)) } - r.stuttersMsg = "stutters" switch flagStr { case "checkPrivateReceivers": r.checkPrivateReceivers = true @@ -203,14 +203,18 @@ func (w *lintExported) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) { break } } - if !strings.HasPrefix(s, t.Name.Name+" ") { - w.onFailure(lint.Failure{ - Node: doc, - Confidence: 1, - Category: "comments", - Failure: fmt.Sprintf(`comment on exported type %v should be of the form "%v ..." (with optional leading article)`, t.Name, t.Name), - }) + // if comment starts wih name of type and has some text after - it's ok + expectedPrefix := t.Name.Name+" " + if strings.HasPrefix(s, expectedPrefix){ + return } + w.onFailure(lint.Failure{ + Node: doc, + Confidence: 1, + Category: "comments", + Failure: fmt.Sprintf(`comment on exported type %v should be of the form "%s..." (with optional leading article)`, t.Name, expectedPrefix), + }) + } func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genDeclMissingComments map[*ast.GenDecl]bool) { @@ -290,7 +294,7 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD // // This function is needed because ast.CommentGroup.Text() does not handle //-style and /*-style comments uniformly func normalizeText(t string) string { - return strings.TrimPrefix(t, " ") + return strings.TrimSpace(t) } func (w *lintExported) Visit(n ast.Node) ast.Visitor { @@ -338,17 +342,17 @@ func (w *lintExported) Visit(n ast.Node) ast.Visitor { func (w *lintExported) doCheckPublicInterface(typeName string, iface *ast.InterfaceType) { for _, m := range iface.Methods.List { - // case of ast.Ident and other implicit fields - if len(m.Names) == 0 { - continue - } - if ast.IsExported(m.Names[0].Name) { - w.lintInterfaceMethod(typeName, m) - } + w.lintInterfaceMethod(typeName, m) } } func (w *lintExported) lintInterfaceMethod(typeName string, m *ast.Field) { + if len(m.Names) == 0 { + return + } + if !ast.IsExported(m.Names[0].Name) { + return + } name := m.Names[0].Name if m.Doc == nil { w.onFailure(lint.Failure{ @@ -360,13 +364,13 @@ func (w *lintExported) lintInterfaceMethod(typeName string, m *ast.Field) { return } s := normalizeText(m.Doc.Text()) - prefix := m.Names[0].Name + " " - if !strings.HasPrefix(s, prefix) { + expectedPrefix := m.Names[0].Name + " " + if !strings.HasPrefix(s, expectedPrefix) { w.onFailure(lint.Failure{ Node: m.Doc, Confidence: 0.8, Category: "comments", - Failure: fmt.Sprintf(`comment on exported interface method %s.%s should be of the form "%s..."`, typeName, name, prefix), + Failure: fmt.Sprintf(`comment on exported interface method %s.%s should be of the form "%s..."`, typeName, name, expectedPrefix), }) } } diff --git a/testdata/golint/exported.go b/testdata/golint/exported.go index 9b93503f6..022d422c3 100644 --- a/testdata/golint/exported.go +++ b/testdata/golint/exported.go @@ -33,6 +33,13 @@ const FirstLetter = "A" /*Bar2 something */ type Bar2 struct{} + +/* Bar3 */ // MATCH /comment on exported type Bar3 should be of the form "Bar3 ..." (with optional leading article)/ +type Bar3 struct{} + +/* BarXXX invalid */ // MATCH /comment on exported type Bar4 should be of the form "Bar4 ..." (with optional leading article)/ +type Bar4 struct{} + /*Toto2 something */ func Toto2() {}