From: Aliaksandr Valialkin Date: Sat, 23 Apr 2016 18:00:38 +0000 (+0300) Subject: cmd/vet: improve checking unkeyed fields in composite literals X-Git-Tag: go1.7beta1~436 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=22db3c5a62b01dba6122230aa71d35c48107c70c;p=gostls13.git cmd/vet: improve checking unkeyed fields in composite literals - Simplified the code. - Removed types for slice aliases from composite literals' whitelist, since they are properly handled by vet. Fixes #15408 Updates #9171 Updates #11041 Change-Id: Ia1806c9eb3f327c09d2e28da4ffdb233b5a159b0 Reviewed-on: https://go-review.googlesource.com/22318 Run-TryBot: Rob Pike TryBot-Result: Gobot Gobot Reviewed-by: Rob Pike --- diff --git a/src/cmd/vet/composite.go b/src/cmd/vet/composite.go index ac6a598b0b..f704f181bf 100644 --- a/src/cmd/vet/composite.go +++ b/src/cmd/vet/composite.go @@ -10,6 +10,7 @@ import ( "cmd/vet/internal/whitelist" "flag" "go/ast" + "go/types" "strings" ) @@ -25,102 +26,57 @@ func init() { // checkUnkeyedLiteral checks if a composite literal is a struct literal with // unkeyed fields. func checkUnkeyedLiteral(f *File, node ast.Node) { - c := node.(*ast.CompositeLit) - typ := c.Type - for { - if typ1, ok := c.Type.(*ast.ParenExpr); ok { - typ = typ1 - continue - } - break - } + cl := node.(*ast.CompositeLit) - switch typ.(type) { - case *ast.ArrayType: - return - case *ast.MapType: + typ := f.pkg.types[cl].Type + if typ == nil { + // cannot determine composite literals' type, skip it return - case *ast.StructType: - return // a literal struct type does not need to use keys - case *ast.Ident: - // A simple type name like t or T does not need keys either, - // since it is almost certainly declared in the current package. - // (The exception is names being used via import . "pkg", but - // those are already breaking the Go 1 compatibility promise, - // so not reporting potential additional breakage seems okay.) + } + typeName := typ.String() + if *compositeWhiteList && whitelist.UnkeyedLiteral[typeName] { + // skip whitelisted types return } - - // Otherwise the type is a selector like pkg.Name. - // We only care if pkg.Name is a struct, not if it's a map, array, or slice. - isStruct, typeString := f.pkg.isStruct(c) - if !isStruct { + if _, ok := typ.Underlying().(*types.Struct); !ok { + // skip non-struct composite literals return } - - if typeString == "" { // isStruct doesn't know - typeString = f.gofmt(typ) + if isLocalType(f, typeName) { + // allow unkeyed locally defined composite literal + return } - // It's a struct, or we can't tell it's not a struct because we don't have types. - - // Check if the CompositeLit contains an unkeyed field. + // check if the CompositeLit contains an unkeyed field allKeyValue := true - for _, e := range c.Elts { + for _, e := range cl.Elts { if _, ok := e.(*ast.KeyValueExpr); !ok { - if cl, ok := e.(*ast.CompositeLit); !ok || cl.Type != nil { - allKeyValue = false - break - } + allKeyValue = false + break } } if allKeyValue { + // all the composite literal fields are keyed return } - // Check that the CompositeLit's type has the form pkg.Typ. - s, ok := c.Type.(*ast.SelectorExpr) - if !ok { - return - } - pkg, ok := s.X.(*ast.Ident) - if !ok { - return - } + f.Badf(cl.Pos(), "%s composite literal uses unkeyed fields", typeName) +} - // Convert the package name to an import path, and compare to a whitelist. - path := pkgPath(f, pkg.Name) - if path == "" { - f.Badf(c.Pos(), "unresolvable package for %s.%s literal", pkg.Name, s.Sel.Name) - return - } - typeName := path + "." + s.Sel.Name - if *compositeWhiteList && whitelist.UnkeyedLiteral[typeName] { - return +func isLocalType(f *File, typeName string) bool { + if strings.HasPrefix(typeName, "struct{") { + // struct literals are local types + return true } - f.Bad(c.Pos(), typeString+" composite literal uses unkeyed fields") -} + pkgname := f.pkg.path + if strings.HasPrefix(typeName, pkgname+".") { + return true + } -// pkgPath returns the import path "image/png" for the package name "png". -// -// This is based purely on syntax and convention, and not on the imported -// package's contents. It will be incorrect if a package name differs from the -// leaf element of the import path, or if the package was a dot import. -func pkgPath(f *File, pkgName string) (path string) { - for _, x := range f.file.Imports { - s := strings.Trim(x.Path.Value, `"`) - if x.Name != nil { - // Catch `import pkgName "foo/bar"`. - if x.Name.Name == pkgName { - return s - } - } else { - // Catch `import "pkgName"` or `import "foo/bar/pkgName"`. - if s == pkgName || strings.HasSuffix(s, "/"+pkgName) { - return s - } - } + // treat types as local inside test packages with _test name suffix + if strings.HasSuffix(pkgname, "_test") { + pkgname = pkgname[:len(pkgname)-len("_test")] } - return "" + return strings.HasPrefix(typeName, pkgname+".") } diff --git a/src/cmd/vet/internal/whitelist/whitelist.go b/src/cmd/vet/internal/whitelist/whitelist.go index 696f7a533d..fdd65d3732 100644 --- a/src/cmd/vet/internal/whitelist/whitelist.go +++ b/src/cmd/vet/internal/whitelist/whitelist.go @@ -5,38 +5,9 @@ // Package whitelist defines exceptions for the vet tool. package whitelist -// UnkeyedLiteral are types that are actually slices, but -// syntactically, we cannot tell whether the Typ in pkg.Typ{1, 2, 3} -// is a slice or a struct, so we whitelist all the standard package -// library's exported slice types. +// UnkeyedLiteral is a white list of types in the standard packages +// that are used with unkeyed literals we deem to be acceptable. var UnkeyedLiteral = map[string]bool{ - /* - find $GOROOT/src -type f | grep -v _test.go | grep -v /internal/ | grep -v /testdata/ | \ - xargs grep '^type.*\[\]' | grep -v ' func(' | \ - grep -v ' map\[' | sed 's,/[^/]*go.type,,' | sed 's,.*src/,,' | \ - sed 's, ,.,' | sed 's, .*,,' | grep -v '\.[a-z]' | \ - sort | awk '{ print "\"" $0 "\": true," }' - */ - "crypto/x509/pkix.RDNSequence": true, - "crypto/x509/pkix.RelativeDistinguishedNameSET": true, - "database/sql.RawBytes": true, - "debug/macho.LoadBytes": true, - "encoding/asn1.ObjectIdentifier": true, - "encoding/asn1.RawContent": true, - "encoding/json.RawMessage": true, - "encoding/xml.CharData": true, - "encoding/xml.Comment": true, - "encoding/xml.Directive": true, - "go/scanner.ErrorList": true, - "image/color.Palette": true, - "net.HardwareAddr": true, - "net.IP": true, - "net.IPMask": true, - "sort.Float64Slice": true, - "sort.IntSlice": true, - "sort.StringSlice": true, - "unicode.SpecialCase": true, - // These image and image/color struct types are frozen. We will never add fields to them. "image/color.Alpha16": true, "image/color.Alpha": true, @@ -45,10 +16,13 @@ var UnkeyedLiteral = map[string]bool{ "image/color.Gray": true, "image/color.NRGBA64": true, "image/color.NRGBA": true, + "image/color.NYCbCrA": true, "image/color.RGBA64": true, "image/color.RGBA": true, "image/color.YCbCr": true, "image.Point": true, "image.Rectangle": true, "image.Uniform": true, + + "unicode.Range16": true, } diff --git a/src/cmd/vet/testdata/composite.go b/src/cmd/vet/testdata/composite.go index 0355c0b692..2e6ce262cc 100644 --- a/src/cmd/vet/testdata/composite.go +++ b/src/cmd/vet/testdata/composite.go @@ -9,7 +9,10 @@ package testdata import ( "flag" "go/scanner" + "image" "unicode" + + "path/to/unknownpkg" ) var Okay1 = []string{ @@ -34,34 +37,67 @@ var Okay3 = struct { "DefValue", } +var Okay4 = []struct { + A int + B int +}{ + {1, 2}, + {3, 4}, +} + type MyStruct struct { X string Y string Z string } -var Okay4 = MyStruct{ +var Okay5 = &MyStruct{ "Name", "Usage", "DefValue", } +var Okay6 = []MyStruct{ + {"foo", "bar", "baz"}, + {"aa", "bb", "cc"}, +} + // Testing is awkward because we need to reference things from a separate package // to trigger the warnings. -var BadStructLiteralUsedInTests = flag.Flag{ // ERROR "unkeyed fields" +var goodStructLiteral = flag.Flag{ + Name: "Name", + Usage: "Usage", +} +var badStructLiteral = flag.Flag{ // ERROR "unkeyed fields" "Name", "Usage", nil, // Value "DefValue", } -// SpecialCase is an (aptly named) slice of CaseRange to test issue 9171. -var GoodNamedSliceLiteralUsedInTests = unicode.SpecialCase{ +// SpecialCase is a named slice of CaseRange to test issue 9171. +var goodNamedSliceLiteral = unicode.SpecialCase{ {Lo: 1, Hi: 2}, + unicode.CaseRange{Lo: 1, Hi: 2}, +} +var badNamedSliceLiteral = unicode.SpecialCase{ + {1, 2}, // ERROR "unkeyed fields" + unicode.CaseRange{1, 2}, // ERROR "unkeyed fields" +} + +// ErrorList is a named slice, so no warnings should be emitted. +var goodScannerErrorList = scanner.ErrorList{ + &scanner.Error{Msg: "foobar"}, +} +var badScannerErrorList = scanner.ErrorList{ + &scanner.Error{"foobar"}, // ERROR "unkeyed fields" } -// Used to test the check for slices and arrays: If that test is disabled and -// vet is run with --compositewhitelist=false, this line triggers an error. -// Clumsy but sufficient. -var scannerErrorListTest = scanner.ErrorList{nil, nil} +// Check whitelisted structs: if vet is run with --compositewhitelist=false, +// this line triggers an error. +var whitelistedPoint = image.Point{1, 2} + +// Do not check type from unknown package. +// See issue 15408. +var unknownPkgVar = unknownpkg.Foobar{"foo", "bar"} diff --git a/src/cmd/vet/types.go b/src/cmd/vet/types.go index 4358955d93..4d0e6154b8 100644 --- a/src/cmd/vet/types.go +++ b/src/cmd/vet/types.go @@ -85,29 +85,6 @@ func (pkg *Package) check(fs *token.FileSet, astFiles []*ast.File) error { return err } -// isStruct reports whether the composite literal c is a struct. -// If it is not (probably a struct), it returns a printable form of the type. -func (pkg *Package) isStruct(c *ast.CompositeLit) (bool, string) { - // Check that the CompositeLit's type is a slice or array (which needs no field keys), if possible. - typ := pkg.types[c].Type - // If it's a named type, pull out the underlying type. If it's not, the Underlying - // method returns the type itself. - actual := typ - if actual != nil { - actual = actual.Underlying() - } - if actual == nil { - // No type information available. Assume true, so we do the check. - return true, "" - } - switch actual.(type) { - case *types.Struct: - return true, typ.String() - default: - return false, "" - } -} - // matchArgType reports an error if printf verb t is not appropriate // for operand arg. //