cmd/jsontags: add static analyzer for incompatible `json` struct tags (#17670)

This migrates an internal tool to open source
so that we can run it on the tailscale.com module as well.

This PR does not yet set up a CI to run this analyzer.

Updates tailscale/corp#791

Signed-off-by: Joe Tsai <joetsai@digital-static.net>

cmd/jsontags/analyzer.go

@@ -0,0 +1,201 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+// Package jsontags checks for incompatible usage of JSON struct tags.
+package jsontags
+
+import (
+	"go/ast"
+	"go/types"
+	"reflect"
+	"strings"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+var Analyzer = &analysis.Analyzer{
+	Name:     "jsonvet",
+	Doc:      "check for incompatible usages of JSON struct tags",
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (any, error) {
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	// TODO: Report byte arrays fields without an explicit `format` tag option.
+
+	inspect.Preorder([]ast.Node{(*ast.StructType)(nil)}, func(n ast.Node) {
+		structType, ok := pass.TypesInfo.Types[n.(*ast.StructType)].Type.(*types.Struct)
+		if !ok {
+			return // type information may be incomplete
+		}
+		for i := range structType.NumFields() {
+			fieldVar := structType.Field(i)
+			tag := reflect.StructTag(structType.Tag(i)).Get("json")
+			if tag == "" {
+				continue
+			}
+			var seenName, hasFormat bool
+			for opt := range strings.SplitSeq(tag, ",") {
+				if !seenName {
+					seenName = true
+					continue
+				}
+				switch opt {
+				case "omitempty":
+					// For bools, ints, uints, floats, strings, and interfaces,
+					// it is always safe to migrate from `omitempty` to `omitzero`
+					// so long as the type does not have an IsZero method or
+					// the IsZero method is identical to reflect.Value.IsZero.
+					//
+					// For pointers, it is only safe to migrate from `omitempty` to `omitzero`
+					// so long as the type does not have an IsZero method, regardless of
+					// whether the IsZero method is identical to reflect.Value.IsZero.
+					//
+					// For pointers, `omitempty` behaves identically on both v1 and v2
+					// so long as the type does not implement a Marshal method that
+					// might serialize as an empty JSON value (i.e., null, "", [], or {}).
+					hasIsZero := hasIsZeroMethod(fieldVar.Type()) && !hasPureIsZeroMethod(fieldVar.Type())
+					underType := fieldVar.Type().Underlying()
+					basic, isBasic := underType.(*types.Basic)
+					array, isArrayKind := underType.(*types.Array)
+					_, isMapKind := underType.(*types.Map)
+					_, isSliceKind := underType.(*types.Slice)
+					_, isPointerKind := underType.(*types.Pointer)
+					_, isInterfaceKind := underType.(*types.Interface)
+					supportedInV1 := isNumericKind(underType) ||
+						isBasic && basic.Kind() == types.Bool ||
+						isBasic && basic.Kind() == types.String ||
+						isArrayKind && array.Len() == 0 ||
+						isMapKind || isSliceKind || isPointerKind || isInterfaceKind
+					notSupportedInV2 := isNumericKind(underType) ||
+						isBasic && basic.Kind() == types.Bool
+					switch {
+					case isMapKind, isSliceKind:
+						// This operates the same under both v1 and v2 so long as
+						// the map or slice type does not implement Marshal
+						// that could emit an empty JSON value for cases
+						// other than when the map or slice are empty.
+						// This is very rare.
+					case isString(fieldVar.Type()):
+						// This operates the same under both v1 and v2.
+						// These are safe to migrate to `omitzero`,
+						// but doing so is probably unnecessary churn.
+						// Note that this is only for a unnamed string type.
+					case !supportedInV1:
+						// This never worked in v1. Switching to `omitzero`
+						// may lead to unexpected behavior changes.
+						report(pass, structType, fieldVar, OmitEmptyUnsupportedInV1)
+					case notSupportedInV2:
+						// This does not work in v2. Switching to `omitzero`
+						// may lead to unexpected behavior changes.
+						report(pass, structType, fieldVar, OmitEmptyUnsupportedInV2)
+					case !hasIsZero:
+						// These are safe to migrate to `omitzero` such that
+						// it behaves identically under v1 and v2.
+						report(pass, structType, fieldVar, OmitEmptyShouldBeOmitZero)
+					case isPointerKind:
+						// This operates the same under both v1 and v2 so long as
+						// the pointer type does not implement Marshal that
+						// could emit an empty JSON value.
+						// For example, time.Time is safe since the zero value
+						// never marshals as an empty JSON string.
+					default:
+						// This is a non-pointer type with an IsZero method.
+						// If IsZero is not identical to reflect.Value.IsZero,
+						// omission may behave slightly differently when using
+						// `omitzero` instead of `omitempty`.
+						// Thus the finding uses the word "should".
+						report(pass, structType, fieldVar, OmitEmptyShouldBeOmitZeroButHasIsZero)
+					}
+				case "string":
+					if !isNumericKind(fieldVar.Type()) {
+						report(pass, structType, fieldVar, StringOnNonNumericKind)
+					}
+				default:
+					key, _, ok := strings.Cut(opt, ":")
+					hasFormat = key == "format" && ok
+				}
+			}
+			if !hasFormat && isTimeDuration(mayPointerElem(fieldVar.Type())) {
+				report(pass, structType, fieldVar, FormatMissingOnTimeDuration)
+			}
+		}
+	})
+	return nil, nil
+}
+
+// hasIsZeroMethod reports whether t has an IsZero method.
+func hasIsZeroMethod(t types.Type) bool {
+	for method := range types.NewMethodSet(t).Methods() {
+		if fn, ok := method.Type().(*types.Signature); ok && method.Obj().Name() == "IsZero" {
+			if fn.Params().Len() == 0 && fn.Results().Len() == 1 && isBool(fn.Results().At(0).Type()) {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+// isBool reports whether t is a bool type.
+func isBool(t types.Type) bool {
+	basic, ok := t.(*types.Basic)
+	return ok && basic.Kind() == types.Bool
+}
+
+// isString reports whether t is a string type.
+func isString(t types.Type) bool {
+	basic, ok := t.(*types.Basic)
+	return ok && basic.Kind() == types.String
+}
+
+// isTimeDuration reports whether t is a time.Duration type.
+func isTimeDuration(t types.Type) bool {
+	return isNamed(t, "time", "Duration")
+}
+
+// mayPointerElem returns the pointed-at type if t is a pointer,
+// otherwise it returns t as-is.
+func mayPointerElem(t types.Type) types.Type {
+	if pointer, ok := t.(*types.Pointer); ok {
+		return pointer.Elem()
+	}
+	return t
+}
+
+// isNamed reports t is a named typed of the given path and name.
+func isNamed(t types.Type, path, name string) bool {
+	gotPath, gotName := typeName(t)
+	return gotPath == path && gotName == name
+}
+
+// typeName reports the pkgPath and name of the type.
+// It recursively follows type aliases to get the underlying named type.
+func typeName(t types.Type) (pkgPath, name string) {
+	if named, ok := types.Unalias(t).(*types.Named); ok {
+		obj := named.Obj()
+		if pkg := obj.Pkg(); pkg != nil {
+			return pkg.Path(), obj.Name()
+		}
+		return "", obj.Name()
+	}
+	return "", ""
+}
+
+// isNumericKind reports whether t is a numeric kind.
+func isNumericKind(t types.Type) bool {
+	if basic, ok := t.Underlying().(*types.Basic); ok {
+		switch basic.Kind() {
+		case types.Int, types.Int8, types.Int16, types.Int32, types.Int64:
+		case types.Uint, types.Uint8, types.Uint16, types.Uint32, types.Uint64, types.Uintptr:
+		case types.Float32, types.Float64:
+		default:
+			return false
+		}
+		return true
+	}
+	return false
+}

cmd/jsontags/iszero.go

@@ -0,0 +1,75 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+package jsontags
+
+import (
+	"go/types"
+	"reflect"
+
+	"tailscale.com/util/set"
+)
+
+var _ = reflect.Value.IsZero // refer for hot-linking purposes
+
+var pureIsZeroMethods map[string]set.Set[string]
+
+// hasPureIsZeroMethod reports whether the IsZero method is truly
+// identical to [reflect.Value.IsZero].
+func hasPureIsZeroMethod(t types.Type) bool {
+	// TODO: Detect this automatically by checking the method AST?
+	path, name := typeName(t)
+	return pureIsZeroMethods[path].Contains(name)
+}
+
+// PureIsZeroMethodsInTailscaleModule is a list of known IsZero methods
+// in the "tailscale.com" module that are pure.
+var PureIsZeroMethodsInTailscaleModule = map[string]set.Set[string]{
+	"tailscale.com/net/packet": set.Of(
+		"TailscaleRejectReason",
+	),
+	"tailscale.com/tailcfg": set.Of(
+		"UserID",
+		"LoginID",
+		"NodeID",
+		"StableNodeID",
+	),
+	"tailscale.com/tka": set.Of(
+		"AUMHash",
+	),
+	"tailscale.com/types/geo": set.Of(
+		"Point",
+	),
+	"tailscale.com/tstime/mono": set.Of(
+		"Time",
+	),
+	"tailscale.com/types/key": set.Of(
+		"NLPrivate",
+		"NLPublic",
+		"DERPMesh",
+		"MachinePrivate",
+		"MachinePublic",
+		"ControlPrivate",
+		"DiscoPrivate",
+		"DiscoPublic",
+		"DiscoShared",
+		"HardwareAttestationPublic",
+		"ChallengePublic",
+		"NodePrivate",
+		"NodePublic",
+	),
+	"tailscale.com/types/netlogtype": set.Of(
+		"Connection",
+		"Counts",
+	),
+}
+
+// RegisterPureIsZeroMethods specifies a list of pure IsZero methods
+// where it is identical to calling [reflect.Value.IsZero] on the receiver.
+// This is not strictly necessary, but allows for more accurate
+// detection of improper use of `json` tags.
+//
+// This must be called at init and the input must not be mutated.
+func RegisterPureIsZeroMethods(methods map[string]set.Set[string]) {
+	pureIsZeroMethods = methods
+}

cmd/jsontags/report.go

@@ -0,0 +1,135 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+package jsontags
+
+import (
+	"fmt"
+	"go/types"
+	"os"
+	"strings"
+
+	_ "embed"
+
+	"golang.org/x/tools/go/analysis"
+	"tailscale.com/util/set"
+)
+
+var jsontagsAllowlist map[ReportKind]set.Set[string]
+
+// ParseAllowlist parses an allowlist of reports to ignore,
+// which is a newline-delimited list of tuples separated by a tab,
+// where each tuple is a [ReportKind] and a fully-qualified field name.
+//
+// For example:
+//
+//	OmitEmptyUnsupportedInV1	tailscale.com/path/to/package.StructType.FieldName
+//	OmitEmptyUnsupportedInV1	tailscale.com/path/to/package.*.FieldName
+//
+// The struct type name may be "*" for anonymous struct types such
+// as those declared within a function or as a type literal in a variable.
+func ParseAllowlist(b []byte) map[ReportKind]set.Set[string] {
+	var allowlist map[ReportKind]set.Set[string]
+	for line := range strings.SplitSeq(string(b), "\n") {
+		kind, field, _ := strings.Cut(strings.TrimSpace(line), "\t")
+		if allowlist == nil {
+			allowlist = make(map[ReportKind]set.Set[string])
+		}
+		fields := allowlist[ReportKind(kind)]
+		if fields == nil {
+			fields = make(set.Set[string])
+		}
+		fields.Add(field)
+		allowlist[ReportKind(kind)] = fields
+	}
+	return allowlist
+}
+
+// RegisterAllowlist registers an allowlist of reports to ignore,
+// which is represented by a set of fully-qualified field names
+// for each [ReportKind].
+//
+// For example:
+//
+//	{
+//		"OmitEmptyUnsupportedInV1": set.Of(
+//			"tailscale.com/path/to/package.StructType.FieldName",
+//			"tailscale.com/path/to/package.*.FieldName",
+//		),
+//	}
+//
+// The struct type name may be "*" for anonymous struct types such
+// as those declared within a function or as a type literal in a variable.
+//
+// This must be called at init and the input must not be mutated.
+func RegisterAllowlist(allowlist map[ReportKind]set.Set[string]) {
+	jsontagsAllowlist = allowlist
+}
+
+type ReportKind string
+
+const (
+	OmitEmptyUnsupportedInV1              ReportKind = "OmitEmptyUnsupportedInV1"
+	OmitEmptyUnsupportedInV2              ReportKind = "OmitEmptyUnsupportedInV2"
+	OmitEmptyShouldBeOmitZero             ReportKind = "OmitEmptyShouldBeOmitZero"
+	OmitEmptyShouldBeOmitZeroButHasIsZero ReportKind = "OmitEmptyShouldBeOmitZeroButHasIsZero"
+	StringOnNonNumericKind                ReportKind = "StringOnNonNumericKind"
+	FormatMissingOnTimeDuration           ReportKind = "FormatMissingOnTimeDuration"
+)
+
+func (k ReportKind) message() string {
+	switch k {
+	case OmitEmptyUnsupportedInV1:
+		return "uses `omitempty` on an unspported type in json/v1; should probably use `omitzero` instead"
+	case OmitEmptyUnsupportedInV2:
+		return "uses `omitempty` on an unspported type in json/v2; should probably use `omitzero` instead"
+	case OmitEmptyShouldBeOmitZero:
+		return "should use `omitzero` instead of `omitempty`"
+	case OmitEmptyShouldBeOmitZeroButHasIsZero:
+		return "should probably use `omitzero` instead of `omitempty`"
+	case StringOnNonNumericKind:
+		return "must not use `string` on non-numeric types"
+	case FormatMissingOnTimeDuration:
+		return "must use an explicit `format` tag (e.g., `format:nano`) on a time.Duration type; see https://go.dev/issue/71631"
+	default:
+		return string(k)
+	}
+}
+
+func report(pass *analysis.Pass, structType *types.Struct, fieldVar *types.Var, k ReportKind) {
+	// Lookup the full name of the struct type.
+	var fullName string
+	for _, name := range pass.Pkg.Scope().Names() {
+		if obj := pass.Pkg.Scope().Lookup(name); obj != nil {
+			if named, ok := obj.(*types.TypeName); ok {
+				if types.Identical(named.Type().Underlying(), structType) {
+					fullName = fmt.Sprintf("%v.%v.%v", named.Pkg().Path(), named.Name(), fieldVar.Name())
+					break
+				}
+			}
+		}
+	}
+	if fullName == "" {
+		// Full name could not be found since this is probably an anonymous type
+		// or locally declared within a function scope.
+		// Use just the package path and field name instead.
+		// This is imprecise, but better than nothing.
+		fullName = fmt.Sprintf("%s.*.%s", fieldVar.Pkg().Path(), fieldVar.Name())
+	}
+	if jsontagsAllowlist[k].Contains(fullName) {
+		return
+	}
+
+	const appendAllowlist = ""
+	if appendAllowlist != "" {
+		if f, err := os.OpenFile(appendAllowlist, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0664); err == nil {
+			fmt.Fprintf(f, "%v\t%v\n", k, fullName)
+			f.Close()
+		}
+	}
+
+	pass.Report(analysis.Diagnostic{
+		Pos:     fieldVar.Pos(),
+		Message: fmt.Sprintf("field %q %s", fieldVar.Name(), k.message()),
+	})
+}