fix(es/codegen): Restore missing top-level comments #11302
Conversation
This fixes an issue where top-level comments (like copyright headers) were being lost after PR #11199 changed Module/Script spans to start at position 0. The problem was that Module::emit() was taking and emitting all leading comments before any body items were processed. When helper imports were injected at the beginning, comments would either appear before them (wrong) or be lost entirely. The fix is to skip emitting leading comments at Module/Script level when the body is non-empty. Instead, comments are emitted by the first body item with a real source span. Injected imports have DUMMY_SP, so they skip comment emission, and the original source items emit their comments correctly after the imports. This ensures: - Comments appear after injected imports (maintains fix for #11167) - Comments are not lost (fixes regression from PR #11199) - Module span still starts at position 0 (maintains PR #11199 changes) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Donny/강동윤 <[email protected]>
🦋 Changeset detectedLatest commit: 4c3127a 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 |
Code ReviewSummaryThis PR fixes an important issue where top-level comments (like copyright headers and JSDoc annotations) were being incorrectly positioned on the same line as injected imports. The fix is elegant and well-thought-out. Positive Aspects ✅
Observations & Considerations 🔍
Security & Correctness 🔒
Recommendations 📋
ConclusionThis is a high-quality fix that correctly addresses the reported issue. The approach is sound, the code is well-documented, and edge cases are handled. The fix properly restores the expected behavior where top-level comments appear after helper imports but before actual source code. Recommendation: ✅ Approve - This PR is ready to merge. 🤖 Generated with Claude Code |
Binary Sizes
Commit: 4a5dc91 |
CodSpeed Performance ReportMerging #11302 will not alter performanceComparing Summary
Footnotes |
|
|
Code Review - PR #11302SummaryThis PR fixes a critical regression where top-level comments (like copyright headers) were being lost after PR #11199 changed Module/Script spans to start at position 0. The fix is elegant and surgically targeted, restoring the lost comments without reverting the original fix. ✅ Strengths
🔍 Areas for Consideration1. Edge Case: Empty Modules/Scripts✅ Already Handled: The code correctly handles empty modules/scripts by still emitting leading comments in those cases (lines 1438-1441 and 1470-1473). This prevents comment loss when there are no body items to emit them. 2. Performance Considerations (Per CLAUDE.md #1: "Write performant code")✅ Good: The change actually improves performance slightly by reducing unnecessary work - we skip 3. Test File Removal
Recommendation: Consider keeping or recreating this test fixture to ensure the original issue #11167 doesn't regress. The test should verify that the 4. Code Quality (Per CLAUDE.md #7: "Write documentation for your code")✅ Excellent: The comments clearly explain the logic and edge cases. The code is self-documenting with clear conditional structure. 🔒 Security Considerations✅ No Security Concerns: The change is purely about comment placement in code generation. It doesn't introduce any security vulnerabilities. 🧪 Test Coverage AssessmentStrengths:
Recommendation:
Example test case: // Input
/* @ngInject */
class TestClass {
constructor (private testField) {}
}
// Expected Output (with externalHelpers: true)
import { _ as _define_property } from "@swc/helpers/_/_define_property";
/* @ngInject */
class TestClass { ... }📊 Code Metrics
Final Recommendation✅ APPROVE with minor suggestion This is a high-quality fix that elegantly solves the regression without introducing new issues. The solution is:
Suggested Action Before Merge: Excellent Work! 🎉The diagnostic work to identify the root cause and the elegant solution demonstrate strong understanding of the codebase. The fix maintains both the original #11167 fix AND restores the lost comments - a perfect balance. |
Code ReviewSummaryThis PR fixes an issue where top-level comments (like copyright headers) were being lost after PR #11199 changed Module/Script spans to start at position 0. The fix ensures comments appear after injected imports rather than being lost or appearing in the wrong position. Architecture & Approach ✅The fix uses a clever approach:
This is a clean solution that handles the edge cases properly. Code Quality ✅Strengths:
Minor observations:
Testing Coverage ✅Excellent test coverage:
Performance Considerations ✅Performance impact is minimal to positive:
Per CLAUDE.md guideline #1 (prefer performance), this change actually improves performance by removing unnecessary comment manipulation in the minifier. Security Concerns ✅No security concerns identified:
Potential Issues & SuggestionsMinor concerns:
Best Practices Alignment ✅Follows CLAUDE.md guidelines:
Recommendation✅ APPROVE with minor suggestions This is a well-thought-out fix that:
The approach of delegating comment emission to the first body item is elegant and handles edge cases properly. Suggested improvements (optional):
Great work! 🚀 |
Pull Request Review: Fix for Top-Level CommentsSummaryThis PR successfully fixes issue #11167 where top-level comments (like copyright headers and JSDoc annotations) were being lost or misplaced after PR #11199 changed Module/Script spans to start at position 0. ✅ Strengths1. Correct Root Cause AnalysisThe fix correctly identifies that the issue stems from Module/Script spans starting at position 0, which caused leading comments to be emitted before injected imports rather than after them. The solution elegantly skips comment emission at the Module/Script level and lets the first body item with a real span handle it. 2. Well-Documented CodeThe comments in // Don't emit leading comments at Module level when the module has a body.
// This ensures that comments appear after any injected imports (e.g., helper
// imports) rather than before them. The first body item with a real
// span will emit the comments.3. Edge Case HandlingThe code properly handles empty modules/scripts by emitting leading comments only when the body is empty (lines 1438-1440, 1470-1472), ensuring comments aren't lost in edge cases. 4. Comprehensive Test Coverage
5. Cleanup of Technical DebtThe removal of comment position tracking code in
6. Parser SimplificationRemoval of the
🔍 Areas for Consideration1. Edge Case: DUMMY_SP Items OnlyThe current implementation assumes that injected items (with DUMMY_SP) will be followed by original source items with real spans. Consider this scenario: // Copyright header
// [injected imports with DUMMY_SP]
// [more injected code with DUMMY_SP]
// EOF - no original source itemsQuestion: What happens if a module has leading comments but all body items have DUMMY_SP? The comments might be lost since no item will emit them. Recommendation: Consider adding a check or documentation about this case. 2. Performance ConsiderationSince comments are now emitted by the first body item, there's a minor performance consideration: every body item emission might check for leading comments even though only the first one typically has them. However, given that this is already the pattern throughout the codebase (seen in grep results), this is acceptable. 3. Test Coverage for the Specific BugWhile there are 3500+ lines of test updates, I don't see a specific test case that directly reproduces the bug from issue #11167 (JSDoc comment with external helpers). Recommendation: Consider adding a focused regression test like: // Input: test case from issue #11167
/** @ngInject */
class TestClass {
constructor (private testField) {}
}
// Expected output: import should come first, then comment, then class4. Documentation UpdateConsider updating any architecture documentation or design docs that might reference the old comment handling approach, especially around Module/Script span handling. 🔒 Security Considerations✅ No security concerns identified. The changes are purely related to comment positioning in code generation. ⚡ Performance Considerations✅ Performance improvement: The removal of comment tracking logic in the minifier (~25 lines) reduces complexity and potential overhead. The new approach is more efficient as it avoids post-processing comment relocation. 🐛 Potential IssuesNone identified. The approach is sound and well-implemented. 📋 Code Quality Checklist
🎯 Final RecommendationAPPROVE - This is a well-thought-out fix that correctly addresses the root cause of the issue. The simplification of the minifier code by removing complex comment tracking logic is a significant improvement. The only minor suggestion is to add a specific regression test for the reported issue. Great work on this fix! The approach of delegating comment emission to body items is elegant and maintainable. |
Restore missing top-level comments in code generation.
Pull Request Review - Comprehensive AnalysisExecutive Summary✅ APPROVED - This is a high-quality fix that correctly addresses issue #11167 where top-level comments (copyright headers, JSDoc) were being lost after PR #11199. The solution is elegant, well-documented, and includes comprehensive test coverage. Technical AnalysisRoot Cause & Solution ✅The PR correctly identifies and fixes the issue:
Code Quality ✅Strengths:
Alignment with CLAUDE.md:
Test Coverage ✅Excellent coverage:
Performance Considerations ✅Performance improvement:
Security & Correctness 🔒
Areas for Consideration1. Edge Case: All Items with DUMMY_SPQuestion: What happens if a module has leading comments but ALL body items have DUMMY_SP (no original source)? Current behavior: Comments would not be emitted by any item. Assessment: This is likely a theoretical edge case - in practice, if there's source code with comments, there will be at least one item with a real span. However, worth documenting. 2. Regression TestObservation: While there are 3,500+ test updates, there isn't a specific standalone test reproducing the exact scenario from issue #11167. Suggestion (Optional): Add a focused regression test: // Input
/* @ngInject */
class TestClass {
constructor(private field) {}
}
// Expected: helper import first, then comment, then classCode Review DetailsParser Changes (
|
Pull Request ReviewSummaryThis PR addresses a regression where top-level comments (like copyright headers) were being lost after PR #11199 changed Module/Script spans to start at position 0. The approach is to skip emitting leading comments at the Module/Script level when the body is non-empty, allowing the first body item with a real span to emit them after any injected imports (which have DUMMY_SP). ✅ Strengths
🔍 Code Quality Observationscrates/swc_ecma_codegen/src/lib.rsThe changes to // Don't emit leading comments at Module level when the module has a body.
// This ensures that comments appear after any injected imports (e.g., helper
// imports) rather than before them. The first body item with a real
// span will emit the comments.
if self.body.is_empty() {
// For empty modules, emit leading comments to ensure they're not lost
emitter.emit_leading_comments_of_span(self.span(), false)?;
srcmap!(emitter, self, true);
}✅ Positive: The empty body case is properly handled to prevent comment loss. crates/swc_ecma_minifier/src/lib.rsThe removal of comment position tracking code is significant: - let first_span = match &n {
- Program::Module(module) => module.body.first().map(|s| s.span()),
- Program::Script(script) => script.body.first().map(|s| s.span()),
- };
- // ... 20+ lines of comment relocation logic removed✅ Positive: Simplifies the minifier by removing workaround code that's no longer needed. crates/swc_ecma_parser/src/parser/mod.rs & input.rs- let start = self.input.start;
+ let start = self.cur_pos();This reverts the PR #11199 changes. The revert is justified since the new codegen approach handles comment positioning correctly without needing Module spans to start at position 0. 🎯 Performance ConsiderationsPer CLAUDE.md requirement #1 ("Write performant code. Always prefer performance over other things."): ✅ Positive: The changes actually improve performance by:
The new approach is zero-cost: comments are emitted naturally during the first body item traversal. 🐛 Potential Issues & Questions1. Edge Case: All Body Items Have DUMMY_SPScenario: What if ALL body items have Looking at the code, emitter.emit_leading_comments_of_span(self.span(), false)?;If Recommendation: Add a test case for a file that becomes empty or fully synthetic after transformation to ensure comments aren't lost. 2. Test Coverage for Empty ModulesThe changeset shows several tests lost comments: -// Test that passing noLib disables <reference lib> resolution.
"use strict";Question: Are these intentional? For the Recommendation: Review tests where comments were removed (not just moved) to ensure they're expected behavior changes. 3. Parser Span ChangesThe PR reverts PR #11199's parser changes. This is a significant architectural decision. Consideration: The revert means:
Recommendation: Document this span semantic in parser docs if not already done. 🔒 Security Concerns✅ No security issues identified. The changes are purely about comment positioning in generated code, which doesn't affect runtime behavior or introduce vulnerabilities. 📋 Test Coverage AssessmentExcellent: The PR updates 3,460 test files, covering:
Recommendation: Consider adding specific test cases for:
📝 Documentation & StylePer CLAUDE.md requirements: ✅ Requirement #2: Comments and documentation in English ✓ Action Required: Please run: cargo fmt --all🎨 Best Practices
🏁 ConclusionOverall Assessment: ⭐⭐⭐⭐ (4/5) Recommendation: Approve with minor follow-up Strengths:
Before Merge:
Post-Merge:
📚 References
Great work on the thorough investigation and fix! The approach is sound and well-tested. |
|
Seems like this PR would still make some comments missing like in benchmark/{lodash, moment}.js? @kdy1 |
|
I'll try another fix |
This fixes an issue where top-level comments (like copyright headers) were being lost after PR #11199 changed Module/Script spans to start at position 0.
The fix skips emitting leading comments at Module/Script level when the body is non-empty. Instead, comments are emitted by the first body item with a real source span. Injected imports have DUMMY_SP, so they skip comment emission, and original source items emit their comments correctly after the imports.
Closes #11167
🤖 Generated with Claude Code