1
0
mirror of https://github.com/mgechev/revive.git synced 2025-05-31 22:49:41 +02:00

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
This commit is contained in:
chavacava 2025-04-07 20:59:55 +02:00 committed by GitHub
parent 5f32e8bffa
commit 343da9fbf2
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 183 additions and 179 deletions

View File

@ -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...),
})
}

View File

@ -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)
}
})
}