-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
mod(es/parser): Program start at input start #11199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses an issue where the parser incorrectly positioned the program start, causing leading comments and JSDoc annotations to be misplaced after import statements in the generated output. The fix ensures that programs start at the actual beginning of the input, preserving the correct order of comments and code.
Key changes:
- Modified parser to correctly track program start position at input beginning
- Fixed comment positioning for files with leading comments before imports/code
- Updated test fixtures to reflect correct comment placement
Reviewed Changes
Copilot reviewed 293 out of 644 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
Multiple test reference files (.1.normal.js) |
Updated test outputs showing comments now correctly appear before imports instead of after |
| Test fixture output files | Fixed comment positioning in various scenarios including JSDoc, copyright notices, and inline comments |
New test case files (crates/swc/tests/fixture/issues-110xx/11167/) |
Added new test case demonstrating the fix for issue #11167 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Some investigation For code // 123
class A extends B {
}Current swc would output // 123
import { _ as _call_super } from "@swc/helpers/_/_call_super";
import { _ as _class_call_check } from "@swc/helpers/_/_class_call_check";
import { _ as _inherits } from "@swc/helpers/_/_inherits";
var A = /*#__PURE__*/ function(B1) {
"use strict";
_inherits(A, B1);
function A() {
_class_call_check(this, A);
return _call_super(this, A, arguments);
}
return A;
}(B);tsc would output "use strict";
var __extends = (this && this.__extends) || (function () {
var extendStatics = function (d, b) {
extendStatics = Object.setPrototypeOf ||
({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
function (d, b) { for (var p in b) if (Object.prototype.hasOwnProperty.call(b, p)) d[p] = b[p]; };
return extendStatics(d, b);
};
return function (d, b) {
if (typeof b !== "function" && b !== null)
throw new TypeError("Class extends value " + String(b) + " is not a constructor or null");
extendStatics(d, b);
function __() { this.constructor = d; }
d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
})();
// 123
var A = /** @class */ (function (_super) {
__extends(A, _super);
function A() {
return _super !== null && _super.apply(this, arguments) || this;
}
return A;
}(B));babel would output function _callSuper(t, o, e) { return o = babelHelpers.getPrototypeOf(o), babelHelpers.possibleConstructorReturn(t, _isNativeReflectConstruct() ? Reflect.construct(o, e || [], babelHelpers.getPrototypeOf(t).constructor) : o.apply(t, e)); }
function _isNativeReflectConstruct() { try { var t = !Boolean.prototype.valueOf.call(Reflect.construct(Boolean, [], function () {})); } catch (t) {} return (_isNativeReflectConstruct = function _isNativeReflectConstruct() { return !!t; })(); }
// 123
var A = /*#__PURE__*/function (_B) {
function A() {
babelHelpers.classCallCheck(this, A);
return _callSuper(this, A, arguments);
}
babelHelpers.inherits(A, _B);
return babelHelpers.createClass(A);
}(B);This PR would change to babel output. |
|
Some comments would be lost because they're now attached to typescript syntax node which would be eliminated later. |
CodSpeed Performance ReportMerging #11199 will not alter performanceComparing Summary
Footnotes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 293 out of 4003 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 293 out of 4007 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Binary Sizes
Commit: aedecfe |
|
CI failures look like a real failure, but I think it may be better to wait until I finish #11205 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 293 out of 4021 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 293 out of 4022 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* | ||
| @internal | ||
| */ declare class StripInternalClassFields { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be removed.
|
I started reviewing this. Sorry for the late review. |
| @@ -1,11 +1,4 @@ | |||
| /** | |||
| * @license | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is expected. I think we need to preserve some kinds of comments, and @license is one of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens because in original code
/**
* @license
* Lodash <https://lodash.com/>
* Copyright OpenJS Foundation and other contributors <https://openjsf.org/>
* Released under MIT license <https://lodash.com/license>
* Based on Underscore.js 1.8.3 <http://underscorejs.org/LICENSE>
* Copyright Jeremy Ashkenas, DocumentCloud and Investigative Reporters & Editors
*/
;(function() {It's considered as an leading comment for EmptyStatement, but that EmptyStatement is removed during minify. It's expected in the sense that this is how this PR should work, but we still need to get the comment back somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar case for tsc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may need to fix the comment removal of minifier first (I guess? Not sure)
|
@claude Review this PR |
|
Claude finished @kdy1's task —— View job Code Review: PR #11199 - Program start at input start🎯 SummaryThis PR fixes a critical parser positioning issue where leading comments (JSDoc, copyright notices, etc.) were incorrectly placed after import statements instead of before them in compiled output. The fix ensures that AST program spans start at the actual beginning of input rather than the first token position. 🔍 Core Changes AnalysisParser Changes (crates/swc_ecma_parser/src/parser/)Key Fix in // Before: Used current token position
let start = self.cur_pos();
// After: Use actual input start position
let start = self.input.start;Buffer Enhancement in
Impact Assessment✅ Positive Changes:
1. License Comment Removal in MinifierIn // Before:
/**
* @license
* Lodash <https://lodash.com/>
* ...
*/
(function() { ... });
// After:
(function() { ... }); // License comment missingRoot Cause: The license comment was attached to an 2. Massive Test Surface Impact
🏗️ Architecture ReviewDesign Quality: ⭐⭐⭐⭐⭐ The fix is architecturally sound:
Test Coverage: ⭐⭐⭐⭐⭐
🐛 Issues Requiring AttentionCritical: Minifier Comment LossThe lodash.js case reveals a systematic issue where license/copyright comments attached to removed AST nodes are lost. This needs addressing because:
Recommendation: Implement comment preservation logic in the minifier that:
🎯 Overall AssessmentScore: 8.5/10 Strengths:
Areas for Improvement:
🚀 RecommendationAPPROVE with follow-up work required This is a high-quality fix that addresses a legitimate parsing issue. The core change is correct and well-implemented. However, the minifier comment loss issue should be addressed in a follow-up PR to ensure license compliance. Suggested Follow-up Tasks:
Generated by Claude Code • |
Description:
BREAKING CHANGE:
Related issue (if exists):
Closes #11167