From 2de7f9bf822ce37065c31659ae7187ea9e416dde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Thu, 30 May 2019 17:20:57 +0100 Subject: [PATCH] go/analysis/passes/structtag: allow field tag shadowing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the following piece of code: type T1 struct { Shadowed string `json:"foo"` } type T2 struct { T1 Shadowing int `json:"foo"` } encoding/json will encode T2 using T2.Shadowing, ignoring T2.Shadowed entirely. This can be a useful feature to replace some of T1's fields when encoding it. Moreover, this feature is already in use in the wild, even though it's probably never been documented. This started being a problem, as the structtag pass started walking through embedded fields a few months ago. To keep it from complaining about these useful shadowing cases, make it only see duplicate field tag names if they are at the same embedding level, in which case no shadowing is happening. The old code indexed these tags by encoding key and name, using a [2]string. The new code needs to add a level integer, so start declaring named types for the map, and use methods to simplify the code further below. We still use a map pointer, to avoid allocating on every single struct definition. Updates golang/go#30846. Change-Id: Iae53228d4f8bd91584c59dcc982cb1300970bc8f Reviewed-on: https://go-review.googlesource.com/c/tools/+/179360 Run-TryBot: Daniel Martí TryBot-Result: Gobot Gobot Reviewed-by: Michael Matloob --- go/analysis/passes/structtag/structtag.go | 71 +++++++++++++------ .../passes/structtag/testdata/src/a/a.go | 29 ++++---- 2 files changed, 65 insertions(+), 35 deletions(-) diff --git a/go/analysis/passes/structtag/structtag.go b/go/analysis/passes/structtag/structtag.go index bcdb0429..acc6e6c7 100644 --- a/go/analysis/passes/structtag/structtag.go +++ b/go/analysis/passes/structtag/structtag.go @@ -41,7 +41,7 @@ func run(pass *analysis.Pass) (interface{}, error) { } inspect.Preorder(nodeFilter, func(n ast.Node) { styp := pass.TypesInfo.Types[n.(*ast.StructType)].Type.(*types.Struct) - var seen map[[2]string]token.Pos + var seen namesSeen for i := 0; i < styp.NumFields(); i++ { field := styp.Field(i) tag := styp.Tag(i) @@ -51,11 +51,38 @@ func run(pass *analysis.Pass) (interface{}, error) { return nil, nil } +// namesSeen keeps track of encoding tags by their key, name, and nested level +// from the initial struct. The level is taken into account because equal +// encoding key names only conflict when at the same level; otherwise, the lower +// level shadows the higher level. +type namesSeen map[uniqueName]token.Pos + +type uniqueName struct { + key string // "xml" or "json" + name string // the encoding name + level int // anonymous struct nesting level +} + +func (s *namesSeen) Get(key, name string, level int) (token.Pos, bool) { + if *s == nil { + *s = make(map[uniqueName]token.Pos) + } + pos, ok := (*s)[uniqueName{key, name, level}] + return pos, ok +} + +func (s *namesSeen) Set(key, name string, level int, pos token.Pos) { + if *s == nil { + *s = make(map[uniqueName]token.Pos) + } + (*s)[uniqueName{key, name, level}] = pos +} + var checkTagDups = []string{"json", "xml"} var checkTagSpaces = map[string]bool{"json": true, "xml": true, "asn1": true} // checkCanonicalFieldTag checks a single struct field tag. -func checkCanonicalFieldTag(pass *analysis.Pass, field *types.Var, tag string, seen *map[[2]string]token.Pos) { +func checkCanonicalFieldTag(pass *analysis.Pass, field *types.Var, tag string, seen *namesSeen) { switch pass.Pkg.Path() { case "encoding/json", "encoding/xml": // These packages know how to use their own APIs. @@ -64,7 +91,7 @@ func checkCanonicalFieldTag(pass *analysis.Pass, field *types.Var, tag string, s } for _, key := range checkTagDups { - checkTagDuplicates(pass, tag, key, field, field, seen) + checkTagDuplicates(pass, tag, key, field, field, seen, 1) } if err := validateStructTag(tag); err != nil { @@ -95,28 +122,29 @@ func checkCanonicalFieldTag(pass *analysis.Pass, field *types.Var, tag string, s // checkTagDuplicates checks a single struct field tag to see if any tags are // duplicated. nearest is the field that's closest to the field being checked, // while still being part of the top-level struct type. -func checkTagDuplicates(pass *analysis.Pass, tag, key string, nearest, field *types.Var, seen *map[[2]string]token.Pos) { +func checkTagDuplicates(pass *analysis.Pass, tag, key string, nearest, field *types.Var, seen *namesSeen, level int) { val := reflect.StructTag(tag).Get(key) if val == "-" { // Ignored, even if the field is anonymous. return } if val == "" || val[0] == ',' { - if field.Anonymous() { - typ, ok := field.Type().Underlying().(*types.Struct) - if !ok { - return - } - for i := 0; i < typ.NumFields(); i++ { - field := typ.Field(i) - if !field.Exported() { - continue - } - tag := typ.Tag(i) - checkTagDuplicates(pass, tag, key, nearest, field, seen) - } + if !field.Anonymous() { + // Ignored if the field isn't anonymous. + return + } + typ, ok := field.Type().Underlying().(*types.Struct) + if !ok { + return + } + for i := 0; i < typ.NumFields(); i++ { + field := typ.Field(i) + if !field.Exported() { + continue + } + tag := typ.Tag(i) + checkTagDuplicates(pass, tag, key, nearest, field, seen, level+1) } - // Ignored if the field isn't anonymous. return } if key == "xml" && field.Name() == "XMLName" { @@ -139,10 +167,7 @@ func checkTagDuplicates(pass *analysis.Pass, tag, key string, nearest, field *ty } val = val[:i] } - if *seen == nil { - *seen = map[[2]string]token.Pos{} - } - if pos, ok := (*seen)[[2]string{key, val}]; ok { + if pos, ok := seen.Get(key, val, level); ok { alsoPos := pass.Fset.Position(pos) alsoPos.Column = 0 @@ -161,7 +186,7 @@ func checkTagDuplicates(pass *analysis.Pass, tag, key string, nearest, field *ty pass.Reportf(nearest.Pos(), "struct field %s repeats %s tag %q also at %s", field.Name(), key, val, alsoPos) } else { - (*seen)[[2]string{key, val}] = field.Pos() + seen.Set(key, val, level, field.Pos()) } } diff --git a/go/analysis/passes/structtag/testdata/src/a/a.go b/go/analysis/passes/structtag/testdata/src/a/a.go index abb67fe4..3394f4c8 100644 --- a/go/analysis/passes/structtag/testdata/src/a/a.go +++ b/go/analysis/passes/structtag/testdata/src/a/a.go @@ -75,29 +75,27 @@ type DuplicateJSONFields struct { } AnonymousJSON `json:"a"` // want "struct field AnonymousJSON repeats json tag .a. also at a.go:64" - AnonymousJSONField // want "struct field DuplicateAnonJSON repeats json tag .a. also at a.go:64" - XML int `xml:"a"` - DuplicateXML int `xml:"a"` // want "struct field DuplicateXML repeats xml tag .a. also at a.go:80" + DuplicateXML int `xml:"a"` // want "struct field DuplicateXML repeats xml tag .a. also at a.go:78" IgnoredXML int `xml:"-"` OtherIgnoredXML int `xml:"-"` OmitXML int `xml:",omitempty"` OtherOmitXML int `xml:",omitempty"` - DuplicateOmitXML int `xml:"a,omitempty"` // want "struct field DuplicateOmitXML repeats xml tag .a. also at a.go:80" + DuplicateOmitXML int `xml:"a,omitempty"` // want "struct field DuplicateOmitXML repeats xml tag .a. also at a.go:78" NonXML int `foo:"a"` DuplicateNonXML int `foo:"a"` Embedded2 struct { DuplicateXML int `xml:"a"` // OK because it's not in the same struct type } - AnonymousXML `xml:"a"` // want "struct field AnonymousXML repeats xml tag .a. also at a.go:80" + AnonymousXML `xml:"a"` // want "struct field AnonymousXML repeats xml tag .a. also at a.go:78" Attribute struct { XMLName xml.Name `xml:"b"` NoDup int `xml:"b"` // OK because XMLName above affects enclosing struct. Attr int `xml:"b,attr"` // OK because 0 is valid. - DupAttr int `xml:"b,attr"` // want "struct field DupAttr repeats xml attribute tag .b. also at a.go:96" - DupOmitAttr int `xml:"b,omitempty,attr"` // want "struct field DupOmitAttr repeats xml attribute tag .b. also at a.go:96" + DupAttr int `xml:"b,attr"` // want "struct field DupAttr repeats xml attribute tag .b. also at a.go:94" + DupOmitAttr int `xml:"b,omitempty,attr"` // want "struct field DupOmitAttr repeats xml attribute tag .b. also at a.go:94" - AnonymousXML `xml:"b,attr"` // want "struct field AnonymousXML repeats xml attribute tag .b. also at a.go:96" + AnonymousXML `xml:"b,attr"` // want "struct field AnonymousXML repeats xml attribute tag .b. also at a.go:94" } AnonymousJSONField2 `json:"not_anon"` // ok; fields aren't embedded in JSON @@ -124,10 +122,17 @@ type UnexpectedSpacetest struct { Q int `foo:" doesn't care "` } +// Nested fiels can be shadowed by fields further up. For example, +// ShadowingAnonJSON replaces the json:"a" field in AnonymousJSONField. +// However, if the two conflicting fields appear at the same level like in +// DuplicateWithAnotherPackage, we should error. + +type ShadowingJsonFieldName struct { + AnonymousJSONField + ShadowingAnonJSON int `json:"a"` +} + type DuplicateWithAnotherPackage struct { b.AnonymousJSONField - - // The "also at" position is in a different package and directory. Use - // "b.b" instead of "b/b" to match the relative path on Windows easily. - DuplicateJSON int `json:"a"` // want "struct field DuplicateJSON repeats json tag .a. also at b.b.go:8" + AnonymousJSONField2 // want "struct field DuplicateAnonJSON repeats json tag .a. also at b.b.go:8" }