Skip to content

Conversation

nicolo-ribaudo
Copy link
Member

Q                       A
Fixed Issues? Fixes #1, Fixes #2
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

See the first commit for the current wrong output :)

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: generator labels Nov 8, 2024

this._maybeAddAuxComment();
this._appendChar(char);
this._appendChar(char, () => {
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Nov 8, 2024

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.

@babel-bot
Copy link
Collaborator

babel-bot commented Nov 8, 2024

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/58343

Copy link
Member

@liuxingbaoyu liuxingbaoyu left a 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 <30521560+liuxingbaoyu@users.noreply.github.com>
@nicolo-ribaudo
Copy link
Member Author

Thanks for the patch, applied :)

@nicolo-ribaudo nicolo-ribaudo merged commit 0613b92 into babel:main Nov 21, 2024
54 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the preserve-format-asi-bug branch November 21, 2024 22:39
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 21, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants