Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rule.exported: add feature to disable selected warnings for rule (const,method,function) #1047

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion RULES_DESCRIPTIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -475,12 +475,17 @@ Available flags are:
* _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
* _disableChecksOnConstants_ disable all checks on constant declarations
* _disableChecksOnFunctions_ disable all checks on function declarations
* _disableChecksOnMethods_ disable all checks on method declarations
* _disableChecksOnTypes_ disable all checks on type declarations
* _disableChecksOnVariables_ disable all checks on variable declarations

Example:

```toml
[rule.exported]
arguments = ["checkPrivateReceivers", "disableStutteringCheck", "checkPublicInterface"]
arguments = ["checkPrivateReceivers", "disableStutteringCheck", "checkPublicInterface", "disableChecksOnFunctions"]
```

## file-header
Expand Down
132 changes: 99 additions & 33 deletions rule/exported.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,40 +13,92 @@ import (
"github.com/mgechev/revive/lint"
)

// disabledChecks store ignored warnings types
type disabledChecks struct {
Const bool
Function bool
Method bool
PrivateReceivers bool
PublicInterfaces bool
Stuttering bool
Type bool
Var bool
}

const checkNamePrivateReceivers = "privateReceivers"
const checkNamePublicInterfaces = "publicInterfaces"
const checkNameStuttering = "stuttering"

mfederowicz marked this conversation as resolved.
Show resolved Hide resolved
// isDisabled returns true if the given check is disabled, false otherwise
func (dc *disabledChecks) isDisabled(checkName string) bool {
switch checkName {
case "var":
return dc.Var
case "const":
return dc.Const
case "function":
return dc.Function
case "method":
return dc.Method
case checkNamePrivateReceivers:
return dc.PrivateReceivers
case checkNamePublicInterfaces:
return dc.PublicInterfaces
case checkNameStuttering:
return dc.Stuttering
case "type":
return dc.Type
default:
return false
}
}

// ExportedRule lints given else constructs.
type ExportedRule struct {
configured bool
checkPrivateReceivers bool
disableStutteringCheck bool
checkPublicInterface bool
stuttersMsg string
configured bool
stuttersMsg string
disabledChecks disabledChecks
sync.Mutex
}

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))
}
switch flagStr {
if r.configured {
return
}
r.configured = true

r.disabledChecks = disabledChecks{PrivateReceivers: true, PublicInterfaces: true}
r.stuttersMsg = "stutters"
for _, flag := range arguments {
switch flag := flag.(type) {
case string:
switch flag {
case "checkPrivateReceivers":
r.checkPrivateReceivers = true
r.disabledChecks.PrivateReceivers = false
case "disableStutteringCheck":
r.disableStutteringCheck = true
r.disabledChecks.Stuttering = true
case "sayRepetitiveInsteadOfStutters":
r.stuttersMsg = "is repetitive"
case "checkPublicInterface":
r.checkPublicInterface = true
r.disabledChecks.PublicInterfaces = false
case "disableChecksOnConstants":
r.disabledChecks.Const = true
case "disableChecksOnFunctions":
r.disabledChecks.Function = true
case "disableChecksOnMethods":
r.disabledChecks.Method = true
case "disableChecksOnTypes":
r.disabledChecks.Type = true
case "disableChecksOnVariables":
r.disabledChecks.Var = true
default:
panic(fmt.Sprintf("Unknown configuration flag %s for %s rule", flagStr, r.Name()))
panic(fmt.Sprintf("Unknown configuration flag %s for %s rule", flag, r.Name()))
}
default:
panic(fmt.Sprintf("Invalid argument for the %s rule: expecting a string, got %T", r.Name(), flag))
}
r.configured = true
}
}

Expand All @@ -68,10 +120,8 @@ func (r *ExportedRule) Apply(file *lint.File, args lint.Arguments) []lint.Failur
failures = append(failures, failure)
},
genDeclMissingComments: make(map[*ast.GenDecl]bool),
checkPrivateReceivers: r.checkPrivateReceivers,
disableStutteringCheck: r.disableStutteringCheck,
checkPublicInterface: r.checkPublicInterface,
stuttersMsg: r.stuttersMsg,
disabledChecks: r.disabledChecks,
}

