Skip to content
This repository has been archived by the owner on May 9, 2021. It is now read-only.

Commit

Permalink
Provide lint problem categorisation.
Browse files Browse the repository at this point in the history
  • Loading branch information
@dsymonds
dsymonds committed Jul 8, 2014
1 parent 9646f1a commit 7430178
Showing 1 changed file with 60 additions and 38 deletions.
98 changes: 60 additions & 38 deletions lint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ type Problem struct {
Link string // (optional) the link to the style guide for the problem
Confidence float64 // a value in (0,1] estimating the confidence in this problem's correctness
LineText string // the source line
Category string // a short name for the general category of the problem
}

func (p *Problem) String() string {
Expand Down Expand Up @@ -93,15 +94,35 @@ func (f *file) lint() []Problem {
return f.problems
}

func (f *file) errorf(n ast.Node, confidence float64, link, format string, a ...interface{}) {
type link string
type category string

// The variadic arguments may start with link and category types,
// and must end with a format string and any arguments.
func (f *file) errorf(n ast.Node, confidence float64, args ...interface{}) {
p := f.fset.Position(n.Pos())
f.problems = append(f.problems, Problem{
problem := Problem{
Position: p,
Text: fmt.Sprintf(format, a...),
Link: link,
Confidence: confidence,
LineText: srcLine(f.src, p),
})
}

argLoop:
for len(args) > 1 { // always leave at least the format string in args
switch v := args[0].(type) {
case link:
problem.Link = string(v)
case category:
problem.Category = string(v)
default:
break argLoop
}
args = args[1:]
}

problem.Text = fmt.Sprintf(args[0].(string), args[1:]...)

f.problems = append(f.problems, problem)
}

func (f *file) scanSortable() {
Expand Down Expand Up @@ -151,20 +172,20 @@ func (f *file) lintPackageComment() {
return
}

const link = styleGuideBase + "#Package_Comments"
const ref = styleGuideBase + "#Package_Comments"
if f.f.Doc == nil {
f.errorf(f.f, 0.2, link, "should have a package comment, unless it's in another file for this package")
f.errorf(f.f, 0.2, link(ref), category("comments"), "should have a package comment, unless it's in another file for this package")
return
}
s := f.f.Doc.Text()
prefix := "Package " + f.f.Name.Name + " "
if ts := strings.TrimLeft(s, " \t"); ts != s {
f.errorf(f.f.Doc, 1, link, "package comment should not have leading space")
f.errorf(f.f.Doc, 1, link(ref), category("comments"), "package comment should not have leading space")
s = ts
}
// Only non-main packages need to keep to this form.
if f.f.Name.Name != "main" && !strings.HasPrefix(s, prefix) {
f.errorf(f.f.Doc, 1, link, `package comment should be of the form "%s..."`, prefix)
f.errorf(f.f.Doc, 1, link(ref), category("comments"), `package comment should be of the form "%s..."`, prefix)
}
}

Expand Down Expand Up @@ -194,8 +215,8 @@ func (f *file) lintBlankImports() {

// This is the first blank import of a group.
if imp.Doc == nil && imp.Comment == nil {
link := ""
f.errorf(imp, 1, link, "a blank import should be only in a main or test package, or have a comment justifying it")
ref := ""
f.errorf(imp, 1, link(ref), category("imports"), "a blank import should be only in a main or test package, or have a comment justifying it")
}
}
}
Expand All @@ -206,7 +227,7 @@ func (f *file) lintImports() {
for i, is := range f.f.Imports {
_ = i
if is.Name != nil && is.Name.Name == "." && !f.isTest() {
f.errorf(is, 1, styleGuideBase+"#Import_Dot", "should not use dot imports")
f.errorf(is, 1, link(styleGuideBase+"#Import_Dot"), category("imports"), "should not use dot imports")
}

}
Expand Down Expand Up @@ -268,7 +289,7 @@ var allCapsRE = regexp.MustCompile(`^[A-Z0-9_]+$`)
func (f *file) lintNames() {
// Package names need slightly different handling than other names.
if strings.Contains(f.f.Name.Name, "_") && !strings.HasSuffix(f.f.Name.Name, "_test") {
f.errorf(f.f, 1, "http://golang.org/doc/effective_go.html#package-names", "don't use an underscore in package name")
f.errorf(f.f, 1, link("http://golang.org/doc/effective_go.html#package-names"), category("naming"), "don't use an underscore in package name")
}

check := func(id *ast.Ident, thing string) {
Expand All @@ -278,23 +299,23 @@ func (f *file) lintNames() {

// Handle two common styles from other languages that don't belong in Go.
if len(id.Name) >= 5 && allCapsRE.MatchString(id.Name) && strings.Contains(id.Name, "_") {
f.errorf(id, 0.8, styleGuideBase+"#Mixed_Caps", "don't use ALL_CAPS in Go names; use CamelCase")
f.errorf(id, 0.8, link(styleGuideBase+"#Mixed_Caps"), category("naming"), "don't use ALL_CAPS in Go names; use CamelCase")
return
}
if len(id.Name) > 2 && id.Name[0] == 'k' && id.Name[1] >= 'A' && id.Name[1] <= 'Z' {
should := string(id.Name[1]+'a'-'A') + id.Name[2:]
f.errorf(id, 0.8, "", "don't use leading k in Go names; %s %s should be %s", thing, id.Name, should)
f.errorf(id, 0.8, link("don't use leading k in Go names; %s %s should be %s"), category("naming"), thing, id.Name, should)
}

should := lintName(id.Name)
if id.Name == should {
return
}
if len(id.Name) > 2 && strings.Contains(id.Name[1:], "_") {
f.errorf(id, 0.9, "http://golang.org/doc/effective_go.html#mixed-caps", "don't use underscores in Go names; %s %s should be %s", thing, id.Name, should)
f.errorf(id, 0.9, link("http://golang.org/doc/effective_go.html#mixed-caps"), category("naming"), "don't use underscores in Go names; %s %s should be %s", thing, id.Name, should)
return
}
f.errorf(id, 0.8, styleGuideBase+"#Initialisms", "%s %s should be %s", thing, id.Name, should)
f.errorf(id, 0.8, link(styleGuideBase+"#Initialisms"), category("naming"), "%s %s should be %s", thing, id.Name, should)
}
checkList := func(fl *ast.FieldList, thing string) {
if fl == nil {
Expand All @@ -321,6 +342,7 @@ func (f *file) lintNames() {
if f.isTest() && (strings.HasPrefix(v.Name.Name, "Example") || strings.HasPrefix(v.Name.Name, "Test") || strings.HasPrefix(v.Name.Name, "Benchmark")) {
return true
}

check(v.Name, "func")

thing := "func"
Expand Down Expand Up @@ -487,7 +509,7 @@ func (f *file) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) {
return
}
if doc == nil {
f.errorf(t, 1, docCommentsLink, "exported type %v should have comment or be unexported", t.Name)
f.errorf(t, 1, link(docCommentsLink), category("comments"), "exported type %v should have comment or be unexported", t.Name)
return
}

Expand All @@ -500,7 +522,7 @@ func (f *file) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) {
}
}
if !strings.HasPrefix(s, t.Name.Name+" ") {
f.errorf(doc, 1, docCommentsLink, `comment on exported type %v should be of the form "%v ..." (with optional leading article)`, t.Name, t.Name)
f.errorf(doc, 1, link(docCommentsLink), category("comments"), `comment on exported type %v should be of the form "%v ..." (with optional leading article)`, t.Name, t.Name)
}
}

Expand Down Expand Up @@ -542,13 +564,13 @@ func (f *file) lintFuncDoc(fn *ast.FuncDecl) {
name = recv + "." + name
}
if fn.Doc == nil {
f.errorf(fn, 1, docCommentsLink, "exported %s %s should have comment or be unexported", kind, name)
f.errorf(fn, 1, link(docCommentsLink), category("comments"), "exported %s %s should have comment or be unexported", kind, name)
return
}
s := fn.Doc.Text()
prefix := fn.Name.Name + " "
if !strings.HasPrefix(s, prefix) {
f.errorf(fn.Doc, 1, docCommentsLink, `comment on exported %s %s should be of the form "%s..."`, kind, name, prefix)
f.errorf(fn.Doc, 1, link(docCommentsLink), category("comments"), `comment on exported %s %s should be of the form "%s..."`, kind, name, prefix)
}
}

Expand All @@ -565,7 +587,7 @@ func (f *file) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genDeclMissi
// Check that none are exported except for the first.
for _, n := range vs.Names[1:] {
if ast.IsExported(n.Name) {
f.errorf(vs, 1, "", "exported %s %s should have its own declaration", kind, n.Name)
f.errorf(vs, 1, category("comments"), "exported %s %s should have its own declaration", kind, n.Name)
return
}
}
Expand All @@ -583,14 +605,14 @@ func (f *file) lintValueSpecDoc(vs *ast.ValueSpec, gd *ast.GenDecl, genDeclMissi
if kind == "const" && gd.Lparen.IsValid() {
block = " (or a comment on this block)"
}
f.errorf(vs, 1, docCommentsLink, "exported %s %s should have comment%s or be unexported", kind, name, block)
f.errorf(vs, 1, link(docCommentsLink), category("comments"), "exported %s %s should have comment%s or be unexported", kind, name, block)
genDeclMissingComments[gd] = true
}
return
}
prefix := name + " "
if !strings.HasPrefix(vs.Doc.Text(), prefix) {
f.errorf(vs.Doc, 1, docCommentsLink, `comment on exported %s %s should be of the form "%s..."`, kind, name, prefix)
f.errorf(vs.Doc, 1, link(docCommentsLink), category("comments"), `comment on exported %s %s should be of the form "%s..."`, kind, name, prefix)
}
}

