From 184bc4e2b1ba6858f482dd23a454254777206ce7 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Thu, 19 Sep 2024 06:21:31 +0200 Subject: [PATCH 1/3] Detect variable reassignments in modules without side effects (#5658) --- src/Graph.ts | 1 + src/Module.ts | 9 +++++---- src/ast/nodes/Identifier.ts | 17 ++++++++++------- .../_config.js | 8 ++++++++ .../dep.js | 4 ++++ .../dep2.js | 4 ++++ .../main.js | 3 +++ 7 files changed, 35 insertions(+), 11 deletions(-) create mode 100644 test/function/samples/tree-shake-module-side-effects-update/_config.js create mode 100644 test/function/samples/tree-shake-module-side-effects-update/dep.js create mode 100644 test/function/samples/tree-shake-module-side-effects-update/dep2.js create mode 100644 test/function/samples/tree-shake-module-side-effects-update/main.js diff --git a/src/Graph.ts b/src/Graph.ts index eb7243b9b..9e9f22559 100644 --- a/src/Graph.ts +++ b/src/Graph.ts @@ -172,6 +172,7 @@ export default class Graph { this.needsTreeshakingPass = false; for (const module of this.modules) { if (module.isExecuted) { + module.hasTreeShakingPassStarted = true; if (module.info.moduleSideEffects === 'no-treeshake') { module.includeAllInBundle(); } else { diff --git a/src/Module.ts b/src/Module.ts index dd2a3543e..9108eeb81 100644 --- a/src/Module.ts +++ b/src/Module.ts @@ -2,10 +2,8 @@ import { extractAssignedNames } from '@rollup/pluginutils'; import { locate } from 'locate-character'; import MagicString from 'magic-string'; import { parseAsync } from '../native'; -import ExternalModule from './ExternalModule'; -import type Graph from './Graph'; -import { createInclusionContext } from './ast/ExecutionContext'; import { convertProgram } from './ast/bufferParsers'; +import { createInclusionContext } from './ast/ExecutionContext'; import { nodeConstructors } from './ast/nodes'; import ExportAllDeclaration from './ast/nodes/ExportAllDeclaration'; import ExportDefaultDeclaration from './ast/nodes/ExportDefaultDeclaration'; @@ -19,8 +17,8 @@ import Literal from './ast/nodes/Literal'; import type MetaProperty from './ast/nodes/MetaProperty'; import * as NodeType from './ast/nodes/NodeType'; import type Program from './ast/nodes/Program'; -import VariableDeclaration from './ast/nodes/VariableDeclaration'; import type { NodeBase } from './ast/nodes/shared/Node'; +import VariableDeclaration from './ast/nodes/VariableDeclaration'; import ModuleScope from './ast/scopes/ModuleScope'; import { type PathTracker, UNKNOWN_PATH } from './ast/utils/PathTracker'; import ExportDefaultVariable from './ast/variables/ExportDefaultVariable'; @@ -29,6 +27,8 @@ import ExternalVariable from './ast/variables/ExternalVariable'; import NamespaceVariable from './ast/variables/NamespaceVariable'; import SyntheticNamedExportVariable from './ast/variables/SyntheticNamedExportVariable'; import type Variable from './ast/variables/Variable'; +import ExternalModule from './ExternalModule'; +import type Graph from './Graph'; import type { AstNode, CustomPluginOptions, @@ -220,6 +220,7 @@ export default class Module { readonly dynamicImports: DynamicImport[] = []; excludeFromSourcemap: boolean; execIndex = Infinity; + hasTreeShakingPassStarted = false; readonly implicitlyLoadedAfter = new Set(); readonly implicitlyLoadedBefore = new Set(); readonly importDescriptions = new Map(); diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts index 8bc4ffcd8..ee50123d3 100644 --- a/src/ast/nodes/Identifier.ts +++ b/src/ast/nodes/Identifier.ts @@ -20,7 +20,6 @@ import GlobalVariable from '../variables/GlobalVariable'; import LocalVariable from '../variables/LocalVariable'; import type Variable from '../variables/Variable'; import * as NodeType from './NodeType'; -import type SpreadElement from './SpreadElement'; import { Flag, isFlagSet, setFlag } from './shared/BitFlags'; import { type ExpressionEntity, @@ -30,6 +29,7 @@ import { import { NodeBase } from './shared/Node'; import type { PatternNode } from './shared/Pattern'; import type { VariableKind } from './shared/VariableKinds'; +import type SpreadElement from './SpreadElement'; export type IdentifierWithVariable = Identifier & { variable: Variable }; @@ -220,9 +220,10 @@ export default class Identifier extends NodeBase implements PatternNode { this.variable instanceof LocalVariable && this.variable.kind && tdzVariableKinds.has(this.variable.kind) && - // we ignore possible TDZs due to circular module dependencies as - // otherwise we get many false positives - this.variable.module === this.scope.context.module + // We ignore modules that did not receive a treeshaking pass yet as that + // causes many false positives due to circular dependencies or disabled + // moduleSideEffects. + this.variable.module.hasTreeShakingPassStarted ) ) { return (this.isTDZAccess = false); @@ -241,9 +242,7 @@ export default class Identifier extends NodeBase implements PatternNode { return (this.isTDZAccess = true); } - // We ignore the case where the module is not yet executed because - // moduleSideEffects are false. - if (!this.variable.initReached && this.scope.context.module.isExecuted) { + if (!this.variable.initReached) { // Either a const/let TDZ violation or // var use before declaration was encountered. return (this.isTDZAccess = true); @@ -294,6 +293,10 @@ export default class Identifier extends NodeBase implements PatternNode { protected applyDeoptimizations(): void { this.deoptimized = true; if (this.variable instanceof LocalVariable) { + // When accessing a variable from a module without side effects, this + // means we use an export of that module and therefore need to potentially + // include it in the bundle. + this.variable.module.isExecuted = true; this.variable.consolidateInitializers(); this.scope.context.requestTreeshakingPass(); } diff --git a/test/function/samples/tree-shake-module-side-effects-update/_config.js b/test/function/samples/tree-shake-module-side-effects-update/_config.js new file mode 100644 index 000000000..eb1aadf40 --- /dev/null +++ b/test/function/samples/tree-shake-module-side-effects-update/_config.js @@ -0,0 +1,8 @@ +module.exports = defineTest({ + description: 'detects variable updates in modules without side effects (#5408)', + options: { + treeshake: { + moduleSideEffects: false + } + } +}); diff --git a/test/function/samples/tree-shake-module-side-effects-update/dep.js b/test/function/samples/tree-shake-module-side-effects-update/dep.js new file mode 100644 index 000000000..41e419798 --- /dev/null +++ b/test/function/samples/tree-shake-module-side-effects-update/dep.js @@ -0,0 +1,4 @@ +export let direct = false; +direct = true; + +export {indirect} from './dep2.js'; diff --git a/test/function/samples/tree-shake-module-side-effects-update/dep2.js b/test/function/samples/tree-shake-module-side-effects-update/dep2.js new file mode 100644 index 000000000..cb08077b0 --- /dev/null +++ b/test/function/samples/tree-shake-module-side-effects-update/dep2.js @@ -0,0 +1,4 @@ +export let indirect = false; +(() => { + indirect = true; +})(); diff --git a/test/function/samples/tree-shake-module-side-effects-update/main.js b/test/function/samples/tree-shake-module-side-effects-update/main.js new file mode 100644 index 000000000..4d7d1f990 --- /dev/null +++ b/test/function/samples/tree-shake-module-side-effects-update/main.js @@ -0,0 +1,3 @@ +import { direct, indirect } from './dep.js'; +assert.ok(direct ? true : false, 'direct'); +assert.ok(indirect ? true : false, 'indirect'); From 447c191ba9bffc8beb49563d2bbebf43c2040a7a Mon Sep 17 00:00:00 2001 From: Matt Kubej <86790511+mattkubej@users.noreply.github.com> Date: Thu, 19 Sep 2024 00:31:30 -0400 Subject: [PATCH 2/3] fix: apply final hashes deterministically with stable placeholders set (#5644) Co-authored-by: Lukas Taegert-Atkinson --- src/utils/renderChunks.ts | 11 ++++++-- test/misc/misc.js | 58 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/src/utils/renderChunks.ts b/src/utils/renderChunks.ts index 7f33594df..dddf792f2 100644 --- a/src/utils/renderChunks.ts +++ b/src/utils/renderChunks.ts @@ -55,10 +55,11 @@ export async function renderChunks( const getHash = hasherByType[outputOptions.hashCharacters]; const chunkGraph = getChunkGraph(chunks); const { + hashDependenciesByPlaceholder, initialHashesByPlaceholder, nonHashedChunksWithPlaceholders, - renderedChunksByPlaceholder, - hashDependenciesByPlaceholder + placeholders, + renderedChunksByPlaceholder } = await transformChunksAndGenerateContentHashes( renderedChunks, chunkGraph, @@ -71,6 +72,7 @@ export async function renderChunks( renderedChunksByPlaceholder, hashDependenciesByPlaceholder, initialHashesByPlaceholder, + placeholders, bundle, getHash ); @@ -283,6 +285,7 @@ async function transformChunksAndGenerateContentHashes( hashDependenciesByPlaceholder, initialHashesByPlaceholder, nonHashedChunksWithPlaceholders, + placeholders, renderedChunksByPlaceholder }; } @@ -291,11 +294,13 @@ function generateFinalHashes( renderedChunksByPlaceholder: Map, hashDependenciesByPlaceholder: Map, initialHashesByPlaceholder: Map, + placeholders: Set, bundle: OutputBundleWithPlaceholders, getHash: GetHash ) { const hashesByPlaceholder = new Map(initialHashesByPlaceholder); - for (const [placeholder, { fileName }] of renderedChunksByPlaceholder) { + for (const placeholder of placeholders) { + const { fileName } = renderedChunksByPlaceholder.get(placeholder)!; let contentToHash = ''; const hashDependencyPlaceholders = new Set([placeholder]); for (const dependencyPlaceholder of hashDependencyPlaceholders) { diff --git a/test/misc/misc.js b/test/misc/misc.js index 1448bde4c..314111184 100644 --- a/test/misc/misc.js +++ b/test/misc/misc.js @@ -114,6 +114,64 @@ describe('misc', () => { }); }); + it('applies consistent hashes regardless of chunk transform order', async () => { + const FILES = { + main: ` + import('folder1/dupe').then(({dupe}) => console.log(dupe)); + import('folder2/dupe').then(({dupe}) => console.log(dupe)); + `, + 'folder1/dupe': `export const dupe = 'dupe content';`, + 'folder2/dupe': `export const dupe = 'dupe content';` + }; + + async function buildBundle(delayedChunk) { + const bundle = await rollup.rollup({ + input: 'main', + plugins: [ + loader(FILES), + { + name: 'delay-chunk', + async renderChunk(_, chunk) { + if (chunk.facadeModuleId === delayedChunk) { + await new Promise(resolve => setTimeout(resolve, 100)); + } + return null; + } + } + ] + }); + return bundle.generate({ + format: 'es', + chunkFileNames: '[name]-[hash].js' + }); + } + + const { output: output1 } = await buildBundle('folder1/dupe'); + const { output: output2 } = await buildBundle('folder2/dupe'); + + assert.strictEqual( + output1.length, + output2.length, + 'Both outputs should have the same number of chunks' + ); + + const sortedOutput1 = output1.sort((a, b) => a.fileName.localeCompare(b.fileName)); + const sortedOutput2 = output2.sort((a, b) => a.fileName.localeCompare(b.fileName)); + + for (let index = 0; index < sortedOutput1.length; index++) { + assert.strictEqual( + sortedOutput1[index].fileName, + sortedOutput2[index].fileName, + `Chunk ${index} should have the same filename in both outputs` + ); + assert.strictEqual( + sortedOutput1[index].code, + sortedOutput2[index].code, + `Chunk ${index} should have the same code in both outputs` + ); + } + }); + it('ignores falsy plugins', () => rollup.rollup({ input: 'x', From 5e7a3631a28a863ddb97a64189c3b76eec9983ca Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Thu, 19 Sep 2024 06:45:26 +0200 Subject: [PATCH 3/3] 4.22.0 --- CHANGELOG.md | 24 ++++++++++++++++++++++++ browser/package.json | 2 +- package-lock.json | 4 ++-- package.json | 2 +- 4 files changed, 28 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8ee3cc11..3b3217604 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,29 @@ # rollup changelog +## 4.22.0 + +_2024-09-19_ + +### Features + +- Add additional known global values to avoid access side effects (#5651) + +### Bug Fixes + +- Ensure deterministic chunk hash generation despite async renderChunk hook (#5644) +- Improve side effect detection when using "smallest" treeshaking preset when imports are optimized away (#5658) + +### Pull Requests + +- [#5644](https://github.com/rollup/rollup/pull/5644): fix: apply final hashes deterministically with stable placeholders set (@mattkubej, @lukastaegert) +- [#5646](https://github.com/rollup/rollup/pull/5646): chore(deps): update dependency @mermaid-js/mermaid-cli to v11 (@renovate[bot]) +- [#5647](https://github.com/rollup/rollup/pull/5647): chore(deps): update dependency concurrently to v9 (@renovate[bot]) +- [#5648](https://github.com/rollup/rollup/pull/5648): chore(deps): lock file maintenance minor/patch updates (@renovate[bot]) +- [#5651](https://github.com/rollup/rollup/pull/5651): feat: add `AggregateError`, `FinalizationRegistry`, `WeakRef` to knownGlobals (@re-taro) +- [#5653](https://github.com/rollup/rollup/pull/5653): Fix example selection in REPL (@lukastaegert) +- [#5657](https://github.com/rollup/rollup/pull/5657): chore(deps): update dependency vite to v5.4.6 [security] (@renovate[bot]) +- [#5658](https://github.com/rollup/rollup/pull/5658): Detect variable reassignments in modules without side effects (@lukastaegert) + ## 4.21.3 _2024-09-12_ diff --git a/browser/package.json b/browser/package.json index f4250fc4d..e72b50e65 100644 --- a/browser/package.json +++ b/browser/package.json @@ -1,6 +1,6 @@ { "name": "@rollup/browser", - "version": "4.21.3", + "version": "4.22.0", "description": "Next-generation ES module bundler browser build", "main": "dist/rollup.browser.js", "module": "dist/es/rollup.browser.js", diff --git a/package-lock.json b/package-lock.json index 1b9943dd3..d1979df42 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "rollup", - "version": "4.21.3", + "version": "4.22.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "rollup", - "version": "4.21.3", + "version": "4.22.0", "license": "MIT", "dependencies": { "@types/estree": "1.0.5" diff --git a/package.json b/package.json index 469857e95..8548681b5 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "rollup", - "version": "4.21.3", + "version": "4.22.0", "description": "Next-generation ES module bundler", "main": "dist/rollup.js", "module": "dist/es/rollup.js",