staticcheck: detect unintentional encoder and decoder implementations

[ainar-g]

(Couldn't think of a more descriptive title, feel free to change.)

See this tweet from @katiehockman.

func main() {
	t := struct {
		time.Time
		N int
	}{time.Now(), 5}

	m, _ := json.Marshal(t)
	fmt.Printf("%s", m)
}

This will print 2009-11-10T23:00:00Z, because time.Time implements json.Marshaler.

The structs that we can definitely mark are the structs that (1) have fields with json tags, (2) embed marshalers, and (3) don't implement the interface themselves:

type bad struct {
	time.Time
	N int `json:"n"`
}

[FiloSottile]

I'd argue that any type that embeds a type that implements TextMarshaler, BinaryMarshaler, or json.Marshaler (and unmarshalers) should get flagged. It's almost certainly not intended, and if it is it can be implemented in the parent type by calling the embedded method.

[ainar-g]

@FiloSottile I think, that's way too radical. Consider something like:

type TimeWithSomeContext struct {
	time.Time

	Processed bool
	Status    int
	// …
}

I want TimeWithSomeContext to look the same as the usual time in my JSON, because the users of the JSON API don't care about Processed, Status, etc., so embedding time.Time is perfectly fine here. Forcing all programmers to add a MarshalJSON method here would be too much, and also not quite something that's in staticcheck's philosophy (at least as I understand it from Dominik's previous comments).

[broady]

@ainar-g you'd do this to resolve the error. seems nice to be explicit:

type TimeWithSomeContext struct {
	Time time.Time

	Processed bool
	Status    int
	// …
}

func (t TimeWithSomeContext) MarshalJSON() ([]byte, error) {
	return t.Time.MarshalJSON()
}

[dominikh]

I agree with Ainar. We can very likely flag structs that use tags, but flagging any struct that embeds a marshaler will lead to some false positives, and we always try to err on the side of false negatives, not false positives. It's fine to miss some bugs, it's not fine to claim bugs where there aren't any.

We could consider adding an opt-in for more strict checking a la Filippo, and then you can decide to enforce that rule in your own code base.

[FiloSottile]

Could you also flag locations where such a type is passed to json.Marshal?

[dominikh]

Could you also flag locations where such a type is passed to json.Marshal?

The statically known ones, yes. For the case of embedding but no struct tags, we can only flag the locations where json.Marshal etc are being called; merely embedding a marshaler in itself is not an error if the type is never being marshaled.