fix(es/codegen): Emit comments of all nodes

[kdy1]

Description:

We want to emit all comments, but at the same time, we don't want Module or Script to take the comment of a statement.

[changeset-bot]

🦋 Changeset detected

Latest commit: 02981bc

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

[claude]

Code Review for PR #11314

Summary

This PR refines the comment emission logic introduced in PR #11302 to better handle the case where Module/Script nodes share the same span starting position as their first statement. The change adds a conditional check to prevent double emission of comments.

---

✅ Positive Aspects

1. Clear Problem Identification: The logic correctly identifies when Module/Script span starts at the same position as the first statement (), preventing duplicate comment emission.

2. Code Duplication: Both and implementations are kept in sync, which is good for consistency.

3. Changeset: Properly includes a changeset file for version tracking.

4. Comments Are Informative: The inline comment "If the first statement and module span are the same, then do not take the comment of the module" clearly explains the logic.

---

🔍 Areas for Improvement

1. Comment Accuracy (Minor)

// If the first statement and module span are the same, then do not take the
// comment of the module.

This comment says "comment of the module" but could be more precise. Consider:

// If the first statement starts at the same position as the Module/Script span,
// skip emitting leading comments here. The first statement will handle them instead.

Location: crates/swc_ecma_codegen/src/lib.rs:1436-1437 and 1475-1476

2. Performance Consideration (Minor)

The code calls .first() and then .map() followed by .unwrap_or():

let should_skip = first_stmt_span_lo
    .map(|lo| lo == self.span.lo)
    .unwrap_or(false);

This is fine, but you could make it slightly more direct:

let should_skip = first_stmt_span_lo == Some(self.span.lo);

This eliminates the closure and unwrap_or, making the intent clearer. However, the current approach is also perfectly acceptable and perhaps more explicit about the logic flow.

Location: crates/swc_ecma_codegen/src/lib.rs:1439-1441 and 1478-1480

3. Test Coverage

