encoding/json: consider implications of using TextAppender in V1 backwards-compatibility mode

[matloob]

cc @adonovan @prattmic

The proposal to introduce encoding.TextAppender (#62384) proposed it with the intention that it would be used instead of MarshalText to avoid unnecessary allocations. It seems clear from the discussions that it was intended to be used by encoding/json/v2 as an optimization instead of using MarshalText. But unfortunately we didn't do that when Go 1.24 was released, so users have started adding the TextAppender interface, expecting that it wouldn't be called by encoding/json. If we had started using it in json when 1.24 was released it would have been better because we had done the work to check that the interface wasn't in use in open source code.

Currently on tip, marshaling JSON for a type implementing TextAppender can result in a different output than in 1.26. That seems like an unnecessarily breaking change and there is a package we've found, github.com/open-policy-agent/opa/v1/ast that depends on the behavior of the json package not using TextAppender.

I think we should reconsider having the encoding/json v1 compatibility mode from using TextAppender to avoid breaking compatibility.

[matloob]

Here's a playground link for a repro https://go.dev/play/p/N0g0axeoSTr?v=gotip

[adonovan]

cc: @dsnet

[neild]

Relying on something calling MarshalText but not AppendText seems really horrible.

[dsnet]

When I proposed #62384, the intent was that AppendText should purely be an optimization on MarshalText. It should be semantically equal to MarshalText (which implies the presence of MarshalText.

[matloob]

So it's definitely clear from the proposal that AppendText should purely be an optimization on MarshalText, but not as clear from reading the encoding package docs.

The current docs say If a type implements both [TextAppender](https://pkg.go.dev/encoding#TextAppender) and [TextMarshaler](https://pkg.go.dev/encoding#TextMarshaler), then v.MarshalText() must be semantically identical to v.AppendText(nil).

But that's a weaker condition than the condition that we're implicitly depending on, which is the condition in the abandoned CL https://go-review.googlesource.com/c/go/+/635336: If a type implements [TextAppender], then it must also implement [TextMarshaler] where v.MarshalText() must be semantically identical to v.AppendText(nil).

I think if we want to keep the original intent, and keep the json behavior unchanged, we should make it more clear in the docs that either of the functions can be called interchangeably.

One middle ground to keep compatibility though while respecting the condition we require in the doc would be that if we're operating in the compatibility mode we check for MarshalText before calling AppendText.

[dsnet]

One middle ground to keep compatibility though while respecting the condition we require in the doc would be that if we're operating in the compatibility mode we check for MarshalText before calling AppendText.

Maybe I'm missing something, but how would that help github.com/open-policy-agent/opa/v1/ast? IIUC, it only implements AppendText and not MarshalText.

[matloob]

If encoding/json doesn't call AppendText if MarshalText isn't implemented, then AppendText won't be called by the json package. They are depending on the standard struct marshaling, so if we don't call AppendText we'll fall through to the struct marshalling and keep the previous behavior.

[dsnet]

Oh I see, I interpreted your earlier comment of "we check for MarshalText before calling AppendText" as saying we support both but MarshalText takes precedence before AppendText, but what's actually being proposed is only respecting AppendText if MarshalText is also present.

This feels a bit gross, but I'm not completely opposed to behavior being configured by CallMethodsWithLegacySemantics. I still lean towards this being an abuse of how AppendText was supposed to function.

[matloob]

I totally agree that it's an abuse of how AppendText was supposed to function. sigh

[dmitshur]

That mentioned package has a comment in its AppendText method implementation:

// NOTE(anderseknert): this DOES allocate still, and while that's unfortunate,
// we'll be better off dealing with that when we have v2 JSON in the stdlib than
// doing manual JSON marshalling (and string length calculations) here.

Which suggests its maintainers are interested in starting to use encoding/json/v2 and would be willing to update that code to handle this better. If this situation is rare enough to only affect it and maybe a handful of others, then it might be preferable to report it so they can handle it on their side. If that package is an indicator of a more widespread breakage, then it's an argument in favor of making a change here instead.

[gabyhelp]

Related Issues

• proposal: encoding/json: new MarshalAppender interface to make custom marshallers more efficient #34701 (closed)
• encoding: add AppendText and AppendBinary #62384 (closed)
• log/slog: TextHandler doesn't use encoding.TextAppender #79672 (closed)
• net,net/netip: implement the encoding.(Binary|Text)Appender #69022 (closed)

Related Code Changes

• encoding/json: support TextAppender

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[matloob]

I filed open-policy-agent/opa#8743 with that project, to see if we could get more insight on how much of an issue this would be for them.

[anderseknert]

Thanks for reaching out. As @dmitshur suggested, this was me possibly jumping the gun a bit after I had experimented with the JSON v2 implementation some time back. While we currently call these functions to implement String() (which is JSON-ish, but not JSON) I’d be more than happy to change that, and I doubt we have downstream consumers calling AppendText directly. So not only are we OK with this landing in 1.27, we’re looking forward to it!

[anderseknert]

So it's definitely clear from the proposal that AppendText should purely be an optimization on MarshalText, but not as clear from reading the encoding package docs.

Yes, those are the only docs I saw on that. I had no idea that was considered a "really horrible" thing to do. Of all the horrible things I've done writing code, I'd rank this fairly low myself.