Go-tools: staticcheck: pointer receiver of MarshalJSON is (almost) never nil

[ainar-g]

(This is either a subissue of #911 or is an issue largely related to that one. I feel like this one should be easier to implement, but feel free to merge.)

The [code]:

type T struct {
        N int `json:"n"`
}

func (t *T) MarshalJSON() (data []byte, err error) {
        if t == nil {
                return []byte(`"defaultdata"`), nil
        }

        return []byte(`"somedata"`), nil
}

From what I can tell, unless MarshalJSON is called direcly on a nil *T pointer, the if t == nil condition will never run, because encoding/json doesn't call it (see the full code and the output on Playground). So unless the code in question actually calls MarshalJSON on a pointer outside of tests, the branch is effectively unreachable.

[dgryski]

This seems like an implementation detail of encoding/json that may not be true for other JSON packages.

[ainar-g]

Ah, yes, true. If only other packages used different method and tag names. Alas.

[dominikh]

It might be safe enough to look at go.mod and look for imports of any differently-behaving JSON marshalers. Hopefully the combination of bizarre MarshalJSON implementation and _cross-module_ use of a different marshaler is extremely rare.

[dominikh]

Unfortunately, it can be nil more often than never, see https://play.golang.org/p/9FivTKSR3HT

[ainar-g]

That is… insidious. Yet another subtle bug to be aware of. I'm not even sure that that isn't a bug in encoding/json.

[dominikh]

I am rather confused by this, too.

The documentation says:

Marshal traverses the value v recursively. If an encountered value implements the Marshaler interface and is not a nil pointer, Marshal calls its MarshalJSON method to produce JSON. [...] The nil pointer exception is not strictly necessary but mimics a similar, necessary exception in the behavior of UnmarshalJSON.

The unit tests say

// golang.org/issue/16042.
// Even if a nil interface value is passed in, as long as
// it implements Marshaler, it should be marshaled.
type nilJSONMarshaler string

func (nm *nilJSONMarshaler) MarshalJSON() ([]byte, error) {
    if nm == nil {
        return Marshal("0zenil0")
    }
    return Marshal("zenil:" + string(*nm))
}

but have the following concrete test cases:

{v: struct{ M Marshaler }{(*nilJSONMarshaler)(nil)}, want: `{"M":"0zenil0"}`},
{v: struct{ M interface{} }{(*nilJSONMarshaler)(nil)}, want: `{"M":null}`},

That is, it explicitly tests that only one of them calls MarshalJSON, even though nothing seems to document this behavior.

/cc @mvdan as an expert on encoding/json corner cases.

[mvdan]

Thanks, I'll take a look next week. Perhaps @dsnet has thoughts in the meantime.

[dsnet]

The json package will never call MarshalJSON on a nil pointer receiver. That said, nothing prevents users from directly calling the MarshalJSON method on a type. As such, I don't think it's fair to say the nil check is always dead code.

[mvdan]

The tests above show a case where encoding/json calls MarshalJSON on a nil pointer receiver, though. That only seems to happen in some edge cases.

[dsnet]

Interesting. I'd say that's a bug in a json package since it fairly consistently does not call MarshalJSON on nil pointers for most cases. The bug probably occurs in https://github.com/golang/go/blob/8e91458b19b19584f2026c227265a02724700a55/src/encoding/json/encode.go#L470-L473, where it reflect.Value at the time of the check is a reflect.Interface and not a reflect.Ptr.

That said, my argument earlier about people calling MarshalJSON still stands, so I'm not sure it makes sense for staticcheck to flag such cases.

[dominikh]

I'd say that's a bug in a json package since it fairly consistently does not call MarshalJSON on nil pointers for most cases.

I find it hard to call it a bug when there are test cases explicitly checking for the current behavior.

where it reflect.Value at the time of the check is a reflect.Interface and not a reflect.Ptr

Do note that the behavior differs for a field of type interface{} that holds a type implementing json.Marshaler at runtime, and a field of type json.Marshaler. I don't think the difference is about pointer versus interface per se.

That said, my argument earlier about people calling MarshalJSON still stands, so I'm not sure it makes sense for staticcheck to flag such cases.

You are of course right that people may call the method directly, but intuition tells me that this happens rarely, and that the combination of manually calling it + having a nil check in it is even more rare. I don't think it would be counterproductive to flag the nil check, anyway, forcing the user to document it, stating that they're aware of the contexts in which it will and will not work.

I'll have to run the check against actual code bases, though, to see how high the rate of false positives would be.

[dsnet]

I find it hard to call it a bug when there are test cases explicitly checking for the current behavior.

That's the situation for few other test cases we would consider bugs. Unfortunately Hyrum's law makes it hard to fix such cases. :sob:

I'll have to run the check against actual code bases, though, to see how high the rate of false positives would be.

At Google, there are several thousand direct calls to MarshalJSON. I didn't analyze whether the callers depend on the call not panicking on nil-pointers, though.