- 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 <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
"cmd/vet/internal/whitelist"
"flag"
"go/ast"
+ "go/types"
"strings"
)
// 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+".")
}
// 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,
"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,
}
import (
"flag"
"go/scanner"
+ "image"
"unicode"
+
+ "path/to/unknownpkg"
)
var Okay1 = []string{
"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"}
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.
//