cmd/vet: fix panic and handling of XML in struct field tag check

The check for duplicate struct field tags introduced in CL 16704
triggers a panic when an anonymous struct field with a duplicate name
is encountered. For such a field, the names slice of the ast.Field is
nil but accessed regardless to generate the warning message.

Additionally, the check produces false positives for XML tags in some
cases:

- When fields are encoded as XML attributes, a warning is produced when
  an attribute reuses a name previously used for an element.

  Example:
    type Foo struct {
        First int `xml:"a"`
        NoDup int `xml:"a,attr"` // warning about reuse of "a"
    }

- When XMLName is used to set the name of the enclosing struct element,
  it is treated as a regular struct field.

  Example:
    type Bar struct {
        XMLName xml.Name `xml:"a"`
        NoDup   int      `xml:"a"` // warning about reuse of "a"
    }

This commit addresses all three issues. The panic is avoided by using
the type name instead of the field name for anonymous struct fields when
generating the warning message. An additional namespace for checking XML
attribute names separately from element names is introduced. Lastly,
fields named XMLName are excluded from the check for duplicate tags.

Updates #18256

Change-Id: Ida48ea8584b56bd4d12ae3ebd588a66ced2594cc
Reviewed-on: https://go-review.googlesource.com/34070
Run-TryBot: Rob Pike <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>

diff --git a/src/cmd/vet/structtag.go b/src/cmd/vet/structtag.go
index 814bbda59499f8a5a5b25dfa5e6bd5a50a2e2d03..872fde79ce37bbb34754e08eaf9e2b7df1a051f3 100644 (file)
--- a/src/cmd/vet/structtag.go
+++ b/src/cmd/vet/structtag.go
@@ -54,14 +54,37 @@ func checkCanonicalFieldTag(f *File, field *ast.Field, seen *map[[2]string]token
                if val == "" || val == "-" || val[0] == ',' {
                        continue
                }
+               if key == "xml" && len(field.Names) > 0 && field.Names[0].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.Names[0].Name, key, val, f.loc(pos))
+                       var name string
+                       if len(field.Names) > 0 {
+                               name = field.Names[0].Name
+                       } else {
+                               name = field.Type.(*ast.Ident).Name
+                       }
+                       f.Badf(field.Pos(), "struct field %s repeats %s tag %q also at %s", name, key, val, f.loc(pos))
                } else {
                        (*seen)[[2]string{key, val}] = field.Pos()
                }

diff --git a/src/cmd/vet/testdata/structtag.go b/src/cmd/vet/testdata/structtag.go
index cba990fccd1a9b0f7e66b20f14da1b3d3a1de2fb..363aa898bf91b536289b6acf61ce01910b16f5ab 100644 (file)
--- a/src/cmd/vet/testdata/structtag.go
+++ b/src/cmd/vet/testdata/structtag.go
@@ -6,6 +6,8 @@
 
 package testdata
 
+import "encoding/xml"
+
 type StructTagTest struct {
        A   int "hello"            // ERROR "not compatible with reflect.StructTag.Get: bad syntax for struct tag pair"
        B   int "\tx:\"y\""        // ERROR "not compatible with reflect.StructTag.Get: bad syntax for struct tag key"
@@ -37,30 +39,44 @@ type JSONEmbeddedField struct {
        unexp                     `is:"embedded,notexported" json:"unexp"` // OK for now, see issue 7363
 }
 
+type AnonymousJSON struct{}
+type AnonymousXML struct{}
+
 type DuplicateJSONFields struct {
        JSON              int `json:"a"`
-       DuplicateJSON     int `json:"a"` // ERROR "struct field DuplicateJSON repeats json tag .a. also at testdata/structtag.go:41"
+       DuplicateJSON     int `json:"a"` // ERROR "struct field DuplicateJSON repeats json tag .a. also at testdata/structtag.go:46"
        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 testdata/structtag.go:41"
+       DuplicateOmitJSON int `json:"a,omitempty"` // ERROR "struct field DuplicateOmitJSON repeats json tag .a. also at testdata/structtag.go:46"
        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 testdata/structtag.go:46"
 
        XML              int `xml:"a"`
-       DuplicateXML     int `xml:"a"` // ERROR "struct field DuplicateXML repeats xml tag .a. also at testdata/structtag.go:54"
+       DuplicateXML     int `xml:"a"` // ERROR "struct field DuplicateXML repeats xml tag .a. also at testdata/structtag.go:60"
        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 testdata/structtag.go:54"
+       DuplicateOmitXML int `xml:"a,omitempty"` // ERROR "struct field DuplicateOmitXML repeats xml tag .a. also at testdata/structtag.go:60"
        NonXML           int `foo:"a"`
        DuplicateNonXML  int `foo:"a"`
        Embedded         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 testdata/structtag.go:60"
+       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 testdata/structtag.go:76"
+               DupOmitAttr int      `xml:"b,omitempty,attr"` // ERROR "struct field DupOmitAttr repeats xml attribute tag .b. also at testdata/structtag.go:76"
+
+               AnonymousXML `xml:"b,attr"` // ERROR "struct field AnonymousXML repeats xml attribute tag .b. also at testdata/structtag.go:76"
+       }
 }