Updated the example to show the proper and improper marshalings.

Updated again to cover the fact the same misbehavior happens when using it as a map value type, whether the map itself is handed as a pointer or not.

What change are you suggesting? Documentation?

When I write custom Marshaler/Unmarshaler types, I tend to use a pointer receiver only for Unmarshal and a value receiver for Marshal, to make this kind of mistake easier for the user to avoid. I think that the usage issue trumps consistency of pointer receivers in that case.

But it's too late to change json.RawMessage now.

The bug should be fixed. The misbehavior is obvious, and undocumented/undesired in every case where it happens.

What does "fixed" mean? Change RawMessage.MarshalJSON to use a pointer receiver? Special-case RawMessage in the encoder?

Fixed means making the output match most people's expectation, rather than silently having surprising undesired side-effects that break actual programs at runtime based on unperceived (and unintended) reasoning.

Has anybody prepared the fix so we know how invasive it'd be?

Would it actually be an API-breaking change to declare MarshalJSON on the
value type? I can't think of any way that it could be at the moment (I've
been thinking I'd proposing that very change for a while now)
On 10 Apr 2016 01:04, "Brad Fitzpatrick" notifications@github.com wrote:

Has anybody prepared the fix so we know how invasive it'd be?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#14493 (comment)

@rogpeppe, that wouldn't work:

$ go install .
# encoding/json
./stream.go:237: method redeclared: RawMessage.MarshalJSON
        method(*RawMessage) func() ([]byte, error)
        method(RawMessage) func() ([]byte, error)

I went to fix this (see https://golang.org/cl/21811) but it breaks the test for issue #6458 which seems to explicitly lock-in this behavior:

--- FAIL: TestIssue6458 (0.00s)
        encode_test.go:437: Marshal(x) = `{"M":"foo"}`; want `{"M":"ImZvbyI="}`
FAIL
exit status 1
FAIL    encoding/json   1.474s

From test:

func TestIssue6458(t *testing.T) {
        type Foo struct {
                M RawMessage
        }
        x := Foo{RawMessage(`"foo"`)}

        b, err := Marshal(&x)
        if err != nil {
                t.Fatal(err)
        }
        if want := `{"M":"foo"}`; string(b) != want {
                t.Errorf("Marshal(&x) = %#q; want %#q", b, want)
        }

        b, err = Marshal(x)
        if err != nil {
                t.Fatal(err)
        }

        if want := `{"M":"ImZvbyI="}`; string(b) != want {
                t.Errorf("Marshal(x) = %#q; want %#q", b, want)
        }
}

I don't really like that behavior, but I'm not sure whether we can change it.

@rsc?

CL https://golang.org/cl/21811 mentions this issue.

The value method would cover the pointer type too because that's how Go
methods work.

However as that issue demonstrates, it could indeed change behaviour where
code is currently expecting a RawMessage to marshal as a []byte, and that's
probably incompatible enough that we wouldn't want to change it.

What a pity.
On 10 Apr 2016 18:05, "Brad Fitzpatrick" notifications@github.com wrote:

@rogpeppe https://github.com/rogpeppe, that wouldn't work:

$ go install .

encoding/json

./stream.go:237: method redeclared: RawMessage.MarshalJSON
method(*RawMessage) func() ([]byte, error)
method(RawMessage) func() ([]byte, error)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#14493 (comment)

It would also change the signature of the function from MarshalJSON(*json.RawMessage) to MarshalJSON(json.RawMessage). Changing any of the types in a function declaration is a backwards incompatible change.

The options I see are (1) warnings in the documentation and (2) RawMessage2.

Could we not make this something that vet handles? Passing anything containing a non-addressable RawMessage (directly or transitively) into any of the json decoding functions is clearly a mistake (since if they want that behavior, they can just use a normal []byte)

dominikh: Yes, you're right, I hadn't thought about method expressions.

extemporalgenome: I don't think so. Vet often can't see the
types that are passed in to json encoding/decoding functions.

In my particular case, I wanted to round-trip map[string]json.RawMessage;
as it is I use map[string]*json.RawMessage but that doesn't seem ideal.

To be honest, the implementation of json.RawMessage is so trivial
that it's not a problem to reimplement with Marshal declared on the
value type, but that's still not ideal.

I think this is an instance of "just lock down the current behavior and move on". The fix is to use *RawMessage, right?

I just noticed that this is actually a duplicate of #6528. Though maybe that one should be closed in favor of this one, which has a lot more discussion.

For me, the fix is defining my own RawMessage and have a custom linter to ban the use of json.RawMessage. Not too bad.

I just spent few hours chasing this issue. Admittedly I didn't look in the issue database, but my guess is that others are likely not to do it as well.

May I suggest at least a warning in the RawMessage documentation?

Despite my comment from 2013 on #6528 and my comment here from April 2016, today I am having a very hard time stomaching the current behavior.

It's hard to see this as anything other than a bug we should fix. The whole reason RawMessage exists is to marshal its raw content. If it's not doing that, it seems like we should fix it. Yes, it might break people expecting something else, but those people can use plain []byte.

The difference in Gustavo's program https://play.golang.org/p/bHuvfyb7qB between Marshal(color1) and Marshal(&color1) further convinces me that we should fix RawMessage. It already "does the right thing" when it appears in an addressable context.

I think we can overrule the test case added in #6458, at least tentatively, and see whether any justifiable real code breaks.

Let's fix this (change RawMessage.Marshal to be a value method) and see if we can make it stick.

Leaving for @quentinmit.

I already had a CL for this (above). I can just resurrect it.

Updated https://go-review.googlesource.com/c/21811/

CL https://golang.org/cl/32472 mentions this issue.