go/analysis/passes/structtag: allow field tag shadowing
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í <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
diff --git a/go/analysis/passes/structtag/structtag.go b/go/analysis/passes/structtag/structtag.go
index bcdb042..acc6e6c 100644
--- a/go/analysis/passes/structtag/structtag.go
+++ b/go/analysis/passes/structtag/structtag.go
@@ -41,7 +41,7 @@
}
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 @@
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 @@
}
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 @@
// 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
}
- // Ignored if the field isn't 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, level+1)
+ }
return
}
if key == "xml" && field.Name() == "XMLName" {
@@ -139,10 +167,7 @@
}
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 @@
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 abb67fe..3394f4c 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 @@
}
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 <b b="0"><b>0</b></b> 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 @@
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"
}
