fix #302: use a different approach for cross-chunk assignments

.github/workflows/ci.yml

@@ -56,3 +56,7 @@ jobs:
     - name: Esprima Tests
       if: matrix.os == 'ubuntu-latest'
       run: make test-esprima
+
+    - name: Preact Splitting Tests
+      if: matrix.os == 'ubuntu-latest'
+      run: make test-preact-splitting

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## Unreleased
+
+* Fix bugs with cross-chunk assignment handling ([#302](https://github.com/evanw/esbuild/issues/302))
+
+    The code splitting process may end up moving the declaration of a file-local variable into a separate chunk from an assignment to that variable. However, it's not possible to assign to a variable in another chunk because assigning to an import is not allowed in ES6. To avoid generating invalid code, esbuild runs an additional pass after code splitting to force all code involved in cross-chunk assignments into the same chunk.
+
+    The logic to do this is quite tricky. For example, moving code between chunks may introduce more cross-chunk assignments that also need to be handled. In this case the bug was caused by not handling complex cases with three or more levels of cross-chunk assignment dependency recursion. These cases now have test coverage and should be handled correctly.
+
 ## 0.6.11
 
 * Code splitting chunks now use content hashes ([#16](https://github.com/evanw/esbuild/issues/16))

Makefile

@@ -12,7 +12,7 @@ test-all:
 	make -j6 test-go vet-go verify-source-map end-to-end-tests js-api-tests test-wasm
 
 # This includes tests of some 3rd-party libraries, which can be very slow
-test-extra: test-all test-sucrase test-esprima test-rollup
+test-extra: test-all test-preact-splitting test-sucrase test-esprima test-rollup
 
 test-go:
 	go test ./internal/...
@@ -276,6 +276,38 @@ test-rollup: esbuild | demo/rollup
 ################################################################################
 # This builds Sucrase using esbuild and then uses it to run Sucrase's test suite
 
+PREACT_SPLITTING += import { h } from 'preact';
+PREACT_SPLITTING += import { USE as use } from 'preact/hooks';
+PREACT_SPLITTING += import { renderToString } from 'preact-render-to-string';
+PREACT_SPLITTING += let Component = () => (use(() => {}), h('div'));
+PREACT_SPLITTING += if (renderToString(h(Component)) !== '<div></div>') throw 'fail';
+
+PREACT_HOOKS += useCallback
+PREACT_HOOKS += useContext
+PREACT_HOOKS += useDebugValue
+PREACT_HOOKS += useEffect
+PREACT_HOOKS += useErrorBoundary
+PREACT_HOOKS += useImperativeHandle
+PREACT_HOOKS += useLayoutEffect
+PREACT_HOOKS += useMemo
+PREACT_HOOKS += useReducer
+PREACT_HOOKS += useRef
+PREACT_HOOKS += useState
+
+demo/preact-splitting:
+	mkdir -p demo/preact-splitting/src
+	cd demo/preact-splitting && echo '{}' > package.json && npm i [email protected] [email protected]
+	cd demo/preact-splitting && for h in $(PREACT_HOOKS); do echo "$(PREACT_SPLITTING)" | sed s/USE/$$h/ > src/$$h.js; done
+
+test-preact-splitting: esbuild | demo/preact-splitting
+	cd demo/preact-splitting && rm -fr out && ../../esbuild --bundle --splitting --format=esm src/*.js --outdir=out --out-extension:.js=.mjs
+	cd demo/preact-splitting && for h in $(PREACT_HOOKS); do set -e && node --experimental-modules out/$$h.mjs; done
+	cd demo/preact-splitting && rm -fr out && ../../esbuild --bundle --splitting --format=esm src/*.js --outdir=out --out-extension:.js=.mjs --minify
+	cd demo/preact-splitting && for h in $(PREACT_HOOKS); do set -e && node --experimental-modules out/$$h.mjs; done
+
+################################################################################
+# This builds Sucrase using esbuild and then uses it to run Sucrase's test suite
+
 github/sucrase:
 	mkdir -p github/sucrase
 	cd github/sucrase && git init && git remote add origin https://github.com/alangpierce/sucrase.git

docs/architecture.md

@@ -422,7 +422,7 @@ export {
 
 The assignment `data = value` will crash at run time with `TypeError: Assignment to constant variable`. To fix this, we must make sure that assignment ends up in the same chunk as the declaration `let data`.
 
-This is done by unioning the entry point sets of the parts with the assignments and the parts with the symbol declarations together. That way all of those parts are marked as reachable from all entry points that can reach any of those parts. This is only relevant for locally-declared symbols so each module can be processed independently.
+This is done by linking the parts with the assignments and the parts with the symbol declarations together such that their entry point sets are the same. That way all of those parts are marked as reachable from all entry points that can reach any of those parts. Note that linking a group of parts together also involves marking all dependencies of all parts in that group as reachable from those entry points too, including propagathing through any part groups those dependencies are already linked with, recursively.
 
 The grouping of parts can be non-trivial because there may be many parts involved and many assignments to different variables. Grouping is done by finding connected components on the graph where nodes are parts and edges are cross-part assignments.
 

internal/bundler/bundler_splitting_test.go

@@ -709,6 +709,127 @@ export {
 	})
 }
 
+func TestSplittingCrossChunkAssignmentDependenciesRecursive(t *testing.T) {
+	expectBundled(t, bundled{
+		files: map[string]string{
+			"/a.js": `
+				import { setX } from './x'
+				setX()
+			`,
+			"/b.js": `
+				import { setZ } from './z'
+				setZ()
+			`,
+			"/c.js": `
+				import { setX2 } from './x'
+				import { setY2 } from './y'
+				import { setZ2 } from './z'
+				setX2();
+				setY2();
+				setZ2();
+			`,
+			"/x.js": `
+				let _x
+				export function setX(v) { _x = v }
+				export function setX2(v) { _x = v }
+			`,
+			"/y.js": `
+				import { setX } from './x'
+				let _y
+				export function setY(v) { _y = v }
+				export function setY2(v) { setX(v); _y = v }
+			`,
+			"/z.js": `
+				import { setY } from './y'
+				let _z
+				export function setZ(v) { _z = v }
+				export function setZ2(v) { setY(v); _z = v }
+			`,
+		},
+		entryPaths: []string{"/a.js", "/b.js", "/c.js"},
+		options: config.Options{
+			IsBundling:    true,
+			CodeSplitting: true,
+			OutputFormat:  config.FormatESModule,
+			AbsOutputDir:  "/out",
+		},
+		expected: map[string]string{
+			"/out/a.js": `import {
+  setX
+} from "./chunk.lf8PB6N3.js";
+
+// /a.js
+setX();
+`,
+			"/out/b.js": `import {
+  setZ
+} from "./chunk.2loht7HC.js";
+import "./chunk.lf8PB6N3.js";
+
+// /b.js
+setZ();
+`,
+			"/out/c.js": `import {
+  setY2,
+  setZ2
+} from "./chunk.2loht7HC.js";
+import {
+  setX2
+} from "./chunk.lf8PB6N3.js";
+
+// /c.js
+setX2();
+setY2();
+setZ2();
+`,
+			"/out/chunk.2loht7HC.js": `import {
+  setX
+} from "./chunk.lf8PB6N3.js";
+
+// /y.js
+let _y;
+function setY(v) {
+  _y = v;
+}
+function setY2(v) {
+  setX(v);
+  _y = v;
+}
+
+// /z.js
+let _z;
+function setZ(v) {
+  _z = v;
+}
+function setZ2(v) {
+  setY(v);
+  _z = v;
+}
+
+export {
+  setZ,
+  setY2,
+  setZ2
+};
+`,
+			"/out/chunk.lf8PB6N3.js": `// /x.js
+let _x;
+function setX(v) {
+  _x = v;
+}
+function setX2(v) {
+  _x = v;
+}
+
+export {
+  setX,
+  setX2
+};
+`,
+		},
+	})
+}
+
 func TestSplittingDuplicateChunkCollision(t *testing.T) {
 	expectBundled(t, bundled{
 		files: map[string]string{

internal/bundler/linker.go

@@ -224,6 +224,15 @@ type partMeta struct {
 	// assign this part to a chunk.
 	entryBits bitSet
 
+	// If present, this is a circular doubly-linked list of all other parts in
+	// this file that need to be in the same chunk as this part to avoid cross-
+	// chunk assignments, which are not allowed in ES6 modules.
+	//
+	// This used to be an array but that was generating lots of allocations.
+	// Changing this to a circular doubly-linked list was a substantial speedup.
+	prevSibling uint32
+	nextSibling uint32
+
 	// These are dependencies that come from other files via import statements.
 	nonLocalDependencies []partRef
 }
@@ -467,6 +476,8 @@ func (c *linkerContext) addPartToFile(sourceIndex uint32, part ast.Part, partMet
 		partMeta.entryBits = newBitSet(uint(len(c.entryPoints)))
 	}
 	partIndex := uint32(len(file.ast.Parts))
+	partMeta.prevSibling = partIndex
+	partMeta.nextSibling = partIndex
 	file.ast.Parts = append(file.ast.Parts, part)
 	fileMeta.partMeta = append(fileMeta.partMeta, partMeta)
 	return partIndex
@@ -1838,7 +1849,10 @@ func (c *linkerContext) markPartsReachableFromEntryPoints() {
 		fileMeta := &c.fileMeta[sourceIndex]
 		fileMeta.entryBits = newBitSet(bitCount)
 		for partIndex := range fileMeta.partMeta {
-			fileMeta.partMeta[partIndex].entryBits = newBitSet(bitCount)
+			partMeta := &fileMeta.partMeta[partIndex]
+			partMeta.entryBits = newBitSet(bitCount)
+			partMeta.prevSibling = uint32(partIndex)
+			partMeta.nextSibling = uint32(partIndex)
 		}
 
 		// If this is a CommonJS file, we're going to need to generate a wrapper
@@ -1910,32 +1924,10 @@ func (c *linkerContext) handleCrossChunkAssignments() {
 	for _, sourceIndex := range c.reachableFiles {
 		file := &c.files[sourceIndex]
 		fileMeta := &c.fileMeta[sourceIndex]
-		partMeta := fileMeta.partMeta
-
-		// Initialize a union-find data structure to merge parts together
-		type unionFindItem struct {
-			entryBits bitSet
-			label     uint32
-		}
-		unionFind := make([]unionFindItem, len(file.ast.Parts))
-		for partIndex := range file.ast.Parts {
-			unionFind[partIndex] = unionFindItem{label: uint32(partIndex)}
-		}
-
-		// Look up a merged label
-		var find func(uint32) uint32
-		find = func(label uint32) uint32 {
-			next := unionFind[label].label
-			if next != label {
-				next = find(next)
-				unionFind[label].label = next
-			}
-			return next
-		}
 
 		for partIndex, part := range file.ast.Parts {
 			// Ignore this part if it's dead code
-			if partMeta[partIndex].entryBits.equals(neverReachedEntryBits) {
+			if fileMeta.partMeta[partIndex].entryBits.equals(neverReachedEntryBits) {
 				continue
 			}
 
@@ -1945,53 +1937,30 @@ func (c *linkerContext) handleCrossChunkAssignments() {
 				if use.IsAssigned {
 					if otherParts, ok := file.ast.TopLevelSymbolToParts[ref]; ok {
 						for _, otherPartIndex := range otherParts {
-							// Union the two labels together
-							fromIndex := find(uint32(partIndex))
-							toIndex := find(otherPartIndex)
-							from := &unionFind[fromIndex]
-							to := &unionFind[toIndex]
-							from.label = toIndex
-
-							// Lazily allocate bit sets only for parts that are relevant
-							if to.entryBits.entries == nil {
-								to.entryBits = newBitSet(uint(len(c.entryPoints)))
-								to.entryBits.copyFrom(partMeta[toIndex].entryBits)
+							partMetaA := &fileMeta.partMeta[partIndex]
+							partMetaB := &fileMeta.partMeta[otherPartIndex]
+
+							// Make sure both sibling subsets have the same entry points
+							for entryPointBit := range c.entryPoints {
+								hasA := partMetaA.entryBits.hasBit(uint(entryPointBit))
+								hasB := partMetaB.entryBits.hasBit(uint(entryPointBit))
+								if hasA && !hasB {
+									c.includePart(sourceIndex, otherPartIndex, uint(entryPointBit), fileMeta.distanceFromEntryPoint)
+								} else if hasB && !hasA {
+									c.includePart(sourceIndex, uint32(partIndex), uint(entryPointBit), fileMeta.distanceFromEntryPoint)
+								}
 							}
 
-							// Union the two bit sets together as well
-							if from.entryBits.entries == nil {
-								to.entryBits.bitwiseOrWith(partMeta[fromIndex].entryBits)
-							} else {
-								to.entryBits.bitwiseOrWith(from.entryBits)
-							}
+							// Perform the merge
+							fileMeta.partMeta[partMetaA.nextSibling].prevSibling = partMetaB.prevSibling
+							fileMeta.partMeta[partMetaB.prevSibling].nextSibling = partMetaA.nextSibling
+							partMetaA.nextSibling = otherPartIndex
+							partMetaB.prevSibling = uint32(partIndex)
 						}
 					}
 				}
 			}
 		}
-
-		// Each part involved in a cross-part assignment must be treated as if it's
-		// reachable from any entry point that can reach any part involved in that
-		// cross-part assignment.
-		for partIndex := range file.ast.Parts {
-			// Check if this part was merged
-			label := find(uint32(partIndex))
-			merged := unionFind[label].entryBits
-			if merged.entries == nil {
-				continue
-			}
-
-			// We can't just set the entry bits of this part to the merged entry bits
-			// because that doesn't handle this part's dependencies. Instead we must
-			// call "includePart()" to mark all dependencies of this part as reachable
-			// by these entry points too.
-			to := &partMeta[partIndex].entryBits
-			for entryPointBit := range c.entryPoints {
-				if merged.hasBit(uint(entryPointBit)) && !to.hasBit(uint(entryPointBit)) {
-					c.includePart(sourceIndex, uint32(partIndex), uint(entryPointBit), fileMeta.distanceFromEntryPoint)
-				}
-			}
-		}
 	}
 }
 
@@ -2124,7 +2093,8 @@ func (c *linkerContext) isExternalDynamicImport(record *ast.ImportRecord) bool {
 }
 
 func (c *linkerContext) includePart(sourceIndex uint32, partIndex uint32, entryPointBit uint, distanceFromEntryPoint uint32) {
-	partMeta := &c.fileMeta[sourceIndex].partMeta[partIndex]
+	fileMeta := &c.fileMeta[sourceIndex]
+	partMeta := &fileMeta.partMeta[partIndex]
 
 	// Don't mark this part more than once
 	if partMeta.entryBits.hasBit(entryPointBit) {
@@ -2134,7 +2104,6 @@ func (c *linkerContext) includePart(sourceIndex uint32, partIndex uint32, entryP
 
 	file := &c.files[sourceIndex]
 	part := &file.ast.Parts[partIndex]
-	fileMeta := &c.fileMeta[sourceIndex]
 
 	// Include the file containing this part
 	c.includeFile(sourceIndex, entryPointBit, distanceFromEntryPoint)
@@ -2149,6 +2118,11 @@ func (c *linkerContext) includePart(sourceIndex uint32, partIndex uint32, entryP
 		c.includePart(nonLocalDependency.sourceIndex, nonLocalDependency.partIndex, entryPointBit, distanceFromEntryPoint)
 	}
 
+	// Also include any cross-chunk assignment siblings
+	for i := partMeta.nextSibling; i != partIndex; i = fileMeta.partMeta[i].nextSibling {
+		c.includePart(sourceIndex, i, entryPointBit, distanceFromEntryPoint)
+	}
+
 	// Also include any require() imports
 	toModuleUses := uint32(0)
 	for _, importRecordIndex := range part.ImportRecordIndices {