cmd/vet: check embedded field tags too

We can no longer use the field's position for the duplicate field tag
warning - since we now check embedded tags, the positions may belong to
copmletely different packages.

Instead, keep track of the lowest field that's still part of the
top-level struct type that we are checking.

Finally, be careful to not repeat the independent struct field warnings
when checking fields again because they are embedded into another
struct. To do this, separate the duplicate tag value logic into a func
that recurses into embedded fields on a per-encoding basis.

Fixes #25593.

Change-Id: I3bd6e01306d8ec63c0314d25e3136d5e067a9517
Reviewed-on: https://go-review.googlesource.com/115677
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>

src/cmd/vet/structtag.go

@@ -25,12 +25,13 @@ func init() {
 
 // checkStructFieldTags checks all the field tags of a struct, including checking for duplicates.
 func checkStructFieldTags(f *File, node ast.Node) {
-	styp := f.pkg.types[node.(*ast.StructType)].Type.(*types.Struct)
+	astType := node.(*ast.StructType)
+	typ := f.pkg.types[astType].Type.(*types.Struct)
 	var seen map[[2]string]token.Pos
-	for i := 0; i < styp.NumFields(); i++ {
-		field := styp.Field(i)
-		tag := styp.Tag(i)
-		checkCanonicalFieldTag(f, field, tag, &seen)
+	for i := 0; i < typ.NumFields(); i++ {
+		field := typ.Field(i)
+		tag := typ.Tag(i)
+		checkCanonicalFieldTag(f, astType, field, tag, &seen)
 	}
 }
 
@@ -38,27 +39,70 @@ 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(f *File, field *types.Var, tag string, seen *map[[2]string]token.Pos) {
-	if tag == "" {
-		return
+// top is the top-level struct type that is currently being checked.
+func checkCanonicalFieldTag(f *File, top *ast.StructType, field *types.Var, tag string, seen *map[[2]string]token.Pos) {
+	for _, key := range checkTagDups {
+		checkTagDuplicates(f, tag, key, field, field, seen)
 	}
 
 	if err := validateStructTag(tag); err != nil {
 		f.Badf(field.Pos(), "struct field tag %#q not compatible with reflect.StructTag.Get: %s", tag, err)
 	}
 
-	for _, key := range checkTagDups {
+	// Check for use of json or xml tags with unexported fields.
+
+	// Embedded struct. Nothing to do for now, but that
+	// may change, depending on what happens with issue 7363.
+	if field.Anonymous() {
+		return
+	}
+
+	if field.Exported() {
+		return
+	}
+
+	for _, enc := range [...]string{"json", "xml"} {
+		if reflect.StructTag(tag).Get(enc) != "" {
+			f.Badf(field.Pos(), "struct field %s has %s tag but is not exported", field.Name(), enc)
+			return
+		}
+	}
+}
+
+// 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(f *File, tag, key string, nearest, field *types.Var, seen *map[[2]string]token.Pos) {
 	val := reflect.StructTag(tag).Get(key)
-		if val == "" || val == "-" || val[0] == ',' {
+	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(f, tag, key, nearest, field, seen)
+			}
+		}
+		// Ignored if the field isn't anonymous.
+		return
+	}
 	if key == "xml" && field.Name() == "XMLName" {
 		// XMLName defines the XML element name of the struct being
 		// checked. That name cannot collide with element or attribute
 		// names defined on other fields of the struct. Vet does not have a
 		// check for untagged fields of type struct defining their own name
 		// by containing a field named XMLName; see issue 18256.
-			continue
+		return
 	}
 	if i := strings.Index(val, ","); i >= 0 {
 		if key == "xml" {
@@ -76,30 +120,10 @@ func checkCanonicalFieldTag(f *File, field *types.Var, tag string, seen *map[[2]
 		*seen = map[[2]string]token.Pos{}
 	}
 	if pos, ok := (*seen)[[2]string{key, val}]; ok {
-			f.Badf(field.Pos(), "struct field %s repeats %s tag %q also at %s", field.Name(), key, val, f.loc(pos))
+		f.Badf(nearest.Pos(), "struct field %s repeats %s tag %q also at %s", field.Name(), key, val, f.loc(pos))
 	} else {
 		(*seen)[[2]string{key, val}] = field.Pos()
 	}
-	}
-
-	// Check for use of json or xml tags with unexported fields.
-
-	// Embedded struct. Nothing to do for now, but that
-	// may change, depending on what happens with issue 7363.
-	if field.Anonymous() {
-		return
-	}
-
-	if field.Exported() {
-		return
-	}
-
-	for _, enc := range [...]string{"json", "xml"} {
-		if reflect.StructTag(tag).Get(enc) != "" {
-			f.Badf(field.Pos(), "struct field %s has %s tag but is not exported", field.Name(), enc)
-			return
-		}
-	}
 }
 
 var (

src/cmd/vet/testdata/structtag.go

@@ -42,43 +42,54 @@ type JSONEmbeddedField struct {
 type AnonymousJSON struct{}
 type AnonymousXML struct{}
 
+type AnonymousJSONField struct {
+	DuplicateAnonJSON int `json:"a"`
+
+	A int "hello" // ERROR "`hello` not compatible with reflect.StructTag.Get: bad syntax for struct tag pair"
+}
+
 type DuplicateJSONFields struct {
 	JSON              int `json:"a"`
-	DuplicateJSON     int `json:"a"` // ERROR "struct field DuplicateJSON repeats json tag .a. also at structtag.go:46"
+	DuplicateJSON     int `json:"a"` // ERROR "struct field DuplicateJSON repeats json tag .a. also at structtag.go:52"
 	IgnoredJSON       int `json:"-"`
 	OtherIgnoredJSON  int `json:"-"`
 	OmitJSON          int `json:",omitempty"`
 	OtherOmitJSON     int `json:",omitempty"`
-	DuplicateOmitJSON int `json:"a,omitempty"` // ERROR "struct field DuplicateOmitJSON repeats json tag .a. also at structtag.go:46"
+	DuplicateOmitJSON int `json:"a,omitempty"` // ERROR "struct field DuplicateOmitJSON repeats json tag .a. also at structtag.go:52"
 	NonJSON           int `foo:"a"`
 	DuplicateNonJSON  int `foo:"a"`
 	Embedded          struct {
 		DuplicateJSON int `json:"a"` // OK because its not in the same struct type
 	}
-	AnonymousJSON `json:"a"` // ERROR "struct field AnonymousJSON repeats json tag .a. also at structtag.go:46"
+	AnonymousJSON `json:"a"` // ERROR "struct field AnonymousJSON repeats json tag .a. also at structtag.go:52"
+
+	AnonymousJSONField // ERROR "struct field DuplicateAnonJSON repeats json tag .a. also at structtag.go:52"
 
 	XML              int `xml:"a"`
-	DuplicateXML     int `xml:"a"` // ERROR "struct field DuplicateXML repeats xml tag .a. also at structtag.go:60"
+	DuplicateXML     int `xml:"a"` // ERROR "struct field DuplicateXML repeats xml tag .a. also at structtag.go:68"
 	IgnoredXML       int `xml:"-"`
 	OtherIgnoredXML  int `xml:"-"`
 	OmitXML          int `xml:",omitempty"`
 	OtherOmitXML     int `xml:",omitempty"`
-	DuplicateOmitXML int `xml:"a,omitempty"` // ERROR "struct field DuplicateOmitXML repeats xml tag .a. also at structtag.go:60"
+	DuplicateOmitXML int `xml:"a,omitempty"` // ERROR "struct field DuplicateOmitXML repeats xml tag .a. also at structtag.go:68"
 	NonXML           int `foo:"a"`
 	DuplicateNonXML  int `foo:"a"`
 	Embedded2        struct {
 		DuplicateXML int `xml:"a"` // OK because its not in the same struct type
 	}
-	AnonymousXML `xml:"a"` // ERROR "struct field AnonymousXML repeats xml tag .a. also at structtag.go:60"
+	AnonymousXML `xml:"a"` // ERROR "struct field AnonymousXML repeats xml tag .a. also at structtag.go:68"
 	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"`           // ERROR "struct field DupAttr repeats xml attribute tag .b. also at structtag.go:76"
-		DupOmitAttr int      `xml:"b,omitempty,attr"` // ERROR "struct field DupOmitAttr repeats xml attribute tag .b. also at structtag.go:76"
+		DupAttr     int      `xml:"b,attr"`           // ERROR "struct field DupAttr repeats xml attribute tag .b. also at structtag.go:84"
+		DupOmitAttr int      `xml:"b,omitempty,attr"` // ERROR "struct field DupOmitAttr repeats xml attribute tag .b. also at structtag.go:84"
 
-		AnonymousXML `xml:"b,attr"` // ERROR "struct field AnonymousXML repeats xml attribute tag .b. also at structtag.go:76"
+		AnonymousXML `xml:"b,attr"` // ERROR "struct field AnonymousXML repeats xml attribute tag .b. also at structtag.go:84"
 	}
+
+	AnonymousJSONField `json:"not_anon"` // ok; fields aren't embedded in JSON
+	AnonymousJSONField `json:"-"`        // ok; entire field is ignored in JSON
 }
 
 type UnexpectedSpacetest struct {