cmd/vet: check for duplicate json, xml struct field tags

It is easy to make the mistake of duplicating json struct field
tags especially when copy/pasting. This commit causes go vet to
report the mistake. Only field tags in the same struct type are
considered, because that is the only case which is undoubtedly an
error.

Fixes #12791.

Change-Id: I4130e4c04b177694cc0daf8f1acaf0751d4f062b
Reviewed-on: https://go-review.googlesource.com/16704
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>

diff --git a/src/cmd/vet/main.go b/src/cmd/vet/main.go
index b317c887384ac24cbe26823652230ee357a3b493..8149ba04e01a600780e49e1738b373aaaee961d5 100644 (file)
--- a/src/cmd/vet/main.go
+++ b/src/cmd/vet/main.go
@@ -133,13 +133,13 @@ var (
        callExpr      *ast.CallExpr
        compositeLit  *ast.CompositeLit
        exprStmt      *ast.ExprStmt
-       field         *ast.Field
        funcDecl      *ast.FuncDecl
        funcLit       *ast.FuncLit
        genDecl       *ast.GenDecl
        interfaceType *ast.InterfaceType
        rangeStmt     *ast.RangeStmt
        returnStmt    *ast.ReturnStmt
+       structType    *ast.StructType
 
        // checkers is a two-level map.
        // The outer level is keyed by a nil pointer, one of the AST vars above.
@@ -478,8 +478,6 @@ func (f *File) Visit(node ast.Node) ast.Visitor {
                key = compositeLit
        case *ast.ExprStmt:
                key = exprStmt
-       case *ast.Field:
-               key = field
        case *ast.FuncDecl:
                key = funcDecl
        case *ast.FuncLit:
@@ -492,6 +490,8 @@ func (f *File) Visit(node ast.Node) ast.Visitor {
                key = rangeStmt
        case *ast.ReturnStmt:
                key = returnStmt
+       case *ast.StructType:
+               key = structType
        }
        for _, fn := range f.checkers[key] {
                fn(f, node)

diff --git a/src/cmd/vet/structtag.go b/src/cmd/vet/structtag.go
index 8134c585b33a65819781c0b3d25d2c0d54a441f5..1b92aaf51b74846a57a50d8a9ce5b7eded293337 100644 (file)
--- a/src/cmd/vet/structtag.go
+++ b/src/cmd/vet/structtag.go
@@ -9,20 +9,31 @@ package main
 import (
        "errors"
        "go/ast"
+       "go/token"
        "reflect"
        "strconv"
+       "strings"
 )
 
 func init() {
        register("structtags",
                "check that struct field tags have canonical format and apply to exported fields as needed",
-               checkCanonicalFieldTag,
-               field)
+               checkStructFieldTags,
+               structType)
 }
 
-// checkCanonicalFieldTag checks a struct field tag.
-func checkCanonicalFieldTag(f *File, node ast.Node) {
-       field := node.(*ast.Field)
+// checkStructFieldTags checks all the field tags of a struct, including checking for duplicates.
+func checkStructFieldTags(f *File, node ast.Node) {
+       var seen map[[2]string]token.Pos
+       for _, field := range node.(*ast.StructType).Fields.List {
+               checkCanonicalFieldTag(f, field, &seen)
+       }
+}
+
+var checkTagDups = []string{"json", "xml"}
+
+// checkCanonicalFieldTag checks a single struct field tag.
+func checkCanonicalFieldTag(f *File, field *ast.Field, seen *map[[2]string]token.Pos) {
        if field.Tag == nil {
                return
        }
@@ -38,6 +49,24 @@ func checkCanonicalFieldTag(f *File, node ast.Node) {
                f.Badf(field.Pos(), "struct field tag %q not compatible with reflect.StructTag.Get: %s", raw, err)
        }
 
+       for _, key := range checkTagDups {
+               val := reflect.StructTag(tag).Get(key)
+               if val == "" || val == "-" || val[0] == ',' {
+                       continue
+               }
+               if i := strings.Index(val, ","); i >= 0 {
+                       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))
+               } 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
@@ -50,9 +79,8 @@ func checkCanonicalFieldTag(f *File, node ast.Node) {
                return
        }
 
-       st := reflect.StructTag(tag)
        for _, enc := range [...]string{"json", "xml"} {
-               if st.Get(enc) != "" {
+               if reflect.StructTag(tag).Get(enc) != "" {
                        f.Badf(field.Pos(), "struct field %s has %s tag but is not exported", field.Names[0].Name, enc)
                        return
                }

diff --git a/src/cmd/vet/testdata/structtag.go b/src/cmd/vet/testdata/structtag.go
index 6878f5642d9a920fdfd482c8b7936df485061ab5..74c7b541cbe1e561ae19d4b18ad0549223990bea 100644 (file)
--- a/src/cmd/vet/testdata/structtag.go
+++ b/src/cmd/vet/testdata/structtag.go
@@ -34,3 +34,31 @@ type JSONEmbeddedField struct {
        UnexportedEncodingTagTest `is:"embedded"`
        unexp                     `is:"embedded,notexported" json:"unexp"` // OK for now, see issue 7363
 }
+
+type DuplicateJSONFields struct {
+       JSON              int `json:"a"`
+       DuplicateJSON     int `json:"a"` // ERROR "struct field DuplicateJSON repeats json tag .a. also at testdata/structtag.go:39"
+       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:39"
+       NonJSON           int `foo:"a"`
+       DuplicateNonJSON  int `foo:"a"`
+       Embedded          struct {
+               DuplicateJSON int `json:"a"` // OK because its not in the same struct type
+       }
+
+       XML              int `xml:"a"`
+       DuplicateXML     int `xml:"a"` // ERROR "struct field DuplicateXML repeats xml tag .a. also at testdata/structtag.go:52"
+       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:52"
+       NonXML           int `foo:"a"`
+       DuplicateNonXML  int `foo:"a"`
+       Embedded         struct {
+               DuplicateXML int `xml:"a"` // OK because its not in the same struct type
+       }
+}