Expand Down Expand Up @@ -645,7 +667,7 @@ func (f *file) lintVarDecls() {
zero = true
}
if zero {
f.errorf(rhs, 0.9, "", "should drop = %s from declaration of var %s; it is the zero value", f.render(rhs), v.Names[0])
f.errorf(rhs, 0.9, category("zero-value"), "should drop = %s from declaration of var %s; it is the zero value", f.render(rhs), v.Names[0])
return false
}
// If the LHS type is an interface, don't warn, since it is probably a
Expand All @@ -660,7 +682,7 @@ func (f *file) lintVarDecls() {
if defType, ok := isUntypedConst(rhs); ok && !isIdent(v.Type, defType) {
return false
}
f.errorf(v.Type, 0.8, "", "should omit type %s from declaration of var %s; it will be inferred from the right-hand side", f.render(v.Type), v.Names[0])
f.errorf(v.Type, 0.8, category("type-inference"), "should omit type %s from declaration of var %s; it will be inferred from the right-hand side", f.render(v.Type), v.Names[0])
return false
}
return true
Expand Down Expand Up @@ -705,7 +727,7 @@ func (f *file) lintElses() {
if shortDecl {
extra = " (move short variable declaration to its own line if necessary)"
}
f.errorf(ifStmt.Else, 1, styleGuideBase+"#Indent_Error_Flow", "if block ends with a return statement, so drop this else and outdent its block"+extra)
f.errorf(ifStmt.Else, 1, link(styleGuideBase+"#Indent_Error_Flow"), category("indent"), "if block ends with a return statement, so drop this else and outdent its block"+extra)
}
return true
})
Expand All @@ -727,7 +749,7 @@ func (f *file) lintRanges() {
return true
}

