staticcheck: detect marshaler implementations with wrong receiver types

[ainar-g]

Consider the following program:

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

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

func main() {
	var resp struct {
		T  T            `json:"t"`
		TA [1]T         `json:"ta"`
		TM map[string]T `json:"tm"`
		TS []T          `json:"ts"`
	}

	resp.T = T{1}
	resp.TA = [1]T{{2}}
	resp.TM = map[string]T{"3": {3}}
	resp.TS = []T{{4}}

	_ = json.NewEncoder(os.Stdout).Encode(resp)
}

https://play.golang.org/p/O54uPIT7x9z

A developer could assume that MarshalJSON will be called for all four fields of the structure, but in fact it's only called for the TS field. The actual output, prettified:

{
  "t": {
    "n": 1
  },
  "ta": [
    {
      "n": 2
    }
  ],
  "tm": {
    "3": {
      "n": 3
    }
  },
  "ts": [
    "somedata"
  ]
}

If we change the receiver of MarshalJSON from *T to T, it gets called for all fields as, probably, expected.

As a simple and mostly false-positive-free first approximation, we could mark MarshalJSON implementations that have a pointer receiver, but are only used in JSON-encoded structures as non-pointer fields.

I haven't inspected other encoders, like XML and YAML ones, but I assume they may have similar issues.

[josharian]

I just got bitten by this, and it wasn't trivial to debug. (The ultimate failure occurred far away.) I'd love to see this in static check. Upstream issue: golang/go#22967.

I'd suggest just flagging any MarshalJSON that uses a pointer receiver. Tracking down all uses of a type is ~impossible.

[ainar-g]

@josharian, I'm afraid that that would generate way too many false positives for staticcheck, as there is technically one valid way of using them, and a lot of projects will just make all receivers pointers without applying too much thought to that.

In the meantime, perhaps @dgryski's semgrep-go could implement that kind of check?

[josharian]

I think staticcheck can tolerate some level of false positives(?). And a pointer receiver is a code rake; just because no one has stepped on it yet doesn't really make it a good idea.

[ainar-g]

From what I've seen, Dominik strives to keep staticcheck false-positive-free, unless the false-positive case is extremely unlikely. I personally am not against your solution. In fact, I think I've found another issue related to pointer receivers on marshalers, but I'll need more time to investigate.

[dgryski]

rules:
  - id: marshal-json-pointer-receiver
    patterns:
            - pattern-either:
                - pattern: func ($T *$TYPE) MarshalJSON() ($DATA []byte, $ERR error)
                - pattern: func ($T *$TYPE) MarshalJSON() ([]byte, error)
    message: "MarshalJSON with a pointer receiver has surprising results: https://github.com/golang/go/issues/22967"
    languages: [go]
    severity: ERROR

dgryski/semgrep-go@e5d9684

[dgryski]

A quick grep through the Go code on my laptop shows a lot of MarshalJSON methods with pointer receivers. That's either a lot of broken code or a lot of false positives.

[ainar-g]

@dgryski, probably a reasonable amount of false-positives, since the code actually works if pointers are used in the parent structure. See https://play.golang.org/p/VDQOP00xyv4.

[dominikh]

I'm definitely not comfortable with outright banning MarshalJSON implementations on pointer receivers. A lot of people use the rule of "if some methods on a type use pointer receivers, then all methods should", to keep method sets consistent.

We'll have to make do with finding actual bugs.

[josharian]

Oh silly me, we can do it the other way: look for calls to json.Marshal in which the concrete type has a value for which this will be a problem.

[nikolaydubina]

nice! TIL!

[nikolaydubina]

even if not enforced, this is super useful.

for one, I was not aware of this. think 99% of devs are same.

if this is not enforced linter, and purely for showing warnings and information to users — do you guys know how to call such tool? code-scanner? static-analyser? there is def value in such things!

[ainar-g]

@nikolaydubina, that's a bit off-topic, and should generally be discussed in Discussions, not in issues. The general term seems to be “static analysis tools” or “linters”.