From 5248e3b8aa31385bf609cd15c3d896a6a8164168 Mon Sep 17 00:00:00 2001 From: Varixo Date: Thu, 5 Nov 2026 19:18:59 +0100 Subject: [PATCH 1/2] fix(optimizer): preserve purity of migrated QRLs --- .changeset/pure-migrated-qrls.md | 6 + packages/optimizer/core/src/code_move.rs | 7 +- .../core/src/fixtures/migrated-qrl-pure.js | 23 +++ ...ore__test__example_qwik_router_client.snap | 7 +- ..._self_referential_component_migration.snap | 3 +- .../qwik_core__test__migrated_qrl_pure.snap | 174 ++++++++++++++++++ packages/optimizer/core/src/test.rs | 50 +++++ .../qwik-vite/src/plugins/qrl-purity.unit.ts | 75 ++++++++ .../src/optimizer/rewrite/output-assembly.ts | 2 +- .../optimizer/segment/qrl-purity.test.ts | 32 ++++ .../tests/optimizer/snapshot-options.ts | 6 + 11 files changed, 376 insertions(+), 9 deletions(-) create mode 100644 .changeset/pure-migrated-qrls.md create mode 100644 packages/optimizer/core/src/fixtures/migrated-qrl-pure.js create mode 100644 packages/optimizer/core/src/snapshots/qwik_core__test__migrated_qrl_pure.snap create mode 100644 packages/qwik-vite/src/plugins/qrl-purity.unit.ts create mode 100644 packages/ts-optimizer/tests/optimizer/segment/qrl-purity.test.ts diff --git a/.changeset/pure-migrated-qrls.md b/.changeset/pure-migrated-qrls.md new file mode 100644 index 00000000000..0fa2337ae2f --- /dev/null +++ b/.changeset/pure-migrated-qrls.md @@ -0,0 +1,6 @@ +--- +'@qwik.dev/core': patch +'@qwik.dev/optimizer': patch +--- + +fix: remove unused callbacks retained after moving helpers into segments diff --git a/packages/optimizer/core/src/code_move.rs b/packages/optimizer/core/src/code_move.rs index e77b9a34bef..5d22f15e466 100644 --- a/packages/optimizer/core/src/code_move.rs +++ b/packages/optimizer/core/src/code_move.rs @@ -7,7 +7,9 @@ use crate::words::*; use anyhow::Error; use indexmap::IndexMap; +use std::cell::RefCell; use std::collections::{BTreeMap, HashMap, HashSet}; +use std::rc::Rc; use swc_atoms::Atom; use swc_common::comments::{SingleThreadedComments, SingleThreadedCommentsMap}; use swc_common::Spanned; @@ -120,9 +122,10 @@ pub struct NewModuleCtx<'a> { } pub fn new_module(ctx: NewModuleCtx) -> Result<(ast::Module, SingleThreadedComments), Error> { + // Emitters consume comments, so each module needs independent storage. let comments = SingleThreadedComments::from_leading_and_trailing( - ctx.leading_comments, - ctx.trailing_comments, + Rc::new(RefCell::new(ctx.leading_comments.borrow().clone())), + Rc::new(RefCell::new(ctx.trailing_comments.borrow().clone())), ); let max_cap = ctx.global.imports.len() + ctx.global.exports.len() + ctx.extra_top_items.len(); let mut module = ast::Module { diff --git a/packages/optimizer/core/src/fixtures/migrated-qrl-pure.js b/packages/optimizer/core/src/fixtures/migrated-qrl-pure.js new file mode 100644 index 00000000000..2304d096f6d --- /dev/null +++ b/packages/optimizer/core/src/fixtures/migrated-qrl-pure.js @@ -0,0 +1,23 @@ +import { componentQrl, inlinedQrl } from '@qwik.dev/core'; + +function useUnused() { + return /*#__PURE__*/ inlinedQrl(() => 'UNUSED_CALLBACK', 'useUnused_callback_AAA111aaa11'); +} + +export const Unused = /*#__PURE__*/ componentQrl( + /*#__PURE__*/ inlinedQrl(() => { + useUnused(); + return null; + }, 'Unused_component_BBB222bbb22') +); + +function useRead() { + const read = /*#__PURE__*/ inlinedQrl(() => 'LIVE_READ', 'useRead_read_CCC333ccc33'); + return read(); +} + +export const App = /*#__PURE__*/ componentQrl( + /*#__PURE__*/ inlinedQrl(() => useRead(), 'App_component_DDD444ddd44') +); + +console.log('KEEP_SIDE_EFFECT'); diff --git a/packages/optimizer/core/src/snapshots/qwik_core__test__example_qwik_router_client.snap b/packages/optimizer/core/src/snapshots/qwik_core__test__example_qwik_router_client.snap index 5da5591cd08..a45de5a4145 100644 --- a/packages/optimizer/core/src/snapshots/qwik_core__test__example_qwik_router_client.snap +++ b/packages/optimizer/core/src/snapshots/qwik_core__test__example_qwik_router_client.snap @@ -1,6 +1,5 @@ --- source: packages/optimizer/core/src/test.rs -assertion_line: 3588 expression: output --- ==INPUT== @@ -4105,9 +4104,9 @@ const q_routeActionQrl_action_submit_YuS5bpdQ360 = /*#__PURE__*/ qrl(()=>import( const q_serverQrl_w03grD0Ag68 = /*#__PURE__*/ qrl(()=>import("./index.qwik.mjs_serverQrl_w03grD0Ag68.mjs"), "serverQrl_w03grD0Ag68"); const q_spa_init_event_igI1pUsax0E = /*#__PURE__*/ qrl(()=>import("./index.qwik.mjs_spa_init_event_igI1pUsax0E.mjs"), "spa_init_event_igI1pUsax0E"); // -qrl(()=>import("./index.qwik.mjs_useQwikMockRouter_createAsync_clbxpuXqpEU.mjs"), "useQwikMockRouter_createAsync_clbxpuXqpEU"); -qrl(()=>import("./index.qwik.mjs_useQwikMockRouter_goto_aViHFxQ1a3s.mjs"), "useQwikMockRouter_goto_aViHFxQ1a3s"); -qrl(()=>import("./index.qwik.mjs_useQwikMockRouter_useTask_tXTLR4tzCy0.mjs"), "useQwikMockRouter_useTask_tXTLR4tzCy0"); +/*#__PURE__*/ qrl(()=>import("./index.qwik.mjs_useQwikMockRouter_createAsync_clbxpuXqpEU.mjs"), "useQwikMockRouter_createAsync_clbxpuXqpEU"); +/*#__PURE__*/ qrl(()=>import("./index.qwik.mjs_useQwikMockRouter_goto_aViHFxQ1a3s.mjs"), "useQwikMockRouter_goto_aViHFxQ1a3s"); +/*#__PURE__*/ qrl(()=>import("./index.qwik.mjs_useQwikMockRouter_useTask_tXTLR4tzCy0.mjs"), "useQwikMockRouter_useTask_tXTLR4tzCy0"); // const q_useQwikRouter_getScroller_0UhDFwlxeFQ = /*#__PURE__*/ qrl(()=>import("./index.qwik.mjs_useQwikRouter_getScroller_0UhDFwlxeFQ.mjs"), "useQwikRouter_getScroller_0UhDFwlxeFQ"); const q_useQwikRouter_goto_8j8Vrz2yUIM = /*#__PURE__*/ qrl(()=>import("./index.qwik.mjs_useQwikRouter_goto_8j8Vrz2yUIM.mjs"), "useQwikRouter_goto_8j8Vrz2yUIM"); diff --git a/packages/optimizer/core/src/snapshots/qwik_core__test__example_self_referential_component_migration.snap b/packages/optimizer/core/src/snapshots/qwik_core__test__example_self_referential_component_migration.snap index 63db05e71ca..fc887ec4b59 100644 --- a/packages/optimizer/core/src/snapshots/qwik_core__test__example_self_referential_component_migration.snap +++ b/packages/optimizer/core/src/snapshots/qwik_core__test__example_self_referential_component_migration.snap @@ -1,6 +1,5 @@ --- source: packages/optimizer/core/src/test.rs -assertion_line: 5888 expression: output --- ==INPUT== @@ -241,7 +240,7 @@ import { qrl } from "@qwik.dev/core"; // const q_ComponentA_component_100sEyGkGuA = /*#__PURE__*/ qrl(()=>import("./test.tsx_ComponentA_component_100sEyGkGuA"), "ComponentA_component_100sEyGkGuA"); // -qrl(()=>import("./test.tsx_ComponentB_component_WfCOFVxlmq4"), "ComponentB_component_WfCOFVxlmq4"); +/*#__PURE__*/ qrl(()=>import("./test.tsx_ComponentB_component_WfCOFVxlmq4"), "ComponentB_component_WfCOFVxlmq4"); // const q_MutualExample_component_LXt10RL0k44 = /*#__PURE__*/ qrl(()=>import("./test.tsx_MutualExample_component_LXt10RL0k44"), "MutualExample_component_LXt10RL0k44"); const q_Nested_component_8cqIQQSPqhE = /*#__PURE__*/ qrl(()=>import("./test.tsx_Nested_component_8cqIQQSPqhE"), "Nested_component_8cqIQQSPqhE"); diff --git a/packages/optimizer/core/src/snapshots/qwik_core__test__migrated_qrl_pure.snap b/packages/optimizer/core/src/snapshots/qwik_core__test__migrated_qrl_pure.snap new file mode 100644 index 00000000000..b369eb15654 --- /dev/null +++ b/packages/optimizer/core/src/snapshots/qwik_core__test__migrated_qrl_pure.snap @@ -0,0 +1,174 @@ +--- +source: packages/optimizer/core/src/test.rs +expression: output +--- +==INPUT== + +import { componentQrl, inlinedQrl } from '@qwik.dev/core'; + +function useUnused() { + return /*#__PURE__*/ inlinedQrl(() => 'UNUSED_CALLBACK', 'useUnused_callback_AAA111aaa11'); +} + +export const Unused = /*#__PURE__*/ componentQrl( + /*#__PURE__*/ inlinedQrl(() => { + useUnused(); + return null; + }, 'Unused_component_BBB222bbb22') +); + +function useRead() { + const read = /*#__PURE__*/ inlinedQrl(() => 'LIVE_READ', 'useRead_read_CCC333ccc33'); + return read(); +} + +export const App = /*#__PURE__*/ componentQrl( + /*#__PURE__*/ inlinedQrl(() => useRead(), 'App_component_DDD444ddd44') +); + +console.log('KEEP_SIDE_EFFECT'); + +============================= node_modules/library/index.qwik.mjs_useUnused_callback_AAA111aaa11.mjs == + +export const s_AAA111aaa11 = ()=>'UNUSED_CALLBACK'; + + +Some("{\"version\":3,\"sources\":[\"/user/qwik/src/node_modules/library/index.qwik.mjs\"],\"names\":[],\"mappings\":\"6BAGkC,IAAM\"}") +/* +{ + "origin": "node_modules/library/index.qwik.mjs", + "name": "s_AAA111aaa11", + "entry": "node_modules/library/index.qwik.mjs_entry_useUnused", + "displayName": "index.qwik.mjs_useUnused_callback", + "hash": "AAA111aaa11", + "canonicalFilename": "index.qwik.mjs_useUnused_callback_AAA111aaa11", + "path": "node_modules/library", + "extension": "mjs", + "parent": null, + "ctxKind": "function", + "ctxName": "useUnused", + "captures": false, + "loc": [ + 118, + 141 + ] +} +*/ +============================= node_modules/library/index.qwik.mjs_Unused_component_BBB222bbb22.mjs == + +import { qrl } from "@qwik.dev/core"; +// +const q_s_AAA111aaa11 = /*#__PURE__*/ qrl(()=>import("./index.qwik.mjs_useUnused_callback_AAA111aaa11"), "s_AAA111aaa11"); +// +function useUnused() { + return q_s_AAA111aaa11; +} +export const s_BBB222bbb22 = ()=>{ + useUnused(); + return null; +}; + + +Some("{\"version\":3,\"sources\":[\"/user/qwik/src/node_modules/library/index.qwik.mjs\"],\"names\":[],\"mappings\":\";;;;AAEA,SAAS;IACP;AACF;6BAG2B;IACvB;IACA,OAAO;AACT\"}") +/* +{ + "origin": "node_modules/library/index.qwik.mjs", + "name": "s_BBB222bbb22", + "entry": "node_modules/library/index.qwik.mjs_entry_Unused", + "displayName": "index.qwik.mjs_Unused_component", + "hash": "BBB222bbb22", + "canonicalFilename": "index.qwik.mjs_Unused_component_BBB222bbb22", + "path": "node_modules/library", + "extension": "mjs", + "parent": null, + "ctxKind": "function", + "ctxName": "component$", + "captures": false, + "loc": [ + 258, + 303 + ] +} +*/ +============================= node_modules/library/index.qwik.mjs_useRead_read_CCC333ccc33.mjs == + +export const s_CCC333ccc33 = ()=>'LIVE_READ'; + + +Some("{\"version\":3,\"sources\":[\"/user/qwik/src/node_modules/library/index.qwik.mjs\"],\"names\":[],\"mappings\":\"6BAcwC,IAAM\"}") +/* +{ + "origin": "node_modules/library/index.qwik.mjs", + "name": "s_CCC333ccc33", + "entry": "node_modules/library/index.qwik.mjs_entry_useRead", + "displayName": "index.qwik.mjs_useRead_read", + "hash": "CCC333ccc33", + "canonicalFilename": "index.qwik.mjs_useRead_read_CCC333ccc33", + "path": "node_modules/library", + "extension": "mjs", + "parent": null, + "ctxKind": "function", + "ctxName": "read", + "captures": false, + "loc": [ + 402, + 419 + ] +} +*/ +============================= node_modules/library/index.qwik.mjs_App_component_DDD444ddd44.mjs == + +import { qrl } from "@qwik.dev/core"; +// +const q_s_CCC333ccc33 = /*#__PURE__*/ qrl(()=>import("./index.qwik.mjs_useRead_read_CCC333ccc33"), "s_CCC333ccc33"); +// +function useRead() { + const read = q_s_CCC333ccc33; + return read(); +} +export const s_DDD444ddd44 = ()=>useRead(); + + +Some("{\"version\":3,\"sources\":[\"/user/qwik/src/node_modules/library/index.qwik.mjs\"],\"names\":[],\"mappings\":\";;;;AAaA,SAAS;IACP,MAAM;IACN,OAAO;AACT;6BAG2B,IAAM\"}") +/* +{ + "origin": "node_modules/library/index.qwik.mjs", + "name": "s_DDD444ddd44", + "entry": "node_modules/library/index.qwik.mjs_entry_App", + "displayName": "index.qwik.mjs_App_component", + "hash": "DDD444ddd44", + "canonicalFilename": "index.qwik.mjs_App_component_DDD444ddd44", + "path": "node_modules/library", + "extension": "mjs", + "parent": null, + "ctxKind": "function", + "ctxName": "component$", + "captures": false, + "loc": [ + 544, + 559 + ] +} +*/ +============================= node_modules/library/index.qwik.mjs == + +import { qrl } from "@qwik.dev/core"; +import { componentQrl } from '@qwik.dev/core'; +// +/*#__PURE__*/ qrl(()=>import("./index.qwik.mjs_useUnused_callback_AAA111aaa11"), "s_AAA111aaa11"); +// +const q_s_BBB222bbb22 = /*#__PURE__*/ qrl(()=>import("./index.qwik.mjs_Unused_component_BBB222bbb22"), "s_BBB222bbb22"); +// +/*#__PURE__*/ qrl(()=>import("./index.qwik.mjs_useRead_read_CCC333ccc33"), "s_CCC333ccc33"); +// +const q_s_DDD444ddd44 = /*#__PURE__*/ qrl(()=>import("./index.qwik.mjs_App_component_DDD444ddd44"), "s_DDD444ddd44"); +// +export const Unused = /*#__PURE__*/ componentQrl(q_s_BBB222bbb22); +export const App = /*#__PURE__*/ componentQrl(q_s_DDD444ddd44); +console.log('KEEP_SIDE_EFFECT'); + + +Some("{\"version\":3,\"sources\":[\"/user/qwik/src/node_modules/library/index.qwik.mjs\"],\"names\":[],\"mappings\":\";AAAA,SAAS,YAAY,QAAoB,iBAAiB;;;;;;;;;;AAM1D,OAAO,MAAM,SAAS,WAAW,GAAG,8BAKlC;AAOF,OAAO,MAAM,MAAM,WAAW,GAAG,8BAE/B;AAEF,QAAQ,GAAG,CAAC\"}") +== DIAGNOSTICS == + +[] diff --git a/packages/optimizer/core/src/test.rs b/packages/optimizer/core/src/test.rs index 05d08d2b092..cb90805f986 100644 --- a/packages/optimizer/core/src/test.rs +++ b/packages/optimizer/core/src/test.rs @@ -88,6 +88,56 @@ fn test_input_fn(input: TestInput) -> Result { }) } +#[test] +fn migrated_qrl_pure() { + let code = include_str!("fixtures/migrated-qrl-pure.js").to_string(); + for minify in [MinifyMode::None, MinifyMode::Simplify] { + let output = test_input_fn(TestInput { + code: code.clone(), + filename: "node_modules/library/index.qwik.mjs".into(), + entry_strategy: EntryStrategy::Smart, + mode: EmitMode::Prod, + minify, + is_server: Some(false), + ..TestInput::default() + }) + .unwrap(); + let parent = output + .modules + .iter() + .find(|module| module.segment.is_none()) + .unwrap(); + let calls: Vec<_> = parent + .code + .lines() + .filter(|line| line.contains("qrl(")) + .collect(); + assert!(!calls.is_empty()); + for call in calls { + assert!(call.contains("/*#__PURE__*/"), "{call}"); + } + assert!(parent.code.contains("KEEP_SIDE_EFFECT")); + let render = output + .modules + .iter() + .find(|module| { + module + .segment + .as_ref() + .is_some_and(|segment| segment.display_name.ends_with("_App_component")) + }) + .unwrap(); + assert!(render.code.contains("useRead")); + assert!(render.code.contains("q_s_CCC333ccc33")); + if matches!(minify, MinifyMode::Simplify) { + snapshot_res!( + Ok::<_, anyhow::Error>(output), + format!("==INPUT==\n\n{}", code) + ); + } + } +} + #[test] fn smart_visible_task_heavy_library() { test_input!(TestInput { diff --git a/packages/qwik-vite/src/plugins/qrl-purity.unit.ts b/packages/qwik-vite/src/plugins/qrl-purity.unit.ts new file mode 100644 index 00000000000..edbdf9e0112 --- /dev/null +++ b/packages/qwik-vite/src/plugins/qrl-purity.unit.ts @@ -0,0 +1,75 @@ +import { readFileSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; +import { expect, test } from 'vitest'; +import { normalizePath } from '../../../qwik/src/testing/util'; +import { createQwikPlugin } from './plugin'; + +test.each( + [true, false].flatMap((tsOptimizer) => [false, true].map((minify) => ({ tsOptimizer, minify }))) +)( + 'tree shakes unused compiled QRLs and retains render calls (TS=$tsOptimizer, minify=$minify)', + async ({ tsOptimizer, minify }) => { + const root = normalizePath(resolve(process.cwd(), 'qrl-purity-fixture')); + const libraryId = root + '/node_modules/library/index.qwik.mjs'; + const entryId = root + '/entry.js'; + const plugin = createQwikPlugin({ tsOptimizer }); + await plugin.init(); + await plugin.normalizeOptions({ + target: 'client', + buildMode: 'production', + entryStrategy: { type: 'smart' }, + rootDir: root, + srcDir: root, + }); + const source = readFileSync( + new URL('../../../optimizer/core/src/fixtures/migrated-qrl-pure.js', import.meta.url), + 'utf8' + ); + const transformed = await plugin.transform( + { addWatchFile() {}, emitFile() {} } as any, + source, + libraryId + ); + const modules = new Map([ + [entryId, { code: `export {App} from '${libraryId}';` }], + [libraryId, transformed], + ]); + for (const id of transformed!.meta!.qwikdeps) { + modules.set(id, await plugin.load({} as any, id)); + } + const { rolldown } = await import('rolldown'); + const build = await rolldown({ + input: [entryId], + preserveEntrySignatures: 'allow-extension', + external: (id) => id.startsWith('@qwik.dev/'), + plugins: [ + { + name: 'qrl-purity-fixture', + resolveId(id, importer) { + const resolved = normalizePath( + id.startsWith('.') ? resolve(dirname(importer!), id) : id + ); + return modules.has(resolved) ? resolved : null; + }, + load: (id) => modules.get(id), + }, + ], + }); + try { + const { output } = await build.generate({ + format: 'es', + minify, + codeSplitting: { ...plugin.codeSplitting(), includeDependenciesRecursively: false }, + }); + const code = output + .filter((chunk) => chunk.type === 'chunk') + .map((chunk) => chunk.code) + .join('\n'); + expect(code).not.toContain('UNUSED_CALLBACK'); + expect(code).toContain('LIVE_READ'); + expect(code).toContain('KEEP_SIDE_EFFECT'); + } finally { + await build.close(); + } + } +); diff --git a/packages/ts-optimizer/src/optimizer/rewrite/output-assembly.ts b/packages/ts-optimizer/src/optimizer/rewrite/output-assembly.ts index 7c098b8f575..ea98f684ad0 100644 --- a/packages/ts-optimizer/src/optimizer/rewrite/output-assembly.ts +++ b/packages/ts-optimizer/src/optimizer/rewrite/output-assembly.ts @@ -414,7 +414,7 @@ export function buildQrlDeclarations(ctx: RewriteContext): void { if (movedMarkerSymbols.has(ext.symbolName) && !(isDevMode && devFilePath)) { const fileExt = explicitExtensions ? (outputExtension ?? '.js') : ''; ctx.qrlDecls.push( - `qrl(()=>import("./${ext.canonicalFilename}${fileExt}"), "${ext.symbolName}");` + `/*#__PURE__*/ qrl(()=>import("./${ext.canonicalFilename}${fileExt}"), "${ext.symbolName}");` ); // Intentionally not registering `q_` in `qrlVarNames`: the // parent no longer declares it, so a stray reference should surface diff --git a/packages/ts-optimizer/tests/optimizer/segment/qrl-purity.test.ts b/packages/ts-optimizer/tests/optimizer/segment/qrl-purity.test.ts new file mode 100644 index 00000000000..a40387a546c --- /dev/null +++ b/packages/ts-optimizer/tests/optimizer/segment/qrl-purity.test.ts @@ -0,0 +1,32 @@ +import { readFileSync } from 'node:fs'; +import { expect, it } from 'vitest'; +import { transformModule } from '../../../src/optimizer/transform/index.js'; +import { mkFilePath, mkSourceText } from '../../../src/optimizer/types/brands.js'; + +it.each(['none', 'simplify'] as const)('preserves migrated QRL purity (%s)', (minify) => { + const code = readFileSync( + new URL('../../../../optimizer/core/src/fixtures/migrated-qrl-pure.js', import.meta.url), + 'utf8' + ); + const result = transformModule({ + input: [{ path: mkFilePath('node_modules/library/index.qwik.mjs'), code: mkSourceText(code) }], + srcDir: mkFilePath('.'), + entryStrategy: { type: 'smart' }, + mode: 'prod', + minify, + isServer: false, + }); + expect(result.diagnostics.filter((diagnostic) => diagnostic.category === 'error')).toEqual([]); + const parent = result.modules.find((module) => module.kind !== 'segment')!; + const calls = parent.code.split('\n').filter((line) => /\bqrl\(/.test(line)); + expect(calls.length).toBeGreaterThan(0); + for (const line of calls) { + expect(line).toContain('/*#__PURE__*/'); + } + expect(parent.code).toContain('KEEP_SIDE_EFFECT'); + const render = result.modules.find( + (module) => module.kind === 'segment' && module.segment.displayName.endsWith('_App_component') + )!; + expect(render.code).toContain('useRead'); + expect(render.code).toContain('q_s_CCC333ccc33'); +}); diff --git a/packages/ts-optimizer/tests/optimizer/snapshot-options.ts b/packages/ts-optimizer/tests/optimizer/snapshot-options.ts index 3353b9bc05c..c636c24ce65 100644 --- a/packages/ts-optimizer/tests/optimizer/snapshot-options.ts +++ b/packages/ts-optimizer/tests/optimizer/snapshot-options.ts @@ -37,6 +37,12 @@ export const DEFAULT_OPTIONS: Required< }; export const SNAPSHOT_OPTIONS: Record = { + migrated_qrl_pure: { + filename: 'node_modules/library/index.qwik.mjs', + entryStrategy: { type: 'smart' }, + mode: 'prod', + isServer: false, + }, should_transform_loop_stable_hoisted_var: { transpileTs: true, transpileJsx: true }, should_transform_loop_stable_hoisted_var_declarators: { transpileTs: true, transpileJsx: true }, should_transform_loop_hoisted_function_reference: { transpileTs: true, transpileJsx: true }, From 798173c3e24d5a5653b93014f083b3d78fa11aba Mon Sep 17 00:00:00 2001 From: Varixo Date: Thu, 5 Nov 2026 21:19:01 +0100 Subject: [PATCH 2/2] perf(optimizer): limit comment copies to generated modules Avoid quadratic copying in libraries with many QRL segments. --- packages/optimizer/core/src/code_move.rs | 255 ++++++++++++++++++++++- 1 file changed, 248 insertions(+), 7 deletions(-) diff --git a/packages/optimizer/core/src/code_move.rs b/packages/optimizer/core/src/code_move.rs index 5d22f15e466..0fb03dc44c3 100644 --- a/packages/optimizer/core/src/code_move.rs +++ b/packages/optimizer/core/src/code_move.rs @@ -11,9 +11,11 @@ use std::cell::RefCell; use std::collections::{BTreeMap, HashMap, HashSet}; use std::rc::Rc; use swc_atoms::Atom; -use swc_common::comments::{SingleThreadedComments, SingleThreadedCommentsMap}; -use swc_common::Spanned; +use swc_common::comments::{ + SingleThreadedComments, SingleThreadedCommentsMap, SingleThreadedCommentsMapInner, +}; use swc_common::DUMMY_SP; +use swc_common::{BytePos, Span, Spanned}; use swc_ecmascript::ast; use swc_ecmascript::utils::private_ident; use swc_ecmascript::visit::{noop_visit_type, Fold, FoldWith, Visit, VisitWith}; @@ -122,11 +124,6 @@ pub struct NewModuleCtx<'a> { } pub fn new_module(ctx: NewModuleCtx) -> Result<(ast::Module, SingleThreadedComments), Error> { - // Emitters consume comments, so each module needs independent storage. - let comments = SingleThreadedComments::from_leading_and_trailing( - Rc::new(RefCell::new(ctx.leading_comments.borrow().clone())), - Rc::new(RefCell::new(ctx.trailing_comments.borrow().clone())), - ); let max_cap = ctx.global.imports.len() + ctx.global.exports.len() + ctx.extra_top_items.len(); let mut module = ast::Module { span: DUMMY_SP, @@ -451,9 +448,102 @@ pub fn new_module(ctx: NewModuleCtx) -> Result<(ast::Module, SingleThreadedComme module.body = final_body; module.body.push(create_named_export(expr, ctx.name)); + let comments = copy_module_comments(&module, &ctx.leading_comments, &ctx.trailing_comments); Ok((module, comments)) } +fn copy_module_comments( + module: &ast::Module, + leading: &SingleThreadedCommentsMap, + trailing: &SingleThreadedCommentsMap, +) -> SingleThreadedComments { + struct ModuleComments<'a> { + source_leading: &'a SingleThreadedCommentsMapInner, + source_trailing: &'a SingleThreadedCommentsMapInner, + leading: SingleThreadedCommentsMapInner, + trailing: SingleThreadedCommentsMapInner, + } + + impl ModuleComments<'_> { + fn copy_leading(&mut self, pos: BytePos) { + if !pos.is_dummy() { + if let Some(comments) = self.source_leading.get(&pos) { + self.leading.entry(pos).or_insert_with(|| comments.clone()); + } + } + } + + fn copy_trailing(&mut self, pos: BytePos) { + if !pos.is_dummy() { + if let Some(comments) = self.source_trailing.get(&pos) { + self.trailing.entry(pos).or_insert_with(|| comments.clone()); + } + } + } + } + + impl Visit for ModuleComments<'_> { + fn visit_span(&mut self, span: &Span) { + self.copy_leading(span.lo); + self.copy_trailing(span.hi); + // SWC uses opening positions for empty-list trailing comments. + self.copy_trailing(span.lo); + // SWC also emits leading comments before closing delimiters. + if span.hi.0 > 0 { + self.copy_leading(span.hi - BytePos(1)); + } + } + + fn visit_member_expr(&mut self, member: &ast::MemberExpr) { + member.visit_children_with(self); + if member.prop.is_computed() { + return; + } + let start = member.prop.span().lo; + // SWC checks dot positions before member property identifiers. + if start.0 >= 1 { + self.copy_leading(start - BytePos(1)); + } + if start.0 >= 2 { + self.copy_leading(start - BytePos(2)); + } + } + + fn visit_super_prop_expr(&mut self, member: &ast::SuperPropExpr) { + member.visit_children_with(self); + if member.prop.is_computed() { + return; + } + let start = member.prop.span().lo; + if start.0 >= 1 { + self.copy_leading(start - BytePos(1)); + } + } + + fn visit_jsx_expr_container(&mut self, container: &ast::JSXExprContainer) { + container.visit_children_with(self); + self.copy_trailing(container.span.lo + BytePos(1)); + } + } + + let source_leading = leading.borrow(); + let source_trailing = trailing.borrow(); + if source_leading.is_empty() && source_trailing.is_empty() { + return SingleThreadedComments::default(); + } + let mut collector = ModuleComments { + source_leading: &source_leading, + source_trailing: &source_trailing, + leading: Default::default(), + trailing: Default::default(), + }; + module.visit_with(&mut collector); + SingleThreadedComments::from_leading_and_trailing( + Rc::new(RefCell::new(collector.leading)), + Rc::new(RefCell::new(collector.trailing)), + ) +} + fn collect_needed_extra_top_items( extra_top_items: &BTreeMap, local_idents: &[Id], @@ -1376,6 +1466,157 @@ impl Fold for QrlHoistingVisitor<'_> { } } +#[cfg(test)] +mod module_comment_tests { + use super::*; + use swc_common::comments::Comments; + use swc_common::{sync::Lrc, FileName, Globals, SourceMap, GLOBALS}; + use swc_ecmascript::parser::{parse_file_as_module, EsSyntax, Syntax}; + + fn parse_comments(code: &'static str) -> (Lrc, ast::Module, SingleThreadedComments) { + let comments = SingleThreadedComments::default(); + let source_map: Lrc = Default::default(); + let file = + source_map.new_source_file(Lrc::new(FileName::Custom("comments.jsx".into())), code); + let mut errors = vec![]; + let module = parse_file_as_module( + &file, + Syntax::Es(EsSyntax { + jsx: true, + ..Default::default() + }), + Default::default(), + Some(&comments), + &mut errors, + ) + .unwrap(); + assert!(errors.is_empty(), "{errors:?}"); + (source_map, module, comments) + } + + #[test] + fn bracket_access_skips_dot_comments_but_keeps_child_comments() { + use swc_common::comments::{Comment, CommentKind}; + + for code in [ + "/* object */ items [/* key */ index];", + "class Child extends Parent { read() { return /* object */ super [/* key */ index]; } }", + ] { + let (source_map, module, source) = parse_comments(code); + let file_start = source_map.lookup_source_file(module.span.lo).start_pos; + let bracket = file_start + BytePos(code.find('[').unwrap() as u32); + let unused_positions = [bracket - BytePos(1), bracket - BytePos(2)]; + for pos in unused_positions { + source.add_leading(pos, Comment { + kind: CommentKind::Block, + span: DUMMY_SP, + text: "unused dot position".into(), + }); + } + let (leading, trailing) = source.clone().take_all(); + let copied = copy_module_comments(&module, &leading, &trailing); + for pos in unused_positions { + assert!(copied.get_leading(pos).is_none(), "{code}: {pos:?}"); + } + let expected = crate::parse::emit_source_code(source_map.clone(), Some(source), &module, None, false).unwrap().0; + let actual = crate::parse::emit_source_code(source_map, Some(copied), &module, None, false).unwrap().0; + assert!(actual.contains("/* object */"), "{actual}"); + assert!(actual.contains("/* key */"), "{actual}"); + assert_eq!(actual, expected); + } + } + + #[test] + fn preserves_comments_used_by_codegen_at_delimiters() { + let (source_map, module, source) = parse_comments( + r#" +/** helper */ function helper() { /*#__PURE__*/ factory(/* argument */); /* closing */ } +const object = { /* object */ }; +const array = [ /* array */ ]; +object /* before dot */ . /* after dot */ field; +1 /* number */ .toString(); +class Child extends Parent { read() { return super /* super */ .field; } } +const view =
{/* jsx */}
; +"#, + ); + let (leading, trailing) = source.clone().take_all(); + let copied = copy_module_comments(&module, &leading, &trailing); + let expected = + crate::parse::emit_source_code(source_map.clone(), Some(source), &module, None, false) + .unwrap() + .0; + let actual = crate::parse::emit_source_code(source_map, Some(copied), &module, None, false) + .unwrap() + .0; + for marker in ["closing", "before dot", "after dot", "super", "jsx"] { + assert!( + expected.contains(&format!("/*{marker}*/")) + || expected.contains(&format!("/* {marker} */")), + "{marker}: {expected}" + ); + } + assert_eq!(actual, expected); + } + + #[test] + fn copies_only_final_module_comments_including_migrated_helpers() { + GLOBALS.set(&Globals::new(), || { + let (_, parsed, source) = parse_comments( + r#" +/** helper docs */ function helper() { /*#__PURE__*/ factory(); /* closing */ } +/* render expression */ value /* render tail */; +/* unrelated segment */ function other() {} // unrelated tail +"#, + ); + let helper = parsed.body[0].clone(); + let helper_start = helper.span().lo; + let unrelated = parsed.body[2].span(); + let ast::ModuleItem::Stmt(ast::Stmt::Expr(expression)) = &parsed.body[1] else { + panic!("expected render expression") + }; + let global = crate::collector::global_collect(&ast::Program::Module(parsed.clone())); + let path = crate::parse::parse_path("test.tsx", std::path::Path::new("/src")).unwrap(); + let (leading, trailing) = source.clone().take_all(); + let (_, copied) = new_module(NewModuleCtx { + expr: expression.expr.clone(), + path: &path, + name: "render", + local_idents: &[], + scoped_idents: &[], + global: &global, + core_module: &"@qwik.dev/core".into(), + need_transform: false, + explicit_extensions: true, + leading_comments: leading, + trailing_comments: trailing, + extra_top_items: &BTreeMap::new(), + migrated_root_vars: &[helper], + explicit_imports: &IndexMap::new(), + }) + .unwrap(); + let (leading, trailing) = copied.borrow_all(); + let texts: Vec<_> = leading + .values() + .chain(trailing.values()) + .flatten() + .map(|comment| comment.text.to_string()) + .collect(); + for marker in ["helper docs", "closing", "render expression", "render tail"] { + assert!( + texts.iter().any(|text| text.contains(marker)), + "{marker}: {texts:?}" + ); + } + assert!(!texts.iter().any(|text| text.contains("unrelated"))); + drop((leading, trailing)); + assert!(copied.get_leading(unrelated.lo).is_none()); + assert!(copied.get_trailing(unrelated.hi).is_none()); + copied.take_leading(helper_start).unwrap(); + assert!(source.get_leading(helper_start).is_some()); + }); + } +} + #[cfg(test)] mod import_dedup_tests { use super::*;