f.errorf(rs.Value, 1, "", "should omit 2nd value from range; this loop is equivalent to `for %s %s range ...`", f.render(rs.Key), rs.Tok)
f.errorf(rs.Value, 1, category("range-loop"), "should omit 2nd value from range; this loop is equivalent to `for %s %s range ...`", f.render(rs.Key), rs.Tok)
return true
})
}
Expand All @@ -747,7 +769,7 @@ func (f *file) lintErrorf() {
if !ok || !isPkgDot(ce.Fun, "fmt", "Sprintf") {
return true
}
f.errorf(node, 1, "", "should replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...)")
f.errorf(node, 1, category("errors"), "should replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...)")
return true
})
}
Expand Down Expand Up @@ -778,7 +800,7 @@ func (f *file) lintErrors() {
prefix = "Err"
}
if !strings.HasPrefix(id.Name, prefix) {
f.errorf(id, 0.9, "", "error var %s should have name of the form %sFoo", id.Name, prefix)
f.errorf(id, 0.9, category("naming"), "error var %s should have name of the form %sFoo", id.Name, prefix)
}
}
}
Expand Down Expand Up @@ -837,7 +859,7 @@ func (f *file) lintErrorStrings() {
if isCap {
conf = 0.6
}
f.errorf(str, conf, styleGuideBase+"#Error_Strings", msg)
f.errorf(str, conf, link(styleGuideBase+"#Error_Strings"), category("errors"), msg)
return true
})
}
Expand All @@ -862,18 +884,18 @@ func (f *file) lintReceiverNames() {
return true
}
name := names[0].Name
const link = styleGuideBase + "#Receiver_Names"
const ref = styleGuideBase + "#Receiver_Names"
if name == "_" {
f.errorf(n, 1, link, `receiver name should not be an underscore`)
f.errorf(n, 1, link(ref), category("naming"), `receiver name should not be an underscore`)
return true
}
if badReceiverNames[name] {
f.errorf(n, 1, link, `receiver name should be a reflection of its identity; don't use generic names such as "me", "this", or "self"`)
f.errorf(n, 1, link(ref), category("naming"), `receiver name should be a reflection of its identity; don't use generic names such as "me", "this", or "self"`)
return true
}
recv := receiverType(fn)
if prev, ok := typeReceiver[recv]; ok && prev != name {
f.errorf(n, 1, link, "receiver name %s should be consistent with previous receiver name %s for %s", name, prev, recv)
f.errorf(n, 1, link(ref), category("naming"), "receiver name %s should be consistent with previous receiver name %s for %s", name, prev, recv)
return true
}
typeReceiver[recv] = name
Expand Down Expand Up @@ -904,7 +926,7 @@ func (f *file) lintIncDec() {
default:
return true
}
f.errorf(as, 0.8, "", "should replace %s with %s%s", f.render(as), f.render(as.Lhs[0]), suffix)
f.errorf(as, 0.8, category("unary-op"), "should replace %s with %s%s", f.render(as), f.render(as.Lhs[0]), suffix)
return true
})
}
Expand Down Expand Up @@ -933,7 +955,7 @@ func (f *file) lintMake() {
if !ok || at.Len != nil {
return true
}
f.errorf(as, 0.8, "", `can probably use "var %s %s" instead`, f.render(as.Lhs[0]), f.render(at))
f.errorf(as, 0.8, category("slice"), `can probably use "var %s %s" instead`, f.render(as.Lhs[0]), f.render(at))
return true
})
}
Expand All @@ -954,7 +976,7 @@ func (f *file) lintErrorReturn() {
// Flag any error parameters found before the last.
for _, r := range ret[:len(ret)-1] {
if isIdent(r.Type, "error") {
f.errorf(fn, 0.9, "", "error should be the last type when returning multiple items")
f.errorf(fn, 0.9, category("arg-order"), "error should be the last type when returning multiple items")
break // only flag one
}
}
Expand Down

0 comments on commit 7430178

Please sign in to comment.