fix(optimizer): preserve purity of migrated QRLs

.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

packages/optimizer/core/src/code_move.rs

@@ -7,11 +7,15 @@ 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;
+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};
@@ -120,10 +124,6 @@ pub struct NewModuleCtx<'a> {
 }
 
 pub fn new_module(ctx: NewModuleCtx) -> Result<(ast::Module, SingleThreadedComments), Error> {
-	let comments = SingleThreadedComments::from_leading_and_trailing(
-		ctx.leading_comments,
-		ctx.trailing_comments,
-	);
 	let max_cap = ctx.global.imports.len() + ctx.global.exports.len() + ctx.extra_top_items.len();
 	let mut module = ast::Module {
 		span: DUMMY_SP,
@@ -448,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<Id, ast::ModuleItem>,
 	local_idents: &[Id],
@@ -1373,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<SourceMap>, ast::Module, SingleThreadedComments) {
+		let comments = SingleThreadedComments::default();
+		let source_map: Lrc<SourceMap> = 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 = <div>{/* jsx */}</div>;
+"#,
+		);
+		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::*;

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');

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");

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");