Fix safeStringify treating shared references as circular

[Leo Camus (Dev-next-gen)]

safeStringify in src/vs/base/common/objects.ts puts every object it visits into a Set and never takes anything out, so it can't tell a cycle apart from an object that is simply referenced twice. Every occurrence of a shared value after the first comes out as "[Circular]":

const shared = { a: 1 };
safeStringify([shared, { x: shared, y: [shared] }]);
// before: [{"a":1},{"x":"[Circular]","y":["[Circular]"]}]
// after:  [{"a":1},{"x":{"a":1},"y":[{"a":1}]}]

#327398 fixed the same bug in stableStringify in July. Here I track only the current ancestor path. The replacer is now a function, so it receives the holder object as this, and it pops finished subtrees off the stack until the holder is back on top. Real cycles are still replaced with "[Circular]".

The existing safeStringify test asserted the old behaviour. In that fixture obj2 is obj1.friend and also the second element of c. That second occurrence is not inside itself, so it is now serialized as { friend: { friend: '[Circular]' } }, and the cycle back through obj1 is still caught one level down. I updated that expectation and added a test for shared references in both object and array positions.

How I tested it: I ran objects.test.ts against the original objects.ts, where the new test fails with the output shown above, and then with the fix, where all 10 tests in the file pass (I ran mocha directly on the transpiled file because the machine I used has Node 22). I also compared the patched function with JSON.stringify on 20,000 random acyclic graphs containing shared nodes. The output matched every time, while the original differed on 7,751 of them. On 20,000 random cyclic graphs it matched a recursive ancestor-set reference every time. saveCachedSessions goes through this function and #314404 is about its cost, so I also timed it on 2,000 session-like records without sharing. The output is identical and the new version is slightly faster (about 6.0 ms per call against 6.7 ms), since it no longer builds a set of every visited object.

One trade-off: an object reachable along several paths is now written out once per path, as JSON.stringify would do, so the output can be larger than before for heavily shared graphs. That is what the fix is for, and #327398 accepted the same trade-off.

I didn't find an existing issue for this. I came across it while reading the code.

AI tools used

[Copilot]

🟢 Approval recommended

The implementation correctly handles shared and circular references; only minor comment-style cleanup remains.

Pull request overview

Updates safeStringify to distinguish shared references from genuine circular references.

Changes:

• Tracks only the active ancestor path during serialization.
• Updates circular-reference expectations and adds shared-reference coverage.

File summaries

File	Description
src/vs/base/common/objects.ts	Implements ancestor-path cycle detection.
src/vs/base/test/common/objects.test.ts	Updates and expands serialization tests.

Review details

Suppressed comments (1)

src/vs/base/common/objects.ts:176

• This comment only narrates the while loop immediately below it, which the repository's comment guidance explicitly asks us to avoid. The holder comparison is already clear from the code, so please remove the comment.

			// `this` is the object holding `key`, pop the subtrees that are already done

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.