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>

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() {
+		if !field.Anonymous() {
-			typ, ok := field.Type().Underlying().(*types.Struct)
+			// Ignored if the field isn't anonymous.
-			if !ok {
+			return
-				return
+		}
-			}
+		typ, ok := field.Type().Underlying().(*types.Struct)
-			for i := 0; i < typ.NumFields(); i++ {
+		if !ok {
-				field := typ.Field(i)
+			return
-				if !field.Exported() {
+		}
-					continue
+		for i := 0; i < typ.NumFields(); i++ {
-				}
+			field := typ.Field(i)
-				tag := typ.Tag(i)
+			if !field.Exported() {
-				checkTagDuplicates(pass, tag, key, nearest, field, seen)
+				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 {
+	if pos, ok := seen.Get(key, val, level); ok {
-		*seen = map[[2]string]token.Pos{}
-	}
-	if pos, ok := (*seen)[[2]string{key, val}]; 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())
 	}
 }
 

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 <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"
+		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:96"
+		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
-
+	AnonymousJSONField2 // want "struct field DuplicateAnonJSON repeats json tag .a. also at b.b.go:8"
-	// 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"
 }