refactor(es/minifier): Improve tpl to str

[cuyl]

Description:

• Enhance Pure::convert_tpl_to_str to prefer cooked values as much as possible.
• Improve Str::from_tpl_raw to return Wtf8Atom instead of Atom.
• Add more test cases.

[changeset-bot]

🦋 Changeset detected

Latest commit: 02a7aab

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

Binary Sizes

File	Size
swc.linux-x64-gnu.node	28M (28443720 bytes)

Commit: 58e014a

[codspeed-hq]

Merging this PR will not alter performance

✅ 185 untouched benchmarks

---

_{Comparing cuyl:refact-convert-tpl-to-str (02a7aab) with main (144be84)}

[cuyl]

I'm not sure why we have force_str_for_tpl here, I can see 2 definition:

1. Minification::force_str_for_tpl returns false
2. Eval::force_str_for_tpl returns true

There are some encoding logic in convert_str_value_to_tpl_cooked, why we need convert Str::value to Tpl::cooked using convert_str_value_to_tpl_cooked, Aren't they the same thing?
Can I just remove convert_str_value_to_tpl_cooked?
@kdy1

[kdy1]

Does CI passes, if you remove it?

[cuyl]

I have changed some outputs here, but looks not a bug.
@kdy1

[kdy1]

@claude Review this PR. My concerns are

• the correctness of compress_array_join_as_tpl in crates/swc_ecma_minifier/src/compress/pure/misc.rs
• (as many as) edge cases you can think of, based on the code changes

[claude]

Claude encountered an error —— View job

Command failed: git fetch origin --depth=20 refact-convert-tpl-to-str

---

I'll analyze this and get back to you.

[kdy1]

@claude Review this PR. My concerns are

• the correctness of compress_array_join_as_tpl in crates/swc_ecma_minifier/src/compress/pure/misc.rs
• (as many as) edge cases you can think of, based on the code changes

You should do gh pr checkout 11415 to checkout the PR branch

[claude]

Claude encountered an error —— View job

Command failed: git fetch origin --depth=20 refact-convert-tpl-to-str

---

I'll analyze this and get back to you.

[kdy1]

Review Summary

I've identified several critical issues that need to be addressed:

1. CRITICAL: Compilation error in - calls removed function
2. HIGH: Surrogate pair handling logic has multiple bugs in
3. MEDIUM: Error handling uses instead of graceful degradation
4. LOW: Missing test coverage for edge cases

Detailed comments are provided on each file. Please address the critical and high priority issues before merging.

[kdy1]

Review Summary

I have identified several critical issues that need to be addressed:

1. CRITICAL: Compilation error in misc.rs line 1429 - calls removed function
2. HIGH: Surrogate pair handling logic has multiple bugs in lit.rs
3. MEDIUM: Error handling uses panic instead of graceful degradation
4. LOW: Missing test coverage for edge cases

Detailed comments are provided on each file. Please address the critical and high priority issues before merging.

[kdy1]

Correction on my first comment:

You are absolutely right that my comment about line 1429 in misc.rs was incorrect. Looking at the diff more carefully, the change on that line is:

- cur_cooked.push_wtf8(&convert_str_value_to_tpl_cooked(&s.value));
+ cur_cooked.push_wtf8(&Cow::Borrowed(&s.value));

This shows you already fixed this correctly in the PR - the new version uses Cow::Borrowed. I misread the diff and thought the old function was still being called. My apologies for the confusion!

The other comments about surrogate pair handling, iterator advancement, and panic usage remain valid concerns for the from_tpl_raw implementation in lit.rs.

[cuyl]

I assumed the panic is unreachable code, I will add error handing later.

[cuyl]

So, Can I try to remove force_str_for_tpl in this PR?

[kdy1]

Yeap, it would be nice. Thank you!

[cuyl]

Seems Optimizer::reduce_escaped_newline_for_str_lit is a inverse operation of Pure::convert_tpl_to_str, added some code to prevent infinity loop.

[cuyl]

Remove force_str_for_tpl is complicated, I would open another PR instead.

[kdy1]

I see, it sounds good. Thanks!

[cuyl]

OK, now force_str_for_tpl has been removed, and all tests passed

[kdy1]

@claude Review this PR

[claude]

Claude encountered an error —— View job

Command failed: git fetch origin --depth=20 refact-convert-tpl-to-str

---

I'll analyze this and get back to you.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

After pushing the previous high surrogate at lines 369-371, prev_result should be reset to NO_PREV_RESULT to avoid pushing it again at line 396. Without this reset, if the current result is also a high surrogate but not followed by \u, the previous surrogate will be pushed twice. Add prev_result = NO_PREV_RESULT; after line 371.

Suggested change

	});
	});
	prev_result = NO_PREV_RESULT;

[Copilot]

When count >= max_len and result > 0x10ffff, the code emits an error but doesn't break out of the loop, allowing it to continue parsing more hex digits beyond the maximum length. This should break after emitting the error to prevent parsing extra digits. Change line 350 to include a break statement or restructure the logic to break after the if-else at line 351-353.

Suggested change

	)
	} else {
	break;
	}
	}
	);
	}
	break;
	}

[Copilot]

The comment "result is a 4 digit hex value" is misleading because the result can be 2 digits for \x escapes, 4 digits for \uXXXX escapes, or up to 6 digits for \u{...} escapes. Consider updating the comment to be more accurate, such as "result accumulates the hex digits".

Suggested change

	// result is a 4 digit hex value
	// result accumulates the hex digits of the escape sequence

[Copilot]

The logic for handling ${ sequences in string-to-template conversion is incorrect. When converting a string literal containing ${ to a template, only the ${ needs to be escaped as \${ (adding 1 character for the backslash). The current code incorrectly tries to find a matching } and only increments the count if found. This should be simplified to: when ${ is encountered, increment template_longer_count by 1 (for the added backslash) without looking for a closing brace.

Suggested change

	iter.next();
	let mut cloned_iter = iter.clone();
	while let Some(cp) = cloned_iter.next() {
	if let Some(ch) = cp.to_char() {
	if ch == '}' {
	iter = cloned_iter;
	template_longer_count += 1;
	break;
	}
	}
	}
	// When converting to a template, `${` becomes `\${`,
	// adding exactly one extra character (the backslash).
	iter.next(); // Consume the '{'
	template_longer_count += 1;

[kdy1]

Opus is happy with it

The removal of convert_str_value_to_tpl_cooked appears to be correct. The function was doing an extra level of unescaping that was unnecessary because Str::value already contains the decoded string value. The direct use of Cow::Borrowed(&s.value) is semantically appropriate.

[kdy1]

Thank you!