]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/vet: check embedded field tags too
authorDaniel Martí <mvdan@mvdan.cc>
Fri, 1 Jun 2018 11:17:41 +0000 (12:17 +0100)
committerDaniel Martí <mvdan@mvdan.cc>
Thu, 23 Aug 2018 19:22:43 +0000 (19:22 +0000)
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
src/cmd/vet/testdata/structtag.go

index a2571419c75ad2411889ae1ddfb9fb899e8a08c5..32366eab44bac5a850df62fef739401e00fd7ad3 100644 (file)
@@ -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,50 +39,16 @@ 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 {
-               val := reflect.StructTag(tag).Get(key)
-               if val == "" || val == "-" || val[0] == ',' {
-                       continue
-               }
-               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
-               }
-               if i := strings.Index(val, ","); i >= 0 {
-                       if key == "xml" {
-                               // Use a separate namespace for XML attributes.
-                               for _, opt := range strings.Split(val[i:], ",") {
-                                       if opt == "attr" {
-                                               key += " attribute" // Key is part of the error message.
-                                               break
-                                       }
-                               }
-                       }
-                       val = val[:i]
-               }
-               if *seen == nil {
-                       *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))
-               } 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
@@ -102,6 +69,63 @@ func checkCanonicalFieldTag(f *File, field *types.Var, tag string, seen *map[[2]
        }
 }
 
+// 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 == "-" {
+               // 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.
+               return
+       }
+       if i := strings.Index(val, ","); i >= 0 {
+               if key == "xml" {
+                       // Use a separate namespace for XML attributes.
+                       for _, opt := range strings.Split(val[i:], ",") {
+                               if opt == "attr" {
+                                       key += " attribute" // Key is part of the error message.
+                                       break
+                               }
+                       }
+               }
+               val = val[:i]
+       }
+       if *seen == nil {
+               *seen = map[[2]string]token.Pos{}
+       }
+       if pos, ok := (*seen)[[2]string{key, val}]; ok {
+               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()
+       }
+}
+
 var (
        errTagSyntax      = errors.New("bad syntax for struct tag pair")
        errTagKeySyntax   = errors.New("bad syntax for struct tag key")
index 34bf9f6599b411fd598ce3c5553b6643f69b1631..ad55c4ab64daaab69ee3878c91a6eb323b6a6992 100644 (file)
@@ -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 {