Skip to content

🐛 Fix not emitting comments between sibling fields of object literals #50097

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Sep 15, 2023
Merged

🐛 Fix not emitting comments between sibling fields of object literals #50097

merged 8 commits into from
Sep 15, 2023

Conversation

babakks
Copy link
Contributor

@babakks babakks commented Jul 29, 2022

Fixes #49784

babakks added 4 commits July 29, 2022 17:39
@babakks
🐛 Fix not emitting comments between siblings with multiline preference
420efab 
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks
⚗️ Add emitter test case for comment in object literals
4335759 
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks
⚗️ Add reference baselines for commentsOnObjectLiteral5.ts
282094e 
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks
🔨 Remove unnecessary check for trailing comments existence
7878696 
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jul 29, 2022
@babakks
Copy link
Contributor Author

babakks commented Jul 29, 2022

This approach breaks many other test cases. So, I close it until I've found a viable solution.

@babakks babakks closed this Jul 29, 2022
@babakks
🔨 Emit trailing comments on delimited lists
0e95234 
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks babakks reopened this Jul 30, 2022
babakks added 2 commits July 31, 2022 11:18
@babakks
🔨 Exclude synthesized nodes when emitting trailing comments at line b…
83ed2de 
…reaks

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>
@babakks
⚗️ Update reference baselines that had missing trailing comments
108a293 
Signed-off-by: Babak K. Shandiz <babak.k.shandiz@gmail.com>

