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 <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>

Author: valyala

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+".")
 }

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,
 }

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"}

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.
 //