ast.Walk(&walker, fileAst)
Expand All @@ -90,30 +140,30 @@ type lintExported struct {
lastGen *ast.GenDecl
genDeclMissingComments map[*ast.GenDecl]bool
onFailure func(lint.Failure)
checkPrivateReceivers bool
disableStutteringCheck bool
checkPublicInterface bool
stuttersMsg string
disabledChecks disabledChecks
}

func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) {
if !ast.IsExported(fn.Name.Name) {
// func is unexported
return
return // func is unexported, nothing to do
}

kind := "function"
name := fn.Name.Name
if fn.Recv != nil && len(fn.Recv.List) > 0 {
// method
isMethod := fn.Recv != nil && len(fn.Recv.List) > 0
if isMethod {
kind = "method"
recv := typeparams.ReceiverType(fn)
if !w.checkPrivateReceivers && !ast.IsExported(recv) {
// receiver is unexported

if !ast.IsExported(recv) && w.disabledChecks.PrivateReceivers {
return
}

if commonMethods[name] {
return
}

switch name {
case "Len", "Less", "Swap":
sortables := w.file.Pkg.Sortable()
Expand All @@ -123,6 +173,11 @@ func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) {
}
name = recv + "." + name
}

if w.disabledChecks.isDisabled(kind) {
return
}

if fn.Doc == nil {
w.onFailure(lint.Failure{
Node: fn,
Expand All @@ -132,6 +187,7 @@ func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) {
})
return
}

s := normalizeText(fn.Doc.Text())
prefix := fn.Name.Name + " "
if !strings.HasPrefix(s, prefix) {
Expand All @@ -145,7 +201,7 @@ func (w *lintExported) lintFuncDoc(fn *ast.FuncDecl) {
}

func (w *lintExported) checkStutter(id *ast.Ident, thing string) {
if w.disableStutteringCheck {
if w.disabledChecks.Stuttering {
return
}

Expand Down Expand Up @@ -179,9 +235,14 @@ func (w *lintExported) checkStutter(id *ast.Ident, thing string) {
}

func (w *lintExported) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) {
if w.disabledChecks.isDisabled("type") {
return
}

if !ast.IsExported(t.Name.Name) {
return
}

if doc == nil {
w.onFailure(lint.Failure{
Node: t,
Expand All @@ -203,18 +264,19 @@ func (w *lintExported) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) {
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) {
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) {
Expand All @@ -223,6 +285,10 @@ func (w *lintExported) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genD
kind = "const"
}

if w.disabledChecks.isDisabled(kind) {
return
}

if len(vs.Names) > 1 {
// Check that none are exported except for the first.
for _, n := range vs.Names[1:] {
Expand Down Expand Up @@ -324,7 +390,7 @@ func (w *lintExported) Visit(n ast.Node) ast.Visitor {
w.lintTypeDoc(v, doc)
w.checkStutter(v.Name, "type")

if w.checkPublicInterface {
if !w.disabledChecks.PublicInterfaces {
if iface, ok := v.Type.(*ast.InterfaceType); ok {
if ast.IsExported(v.Name.Name) {
w.doCheckPublicInterface(v.Name.Name, iface)
Expand Down
6 changes: 6 additions & 0 deletions test/exported_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,9 @@ func TestCheckPublicInterfaceOption(t *testing.T) {

testRule(t, "exported-issue-1002", &rule.ExportedRule{}, &lint.RuleConfig{Arguments: args})
}

func TestCheckDisablingOnDeclarationTypes(t *testing.T) {
args := []any{"disableChecksOnConstants", "disableChecksOnFunctions", "disableChecksOnMethods", "disableChecksOnTypes", "disableChecksOnVariables"}

mfederowicz marked this conversation as resolved.
Show resolved Hide resolved
testRule(t, "exported-issue-1045", &rule.ExportedRule{}, &lint.RuleConfig{Arguments: args})
}
25 changes: 25 additions & 0 deletions testdata/exported-issue-1045.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Package golint comment
package golint


// path separator defined by os.Separator.
const FilePath = "xyz"


// Rewrite string to remove non-standard path characters
func UnicodeSanitize(s string) string {}


// Tags returns a slice of tags. The order is the original tag order unless it
// was changed.
func (t *Tags) Keys() []string {}

// A value which may be passed to the which parameter for Getitimer and
type ItimerWhich int


/*
// PropertyBag
*/
// Rectangle An area within an image.
type Rectangle struct {}
Loading