code review 7305075: vet: improve flag handling

vet: improve flag handling
Simplify the internal logic for flags controlling what to vet,
by introducing a map of flags that gathers them all together.
This change should simplify the process of adding further flags.

Add a test for untagged struct literals.
Delete a redundant test that was also in the wrong file.
Clean up some ERROR patterns that weren't working.

"make test" passes again.

[r, 2013-02-11 21:23:34]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-02-11 21:31:06]

LGTM

On Mon, Feb 11, 2013 at 1:23 PM, <r@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> vet: improve flag handling
> Simplify the internal logic for flags controlling what to vet,
> by introducing a map of flags that gathers them all together.
> This change should simplify the process of adding further flags.
>
> Add a test for untagged struct literals.
> Delete a redundant test that was also in the wrong file.
> Clean up some ERROR patterns that weren't working.
>
> "make test" passes again.
>
> Please review this at
https://codereview.appspot.**com/7305075/<https://codereview.appspot.com/7305...
>
> Affected files:
>   M src/cmd/vet/atomic.go
>   M src/cmd/vet/buildtag.go
>   M src/cmd/vet/main.go
>   M src/cmd/vet/method.go
>   M src/cmd/vet/print.go
>   M src/cmd/vet/rangeloop.go
>   M src/cmd/vet/structtag.go
>   M src/cmd/vet/taglit.go
>
>
> Index: src/cmd/vet/atomic.go
> ==============================**==============================**=======
> --- a/src/cmd/vet/atomic.go
> +++ b/src/cmd/vet/atomic.go
> @@ -13,7 +13,7 @@
>  // checkAtomicAssignment walks the assignment statement checking for
> comomon
>  // mistaken usage of atomic package, such as: x = atomic.AddUint64(&x, 1)
>  func (f *File) checkAtomicAssignment(n *ast.AssignStmt) {
> -       if !*vetAtomic && !*vetAll {
> +       if !vet("atomic") {
>                 return
>         }
>
> Index: src/cmd/vet/buildtag.go
> ==============================**==============================**=======
> --- a/src/cmd/vet/buildtag.go
> +++ b/src/cmd/vet/buildtag.go
> @@ -25,7 +25,7 @@
>
>  // checkBuildTag checks that build tags are in the correct location and
> well-formed.
>  func checkBuildTag(name string, data []byte) {
> -       if !*vetBuildTags && !*vetAll {
> +       if !vet("buildtags") {
>                 return
>         }
>         lines := bytes.SplitAfter(data, nl)
> Index: src/cmd/vet/main.go
> ==============================**==============================**=======
> --- a/src/cmd/vet/main.go
> +++ b/src/cmd/vet/main.go
> @@ -25,18 +25,23 @@
>  var verbose = flag.Bool("v", false, "verbose")
>  var exitCode = 0
>
> -// Flags to control which checks to perform.
> -// NOTE: Add new flags to the if statement at the top of func main too.
> -var (
> -       vetAll             = flag.Bool("all", true, "check everything;
> disabled if any explicit check is requested")
> -       vetAtomic          = flag.Bool("atomic", false, "check for common
> mistaken usages of the sync/atomic package")
> -       vetBuildTags       = flag.Bool("buildtags", false, "check that
> +build tags are valid")
> -       vetMethods         = flag.Bool("methods", false, "check that
> canonically named methods are canonically defined")
> -       vetPrintf          = flag.Bool("printf", false, "check printf-like
> invocations")
> -       vetStructTags      = flag.Bool("structtags", false, "check that
> struct field tags have canonical format")
> -       vetRangeLoops      = flag.Bool("rangeloops", false, "check that
> range loop variables are used correctly")
> -       vetUntaggedLiteral = flag.Bool("composites", false, "check that
> composite literals used type-tagged elements")
> -)
> +// Flags to control which checks to perform. "all" is set to true here,
> and disabled later if
> +// a flag is set explicitly.
> +var report = map[string]*bool{
> +       "all":        flag.Bool("all", true, "check everything; disabled
> if any explicit check is requested"),
> +       "atomic":     flag.Bool("atomic", false, "check for common
> mistaken usages of the sync/atomic package"),
> +       "buildtags":  flag.Bool("buildtags", false, "check that +build
> tags are valid"),
> +       "methods":    flag.Bool("methods", false, "check that canonically
> named methods are canonically defined"),
> +       "printf":     flag.Bool("printf", false, "check printf-like
> invocations"),
> +       "structtags": flag.Bool("structtags", false, "check that struct
> field tags have canonical format"),
> +       "rangeloops": flag.Bool("rangeloops", false, "check that range
> loop variables are used correctly"),
> +       "composites": flag.Bool("composites", false, "check that composite
> literals used type-tagged elements"),
> +}
> +
> +// vet tells whether to report errors for the named check, a flag name.
> +func vet(name string) bool {
> +       return *report["all"] || *report[name]
> +}
>
>  // setExit sets the value for os.Exit when it is called, later.  It
>  // remembers the highest value.
> @@ -66,8 +71,12 @@
>         flag.Parse()
>
>         // If a check is named explicitly, turn off the 'all' flag.
> -       if *vetAtomic || *vetBuildTags || *vetMethods || *vetPrintf ||
> *vetStructTags || *vetRangeLoops || *vetUntaggedLiteral {
> -               *vetAll = false
> +       for name, ptr := range report {
> +               if name != "all" && *ptr {
> +                       *report["all"] = false
> +                       break
> +               }
> +
>         }
>
>         if *printfuncs != "" {
> Index: src/cmd/vet/method.go
> ==============================**==============================**=======
> --- a/src/cmd/vet/method.go
> +++ b/src/cmd/vet/method.go
> @@ -55,7 +55,7 @@
>  }
>
>  func (f *File) checkCanonicalMethod(id *ast.Ident, t *ast.FuncType) {
> -       if !*vetMethods && !*vetAll {
> +       if !vet("methods") {
>                 return
>         }
>         // Expected input/output.
> @@ -161,9 +161,9 @@
>         return f.b.String() == expect
>  }
>
> -func (t *BadTypeUsedInTests) Scan(x fmt.ScanState, c byte) { // ERROR
> "method Scan[(]x fmt.ScanState, c byte[)] should have signature
> Scan[(]fmt.ScanState, rune[)] error"
> +func (t *BadTypeUsedInTests) Scan(x fmt.ScanState, c byte) { // ERROR
> "should have signature Scan"
>  }
>
>  type BadInterfaceUsedInTests interface {
> -       ReadByte() byte // ERROR "method ReadByte[(][)] byte should have
> signature ReadByte[(][)] [(]byte, error[)]"
> +       ReadByte() byte // ERROR "should have signature ReadByte"
>  }
> Index: src/cmd/vet/print.go
> ==============================**==============================**=======
> --- a/src/cmd/vet/print.go
> +++ b/src/cmd/vet/print.go
> @@ -44,7 +44,7 @@
>
>  // checkCall triggers the print-specific checks if the call invokes a
> print function.
>  func (f *File) checkFmtPrintfCall(call *ast.CallExpr, Name string) {
> -       if !*vetPrintf && !*vetAll {
> +       if !vet("printf") {
>                 return
>         }
>         name := strings.ToLower(Name)
> Index: src/cmd/vet/rangeloop.go
> ==============================**==============================**=======
> --- a/src/cmd/vet/rangeloop.go
> +++ b/src/cmd/vet/rangeloop.go
> @@ -27,7 +27,7 @@
>  // its index or value variables are used unsafely inside goroutines or
> deferred
>  // function literals.
>  func checkRangeLoop(f *File, n *ast.RangeStmt) {
> -       if !*vetRangeLoops && !*vetAll {
> +       if !vet("rangeloops") {
>                 return
>         }
>         key, _ := n.Key.(*ast.Ident)
> Index: src/cmd/vet/structtag.go
> ==============================**==============================**=======
> --- a/src/cmd/vet/structtag.go
> +++ b/src/cmd/vet/structtag.go
> @@ -14,7 +14,7 @@
>
>  // checkField checks a struct field tag.
>  func (f *File) checkCanonicalFieldTag(field *ast.Field) {
> -       if !*vetStructTags && !*vetAll {
> +       if !vet("structtags") {
>                 return
>         }
>         if field.Tag == nil {
> @@ -37,5 +37,5 @@
>  }
>
>  type BadTypeUsedInTests struct {
> -       X int "hello" // ERROR "struct field tag"
> +       X int "hello" // ERROR "not compatible with reflect.StructTag.Get"
>  }
> Index: src/cmd/vet/taglit.go
> ==============================**==============================**=======
> --- a/src/cmd/vet/taglit.go
> +++ b/src/cmd/vet/taglit.go
> @@ -9,12 +9,14 @@
>  import (
>         "go/ast"
>         "strings"
> +
> +       "flag" // for test
>  )
>
>  // checkUntaggedLiteral checks if a composite literal is an struct
> literal with
>  // untagged fields.
>  func (f *File) checkUntaggedLiteral(c *ast.CompositeLit) {
> -       if !*vetUntaggedLiteral && !*vetAll {
> +       if !vet("composites") {
>                 return
>         }
>         // Check if the CompositeLit contains an untagged field.
> @@ -123,6 +125,9 @@
>         "image.Rectangle":     true,
>  }
>
> -type BadTag struct {
> -       S string `this is a bad tag` // ERROR "not compatible with
> reflect.StructTag.Get"
> +var BadStructLiteralUsedInTests = flag.Flag{ // ERROR "untagged fields"
> +       "Name",
> +       "Usage",
> +       nil, // Value
> +       "DefValue",
>  }
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[r, 2013-02-11 21:33:17]

*** Submitted as https://code.google.com/p/go/source/detail?r=7d3de949de92 ***

vet: improve flag handling
Simplify the internal logic for flags controlling what to vet,
by introducing a map of flags that gathers them all together.
This change should simplify the process of adding further flags.

Add a test for untagged struct literals.
Delete a redundant test that was also in the wrong file.
Clean up some ERROR patterns that weren't working.

"make test" passes again.

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/7305075

[EduRam, 2013-02-11 23:09:32]

https://codereview.appspot.com/7305075/diff/6001/src/cmd/vet/atomic.go
File src/cmd/vet/atomic.go (left):

https://codereview.appspot.com/7305075/diff/6001/src/cmd/vet/atomic.go#oldcode13
src/cmd/vet/atomic.go:13: // checkAtomicAssignment walks the assignment
statement checking for comomon
Is "comomon" misspelled ?

[r, 2013-02-12 00:10:44]

On Mon, Feb 11, 2013 at 3:09 PM,  <eduardo.ramalho@gmail.com> wrote:
>
> https://codereview.appspot.com/7305075/diff/6001/src/cmd/vet/atomic.go
> File src/cmd/vet/atomic.go (left):
>
>
https://codereview.appspot.com/7305075/diff/6001/src/cmd/vet/atomic.go#oldcode13
> src/cmd/vet/atomic.go:13: // checkAtomicAssignment walks the assignment
> statement checking for comomon
> Is "comomon" misspelled ?

No, but "common" is :)

We'll get it next round.

-rob