Skip to content
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

Merged
merged 3 commits into from
Nov 21, 2024

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 <[email protected]>
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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