vet: improve validation of struct tags

Actually parse the strings to check them more accurately.
The particular problem it missed was that it didn't check
for control characters in the key. The only valid separator
is a space.

More tests.

Fixes #9500

Change-Id: Ib547e11c7e8d47d81eb8b1e8f1ab9c26174933df
Reviewed-on: https://go-review.googlesource.com/2685
Reviewed-by: Russ Cox <rsc@golang.org>

cmd/vet/structtag.go

@@ -7,9 +7,12 @@
 package main
 
 import (
+	"errors"
 	"go/ast"
 	"reflect"
 	"strconv"
+	"strings"
+	"unicode"
 )
 
 func init() {
@@ -32,13 +35,8 @@ func checkCanonicalFieldTag(f *File, node ast.Node) {
 		return
 	}
 
-	// Check tag for validity by appending
-	// new key:value to end and checking that
-	// the tag parsing code can find it.
-	st := reflect.StructTag(tag + ` _gofix:"_magic"`)
-	if st.Get("_gofix") != "_magic" {
-		f.Badf(field.Pos(), "struct field tag %s not compatible with reflect.StructTag.Get", field.Tag.Value)
-		return
+	if err := validateStructTag(tag); err != nil {
+		f.Badf(field.Pos(), "struct field tag %s not compatible with reflect.StructTag.Get: %s", field.Tag.Value, err)
 	}
 
 	// Check for use of json or xml tags with unexported fields.
@@ -53,6 +51,7 @@ func checkCanonicalFieldTag(f *File, node ast.Node) {
 		return
 	}
 
+	st := reflect.StructTag(tag)
 	for _, enc := range [...]string{"json", "xml"} {
 		if st.Get(enc) != "" {
 			f.Badf(field.Pos(), "struct field %s has %s tag but is not exported", field.Names[0].Name, enc)
@@ -60,3 +59,44 @@ func checkCanonicalFieldTag(f *File, node ast.Node) {
 		}
 	}
 }
+
+var (
+	errTagSyntax      = errors.New("bad syntax for struct tag pair")
+	errTagKeySyntax   = errors.New("bad syntax for struct tag key")
+	errTagValueSyntax = errors.New("bad syntax for struct tag value")
+)
+
+// validateStructTag parses the struct tag and returns an error if it is not
+// in the canonical format, which is a space-separated list of key:"value"
+// settings.
+func validateStructTag(tag string) error {
+	elems := strings.Split(tag, " ")
+	for _, elem := range elems {
+		if elem == "" {
+			continue
+		}
+		fields := strings.SplitN(elem, ":", 2)
+		if len(fields) != 2 {
+			return errTagSyntax
+		}
+		key, value := fields[0], fields[1]
+		if len(key) == 0 || len(value) < 2 {
+			return errTagSyntax
+		}
+		// Key must not contain control characters or quotes.
+		for _, r := range key {
+			if r == '"' || unicode.IsControl(r) {
+				return errTagKeySyntax
+			}
+		}
+		if value[0] != '"' || value[len(value)-1] != '"' {
+			return errTagValueSyntax
+		}
+		// Value must be quoted string
+		_, err := strconv.Unquote(value)
+		if err != nil {
+			return errTagValueSyntax
+		}
+	}
+	return nil
+}

cmd/vet/testdata/structtag.go

@@ -2,14 +2,17 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-// This file contains tests for the structtag checker.
-
 // This file contains the test for canonical struct tags.
 
 package testdata
 
 type StructTagTest struct {
-	X int "hello" // ERROR "not compatible with reflect.StructTag.Get"
+	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"
+	C   int "x:\"y\"\tx:\"y\"" // ERROR "not compatible with reflect.StructTag.Get"
+	D   int "x:`y`"            // ERROR "not compatible with reflect.StructTag.Get: bad syntax for struct tag value"
+	OK0 int `x:"y" u:"v" w:""`
+	OK1 int `x:"y:z" u:"v" w:""` // note multiple colons.
 }
 
 type UnexportedEncodingTagTest struct {