-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
[preserveFormat] force semicolons when invalidating ASI #16958
[preserveFormat] force semicolons when invalidating ASI #16958
Conversation
|
||
this._maybeAddAuxComment(); | ||
this._appendChar(char); | ||
this._appendChar(char, () => { |
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 change has also the effect that we don't inject the space anymore if it's made unnecessary by the aux comment (e.g. 34/* aux comment*/.foo
), because now we check if we need a space right before appending the new char.
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/58343 |
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.
Uh... Benchmarks show that both our main and this PR have performance regressions.
The performance regression in this PR should come from closures, and the following patch can avoid it.
I will continue to investigate the cause of the regression in main.
PR
PS F:\babel\benchmark> node --expose-gc .\babel-generator\real-case\jquery.mjs
current 1 jquery 3.6: 143 ops/sec ±1.21% (7.017ms)
current 4 jquery 3.6: 32.81 ops/sec ±0.62% (30ms)
current 16 jquery 3.6: 8.29 ops/sec ±0.5% (121ms)
current 64 jquery 3.6: 2.02 ops/sec ±1.5% (496ms)
baseline 1 jquery 3.6: 162 ops/sec ±0.61% (6.17ms)
baseline 4 jquery 3.6: 36.94 ops/sec ±0.62% (27ms)
baseline 16 jquery 3.6: 9.27 ops/sec ±0.5% (108ms)
baseline 64 jquery 3.6: 2.19 ops/sec ±6.33% (456ms)
main
PS F:\babel\benchmark> node --expose-gc .\babel-generator\real-case\jquery.mjs
current 1 jquery 3.6: 151 ops/sec ±2.81% (6.614ms)
current 4 jquery 3.6: 35.75 ops/sec ±0.5% (28ms)
current 16 jquery 3.6: 8.93 ops/sec ±0.7% (112ms)
current 64 jquery 3.6: 2.09 ops/sec ±6.15% (478ms)
baseline 1 jquery 3.6: 162 ops/sec ±0.75% (6.17ms)
baseline 4 jquery 3.6: 37.09 ops/sec ±0.71% (27ms)
baseline 16 jquery 3.6: 9.29 ops/sec ±0.78% (108ms)
baseline 64 jquery 3.6: 2.26 ops/sec ±0.93% (443ms)
diff --git a/packages/babel-generator/src/printer.ts b/packages/babel-generator/src/printer.ts
index e316910249..5e378f64aa 100644
--- a/packages/babel-generator/src/printer.ts
+++ b/packages/babel-generator/src/printer.ts
@@ -308,15 +308,17 @@ class Printer {
this._maybePrintInnerComments(str);
this._maybeAddAuxComment();
- this._append(
- str,
- false,
- undefined,
- // prevent concatenating words and creating // comment out of division and regex
- () =>
- this._endsWithWord ||
- (this._endsWithDiv && str.charCodeAt(0) === charCodes.slash),
- );
+
+ if (this.tokenMap) this._catchUpToCurrentToken(str);
+
+ // prevent concatenating words and creating // comment out of division and regex
+ if (
+ this._endsWithWord ||
+ (this._endsWithDiv && str.charCodeAt(0) === charCodes.slash)
+ ) {
+ this._space();
+ }
+ this._append(str, false);
this._endsWithWord = true;
this._noLineTerminator = noLineTerminatorAfter;
@@ -368,24 +370,27 @@ class Printer {
this._maybePrintInnerComments(str, occurrenceCount);
this._maybeAddAuxComment();
- this._append(str, maybeNewline, occurrenceCount, () => {
- const lastChar = this.getLastChar();
- const strFirst = str.charCodeAt(0);
-
- return (
- (lastChar === charCodes.exclamationMark &&
- // space is mandatory to avoid outputting <!--
- // http://javascript.spec.whatwg.org/#comment-syntax
- (str === "--" ||
- // Needs spaces to avoid changing a! == 0 to a!== 0
- strFirst === charCodes.equalsTo)) ||
- // Need spaces for operators of the same kind to avoid: `a+++b`
- (strFirst === charCodes.plusSign && lastChar === charCodes.plusSign) ||
- (strFirst === charCodes.dash && lastChar === charCodes.dash) ||
- // Needs spaces to avoid changing '34' to '34.', which would still be a valid number.
- (strFirst === charCodes.dot && this._endsWithInteger)
- );
- });
+
+ if (this.tokenMap) this._catchUpToCurrentToken(str, occurrenceCount);
+
+ const lastChar = this.getLastChar();
+ const strFirst = str.charCodeAt(0);
+ if (
+ (lastChar === charCodes.exclamationMark &&
+ // space is mandatory to avoid outputting <!--
+ // http://javascript.spec.whatwg.org/#comment-syntax
+ (str === "--" ||
+ // Needs spaces to avoid changing a! == 0 to a!== 0
+ strFirst === charCodes.equalsTo)) ||
+ // Need spaces for operators of the same kind to avoid: `a+++b`
+ (strFirst === charCodes.plusSign && lastChar === charCodes.plusSign) ||
+ (strFirst === charCodes.dash && lastChar === charCodes.dash) ||
+ // Needs spaces to avoid changing '34' to '34.', which would still be a valid number.
+ (strFirst === charCodes.dot && this._endsWithInteger)
+ ) {
+ this._space();
+ }
+ this._append(str, maybeNewline);
this._noLineTerminator = false;
}
@@ -396,16 +401,20 @@ class Printer {
this._maybePrintInnerComments(str);
this._maybeAddAuxComment();
- this._appendChar(char, () => {
- const lastChar = this.getLastChar();
- return (
- // Need spaces for operators of the same kind to avoid: `a+++b`
- (char === charCodes.plusSign && lastChar === charCodes.plusSign) ||
- (char === charCodes.dash && lastChar === charCodes.dash) ||
- // Needs spaces to avoid changing '34' to '34.', which would still be a valid number.
- (char === charCodes.dot && this._endsWithInteger)
- );
- });
+
+ if (this.tokenMap) this._catchUpToCurrentToken(str);
+
+ const lastChar = this.getLastChar();
+ if (
+ // Need spaces for operators of the same kind to avoid: `a+++b`
+ (char === charCodes.plusSign && lastChar === charCodes.plusSign) ||
+ (char === charCodes.dash && lastChar === charCodes.dash) ||
+ // Needs spaces to avoid changing '34' to '34.', which would still be a valid number.
+ (char === charCodes.dot && this._endsWithInteger)
+ ) {
+ this._space();
+ }
+ this._appendChar(char);
this._noLineTerminator = false;
}
@@ -524,15 +533,7 @@ class Printer {
this._printSemicolonBeforeNextNode = -1;
}
- _append(
- str: string,
- maybeNewline: boolean,
- occurrenceCount: number = 0,
- needsSpace?: () => boolean,
- ): void {
- if (this.tokenMap) this._catchUpToCurrentToken(str, occurrenceCount);
- if (needsSpace?.()) this._space();
-
+ _append(str: string, maybeNewline: boolean): void {
this._maybeIndent(str.charCodeAt(0));
this._buf.append(str, maybeNewline);
@@ -543,10 +544,7 @@ class Printer {
this._endsWithDiv = false;
}
- _appendChar(char: number, needsSpace?: () => boolean): void {
- if (this.tokenMap) this._catchUpToCurrentToken(String.fromCharCode(char));
- if (needsSpace?.()) this._space();
-
+ _appendChar(char: number): void {
this._maybeIndent(char);
this._buf.appendChar(char);
Co-authored-by: liuxingbaoyu <[email protected]>
Thanks for the patch, applied :) |
Fixes #1, Fixes #2
See the first commit for the current wrong output :)