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