From 637d4d4ce5bbd9d1908d1e7f577c06040ee7dbcf Mon Sep 17 00:00:00 2001 From: Dmitry Vyukov Date: Mon, 30 Nov 2020 15:45:45 +0100 Subject: tools/syz-linter: warn about inconsistent flag definitions --- tools/syz-linter/linter.go | 61 +++++++++++++++++----- .../testdata/src/lintertest/lintertest.go | 8 +++ 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/tools/syz-linter/linter.go b/tools/syz-linter/linter.go index d6fae7fe4..457d57935 100644 --- a/tools/syz-linter/linter.go +++ b/tools/syz-linter/linter.go @@ -73,6 +73,7 @@ func run(p *analysis.Pass) (interface{}, error) { case *ast.FuncType: pass.checkFuncArgs(n) case *ast.CallExpr: + pass.checkFlagDefinition(n) pass.checkLogErrorFormat(n) case *ast.GenDecl: pass.checkVarDecl(n) @@ -220,42 +221,66 @@ func (pass *Pass) reportFuncArgs(fields []*ast.Field, first, last int) { pass.report(fields[first], "Use '%v %v'", names[2:], fields[first].Type) } +func (pass *Pass) checkFlagDefinition(n *ast.CallExpr) { + fun, ok := n.Fun.(*ast.SelectorExpr) + if !ok { + return + } + switch fmt.Sprintf("%v.%v", fun.X, fun.Sel) { + case "flag.Bool", "flag.Duration", "flag.Float64", "flag.Int", "flag.Int64", + "flag.String", "flag.Uint", "flag.Uint64": + default: + return + } + if name, ok := stringLit(n.Args[0]); ok { + if name != strings.ToLower(name) { + pass.report(n, "Don't use Capital letters in flag names") + } + } + if desc, ok := stringLit(n.Args[2]); ok { + if desc == "" { + pass.report(n, "Provide flag description") + } else if last := desc[len(desc)-1]; last == '.' || last == '\n' { + pass.report(n, "Don't use %q at the end of flag description", last) + } + if len(desc) >= 2 && unicode.IsUpper(rune(desc[0])) && unicode.IsLower(rune(desc[1])) { + pass.report(n, "Don't start flag description with a Capital letter") + } + } +} + // checkLogErrorFormat warns about log/error messages starting with capital letter or ending with a period. func (pass *Pass) checkLogErrorFormat(n *ast.CallExpr) { arg, newLine, sure := pass.logFormatArg(n) if arg == -1 { return } - lit, ok := n.Args[arg].(*ast.BasicLit) - if !ok || lit.Kind != token.STRING { - return - } - val, err := strconv.Unquote(lit.Value) - if err != nil { + val, ok := stringLit(n.Args[arg]) + if !ok { return } ln := len(val) if ln == 0 { - pass.report(lit, "Don't use empty log/error messages") + pass.report(n, "Don't use empty log/error messages") return } // Some Printf's legitimately don't need \n, so this check is based on a heuristic. // Printf's that don't need \n tend to contain % and are short. - if !sure && len(val) < 25 && (len(val) < 10 || strings.Contains(val, "%")) { + if !sure && ln < 25 && (ln < 10 || strings.Contains(val, "%")) { return } if val[ln-1] == '.' && (ln < 3 || val[ln-2] != '.' || val[ln-3] != '.') { - pass.report(lit, "Don't use period at the end of log/error messages") + pass.report(n, "Don't use period at the end of log/error messages") } if newLine && val[ln-1] != '\n' { - pass.report(lit, "Add \\n at the end of printed messages") + pass.report(n, "Add \\n at the end of printed messages") } if !newLine && val[ln-1] == '\n' { - pass.report(lit, "Don't use \\n at the end of log/error messages") + pass.report(n, "Don't use \\n at the end of log/error messages") } if ln >= 2 && unicode.IsUpper(rune(val[0])) && unicode.IsLower(rune(val[1])) && !publicIdentifier.MatchString(val) { - pass.report(lit, "Don't start log/error messages with a Capital letter") + pass.report(n, "Don't start log/error messages with a Capital letter") } } @@ -282,6 +307,18 @@ func (pass *Pass) logFormatArg(n *ast.CallExpr) (arg int, newLine, sure bool) { var publicIdentifier = regexp.MustCompile(`^[A-Z][[:alnum:]]+(\.[[:alnum:]]+)+ `) +func stringLit(n ast.Node) (string, bool) { + lit, ok := n.(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return "", false + } + val, err := strconv.Unquote(lit.Value) + if err != nil { + return "", false + } + return val, true +} + // checkVarDecl warns about unnecessary long variable declarations "var x type = foo". func (pass *Pass) checkVarDecl(n *ast.GenDecl) { if n.Tok != token.VAR { diff --git a/tools/syz-linter/testdata/src/lintertest/lintertest.go b/tools/syz-linter/testdata/src/lintertest/lintertest.go index 9fb1d89c9..778ccb6ae 100644 --- a/tools/syz-linter/testdata/src/lintertest/lintertest.go +++ b/tools/syz-linter/testdata/src/lintertest/lintertest.go @@ -4,6 +4,7 @@ package lintertest import ( + "flag" "fmt" "log" "os" @@ -56,6 +57,13 @@ func funcArgsBad1() (a int, b int) { // want "Use 'a, b int'" func funcArgsBad2(a int16, b, c uint32, d uint32, e int16) { // want "Use 'b, c, d uint32'" } +func flagDefinitions() { + flag.Int("good", 0, "fine description") + flag.Int("camelCase", 0, "fine description") // want "Don't use Capital letters in flag names" + flag.String("fine", "", "Capital Letter") // want "Don't start flag description with a Capital letter" + flag.Bool("fine", false, "dot at the end.") // want "Don't use '.' at the end of flag description" +} + func logErrorMessages() { msg := "good message" err := fmt.Errorf("good message") -- cgit mrf-deployment