The PR doesn't appear to include new tests or reference existing tests that validate this behavior. According to the project's CLAUDE.md guidelines (#5: "Write unit tests for your code"), it would be good to:

• Add a test case demonstrating the scenario where Module span starts at position 0 and the first statement also starts at position 0
• Verify comments are emitted correctly in this edge case
• Ensure no duplicate comments appear

Suggestion: Add a test in crates/swc_ecma_codegen/tests/ that covers this specific scenario.

---

🎯 Code Quality Assessment

Aspect	Rating	Notes
Correctness	✅ Good	Logic appears sound
Performance	✅ Good	Minimal overhead, .first() is O(1)
Maintainability	✅ Good	Code is clear and documented
Consistency	✅ Good	Both Module and Script implementations match
Testing	⚠️ Needs Improvement	No visible test coverage

---

🔒 Security & Safety

No security concerns identified. The change is purely about comment emission logic and doesn't affect runtime behavior or introduce vulnerabilities.

---

📋 Recommendations

1. High Priority: Add test coverage per CLAUDE.md guideline Handle early errors in ecmascript parser #5
2. Low Priority: Consider the comment clarity improvement suggested above
3. Optional: Consider the more concise should_skip expression

---

📝 Additional Context

This PR builds on PR #11302 which itself fixed issue #11167. The progression is:

• mod(es/parser): Make the span of Program start at input start #11199: Changed Module/Script spans to start at position 0
• Top-level comments should be placed after @swc/helpers imports #11167: Discovered comments were being lost or misplaced
• fix(es/codegen): Restore missing top-level comments #11302: Fixed by skipping comment emission when body is non-empty
• fix(es/codegen): Emit comments of all nodes #11314 (this PR): Further refinement to handle the edge case where spans align

The fix makes sense in this context and addresses a real edge case that could cause issues.

---

✅ Conclusion

Overall: APPROVE with suggestions

The code change is sound and addresses a legitimate edge case. The main improvement needed is adding test coverage to prevent regressions. The code quality is good and follows Rust best practices.

Great work on tracking down and fixing this edge case! 👍

[claude]

Code Review for PR #11314

Summary

This PR refines the comment emission logic introduced in PR #11302 to better handle the case where Module/Script nodes share the same span starting position as their first statement. The change adds a conditional check to prevent double emission of comments.

---

✅ Positive Aspects

1. Clear Problem Identification: The logic correctly identifies when Module/Script span starts at the same position as the first statement (lo == self.span.lo), preventing duplicate comment emission.

2. Code Duplication: Both Module and Script implementations are kept in sync, which is good for consistency.

3. Changeset: Properly includes a changeset file for version tracking.

4. Comments Are Informative: The inline comment "If the first statement and module span are the same, then do not take the comment of the module" clearly explains the logic.

---

🔍 Areas for Improvement

1. Comment Accuracy (Minor)

// If the first statement and module span are the same, then do not take the
// comment of the module.

This comment says "comment of the module" but could be more precise. Consider:

// If the first statement starts at the same position as the Module/Script span,
// skip emitting leading comments here. The first statement will handle them instead.

Location: crates/swc_ecma_codegen/src/lib.rs:1436-1437 and 1475-1476

2. Performance Consideration (Minor)

The code calls .first() and then .map() followed by .unwrap_or():

let should_skip = first_stmt_span_lo
    .map(|lo| lo == self.span.lo)
    .unwrap_or(false);

This is fine, but you could make it slightly more direct:

let should_skip = first_stmt_span_lo == Some(self.span.lo);

This eliminates the closure and unwrap_or, making the intent clearer. However, the current approach is also perfectly acceptable and perhaps more explicit about the logic flow.

Location: crates/swc_ecma_codegen/src/lib.rs:1439-1441 and 1478-1480

3. Test Coverage

The PR does not appear to include new tests or reference existing tests that validate this behavior. According to the project's CLAUDE.md guidelines (#5: "Write unit tests for your code"), it would be good to:

• Add a test case demonstrating the scenario where Module span starts at position 0 and the first statement also starts at position 0
• Verify comments are emitted correctly in this edge case
• Ensure no duplicate comments appear

Suggestion: Add a test in crates/swc_ecma_codegen/tests/ that covers this specific scenario.

---

🎯 Code Quality Assessment

Aspect	Rating	Notes
Correctness	✅ Good	Logic appears sound
Performance	✅ Good	Minimal overhead, .first() is O(1)
Maintainability	✅ Good	Code is clear and documented
Consistency	✅ Good	Both Module and Script implementations match
Testing	⚠️ Needs Improvement	No visible test coverage

---

🔒 Security & Safety

No security concerns identified. The change is purely about comment emission logic and does not affect runtime behavior or introduce vulnerabilities.

---

📋 Recommendations

1. High Priority: Add test coverage per CLAUDE.md guideline Handle early errors in ecmascript parser #5
2. Low Priority: Consider the comment clarity improvement suggested above
3. Optional: Consider the more concise should_skip expression

---

📝 Additional Context

This PR builds on PR #11302 which itself fixed issue #11167. The progression is:

• mod(es/parser): Make the span of Program start at input start #11199: Changed Module/Script spans to start at position 0
• Top-level comments should be placed after @swc/helpers imports #11167: Discovered comments were being lost or misplaced
• fix(es/codegen): Restore missing top-level comments #11302: Fixed by skipping comment emission when body is non-empty
• fix(es/codegen): Emit comments of all nodes #11314 (this PR): Further refinement to handle the edge case where spans align

The fix makes sense in this context and addresses a real edge case that could cause issues.

---

✅ Conclusion

Overall: APPROVE with suggestions

The code change is sound and addresses a legitimate edge case. The main improvement needed is adding test coverage to prevent regressions. The code quality is good and follows Rust best practices.

Great work on tracking down and fixing this edge case! 👍

[claude]

Code Review

Summary

This PR fixes comment emission in the ECMAScript code generator by ensuring that comments associated with Module and Script nodes are properly emitted, except when the first statement shares the same span start position as the module/script itself.

Positive Aspects ✅

1. Logical Fix: The core logic is sound - skipping comment emission when first_stmt_span_lo == module.span.lo prevents duplicate comment emission when the first statement already owns those comments.

2. Consistent Implementation: The same fix is applied symmetrically to both Module and Script implementations.

3. Handles Edge Cases: The code correctly handles empty body cases with unwrap_or(false).

4. Clear Comments: The inline comment explains the intent: "If the first statement and module span are the same, then do not take the comment of the module."

Concerns and Suggestions 🔍

1. Comment Accuracy (Minor)

The comment says "comment of the module" in both Module and Script implementations. For the Script implementation (line 1476), it should say "comment of the script" for accuracy.

// In Script impl:
// If the first statement and script span are the same, then do not take the
// comment of the script.

2. Performance Consideration (Minor)

The code calls .first() which is O(1) for most collections, but you're also calling .map(|s| s.span().lo) on every emit. Given the CLAUDE.md directive to "Write performant code. Always prefer performance over other things," consider if this could be optimized. However, since this is likely not a hot path and the operation is trivial, it's acceptable.

3. Code Duplication (Suggestion)

There's identical logic in both Module and Script implementations. Consider extracting this to a helper function to follow DRY principles:

fn should_skip_leading_comments<S: Spanned>(body: &[S], container_span: Span) -> bool {
    body.first()
        .map(|s| s.span().lo == container_span.lo)
        .unwrap_or(false)
}

Though this might be over-engineering for just 4 lines of code used twice.

4. Test Coverage ⚠️

Important: Based on the CLAUDE.md guidelines (#5: "Write unit tests for your code"), this change should include tests demonstrating:

• Comments are emitted for modules/scripts when the first statement has a different span
• Comments are NOT duplicated when the first statement has the same span
• Empty module/script behavior is preserved
• Comments around injected helper imports work correctly

The PR shows "Update test refs" in the commit history, suggesting existing tests were updated, but I cannot verify if new test cases were added for the specific bug being fixed.

5. Documentation (As per CLAUDE.md #7)

Consider adding a doc comment to these methods explaining the comment emission behavior, especially the span-based logic:

/// Emits a module to the output.
///
/// Leading comments of the module span are emitted unless they would be
/// duplicated by the first statement (i.e., when the first statement's span
/// starts at the same position as the module span).

Questions ❓

1. Related Issue: The PR body mentions "Related issue (if exists):" but it's empty. Is there an issue that describes the bug this fixes? It would help validate the fix is complete.

2. Span Equality: Is comparing only span.lo sufficient, or should we also check span.hi? What happens if two nodes start at the same position but have different lengths?

3. Injected Imports: The old comment mentioned "injected imports (e.g., helper imports)". Does this change affect how comments interact with SWC's helper injection?

Verdict

Approved with minor suggestions. The core logic is correct and follows the codebase patterns. The suggestions above are mostly about code quality, documentation, and test coverage per the CLAUDE.md guidelines, but they don't block merging.

---

Review performed by Claude Code

[claude]

Code Review

Summary

This PR fixes the handling of leading comments in the ECMAScript codegen. The change ensures that comments at the beginning of modules/scripts are properly emitted in all cases, not just for empty bodies.

Changes

**File: ** (+18/-12)

The PR modifies the emit implementations for both Module and Script to fix comment emission logic:

Previous behavior:

• Only emitted leading comments for empty modules/scripts
• Comments were lost when the body was non-empty, supposedly to ensure they appear after injected imports

New behavior:

• Checks if the first body item has the same starting position as the module/script span
• Only skips leading comment emission if the first body item starts at the exact same position
• Otherwise, emits leading comments to preserve them

Code Quality ✅

The code follows good practices:

1. Clear logic: The new should_skip_leading_comments variable makes the intent explicit
2. Consistent: The same pattern is applied to both Module and Script
3. Maintains existing behavior: For cases where the first statement starts at position 0, comments will still be emitted by that statement

Test Coverage ✅

Excellent test coverage with 554 test files updated showing the fix works across many scenarios:

• Comments preserved with Angular annotations (/* @ngInject */)
• Copyright headers preserved in CommonJS output
• Comments preserved in various module systems (AMD, CommonJS, SystemJS, UMD)
• Source maps updated correctly

Example test case from 11167:

// Input:
/* @ngInject */
class TestClass { ... }

// Before: Comment lost, import appeared first
import { _ as s } from "@swc/helpers/_/_define_property";
class e { ... }

// After: Comment preserved correctly  
/* @ngInject */ import { _ as s } from "@swc/helpers/_/_define_property";
class e { ... }

Performance Considerations ✅

The change has minimal performance impact:

• Adds one first() call and span comparison per module/script
• This is O(1) complexity
• Only occurs once per file during codegen
• Follows project guideline to "Write performant code"

Potential Issues & Questions ⚠️

1. Edge case consideration: What happens when self.span.lo is DUMMY_SP or invalid? Should we add a guard?

2. Comment placement with injected imports: The old comment mentioned ensuring comments appear after injected imports. Does this change affect that? From the tests it appears the behavior is actually improved (comments stay with the code they annotate), but it would be good to verify no regression for the original issue that comment was addressing.

3. Documentation: Consider adding a comment explaining why we check s.span().lo == self.span.lo - this is subtle logic that future maintainers should understand.

Security Concerns ✅

No security issues identified.

Suggestions

1. Add a code comment:

// Skip leading comment emission if the first statement starts at the exact
// same position as the module/script. In this case, the statement will handle
// emitting the comments itself, avoiding duplication.
let should_skip_leading_comments = self
    .body
    .first()
    .is_some_and(|s| s.span().lo == self.span.lo);

2. Consider a unit test: While integration tests are comprehensive, a focused unit test specifically for the comment emission logic could be valuable.

3. Verify no regression: Double-check that the original issue requiring the old comment (about injected imports) is still working correctly.

Changeset ✅

Properly documented in .changeset/stupid-numbers-hang.md

Overall Assessment: ✅ Approved with minor suggestions

This is a solid bug fix that:

• Solves a real problem (lost comments)
• Has extensive test coverage
• Minimal performance impact
• Clean implementation

The suggestions above are minor improvements for code clarity and defensive programming.

[claude]

Code Review: PR #11314

Summary

This PR fixes a critical issue where top-level comments (like copyright headers and JSDoc annotations) were being lost during code generation. The fix modifies the Module::emit() and Script::emit() methods to conditionally emit leading comments based on whether the first statement shares the same span position as the module/script.

---

✅ Positive Aspects

1. Correct Problem Identification
The PR correctly identifies and fixes the root cause: when Module/Script spans start at position 0, all leading comments were being consumed at the module level, preventing them from being emitted by body items. This addresses issue #11167.

2. Elegant Solution
The approach is clean and logical:

• Checks if the first statement's span starts at the same position as the module/script span
• If they match, skips module-level comment emission to avoid duplication
• If they don't match (e.g., injected imports with DUMMY_SP come first), emits comments at module level
• Maintains backward compatibility for empty modules/scripts

3. Comprehensive Test Coverage
The PR includes extensive test updates (300+ files) showing the fix works across various scenarios:

• Copyright headers are now correctly emitted (e.g., jest/issue-7506)
• JSDoc comments appear in the right position (e.g., issues-3xxx/3782)
• TypeScript reference test updates confirm consistent behavior

4. Code Quality

• Code is properly formatted (passes cargo fmt)
• Comments explain the logic clearly
• Follows Rust best practices with idiomatic use of Option::map and unwrap_or

---

⚠️ Concerns and Suggestions

1. Potential Edge Case: What if first_stmt_span_lo == BytePos(0)?

The current logic:

let should_skip = first_stmt_span_lo
    .map(|lo| lo == self.span.lo)
    .unwrap_or(false);

If a module span starts at BytePos(0) AND the first real statement also starts at BytePos(0) (which could happen in synthesized code or certain edge cases), this would incorrectly skip emitting comments. Consider whether this edge case needs special handling.

2. Comment Clarity

The comment says:

// If the first statement and module span are the same, then do not take the
// comment of the module.

This could be more precise. Suggested improvement:

// If the first statement starts at the same position as the module/script,
// skip emitting leading comments here. The first statement will emit them
// instead to ensure comments appear after any injected imports (which have DUMMY_SP).

3. Testing Recommendation

While the test updates look good, consider adding explicit unit tests in crates/swc_ecma_codegen/src/tests.rs that specifically test:

• Empty module with leading comments
• Module with injected imports (DUMMY_SP) and leading comments
• Module where first statement has same span as module
• Module where first statement has different span than module

4. Performance Consideration (Minor)

The code calls .first() and .span() on each emission. For performance-critical paths, consider whether these lookups could be cached, though this is likely negligible given the codegen phase occurs once.

5. Documentation

Consider adding a docstring to the Module::emit and Script::emit implementations explaining the comment emission strategy, as this is non-obvious behavior that future maintainers need to understand.

---

🔍 Code Quality Assessment

Best Practices Adherence:

• ✅ Uses Option::map idiomatically
• ✅ Proper error handling with Result
• ✅ Formatted according to project standards
• ✅ No unstable features used
• ✅ Comments in English

Performance:

• ✅ No unnecessary allocations
• ✅ Efficient span comparison
• ✅ Minimal overhead added to emission path

Security:

• ✅ No security concerns identified
• ✅ Safe Rust code with no unsafe blocks added

---

📋 Recommendation

Overall: Approve with minor suggestions

This is a solid fix that addresses a real issue. The implementation is clean and the test coverage is comprehensive. The suggested improvements are minor and mostly about code clarity and edge case handling.

Action Items (Optional):

1. Consider the BytePos(0) edge case and add a comment explaining why it's safe (or add handling)
2. Improve the inline comment to better explain the emission strategy
3. Consider adding explicit unit tests for this behavior

Great work on fixing this issue! The approach of checking span positions to determine comment ownership is the right solution.

---

Reviewed by: Claude Code
Review Date: 2025-11-19

[claude]

Code Review for PR #11314: fix(es/codegen): Emit comments of all nodes

Summary

This PR fixes an issue where leading comments on modules/scripts were not being emitted when they should be. The fix ensures comments appear at the correct location - particularly important when helper imports are injected.

---

✅ Positives

1. Clear Problem & Solution: The fix addresses a real issue where file-level comments (like copyright headers) were being lost or placed incorrectly when SWC injected helper imports.

2. Simple & Focused Logic: The solution uses a straightforward check - if the first statement's span matches the module/script span, skip emitting comments at the module level (they'll be emitted by the first statement). Otherwise, emit them.

3. Comprehensive Test Coverage: The PR includes 343 additions across many test files, showing that the change correctly updates expected outputs for various scenarios.

4. Consistent Implementation: The same logic is applied to both Module and Script implementations, maintaining consistency.

---

🔍 Code Quality Observations

Good Practices

• Performance: Uses .iter().any() which short-circuits on first match - efficient for the common case ✓
• Clarity: Variable name should_skip_leading_comments is descriptive ✓
• Comments: Updated comments explain the logic, though could be slightly clearer (see suggestions below)

Minor Concerns

1. Performance Consideration (Minor)

let should_skip_leading_comments = self.body.iter().any(|s| s.span().lo == self.span.lo);

• This iterates through statements each time, but in practice the check usually only needs the first element
• Since you're checking if ANY statement matches, but in the comment logic you seem to care about the FIRST statement specifically
• Suggestion: Consider making the intent clearer:

let should_skip_leading_comments = self.body.first()
    .map(|s| s.span().lo == self.span.lo)
    .unwrap_or(false);

This is more explicit that you're checking the first statement, matches your comment better, and avoids unnecessary iteration.

2. Edge Cases
The logic assumes that if the first statement has the same lo position as the module, it will emit the comments. This seems correct, but consider:

• What if helper imports are injected AFTER parsing but BEFORE codegen? (The current logic should handle this correctly)
• What if there are multiple statements with the same lo? (The .any() approach would catch this, but it's unclear if this is intentional or just defensive)

3. Comment Clarity
Current comments in lines 1434-1438:

// If the first statement and module span are the same, then do not take the
// comment of the module.

Suggested improvement:

// Skip emitting leading comments at module level if the first statement starts
// at the same position - that statement will emit them instead. This ensures
// comments appear after any injected imports (e.g., helper imports) rather than
// before them.

---

🔒 Security Considerations

✅ No security concerns identified. This is a pure codegen/formatting change.

---

🐛 Potential Bugs

✅ No bugs identified. The logic appears sound for the intended use case.

---

📊 Performance Analysis

✅ Overall: Minimal performance impact

• The .iter().any() call is O(n) worst case, but typically O(1) since it checks the first element
• Called once per module/script during codegen
• Consider the optimization suggestion above for clarity and guaranteed O(1) performance

---

🧪 Test Coverage

✅ Excellent: 343 line changes across multiple test fixtures demonstrate thorough validation:

• Copyright/license headers
• Korean comments (internationalization)
• JSDoc comments
• Various module formats (AMD, CommonJS, SystemJS, UMD)
• TypeScript reference files

All test changes show comments now appearing in the correct location.

---

📋 Adherence to CLAUDE.md Guidelines

Based on /home/runner/work/swc/swc/CLAUDE.md:

1. ✅ Performance: The code is performant (minor optimization possible as noted)
2. ✅ Comments in English: All comments are in English
3. ✅ No nightly features: Uses stable Rust
4. N/A Atom instances: Not applicable to this change
5. ⚠️ Unit tests: Test references updated, but no new unit test added for the specific logic
6. ✅ Test integrity: Existing tests were updated, not removed or modified
7. ⚠️ Documentation: Inline comments present but could be clearer
8. ⚠️ cargo fmt: Should be run before commit (per guidelines)

---

💡 Recommendations

High Priority:

1. Run cargo fmt --all before merging (per CLAUDE.md requirement Unignore "431ecef8c85d4d24.js" and "8386fbff927a9e0e.js". #8)

Medium Priority:
2. Consider refactoring to use .first() instead of .any() for clarity:

let should_skip_leading_comments = self.body.first()
    .map(|s| s.span().lo == self.span.lo)
    .unwrap_or(false);

Low Priority:
3. Improve inline comment clarity (suggested above)
4. Consider adding a specific unit test for this edge case behavior

---

✅ Conclusion

Overall Assessment: ✅ Approve with minor suggestions

This is a solid fix for a real issue. The implementation is clean, focused, and well-tested. The suggestions above are minor improvements that would enhance code clarity and maintainability but don't block merging.

The test updates comprehensively demonstrate the fix works correctly across many scenarios, which is excellent.

Action Items Before Merge:

• Run cargo fmt --all (required by CLAUDE.md)
• Consider the .first() optimization for clarity

Great work! 🎉

[Copilot]

Pull Request Overview

This PR fixes a bug in the SWC code generator where leading comments of Module and Script nodes were not being emitted correctly. The previous logic only emitted comments for empty modules/scripts, causing comments to be lost when code was present. The new logic checks whether any body statement starts at the same span position as the module/script itself, and only skips leading comment emission in that case.

Key changes:

• Updated comment emission logic for both Module and Script implementations
• Comments are now emitted unless a body statement shares the exact same starting position
• Numerous test files updated to reflect the corrected comment output

Reviewed Changes

Copilot reviewed 182 out of 188 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
crates/swc_ecma_codegen/src/lib.rs	Updated Module and Script implementations to fix leading comment emission logic
crates/swc/tests/tsc-references/*.js	Test output files updated to show correctly emitted comments
crates/swc/tests/fixture/**/*.ts	Test fixture outputs updated with correctly positioned comments
crates/swc/tests/fixture/**/*.map	Source map files updated to reflect new comment positions
.changeset/stupid-numbers-hang.md	Changeset documenting the fix

---

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

[github-actions]

Binary Sizes

File	Size
swc.linux-x64-gnu.node	31M (31957832 bytes)

Commit: 526f588

[codspeed-hq]

CodSpeed Performance Report

Merging #11314 will not alter performance

_{Comparing kdy1/cmt-real-fix (02981bc) with main (7a528ce)}

Summary

✅ 138 untouched

[claude]

Code Review

I've reviewed PR #11314 which fixes comment emission in the codegen. Here's my analysis:

Summary

This PR fixes an issue where comments at the beginning of files were being lost during code generation. The fix changes how Module and Script nodes handle leading comments by checking if any statement starts at the same position as the module/script itself.

Code Quality ✅

Strengths:

1. Clear logic: The fix uses a simple and readable condition: self.body.iter().any(|s| s.span().lo == self.span.lo)
2. Consistent implementation: The same fix is applied to both Module and Script implementations
3. Improved comments: The new approach is more intuitive than the previous "only emit for empty bodies" logic

Suggestions:

1. Consider adding inline documentation: The condition s.span().lo == self.span.lo could benefit from a comment explaining that this checks if a statement starts at the same position as the parent, indicating it "owns" those comments

   // Skip leading comments if a statement starts at the same position as the module,
   // meaning the statement will emit them instead
   let should_skip_leading_comments = self.body.iter().any(|s| s.span().lo == self.span.lo);

Potential Issues ❌

1. Performance concern (minor): The .iter().any() call checks every statement in the body. For large modules, this could be optimized to check only the first statement:

   let should_skip_leading_comments = self.body.first()
       .map_or(false, |s| s.span().lo == self.span.lo);

   This would be more efficient as comments at the file start would only collide with the first statement.

2. Edge case consideration: What happens if multiple statements have the same span().lo? This seems unlikely but worth considering for unusual AST structures.

Performance ⚡

The current implementation is acceptable but see optimization suggestion above. The .any() short-circuits on the first match, which helps, but checking only the first element would be more idiomatic for this use case.

Security 🔒

No security concerns identified. This is a pure code generation change.

Test Coverage ✅

Excellent test coverage! The PR includes:

• 219 test files updated showing the fix working correctly
• Tests cover various scenarios: copyright headers, license comments, JSDoc comments
• Multiple module formats tested (AMD, CommonJS, SystemJS, UMD)
• Real-world examples from issues Issue with accessing private properties #1333, "Unterminated string constant" error occurs when using @swc/jest in jest #1345, Output mjs file extension #3067, panic: re-export alias #3782, etc.

Adherence to CLAUDE.md Guidelines ✅

1. ✅ Performance: Code is performant (minor optimization possible)
2. ✅ Comments: Code could use slightly better inline documentation
3. ✅ No unstable features: Uses stable Rust only
4. ✅ Tests included: Comprehensive test coverage via snapshot updates
5. ✅ Formatting: Code appears properly formatted

Recommendations

Before merging:

1. Consider the performance optimization (checking only first statement)
2. Add a brief inline comment explaining the span().lo comparison logic
3. Ensure cargo fmt --all has been run (per project guidelines)

Overall Assessment: ✅ APPROVE with minor suggestions

This is a solid fix that addresses the root cause. The extensive test coverage gives confidence the change works correctly across many scenarios. The suggested optimizations are nice-to-haves but not blocking issues.