fix: preserve structural sharing for no-op array-methods calls

__tests__/base.js

@@ -60,7 +60,7 @@ function runBaseTest(
 	useListener,
 	useArrayMethods = false
 ) {
-	const listener = useListener ? function () {} : undefined
+	const listener = useListener ? function() {} : undefined
 
 	const {produce, produceWithPatches} = createPatchedImmer({
 		autoFreeze,
@@ -73,7 +73,7 @@ function runBaseTest(
 		const immer = new Immer(options)
 
 		const {produce} = immer
-		immer.produce = function (...args) {
+		immer.produce = function(...args) {
 			return typeof args[1] === "function" && args.length < 3
 				? produce(...args, listener)
 				: produce(...args)
@@ -292,6 +292,81 @@ function runBaseTest(
 			})
 
 			describe("mutating array methods", () => {
+				describe("no-op calls preserve structural sharing", () => {
+					it("push() with no arguments returns the base", () => {
+						const base = {list: [1, 2, 3], keep: {x: 1}}
+						const [next, patches] = produceWithPatches(base, d => {
+							d.list.push(...[])
+						})
+						expect(next).toBe(base)
+						expect(next.keep).toBe(base.keep)
+						expect(patches).toHaveLength(0)
+					})
+
+					it("unshift() with no arguments returns the base", () => {
+						const base = {list: [1, 2, 3]}
+						const next = produce(base, d => {
+							d.list.unshift()
+						})
+						expect(next).toBe(base)
+					})
+
+					it("splice() that removes and inserts nothing returns the base", () => {
+						const base = {list: [1, 2, 3]}
+						const [next, patches] = produceWithPatches(base, d => {
+							d.list.splice(1, 0)
+						})
+						expect(next).toBe(base)
+						expect(patches).toHaveLength(0)
+					})
+
+					it("pop()/shift() on an empty array return the base", () => {
+						const base = {popped: [], shifted: []}
+						const next = produce(base, d => {
+							d.popped.pop()
+							d.shifted.shift()
+						})
+						expect(next).toBe(base)
+					})
+
+					it("no-op push() on an array inside a Set preserves the array's identity", () => {
+						const arr = [1, 2, 3]
+						const base = new Set([arr, {x: 1}])
+						const next = produce(base, d => {
+							for (const v of d) {
+								if (Array.isArray(v)) v.push(...[])
+								else v.x = 2
+							}
+						})
+						expect(next).not.toBe(base)
+						expect([...next].find(Array.isArray)).toBe(arr)
+					})
+
+					it("sort()/reverse() on arrays with fewer than two elements return the base", () => {
+						const base = {empty: [], single: [1]}
+						const next = produce(base, d => {
+							d.empty.sort()
+							d.empty.reverse()
+							d.single.sort((a, b) => a - b)
+							d.single.reverse()
+						})
+						expect(next).toBe(base)
+					})
+
+					it("no-op splice() on an array inside a Set preserves the array's identity", () => {
+						const arr = [1, 2, 3]
+						const base = new Set([arr, {x: 1}])
+						const next = produce(base, d => {
+							for (const v of d) {
+								if (Array.isArray(v)) v.splice(1, 0)
+								else v.x = 2
+							}
+						})
+						expect(next).not.toBe(base)
+						expect([...next].find(Array.isArray)).toBe(arr)
+					})
+				})
+
 				// Reported here: https://github.com/mweststrate/immer/issues/116
 				it("can pop then push", () => {
 					const nextState = produce([1, 2, 3], s => {
@@ -3229,13 +3304,13 @@ function runBaseTest(
 
 		it("'this' should not be bound anymore - 1", () => {
 			const base = {x: 3}
-			const next1 = produce(base, function () {
+			const next1 = produce(base, function() {
 				expect(this).toBe(undefined)
 			})
 		})
 
 		it("'this' should not be bound anymore - 2", () => {
-			const incrementor = produce(function () {
+			const incrementor = produce(function() {
 				expect(this).toBe(undefined)
 			})
 			incrementor()
@@ -3244,7 +3319,7 @@ function runBaseTest(
 		it("should be possible to use dynamic bound this", () => {
 			const world = {
 				counter: {count: 1},
-				inc: produce(function (draft) {
+				inc: produce(function(draft) {
 					expect(this).toBe(world)
 					draft.counter.count = this.counter.count + 1
 				})

__tests__/map-set.js

@@ -23,7 +23,7 @@ runBaseTest("proxy (autofreeze)", true, true)
 runBaseTest("proxy (autofreeze)(patch listener)", true, true, true)
 
 function runBaseTest(name, autoFreeze, useListener) {
-	const listener = useListener ? function () {} : undefined
+	const listener = useListener ? function() {} : undefined
 	const {produce, produceWithPatches} = createPatchedImmer({
 		autoFreeze
 	})
@@ -34,7 +34,7 @@ function runBaseTest(name, autoFreeze, useListener) {
 		const immer = new Immer(options)
 
 		const {produce} = immer
-		immer.produce = function (...args) {
+		immer.produce = function(...args) {
 			return typeof args[1] === "function" && args.length < 3
 				? produce(...args, listener)
 				: produce(...args)
@@ -395,6 +395,27 @@ function runBaseTest(name, autoFreeze, useListener) {
 
 			expect(Array.from(newSet)).toEqual([objs[0], objs[1]])
 		})
+
+		test("reading (but not modifying) a nested object inside a Set does not leak the copy", () => {
+			const untouched = {nested: {a: 1}}
+			const base = new Set([untouched, {x: 1}])
+			const next = produce(base, draft => {
+				for (const v of draft) {
+					if (v.nested) {
+						// Read only - creates a child draft (and a stray copy_
+						// on the parent) but modifies nothing
+						expect(v.nested.a).toBe(1)
+					} else {
+						v.x = 2
+					}
+				}
+			})
+			expect(next).not.toBe(base)
+			const finalUntouched = Array.from(next).find(v => v.nested)
+			expect(finalUntouched).toBe(untouched)
+			expect(finalUntouched.nested.a).toBe(1)
+			expect(isDraft(finalUntouched.nested)).toBe(false)
+		})
 	})
 }
 

src/plugins/arrayMethods.ts

@@ -174,9 +174,21 @@ export function enableArrayMethods() {
 	): T {
 		prepareCopy(state)
 		const result = operation()
+		markArrayChanged(state, markLength)
+		return result
+	}
+
+	/**
+	 * Marks the array (and its ancestors) as modified after a mutating operation.
+	 *
+	 * Kept separate from `executeArrayMethod` so that operations which turn out to
+	 * be no-ops (e.g. `push()` with no arguments, `splice(i, 0)`, `pop()`/`shift()`
+	 * on an empty array) can prepare a copy but skip marking, preserving Immer's
+	 * structural-sharing guarantee that a change-free producer returns the base.
+	 */
+	function markArrayChanged(state: ProxyArrayState, markLength = true) {
 		markChanged(state)
 		if (markLength) state.assigned_!.set("length", true)
-		return result
 	}
 
 	function markAllIndicesReassigned(state: ProxyArrayState) {
@@ -234,31 +246,45 @@ export function enableArrayMethods() {
 		method: string,
 		args: any[]
 	) {
-		return executeArrayMethod(state, () => {
-			// For push/unshift, capture the length before the operation
-			// so we can compute insertion indices for handleCrossReference
-			const lengthBefore = state.copy_!.length
+		// Detect no-op calls before creating a copy so structural sharing is
+		// preserved: push/unshift with no arguments add nothing, and pop/shift
+		// on an empty array remove nothing. The check must happen before
+		// prepareCopy - a stray copy_ on an unmodified state would be picked
+		// up by finalization of a parent Set and break identity there.
+		const isInsert = method === "push" || method === "unshift"
+		if (isInsert ? args.length === 0 : latest(state).length === 0) {
+			// Match the native return values: push/unshift return the
+			// (unchanged) length, pop/shift on an empty array return undefined
+			return isInsert ? latest(state).length : undefined
+		}
 
-			const result = (state.copy_! as any)[method](...args)
+		prepareCopy(state)
 
-			// Handle index reassignment for shifting methods
-			if (SHIFTING_METHODS.has(method as MutatingArrayMethod)) {
-				markAllIndicesReassigned(state)
-			}
+		// For push/unshift, capture the length before the operation
+		// so we can compute insertion indices for handleCrossReference
+		const lengthBefore = state.copy_!.length
 
-			// Handle cross-references for newly inserted values.
-			// push appends at the end, unshift inserts at the beginning.
-			if (method === "push" && args.length > 0) {
-				handleInsertedValues(state, lengthBefore, args)
-			} else if (method === "unshift" && args.length > 0) {
-				handleInsertedValues(state, 0, args)
-			}
+		const result = (state.copy_! as any)[method](...args)
 
-			// Return appropriate value based on method
-			return RESULT_RETURNING_METHODS.has(method as MutatingArrayMethod)
-				? result
-				: state.draft_
-		})
+		markArrayChanged(state)
+
+		// Handle index reassignment for shifting methods
+		if (SHIFTING_METHODS.has(method as MutatingArrayMethod)) {
+			markAllIndicesReassigned(state)
+		}
+
+		// Handle cross-references for newly inserted values.
+		// push appends at the end, unshift inserts at the beginning.
+		if (method === "push") {
+			handleInsertedValues(state, lengthBefore, args)
+		} else if (method === "unshift") {
+			handleInsertedValues(state, 0, args)
+		}
+
+		// Return appropriate value based on method
+		return RESULT_RETURNING_METHODS.has(method as MutatingArrayMethod)
+			? result
+			: state.draft_
 	}
 
 	/**
@@ -275,6 +301,11 @@ export function enableArrayMethods() {
 		method: string,
 		args: any[]
 	) {
+		// Reordering an array with fewer than two elements cannot change it,
+		// so preserve structural sharing and skip copying (see
+		// handleSimpleOperation for why this must run before prepareCopy).
+		if (latest(state).length <= 1) return state.draft_
+
 		return executeArrayMethod(
 			state,
 			() => {
@@ -324,12 +355,43 @@ export function enableArrayMethods() {
 					}
 
 					if (method === "splice") {
-						const res = executeArrayMethod(state, () =>
-							state.copy_!.splice(...(args as [number, number, ...any[]]))
+						const insertCount = args.length > 2 ? args.length - 2 : 0
+
+						// Detect no-op calls before creating a copy so structural
+						// sharing is preserved (see handleSimpleOperation). A splice
+						// removes nothing when called with no arguments, when the
+						// normalized start is at/past the end, or when the delete
+						// count coerces to less than 1. splice(start) with a single
+						// argument removes everything from start onwards, so it must
+						// not match. Exotic arguments that slip through are caught
+						// by the post-copy check below.
+						if (insertCount === 0) {
+							const length = latest(state).length
+							if (
+								args.length === 0 ||
+								normalizeSliceIndex(args[0] ?? 0, length) === length ||
+								(args.length > 1 && !(args[1] >= 1))
+							) {
+								return []
+							}
+						}
+
+						prepareCopy(state)
+						const res = state.copy_!.splice(
+							...(args as [number, number, ...any[]])
 						)
+
+						// Backstop for exotic arguments the pre-check above missed:
+						// if nothing was removed and nothing inserted, the array is
+						// unchanged and must not be marked as modified.
+						if (res.length === 0 && insertCount === 0) {
+							return res
+						}
+
+						markArrayChanged(state)
 						markAllIndicesReassigned(state)
 						// Handle cross-references for inserted values (args from index 2+)
-						if (args.length > 2) {
+						if (insertCount > 0) {
 							const startIndex = normalizeSliceIndex(
 								args[0] ?? 0,
 								state.copy_!.length

src/utils/common.ts

@@ -105,12 +105,12 @@ export function getArchtype(thing: any): ArchType {
 	return state
 		? state.type_
 		: isArray(thing)
-			? ArchType.Array
-			: isMap(thing)
-				? ArchType.Map
-				: isSet(thing)
-					? ArchType.Set
-					: ArchType.Object
+		? ArchType.Array
+		: isMap(thing)
+		? ArchType.Map
+		: isSet(thing)
+		? ArchType.Set
+		: ArchType.Object
 }
 
 /*#__PURE__*/
@@ -186,7 +186,11 @@ export let latest = (state: ImmerState): any => state.copy_ || state.base_
 
 export let getValue = <T extends object>(value: T): T => {
 	const proxyDraft = getProxyDraft(value)
-	return proxyDraft ? (proxyDraft.copy_ ?? proxyDraft.base_) : value
+	// Use the copy only if the draft was actually modified. An unmodified
+	// draft can still have a copy_ (e.g. the get trap caches child drafts
+	// there), and using it would both break structural sharing and leak
+	// soon-to-be-revoked child drafts into the result.
+	return proxyDraft ? getFinalValue(proxyDraft) : value
 }
 
 export let getFinalValue = (state: ImmerState): any =>