diff --git a/tests/baselines/reference/arrowFunctionParsingDoesNotConfuseParenthesizedObjectForArrowHead.js b/tests/baselines/reference/arrowFunctionParsingDoesNotConfuseParenthesizedObjectForArrowHead.js
index fbd0136..48b168f 100644
--- a/tests/baselines/reference/arrowFunctionParsingDoesNotConfuseParenthesizedObjectForArrowHead.js
+++ b/tests/baselines/reference/arrowFunctionParsingDoesNotConfuseParenthesizedObjectForArrowHead.js
@@ -18,7 +18,7 @@ const test = () => ({
 //// [arrowFunctionParsingDoesNotConfuseParenthesizedObjectForArrowHead.js]
 var test = function () { return ({
     // "Identifier expected." error on "!" and two "Duplicate identifier '(Missing)'." errors on space.
-    prop: !value,
+    prop: !value, // remove ! to see that errors will be gone
     run: function () {
         // comment next line or remove "()" to see that errors will be gone
         if (!a.b()) {
diff --git a/tests/baselines/reference/assignmentCompatBug3.js b/tests/baselines/reference/assignmentCompatBug3.js
index ac79938..3476bcf 100644
--- a/tests/baselines/reference/assignmentCompatBug3.js
+++ b/tests/baselines/reference/assignmentCompatBug3.js
@@ -28,8 +28,8 @@ foo(x + y);
 //// [assignmentCompatBug3.js]
 function makePoint(x, y) {
     return {
-        get x() { return x; },
-        get y() { return y; },
+        get x() { return x; }, // shouldn't be "void"
+        get y() { return y; }, // shouldn't be "void"
         //x: "yo",
         //y: "boo",
         dist: function () {
diff --git a/tests/baselines/reference/bindingPatternCannotBeOnlyInferenceSource.js b/tests/baselines/reference/bindingPatternCannotBeOnlyInferenceSource.js
index 4f8e285..65f2ca9 100644
--- a/tests/baselines/reference/bindingPatternCannotBeOnlyInferenceSource.js
+++ b/tests/baselines/reference/bindingPatternCannotBeOnlyInferenceSource.js
@@ -43,7 +43,7 @@ var _d = useReduxDispatch1(function (d, f) { return ({
             p[_i] = arguments[_i];
         }
         return d(f.funcA.apply(f, p));
-    },
+    }, // p should be inferrable
     funcB: function () {
         var p = [];
         for (var _i = 0; _i < arguments.length; _i++) {
diff --git a/tests/baselines/reference/callSignaturesWithParameterInitializers2.js b/tests/baselines/reference/callSignaturesWithParameterInitializers2.js
index 10aaaf4..c7c845a 100644
--- a/tests/baselines/reference/callSignaturesWithParameterInitializers2.js
+++ b/tests/baselines/reference/callSignaturesWithParameterInitializers2.js
@@ -47,7 +47,7 @@ c.foo(1);
 var b = {
     foo: function (x) {
         if (x === void 0) { x = 1; }
-    },
+    }, // error
     foo: function (x) {
         if (x === void 0) { x = 1; }
     }
diff --git a/tests/baselines/reference/circularResolvedSignature.js b/tests/baselines/reference/circularResolvedSignature.js
index 79f5fe6..c08aabf 100644
--- a/tests/baselines/reference/circularResolvedSignature.js
+++ b/tests/baselines/reference/circularResolvedSignature.js
@@ -22,7 +22,7 @@ exports.__esModule = true;
 exports.Component = void 0;
 function Component() {
     var _a = useState(function () { return ({
-        value: "string",
+        value: "string", // this should be a number
         foo: function (arg) { return setState(arg); },
         bar: function (arg) { return setState(arg); }
     }); }), state = _a[0], setState = _a[1];
diff --git a/tests/baselines/reference/duplicateObjectLiteralProperty.js b/tests/baselines/reference/duplicateObjectLiteralProperty.js
index c8dc60d..5c134be 100644
--- a/tests/baselines/reference/duplicateObjectLiteralProperty.js
+++ b/tests/baselines/reference/duplicateObjectLiteralProperty.js
@@ -21,9 +21,9 @@ var y = {
 //// [duplicateObjectLiteralProperty.js]
 var x = {
     a: 1,
-    b: true,
-    a: 56,
-    \u0061: "ss",
+    b: true, // OK
+    a: 56, // Duplicate
+    \u0061: "ss", // Duplicate
     a: {
         c: 1,
         "c": 56, // Duplicate
diff --git a/tests/baselines/reference/excessPropertyCheckWithMultipleDiscriminants.js b/tests/baselines/reference/excessPropertyCheckWithMultipleDiscriminants.js
index a847e63..51cd6dc 100644
--- a/tests/baselines/reference/excessPropertyCheckWithMultipleDiscriminants.js
+++ b/tests/baselines/reference/excessPropertyCheckWithMultipleDiscriminants.js
@@ -65,7 +65,7 @@ const c: DisjointDiscriminants = {
 var foo = {
     type: "number",
     value: 10,
-    multipleOf: 5,
+    multipleOf: 5, // excess property
     format: "what?"
 };
 // This has excess error because variant three is the only applicable case.
diff --git a/tests/baselines/reference/excessPropertyCheckWithNestedArrayIntersection.js b/tests/baselines/reference/excessPropertyCheckWithNestedArrayIntersection.js
index 0f9085e..e597416 100644
--- a/tests/baselines/reference/excessPropertyCheckWithNestedArrayIntersection.js
+++ b/tests/baselines/reference/excessPropertyCheckWithNestedArrayIntersection.js
@@ -27,7 +27,7 @@ const repro: BugRepro = {
 var repro = {
     dataType: {
         fields: [{
-                key: 'bla',
+                key: 'bla', // should be OK: Not excess
                 value: null
             }]
     }
diff --git a/tests/baselines/reference/indexSignatures1.js b/tests/baselines/reference/indexSignatures1.js
index b507052..176c61a 100644
--- a/tests/baselines/reference/indexSignatures1.js
+++ b/tests/baselines/reference/indexSignatures1.js
@@ -354,7 +354,7 @@ const y2 = dom.data123;
 dom = { data123: 'hello' };
 dom = { date123: 'hello' }; // Error
 const funcs = {
-    sfoo: x => x.length,
+    sfoo: x => x.length, // x: string
     nfoo: x => x * 2, // n: number
 };
 i1[s0]; // Error
diff --git a/tests/baselines/reference/objectLitArrayDeclNoNew.js b/tests/baselines/reference/objectLitArrayDeclNoNew.js
index c724482..29799b7 100644
--- a/tests/baselines/reference/objectLitArrayDeclNoNew.js
+++ b/tests/baselines/reference/objectLitArrayDeclNoNew.js
@@ -41,7 +41,7 @@ var Test;
     function bug() {
         var state = null;
         return {
-            tokens: Gar[],
+            tokens: Gar[], //IToken[],  // Missing new. Correct syntax is: tokens: new IToken[]
             endState: state
         };
     }
diff --git a/tests/baselines/reference/objectLiteralShorthandPropertiesErrorFromNoneExistingIdentifier.js b/tests/baselines/reference/objectLiteralShorthandPropertiesErrorFromNoneExistingIdentifier.js
index 6050e5b..7944d33 100644
--- a/tests/baselines/reference/objectLiteralShorthandPropertiesErrorFromNoneExistingIdentifier.js
+++ b/tests/baselines/reference/objectLiteralShorthandPropertiesErrorFromNoneExistingIdentifier.js
@@ -7,6 +7,6 @@ var x = {

 //// [objectLiteralShorthandPropertiesErrorFromNoneExistingIdentifier.js]
 var x = {
-    x: x,
+    x: x, // OK
     undefinedVariable: undefinedVariable // Error
 };
diff --git a/tests/baselines/reference/privateNameBadDeclaration.js b/tests/baselines/reference/privateNameBadDeclaration.js
index cf110de..c720357 100644
--- a/tests/baselines/reference/privateNameBadDeclaration.js
+++ b/tests/baselines/reference/privateNameBadDeclaration.js
@@ -20,8 +20,8 @@ class C {
 //// [privateNameBadDeclaration.js]
 function A() { }
 A.prototype = {
-    : 1,
-    : function () { },
+    : 1, // Error
+    : function () { }, // Error
     get () { return ""; } // Error
 };
 var B = /** @Class */ (function () {
@@ -30,8 +30,8 @@ var B = /** @Class */ (function () {
     return B;
 }());
 B.prototype = {
-    : 2,
-    : function () { },
+    : 2, // Error
+    : function () { }, // Error
     get () { return ""; } // Error
 };
 var C = /** @Class */ (function () {
diff --git a/tests/baselines/reference/thisTypeInFunctions.js b/tests/baselines/reference/thisTypeInFunctions.js
index d9dd013..f6865f0 100644
--- a/tests/baselines/reference/thisTypeInFunctions.js
+++ b/tests/baselines/reference/thisTypeInFunctions.js
@@ -252,7 +252,7 @@ function implicitThis(n) {
 }
 var impl = {
     a: 12,
-    explicitVoid2: function () { return _this.a; },
+    explicitVoid2: function () { return _this.a; }, // ok, this: any because it refers to some outer object (window?)
     explicitVoid1: function () { return 12; },
     explicitStructural: function () {
         return this.a;
diff --git a/tests/baselines/reference/thisTypeInFunctionsNegative.js b/tests/baselines/reference/thisTypeInFunctionsNegative.js
index a3fe0b8..cdee9b0 100644
--- a/tests/baselines/reference/thisTypeInFunctionsNegative.js
+++ b/tests/baselines/reference/thisTypeInFunctionsNegative.js
@@ -219,7 +219,7 @@ let impl = {
     explicitVoid1() {
         return this.a; // error, no 'a' in 'void'
     },
-    explicitVoid2: () => this.a,
+    explicitVoid2: () => this.a, // ok, `this:any` because it refers to an outer object
     explicitStructural: () => 12,
     explicitInterface: () => 12,
     explicitThis() {
diff --git a/tests/baselines/reference/variableDeclaratorResolvedDuringContextualTyping.js b/tests/baselines/reference/variableDeclaratorResolvedDuringContextualTyping.js
index e8af78c..eb61b67 100644
--- a/tests/baselines/reference/variableDeclaratorResolvedDuringContextualTyping.js
+++ b/tests/baselines/reference/variableDeclaratorResolvedDuringContextualTyping.js
@@ -151,7 +151,7 @@ var FileService = /** @Class */ (function () {
             data: "someData"
         }).then(function (response) {
             var result = {
-                stat: _this.jsonToStat(newFilePath, "someString"),
+                stat: _this.jsonToStat(newFilePath, "someString"), // _this needs to be emitted to the js file
                 isNew: response.status === 201
             };
             return WinJS.TPromise.as(result);
@babakks babakks marked this pull request as ready for review July 31, 2022 11:57
@babakks
Copy link
Contributor Author

babakks commented Jul 31, 2022

@RyanCavanaugh The fix seems working now. However, the changes to the emitter affected 15 other test cases, fortunately, in a good way. That is, some trailing comments which were dropped in the reference baselines can now be retained as they were. Anyway, I totally get it that accepting any kind of change to old test cases requires careful consideration on your side.

For example, in assignmentCompatBug3.js, the comments below are now kept (as stated by the original test case file, assignmentCompatBug3.ts):

// assignmentCompatBug3.js
- get x() { return x; },
- get y() { return y; },
+ get x() { return x; }, // shouldn't be "void"
+ get y() { return y; }, // shouldn't be "void"
// assignmentCompatBug3.ts
get x() { return x;}, // shouldn't be "void"
get y() { return y;}, // shouldn't be "void"

Affected reference baselines

(15 cases)

  • arrowFunctionParsingDoesNotConfuseParenthesizedObjectForArrowHead.js
  • assignmentCompatBug3.js
  • bindingPatternCannotBeOnlyInferenceSource.js
  • callSignaturesWithParameterInitializers2.js
  • circularResolvedSignature.js
  • duplicateObjectLiteralProperty.js
  • excessPropertyCheckWithMultipleDiscriminants.js
  • excessPropertyCheckWithNestedArrayIntersection.js
  • indexSignatures1.js
  • objectLitArrayDeclNoNew.js
  • objectLiteralShorthandPropertiesErrorFromNoneExistingIdentifier.js
  • privateNameBadDeclaration.js
  • thisTypeInFunctions.js
  • thisTypeInFunctionsNegative.js
  • variableDeclaratorResolvedDuringContextualTyping.js

New test case

I've added a test case, commentsOnObjectLiteral5.ts (at tests/cases/compiler) to confirm that all comments are emitted as they are:

const a = {
    p0: 0, // Comment 0
    p1: 0, /* Comment 1
    A multiline comment. */
    p2: 0, // Comment 2
};

@babakks babakks mentioned this pull request Jul 31, 2022
Closed
@aghArdeshir
Copy link
Contributor

Nice one 👍 Thanks

@jakebailey jakebailey self-requested a review August 1, 2022 17:54
jakebailey
jakebailey approved these changes Aug 1, 2022
View reviewed changes
Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me (this is a bug I hit on the module transformer, "fixed" by switching to ts-morph entirely), but someone else should take a peek too.

@jakebailey jakebailey requested a review from rbuckton August 1, 2022 17:56
@jakebailey jakebailey merged commit b3770e7 into microsoft:main Sep 15, 2023
snovader pushed a commit to EG-A-S/TypeScript that referenced this pull request Sep 23, 2023
@babakks @jakebailey @snovader
🐛 Fix not emitting comments between sibling fields of object literals (…
1b07a00 
…microsoft#50097)

Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
@jakebailey jakebailey mentioned this pull request Nov 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakebailey jakebailey jakebailey approved these changes

@rbuckton rbuckton Awaiting requested review from rbuckton

Assignees

@rbuckton rbuckton

Labels
For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug report: Comment node is removed after "Extract to function in module scope
5 participants
@babakks @aghArdeshir @jakebailey @rbuckton @typescript-bot