From 6bd3206b1fa8e34ec6099541424cbef3bc031dd5 Mon Sep 17 00:00:00 2001 From: Rob Pike Date: Fri, 7 Mar 2014 16:02:49 +1100 Subject: [PATCH] go.tools/cmd/vet: allow checks to be disabled explicitly as well as set explicitly Now we can say vet -printf=false to disable the printf test but run all others. Implemented by creating a tri-state boolean flag that records whether it has been set explicitly; before this, -printf=false was not distinguishable from not having mentioned the printf flag at all. Fixes golang/go#7422. LGTM=rsc R=rsc CC=golang-codereviews https://golang.org/cl/72330043 --- cmd/vet/doc.go | 7 ++- cmd/vet/main.go | 120 +++++++++++++++++++++++++++++++++++++----------- 2 files changed, 98 insertions(+), 29 deletions(-) diff --git a/cmd/vet/doc.go b/cmd/vet/doc.go index 834c5612..72832da9 100644 --- a/cmd/vet/doc.go +++ b/cmd/vet/doc.go @@ -30,8 +30,11 @@ check every possible problem and depends on unreliable heuristics so it should be used as guidance only, not as a firm indicator of program correctness. -By default all checks are performed, but if explicit flags are provided, only -those identified by the flags are performed. +By default all checks are performed. If any flags are explicitly set +to true, only those tests are run. Conversely, if any flag is +explicitly set to false, only those tests are disabled. +Thus -printf=true runs the printf check, -printf=false runs all checks +except the printf check. Available checks: diff --git a/cmd/vet/main.go b/cmd/vet/main.go index 9039532d..b915da32 100644 --- a/cmd/vet/main.go +++ b/cmd/vet/main.go @@ -25,41 +25,105 @@ import ( "code.google.com/p/go.tools/go/types" ) +// TODO: Need a flag to set build tags when parsing the package. + var verbose = flag.Bool("v", false, "verbose") var strictShadowing = flag.Bool("shadowstrict", false, "whether to be strict about shadowing; can be noisy") var testFlag = flag.Bool("test", false, "for testing only: sets -all and -shadow") var exitCode = 0 -// 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"), - "asmdecl": flag.Bool("asmdecl", false, "check assembly against Go declarations"), - "assign": flag.Bool("assign", false, "check for useless assignments"), - "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"), - "composites": flag.Bool("composites", false, "check that composite literals used field-keyed elements"), - "copylocks": flag.Bool("copylocks", false, "check that locks are not passed by value"), - "methods": flag.Bool("methods", false, "check that canonically named methods are canonically defined"), - "nilfunc": flag.Bool("nilfunc", false, "check for comparisons between functions and nil"), - "printf": flag.Bool("printf", false, "check printf-like invocations"), - "rangeloops": flag.Bool("rangeloops", false, "check that range loop variables are used correctly"), - "shadow": flag.Bool("shadow", false, "check for shadowed variables (experimental; must be set explicitly)"), - "structtags": flag.Bool("structtags", false, "check that struct field tags have canonical format"), - "unreachable": flag.Bool("unreachable", false, "check for unreachable code"), +// "all" is here only for the appearance of backwards compatibility. +// It has no effect; the triState flags do the work. +var all = flag.Bool("all", true, "check everything; disabled if any explicit check is requested") + +// Flags to control which individual checks to perform. +var report = map[string]*triState{ + "asmdecl": triStateFlag("asmdecl", unset, "check assembly against Go declarations"), + "assign": triStateFlag("assign", unset, "check for useless assignments"), + "atomic": triStateFlag("atomic", unset, "check for common mistaken usages of the sync/atomic package"), + "buildtags": triStateFlag("buildtags", unset, "check that +build tags are valid"), + "composites": triStateFlag("composites", unset, "check that composite literals used field-keyed elements"), + "copylocks": triStateFlag("copylocks", unset, "check that locks are not passed by value"), + "methods": triStateFlag("methods", unset, "check that canonically named methods are canonically defined"), + "nilfunc": triStateFlag("nilfunc", unset, "check for comparisons between functions and nil"), + "printf": triStateFlag("printf", unset, "check printf-like invocations"), + "rangeloops": triStateFlag("rangeloops", unset, "check that range loop variables are used correctly"), + "shadow": triStateFlag("shadow", unset, "check for shadowed variables (experimental; must be set explicitly)"), + "structtags": triStateFlag("structtags", unset, "check that struct field tags have canonical format"), + "unreachable": triStateFlag("unreachable", unset, "check for unreachable code"), } -// TODO: Need a flag to set build tags when parsing the package. +// experimental records the flags enabling experimental features. These must be +// requested explicitly; they are not enabled by -all. +var experimental = map[string]bool{ + "shadow": true, +} + +// setTrueCount record how many flags are explicitly set to true. +var setTrueCount int + +// A triState is a boolean that knows whether it has been set to either true or false. +// It is used to identify if a flag appears; the standard boolean flag cannot +// distinguish missing from unset. It also satisfies flag.Value. +type triState int + +const ( + unset triState = iota + setTrue + setFalse +) + +func triStateFlag(name string, value triState, usage string) *triState { + flag.Var(&value, name, usage) + return &value +} + +// triState implements flag.Value, flag.Getter, and flag.boolFlag. +// They work like boolean flags: we can say vet -printf as well as vet -printf=true +func (ts *triState) Get() interface{} { + return *ts == setTrue +} + +func (ts triState) isTrue() bool { + return ts == setTrue +} + +func (ts *triState) Set(value string) error { + b, err := strconv.ParseBool(value) + if err != nil { + return err + } + if b { + *ts = setTrue + setTrueCount++ + } else { + *ts = setFalse + } + return nil +} + +func (ts *triState) String() string { + switch *ts { + case unset: + return "unset" + case setTrue: + return "true" + case setFalse: + return "false" + } + panic("not reached") +} + +func (ts triState) IsBoolFlag() bool { + return true +} // vet tells whether to report errors for the named check, a flag name. func vet(name string) bool { if *testFlag { return true } - if name == "shadow" { - return *report["shadow"] - } - return *report["all"] || *report[name] + return report[name].isTrue() } // setExit sets the value for os.Exit when it is called, later. It @@ -101,11 +165,13 @@ func main() { flag.Usage = Usage flag.Parse() - // If a check is named explicitly, turn off the 'all' flag. - for name, ptr := range report { - if name != "all" && *ptr { - *report["all"] = false - break + // If any flag is set, we run only those checks requested. + // If no flags are set true, set all the non-experimental ones not explicitly set (in effect, set the "-all" flag). + if setTrueCount == 0 { + for name, setting := range report { + if *setting == unset && !experimental[name] { + *setting = setTrue + } } }