diff --git a/__tests__/base.js b/__tests__/base.js index 51a7a29a..c76e359c 100644 --- a/__tests__/base.js +++ b/__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 }) diff --git a/__tests__/map-set.js b/__tests__/map-set.js index a8672a60..1a485e11 100644 --- a/__tests__/map-set.js +++ b/__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) + }) }) } diff --git a/src/plugins/arrayMethods.ts b/src/plugins/arrayMethods.ts index 4c11fc33..edd4f1b1 100644 --- a/src/plugins/arrayMethods.ts +++ b/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 diff --git a/src/utils/common.ts b/src/utils/common.ts index 3539d98e..513c153e 100644 --- a/src/utils/common.ts +++ b/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 = (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 =>