From: Alex Browne Date: Sun, 8 Nov 2015 04:54:41 +0000 (-0500) Subject: cmd/vet: check for duplicate json, xml struct field tags X-Git-Tag: go1.8beta1~892 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=4940a8379096a48af682c266a0e104f249a80816;p=gostls13.git 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 TryBot-Result: Gobot Gobot Reviewed-by: Russ Cox --- diff --git a/src/cmd/vet/main.go b/src/cmd/vet/main.go index b317c88738..8149ba04e0 100644 --- 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 8134c585b3..1b92aaf51b 100644 --- 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 6878f5642d..74c7b541cb 100644 --- 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 + } +}