encoding/json/v2: remove detection of cyclic data structures

[dsnet]

Splitting this out from #79733 where @aclements raises a concern about handling of cyclic data structures:

@aclements: The docs say "Passing cyclic structures [to Marshal] will result in an error." Do we actually want to guarantee this, or just leave it as unspecified behavior? Detecting cycles can be expensive, and if we make this guarantee, we can never go back on that. It's also a little odd that everything else is so careful about saying what error type is returned, but this doesn't say.

@dsnet: The v2 API more or less says the same thing and implements the cycle check in the same way (i.e., it only starts to check cycles after the stack reaches a depth of 1000). I deliberately don't think we should document the depth at which it starts to check. It can be future proposal whether to expose a sentinel error for cycles errors. Either way, I don't think this blocks v2 since it's literally the behavior that exists in v1.

@aclements: I wasn't suggesting we make stronger guarantees about cycle detection, I was suggesting we make weaker guarantees. Like, we could allow just blowing the stack, for example. Guaranteeing that it will return an error would disallow blowing the stack, and forces us to have some form of cycle detection no matter what.

Raising the question now: Do we want to drop support for v2 to detect cyclic data structures?

At present, v2 implements the exact same semantics as v1. We could drop support for this in v2 and guard it behind the ReportErrorsWithLegacySemantics option.

Fundamentally detection requires tracking what objects we've seen and then reporting an error if we see it again. This uses a non-trivial amount of compute and memory. However, the way v1 implemented this in a relatively clever way, where it only does the detection if the marshal depth has hit 1000. Thus, in practice it costs almost nothing to have the detection in the common case. Detecting cycles can be legitimately helpful when accidentally marshaling a cyclic data structure. That said, most cases of this are hopefully caught in a unit test rather than blowing up production.

Personally, I don't feel strongly about whether to keep or remove it. What are other people's thoughts?
\cc @neild @prattmic @aclements @mvdan @johanbrandhorst

[dsnet]

I'm scrubbing through issues on https://github.com/go-json-experiment/json/issues and came across https://github.com/go-json-experiment/json/issues/12, which is related.

Right now we have cyclic detection for marshal, but not unmarshal, so there's a bit of assymetry. I do feel like we should either symmetrically support this for both marshal and unmarshal or neither of them.

[aclements]

"Cycle detection is best-effort"?

[dsnet]

I'm not sure if "best-effort" is a workable position. Hyrum's law dictates that someone will be depending on it sooner or later. I think we either need to commit to having it in some form, or not at all (for now).

If we remove cycle detection for v2, we can always choose to add it back in later (if we so choose). However, we can never remove it.

[dmitshur]

@dsnet Please be aware that without a milestone set, the release-blocker label doesn't block any particular Go release.

[dsnet]

Thanks. I put it in the Go 1.27 milestone since this was raised as part of #79733. We primarily just need a decision on what to do. In the worst case, the relevant code change is fairly minor.

[prattmic]

We have a tendency of adding recursion limits to packages (#51112, #71201, etc) as security or "not-quite" security issues.

While cycle detection and recursion limits aren't quite the same thing, I suspect that if we remove detection now we'll end up adding cycle detection or recursion limits back in the future, so I'm inclined to leave it in.

It seems like the primary concern was cost, but we have already have a cheap implementation since we don't start until depth 1000, so I don't think that is a concern.

Is it possible to encounter a cycle in unmarshal? The JSON input must have a finite length, so it seems like it should terminate? Perhaps this is only possible via a recursive custom unmarshaler that doesn't actually consume input?