From 343da9fbf266034f3dc7f128fabd827c8ba64fcd Mon Sep 17 00:00:00 2001 From: chavacava Date: Mon, 7 Apr 2025 20:59:55 +0200 Subject: [PATCH] Refactor/exported (#1299) * simplify comment analysis by just checking the first comment line with documentation information * nit refactorings: common func for adding failures, reduce the use of "stutter", and other nitties * removes use of "stutter" in test code * move string interpolation into addFailuref --- rule/exported.go | 328 +++++++++++++++++++++--------------------- rule/exported_test.go | 34 ++--- 2 files changed, 183 insertions(+), 179 deletions(-) diff --git a/rule/exported.go b/rule/exported.go index 8585f40..2bcc458 100644 --- a/rule/exported.go +++ b/rule/exported.go @@ -19,7 +19,7 @@ type disabledChecks struct { Method bool PrivateReceivers bool PublicInterfaces bool - Stuttering bool + RepetitiveNames bool Type bool Var bool } @@ -46,7 +46,7 @@ func (dc *disabledChecks) isDisabled(checkName string) bool { case checkNamePublicInterfaces: return dc.PublicInterfaces case checkNameStuttering: - return dc.Stuttering + return dc.RepetitiveNames case "type": return dc.Type default: @@ -65,16 +65,16 @@ var commonMethods = map[string]bool{ // ExportedRule lints naming and commenting conventions on exported symbols. type ExportedRule struct { - stuttersMsg string - disabledChecks disabledChecks + isRepetitiveMsg string + disabledChecks disabledChecks } // Configure validates the rule configuration, and configures the rule accordingly. // -// Configuration implements the [lint.ConfigurableRule] interface. +// Configure makes the rule implement the [lint.ConfigurableRule] interface. func (r *ExportedRule) Configure(arguments lint.Arguments) error { r.disabledChecks = disabledChecks{PrivateReceivers: true, PublicInterfaces: true} - r.stuttersMsg = "stutters" + r.isRepetitiveMsg = "stutters" for _, flag := range arguments { switch flag := flag.(type) { case string: @@ -82,9 +82,9 @@ func (r *ExportedRule) Configure(arguments lint.Arguments) error { case isRuleOption(flag, "checkPrivateReceivers"): r.disabledChecks.PrivateReceivers = false case isRuleOption(flag, "disableStutteringCheck"): - r.disabledChecks.Stuttering = true + r.disabledChecks.RepetitiveNames = true case isRuleOption(flag, "sayRepetitiveInsteadOfStutters"): - r.stuttersMsg = "is repetitive" + r.isRepetitiveMsg = "is repetitive" case isRuleOption(flag, "checkPublicInterface"): r.disabledChecks.PublicInterfaces = false case isRuleOption(flag, "disableChecksOnConstants"): @@ -115,20 +115,17 @@ func (r *ExportedRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { return failures } - fileAst := file.AST - walker := lintExported{ - file: file, - fileAst: fileAst, + file: file, onFailure: func(failure lint.Failure) { failures = append(failures, failure) }, genDeclMissingComments: map[*ast.GenDecl]bool{}, - stuttersMsg: r.stuttersMsg, + isRepetitiveMsg: r.isRepetitiveMsg, disabledChecks: r.disabledChecks, } - ast.Walk(&walker, fileAst) + ast.Walk(&walker, file.AST) return failures } @@ -140,11 +137,10 @@ func (*ExportedRule) Name() string { type lintExported struct { file *lint.File - fileAst *ast.File - lastGen *ast.GenDecl + lastGenDecl *ast.GenDecl // the last visited general declaration in the AST genDeclMissingComments map[*ast.GenDecl]bool onFailure func(lint.Failure) - stuttersMsg string + isRepetitiveMsg string disabledChecks disabledChecks } @@ -155,26 +151,13 @@ func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) { kind := "function" name := fn.Name.Name - isMethod := fn.Recv != nil && len(fn.Recv.List) > 0 - if isMethod { + if isMethod := fn.Recv != nil && len(fn.Recv.List) > 0; isMethod { + if !w.mustCheckMethod(fn) { + return + } + kind = "method" recv := typeparams.ReceiverType(fn) - - if !ast.IsExported(recv) && w.disabledChecks.PrivateReceivers { - return - } - - if commonMethods[name] { - return - } - - switch name { - case "Len", "Less", "Swap": - sortables := w.file.Pkg.Sortable() - if sortables[recv] { - return - } - } name = recv + "." + name } @@ -182,42 +165,37 @@ func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) { return } - if !hasDocComment(fn.Doc) { - w.onFailure(lint.Failure{ - Node: fn, - Confidence: 1, - Category: lint.FailureCategoryComments, - Failure: fmt.Sprintf("exported %s %s should have comment or be unexported", kind, name), - }) + firstCommentLine := firstCommentLine(fn.Doc) + + if firstCommentLine == "" { + w.addFailuref(fn, 1, lint.FailureCategoryComments, + "exported %s %s should have comment or be unexported", kind, name, + ) return } - s := normalizeText(fn.Doc.Text()) prefix := fn.Name.Name + " " - if !strings.HasPrefix(s, prefix) { - w.onFailure(lint.Failure{ - Node: fn.Doc, - Confidence: 0.8, - Category: lint.FailureCategoryComments, - Failure: fmt.Sprintf(`comment on exported %s %s should be of the form "%s..."`, kind, name, prefix), - }) + if !strings.HasPrefix(firstCommentLine, prefix) { + w.addFailuref(fn.Doc, 0.8, lint.FailureCategoryComments, + `comment on exported %s %s should be of the form "%s..."`, kind, name, prefix, + ) } } -func (w *lintExported) checkStutter(id *ast.Ident, thing string) { - if w.disabledChecks.Stuttering { +func (w *lintExported) checkRepetitiveNames(id *ast.Ident, thing string) { + if w.disabledChecks.RepetitiveNames { return } - pkg, name := w.fileAst.Name.Name, id.Name + pkg, name := w.file.AST.Name.Name, id.Name if !ast.IsExported(name) { // unexported name return } - // A name stutters if the package name is a strict prefix + // A name is repetitive if the package name is a strict prefix // and the next character of the name starts a new word. if len(name) <= len(pkg) { - // name is too short to stutter. + // name is too short to be a repetition. // This permits the name to be the same as the package name. return } @@ -226,63 +204,74 @@ func (w *lintExported) checkStutter(id *ast.Ident, thing string) { } // We can assume the name is well-formed UTF-8. // If the next rune after the package name is uppercase or an underscore - // the it's starting a new word and thus this name stutters. + // the it's starting a new word and thus this name is repetitive. rem := name[len(pkg):] if next, _ := utf8.DecodeRuneInString(rem); next == '_' || unicode.IsUpper(next) { - w.onFailure(lint.Failure{ - Node: id, - Confidence: 0.8, - Category: lint.FailureCategoryNaming, - Failure: fmt.Sprintf("%s name will be used as %s.%s by other packages, and that %s; consider calling this %s", thing, pkg, name, w.stuttersMsg, rem), - }) + w.addFailuref(id, 0.8, lint.FailureCategoryNaming, + "%s name will be used as %s.%s by other packages, and that %s; consider calling this %s", thing, pkg, name, w.isRepetitiveMsg, rem, + ) } } -func (w *lintExported) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) { +var articles = [...]string{"A", "An", "The", "This"} + +func (w *lintExported) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup, firstCommentLine string) { if w.disabledChecks.isDisabled("type") { return } - if !ast.IsExported(t.Name.Name) { + typeName := t.Name.Name + + if !ast.IsExported(typeName) { return } - if !hasDocComment(doc) { - w.onFailure(lint.Failure{ - Node: t, - Confidence: 1, - Category: lint.FailureCategoryComments, - Failure: fmt.Sprintf("exported type %v should have comment or be unexported", t.Name), - }) + if firstCommentLine == "" { + w.addFailuref(t, 1, lint.FailureCategoryComments, + "exported type %v should have comment or be unexported", t.Name, + ) return } - s := normalizeText(doc.Text()) - articles := [...]string{"A", "An", "The", "This"} for _, a := range articles { - if t.Name.Name == a { + if typeName == a { continue } var found bool - if s, found = strings.CutPrefix(s, a+" "); found { + if firstCommentLine, found = strings.CutPrefix(firstCommentLine, a+" "); found { break } } // if comment starts with name of type and has some text after - it's ok - expectedPrefix := t.Name.Name + " " - if strings.HasPrefix(s, expectedPrefix) { + expectedPrefix := typeName + " " + if strings.HasPrefix(firstCommentLine, expectedPrefix) { return } - w.onFailure(lint.Failure{ - Node: doc, - Confidence: 1, - Category: lint.FailureCategoryComments, - Failure: fmt.Sprintf(`comment on exported type %v should be of the form "%s..." (with optional leading article)`, t.Name, expectedPrefix), - }) + w.addFailuref(doc, 1, lint.FailureCategoryComments, + `comment on exported type %v should be of the form "%s..." (with optional leading article)`, t.Name, expectedPrefix, + ) } +// checkValueNames returns true if names check, false otherwise +func (w *lintExported) checkValueNames(names []*ast.Ident, nodeToBlame ast.Node, kind string) bool { + // Check that none are exported except for the first. + if len(names) < 2 { + return true // nothing to check + } + + for _, n := range names[1:] { + if ast.IsExported(n.Name) { + w.addFailuref(nodeToBlame, 1, lint.FailureCategoryComments, + "exported %s %s should have its own declaration", kind, n.Name, + ) + return false + } + } + + return true +} func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genDeclMissingComments map[*ast.GenDecl]bool) { kind := "var" if gd.Tok == token.CONST { @@ -293,19 +282,8 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD return } - if len(vs.Names) > 1 { - // Check that none are exported except for the first. - for _, n := range vs.Names[1:] { - if ast.IsExported(n.Name) { - w.onFailure(lint.Failure{ - Category: lint.FailureCategoryComments, - Confidence: 1, - Failure: fmt.Sprintf("exported %s %s should have its own declaration", kind, n.Name), - Node: vs, - }) - return - } - } + if !w.checkValueNames(vs.Names, vs, kind) { + return } // Only one name. @@ -314,7 +292,9 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD return } - if !hasDocComment(vs.Doc) && !hasDocComment(gd.Doc) { + vsFirstCommentLine := firstCommentLine(vs.Doc) + gdFirstCommentLine := firstCommentLine(gd.Doc) + if vsFirstCommentLine == "" && gdFirstCommentLine == "" { if genDeclMissingComments[gd] { return } @@ -322,99 +302,95 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD if kind == "const" && gd.Lparen.IsValid() { block = " (or a comment on this block)" } - w.onFailure(lint.Failure{ - Confidence: 1, - Node: vs, - Category: lint.FailureCategoryComments, - Failure: fmt.Sprintf("exported %s %s should have comment%s or be unexported", kind, name, block), - }) + w.addFailuref(vs, 1, lint.FailureCategoryComments, + "exported %s %s should have comment%s or be unexported", kind, name, block, + ) genDeclMissingComments[gd] = true return } + // If this GenDecl has parens and a comment, we don't check its comment form. - if hasDocComment(gd.Doc) && gd.Lparen.IsValid() { + if gdFirstCommentLine != "" && gd.Lparen.IsValid() { return } + // The relevant text to check will be on either vs.Doc or gd.Doc. // Use vs.Doc preferentially. var doc *ast.CommentGroup switch { - case hasDocComment(vs.Doc): + case vsFirstCommentLine != "": doc = vs.Doc - case hasDocComment(vs.Comment) && !hasDocComment(gd.Doc): + case vsFirstCommentLine != "" && gdFirstCommentLine == "": doc = vs.Comment default: doc = gd.Doc } prefix := name + " " - s := normalizeText(doc.Text()) - if !strings.HasPrefix(s, prefix) { - w.onFailure(lint.Failure{ - Confidence: 1, - Node: doc, - Category: lint.FailureCategoryComments, - Failure: fmt.Sprintf(`comment on exported %s %s should be of the form "%s..."`, kind, name, prefix), - }) + if !strings.HasPrefix(firstCommentLine(doc), prefix) { + w.addFailuref(doc, 1, lint.FailureCategoryComments, + `comment on exported %s %s should be of the form "%s..."`, kind, name, prefix, + ) } } -// hasDocComment reports whether the comment group contains a documentation comment, -// excluding directive comments and those consisting solely of a deprecation notice. -// e.g. //go:embed foo.txt a directive comment, not a text comment -// e.g. //nolint:whatever is a directive comment, not a text comment -// e.g. // Deprecated: this is a deprecation comment -func hasDocComment(comment *ast.CommentGroup) bool { +// firstCommentLine yields the first line of interest in comment group or "" if there is nothing of interest. +// An "interesting line" is a comment line that is neither a directive (e.g. //go:...) or a deprecation comment +// (lines from the first line with a prefix // Deprecated: to the end of the comment group) +// Empty or spaces-only lines are discarded. +func firstCommentLine(comment *ast.CommentGroup) (result string) { if comment == nil { - return false + return "" } - // a comment could be directive and not a text comment - text := comment.Text() // removes directives from the comment block - return text != "" && !isOnlyDeprecationComment(text) -} + commentWithoutDirectives := comment.Text() // removes directives from the comment block + lines := strings.Split(commentWithoutDirectives, "\n") + for _, line := range lines { + line := strings.TrimSpace(line) + if line == "" { + continue // ignore empty lines + } + if strings.HasPrefix(line, "Deprecated: ") { + break // ignore deprecation comment line and the subsequent lines of the original comment + } -// isOnlyDeprecationComment returns true if the comment starts with a standard deprecation notice. -// It considers all paragraphs following the deprecation notice as part of the deprecation comment. -// Assumes the comment is following the general ordering convention: (doc comment + deprecation) -func isOnlyDeprecationComment(comment string) bool { - return strings.HasPrefix(comment, "Deprecated: ") -} + result = line + break // first non-directive/non-empty/non-deprecation comment line found + } -// normalizeText is a helper function that normalizes comment strings by: -// * removing one leading space -// -// This function is needed because ast.CommentGroup.Text() does not handle //-style and /*-style comments uniformly -func normalizeText(t string) string { - return strings.TrimSpace(t) + return result } func (w *lintExported) Visit(n ast.Node) ast.Visitor { switch v := n.(type) { case *ast.GenDecl: - if v.Tok == token.IMPORT { + switch v.Tok { + case token.IMPORT: return nil + case token.CONST, token.TYPE, token.VAR: + w.lastGenDecl = v } - // token.CONST, token.TYPE or token.VAR - w.lastGen = v return w case *ast.FuncDecl: w.lintFuncDoc(v) if v.Recv == nil { - // Only check for stutter on functions, not methods. + // Only check for repetitive names on functions, not methods. // Method names are not used package-qualified. - w.checkStutter(v.Name, "func") + w.checkRepetitiveNames(v.Name, "func") } // Don't proceed inside funcs. return nil case *ast.TypeSpec: // inside a GenDecl, which usually has the doc doc := v.Doc - if !hasDocComment(doc) { - doc = w.lastGen.Doc + + fcl := firstCommentLine(doc) + if fcl == "" { + doc = w.lastGenDecl.Doc + fcl = firstCommentLine(doc) } - w.lintTypeDoc(v, doc) - w.checkStutter(v.Name, "type") + w.lintTypeDoc(v, doc, fcl) + w.checkRepetitiveNames(v.Name, "type") if !w.disabledChecks.PublicInterfaces { if iface, ok := v.Type.(*ast.InterfaceType); ok { @@ -426,7 +402,7 @@ func (w *lintExported) Visit(n ast.Node) ast.Visitor { return nil case *ast.ValueSpec: - w.lintValueSpecDoc(v, w.lastGen, w.genDeclMissingComments) + w.lintValueSpecDoc(v, w.lastGenDecl, w.genDeclMissingComments) return nil } return w @@ -446,23 +422,51 @@ func (w *lintExported) lintInterfaceMethod(typeName string, m *ast.Field) { return } name := m.Names[0].Name - if !hasDocComment(m.Doc) { - w.onFailure(lint.Failure{ - Node: m, - Confidence: 1, - Category: lint.FailureCategoryComments, - Failure: fmt.Sprintf("public interface method %s.%s should be commented", typeName, name), - }) + firstCommentLine := firstCommentLine(m.Doc) + if firstCommentLine == "" { + w.addFailuref(m, 1, lint.FailureCategoryComments, + "public interface method %s.%s should be commented", typeName, name, + ) return } - s := normalizeText(m.Doc.Text()) + expectedPrefix := m.Names[0].Name + " " - if !strings.HasPrefix(s, expectedPrefix) { - w.onFailure(lint.Failure{ - Node: m.Doc, - Confidence: 0.8, - Category: lint.FailureCategoryComments, - Failure: fmt.Sprintf(`comment on exported interface method %s.%s should be of the form "%s..."`, typeName, name, expectedPrefix), - }) + if !strings.HasPrefix(firstCommentLine, expectedPrefix) { + w.addFailuref(m.Doc, 0.8, lint.FailureCategoryComments, + `comment on exported interface method %s.%s should be of the form "%s..."`, typeName, name, expectedPrefix, + ) } } + +// mustCheckMethod returns true if the method must be checked by this rule, false otherwise +func (w *lintExported) mustCheckMethod(fn *ast.FuncDecl) bool { + recv := typeparams.ReceiverType(fn) + + if !ast.IsExported(recv) && w.disabledChecks.PrivateReceivers { + return false + } + + name := fn.Name.Name + if commonMethods[name] { + return false + } + + switch name { + case "Len", "Less", "Swap": + sortables := w.file.Pkg.Sortable() + if sortables[recv] { + return false + } + } + + return true +} + +func (w *lintExported) addFailuref(node ast.Node, confidence float64, category lint.FailureCategory, message string, args ...any) { + w.onFailure(lint.Failure{ + Node: node, + Confidence: confidence, + Category: category, + Failure: fmt.Sprintf(message, args...), + }) +} diff --git a/rule/exported_test.go b/rule/exported_test.go index cfbb355..9064148 100644 --- a/rule/exported_test.go +++ b/rule/exported_test.go @@ -9,11 +9,11 @@ import ( func TestExportedRule_Configure(t *testing.T) { tests := []struct { - name string - arguments lint.Arguments - wantErr error - wantDisabledChecks disabledChecks - wantStuttersMsg string + name string + arguments lint.Arguments + wantErr error + wantDisabledChecks disabledChecks + wantIsRepetitiveMsg string }{ { name: "default configuration", @@ -23,7 +23,7 @@ func TestExportedRule_Configure(t *testing.T) { PrivateReceivers: true, PublicInterfaces: true, }, - wantStuttersMsg: "stutters", + wantIsRepetitiveMsg: "stutters", }, { name: "valid arguments", @@ -44,11 +44,11 @@ func TestExportedRule_Configure(t *testing.T) { Const: true, Function: true, Method: true, - Stuttering: true, + RepetitiveNames: true, Type: true, Var: true, }, - wantStuttersMsg: "stutters", + wantIsRepetitiveMsg: "stutters", }, { name: "valid lowercased arguments", @@ -69,11 +69,11 @@ func TestExportedRule_Configure(t *testing.T) { Const: true, Function: true, Method: true, - Stuttering: true, + RepetitiveNames: true, Type: true, Var: true, }, - wantStuttersMsg: "stutters", + wantIsRepetitiveMsg: "stutters", }, { name: "valid kebab-cased arguments", @@ -94,11 +94,11 @@ func TestExportedRule_Configure(t *testing.T) { Const: true, Function: true, Method: true, - Stuttering: true, + RepetitiveNames: true, Type: true, Var: true, }, - wantStuttersMsg: "stutters", + wantIsRepetitiveMsg: "stutters", }, { name: "valid sayRepetitiveInsteadOfStutters", @@ -110,7 +110,7 @@ func TestExportedRule_Configure(t *testing.T) { PrivateReceivers: true, PublicInterfaces: true, }, - wantStuttersMsg: "is repetitive", + wantIsRepetitiveMsg: "is repetitive", }, { name: "valid lowercased sayRepetitiveInsteadOfStutters", @@ -122,7 +122,7 @@ func TestExportedRule_Configure(t *testing.T) { PrivateReceivers: true, PublicInterfaces: true, }, - wantStuttersMsg: "is repetitive", + wantIsRepetitiveMsg: "is repetitive", }, { name: "valid kebab-cased sayRepetitiveInsteadOfStutters", @@ -134,7 +134,7 @@ func TestExportedRule_Configure(t *testing.T) { PrivateReceivers: true, PublicInterfaces: true, }, - wantStuttersMsg: "is repetitive", + wantIsRepetitiveMsg: "is repetitive", }, { name: "unknown configuration flag", @@ -170,8 +170,8 @@ func TestExportedRule_Configure(t *testing.T) { if rule.disabledChecks != tt.wantDisabledChecks { t.Errorf("unexpected disabledChecks: got = %+v, want %+v", rule.disabledChecks, tt.wantDisabledChecks) } - if rule.stuttersMsg != tt.wantStuttersMsg { - t.Errorf("unexpected stuttersMsg: got = %v, want %v", rule.stuttersMsg, tt.wantStuttersMsg) + if rule.isRepetitiveMsg != tt.wantIsRepetitiveMsg { + t.Errorf("unexpected stuttersMsg: got = %v, want %v", rule.isRepetitiveMsg, tt.wantIsRepetitiveMsg) } }) }