From f78a1e934587320e316e5782463234ed4e1280c5 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 8 Oct 2018 20:37:15 -0400 Subject: [PATCH] go/analysis/passes/composite: split out of vet Change-Id: Ie6e05bea1c8c607407479f5f45dea6fcec62f60c Reviewed-on: https://go-review.googlesource.com/c/140739 Reviewed-by: Michael Matloob Run-TryBot: Michael Matloob --- go/analysis/passes/composite/composite.go | 104 ++++++++++++++++++ .../passes/composite/composite_test.go | 13 +++ .../testdata/src/a/a.go} | 25 +++-- .../whitelist => composite}/whitelist.go | 7 +- go/analysis/passes/vet/composite.go | 88 --------------- 5 files changed, 133 insertions(+), 104 deletions(-) create mode 100644 go/analysis/passes/composite/composite.go create mode 100644 go/analysis/passes/composite/composite_test.go rename go/analysis/passes/{vet/testdata/composite.go => composite/testdata/src/a/a.go} (78%) rename go/analysis/passes/{vet/internal/whitelist => composite}/whitelist.go (81%) delete mode 100644 go/analysis/passes/vet/composite.go diff --git a/go/analysis/passes/composite/composite.go b/go/analysis/passes/composite/composite.go new file mode 100644 index 00000000..403cbe93 --- /dev/null +++ b/go/analysis/passes/composite/composite.go @@ -0,0 +1,104 @@ +// Copyright 2012 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package composite + +import ( + "go/ast" + "go/types" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +var Analyzer = &analysis.Analyzer{ + Name: "composites", + Doc: `checked for unkeyed composite literals + +This analyzer reports a diagnostic for composite literals of struct +types imported from another package that do not use the field-keyed +syntax. Such literals are fragile because the addition of a new field +(even if unexported) to the struct will cause compilation to fail.`, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + RunDespiteErrors: true, + Run: run, +} + +var whitelist = true + +func init() { + Analyzer.Flags.BoolVar(&whitelist, "whitelist", whitelist, "use composite white list; for testing only") +} + +// runUnkeyedLiteral checks if a composite literal is a struct literal with +// unkeyed fields. +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{ + (*ast.CompositeLit)(nil), + } + inspect.Preorder(nodeFilter, func(n ast.Node) { + cl := n.(*ast.CompositeLit) + + typ := pass.TypesInfo.Types[cl].Type + if typ == nil { + // cannot determine composite literals' type, skip it + return + } + typeName := typ.String() + if whitelist && unkeyedLiteral[typeName] { + // skip whitelisted types + return + } + under := typ.Underlying() + for { + ptr, ok := under.(*types.Pointer) + if !ok { + break + } + under = ptr.Elem().Underlying() + } + if _, ok := under.(*types.Struct); !ok { + // skip non-struct composite literals + return + } + if isLocalType(pass, typ) { + // allow unkeyed locally defined composite literal + return + } + + // check if the CompositeLit contains an unkeyed field + allKeyValue := true + for _, e := range cl.Elts { + if _, ok := e.(*ast.KeyValueExpr); !ok { + allKeyValue = false + break + } + } + if allKeyValue { + // all the composite literal fields are keyed + return + } + + pass.Reportf(cl.Pos(), "%s composite literal uses unkeyed fields", typeName) + }) + return nil, nil +} + +func isLocalType(pass *analysis.Pass, typ types.Type) bool { + switch x := typ.(type) { + case *types.Struct: + // struct literals are local types + return true + case *types.Pointer: + return isLocalType(pass, x.Elem()) + case *types.Named: + // names in package foo are local to foo_test too + return strings.TrimSuffix(x.Obj().Pkg().Path(), "_test") == strings.TrimSuffix(pass.Pkg.Path(), "_test") + } + return false +} diff --git a/go/analysis/passes/composite/composite_test.go b/go/analysis/passes/composite/composite_test.go new file mode 100644 index 00000000..384898a6 --- /dev/null +++ b/go/analysis/passes/composite/composite_test.go @@ -0,0 +1,13 @@ +package composite_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/composite" +) + +func TestFromFileSystem(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, composite.Analyzer, "a") +} diff --git a/go/analysis/passes/vet/testdata/composite.go b/go/analysis/passes/composite/testdata/src/a/a.go similarity index 78% rename from go/analysis/passes/vet/testdata/composite.go rename to go/analysis/passes/composite/testdata/src/a/a.go index 3fe3eac7..172ac54e 100644 --- a/go/analysis/passes/vet/testdata/composite.go +++ b/go/analysis/passes/composite/testdata/src/a/a.go @@ -4,15 +4,14 @@ // This file contains the test for untagged struct literals. -package testdata +package a import ( "flag" "go/scanner" + "go/token" "image" "unicode" - - "path/to/unknownpkg" ) var Okay1 = []string{ @@ -74,21 +73,23 @@ var goodStructLiteral = flag.Flag{ Name: "Name", Usage: "Usage", } -var badStructLiteral = flag.Flag{ // ERROR "unkeyed fields" +var badStructLiteral = flag.Flag{ // want "unkeyed fields" "Name", "Usage", nil, // Value "DefValue", } +var delta [3]rune + // 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}, + {Lo: 1, Hi: 2, Delta: delta}, + unicode.CaseRange{Lo: 1, Hi: 2, Delta: delta}, } var badNamedSliceLiteral = unicode.SpecialCase{ - {1, 2}, // ERROR "unkeyed fields" - unicode.CaseRange{1, 2}, // ERROR "unkeyed fields" + {1, 2, delta}, // want "unkeyed fields" + unicode.CaseRange{1, 2, delta}, // want "unkeyed fields" } // ErrorList is a named slice, so no warnings should be emitted. @@ -96,7 +97,7 @@ var goodScannerErrorList = scanner.ErrorList{ &scanner.Error{Msg: "foobar"}, } var badScannerErrorList = scanner.ErrorList{ - &scanner.Error{"foobar"}, // ERROR "unkeyed fields" + &scanner.Error{token.Position{}, "foobar"}, // want "unkeyed fields" } // Check whitelisted structs: if vet is run with --compositewhitelist=false, @@ -105,7 +106,7 @@ var whitelistedPoint = image.Point{1, 2} // Do not check type from unknown package. // See issue 15408. -var unknownPkgVar = unknownpkg.Foobar{"foo", "bar"} +var unknownPkgVar = unicode.NoSuchType{"foo", "bar"} // A named pointer slice of CaseRange to test issue 23539. In // particular, we're interested in how some slice elements omit their @@ -115,6 +116,6 @@ var goodNamedPointerSliceLiteral = []*unicode.CaseRange{ &unicode.CaseRange{Lo: 1, Hi: 2}, } var badNamedPointerSliceLiteral = []*unicode.CaseRange{ - {1, 2}, // ERROR "unkeyed fields" - &unicode.CaseRange{1, 2}, // ERROR "unkeyed fields" + {1, 2, delta}, // want "unkeyed fields" + &unicode.CaseRange{1, 2, delta}, // want "unkeyed fields" } diff --git a/go/analysis/passes/vet/internal/whitelist/whitelist.go b/go/analysis/passes/composite/whitelist.go similarity index 81% rename from go/analysis/passes/vet/internal/whitelist/whitelist.go rename to go/analysis/passes/composite/whitelist.go index fdd65d37..67e9aa0b 100644 --- a/go/analysis/passes/vet/internal/whitelist/whitelist.go +++ b/go/analysis/passes/composite/whitelist.go @@ -2,12 +2,11 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// Package whitelist defines exceptions for the vet tool. -package whitelist +package composite -// UnkeyedLiteral is a white list of types in the standard packages +// 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{ +var unkeyedLiteral = map[string]bool{ // These image and image/color struct types are frozen. We will never add fields to them. "image/color.Alpha16": true, "image/color.Alpha": true, diff --git a/go/analysis/passes/vet/composite.go b/go/analysis/passes/vet/composite.go deleted file mode 100644 index 93fd747d..00000000 --- a/go/analysis/passes/vet/composite.go +++ /dev/null @@ -1,88 +0,0 @@ -// +build ignore - -// Copyright 2012 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -// This file contains the test for unkeyed struct literals. - -package main - -import ( - "cmd/vet/internal/whitelist" - "flag" - "go/ast" - "go/types" - "strings" -) - -var compositeWhiteList = flag.Bool("compositewhitelist", true, "use composite white list; for testing only") - -func init() { - register("composites", - "check that composite literals of types from imported packages use field-keyed elements", - checkUnkeyedLiteral, - compositeLit) -} - -// checkUnkeyedLiteral checks if a composite literal is a struct literal with -// unkeyed fields. -func checkUnkeyedLiteral(f *File, node ast.Node) { - cl := node.(*ast.CompositeLit) - - typ := f.pkg.types[cl].Type - if typ == nil { - // cannot determine composite literals' type, skip it - return - } - typeName := typ.String() - if *compositeWhiteList && whitelist.UnkeyedLiteral[typeName] { - // skip whitelisted types - return - } - under := typ.Underlying() - for { - ptr, ok := under.(*types.Pointer) - if !ok { - break - } - under = ptr.Elem().Underlying() - } - if _, ok := under.(*types.Struct); !ok { - // skip non-struct composite literals - return - } - if isLocalType(f, typ) { - // allow unkeyed locally defined composite literal - return - } - - // check if the CompositeLit contains an unkeyed field - allKeyValue := true - for _, e := range cl.Elts { - if _, ok := e.(*ast.KeyValueExpr); !ok { - allKeyValue = false - break - } - } - if allKeyValue { - // all the composite literal fields are keyed - return - } - - f.Badf(cl.Pos(), "%s composite literal uses unkeyed fields", typeName) -} - -func isLocalType(f *File, typ types.Type) bool { - switch x := typ.(type) { - case *types.Struct: - // struct literals are local types - return true - case *types.Pointer: - return isLocalType(f, x.Elem()) - case *types.Named: - // names in package foo are local to foo_test too - return strings.TrimSuffix(x.Obj().Pkg().Path(), "_test") == strings.TrimSuffix(f.pkg.path, "_test") - } - return false -}