fix(refactor): keep comments after refactor

[jsjoeio]

This PR fixes part 1 of "Missing comments after applying refactor/codefix #29972". Here's how I approached solving the problem:

1. Grab the trailing comment from the end of the return statement and copy to an empty string literal token.
2. Take the comment and format it properly. Add padding if needed. Add space for semicolon if needed.
3. Create a new text change that starts at the end of the function body, and ends after the length of the comment.
4. Add that new edit to the collection of text changes.

Changes

• add logic in addOrRemoveBracesToArrowFunction to grab comments that were originally missing
• add 5 new fourslash tests

Screenshots

Checklist

• There is an associated issue in the Backlog milestone (required)
• Code is up-to-date with the master branch
• You've successfully run gulp runtests locally
• There are new or updated unit tests validating the change

Fixes part 1 of #29972

[msftclas]

All CLA requirements met.

[jessetrinity]

I'm not sure which semicolon we want to keep here, but //2 should already be preserved. Either way there is a SyntaxKind.SemicolonToken.

const b = (a: number) => { 
    return { a: a }; //1
}; //2

[jsjoeio]

It's hard to remember why this was happening, but if there is a semicolon, it wasn't being included as part of the refactor when grabbing the trailing comments, hence why I added this.

If I jog my memory, I think it was for the semicolon that come at the end of the function body but before the trailing comment:

const a = (a: number) => a; /* trailing */

I'm not sure what action you want me to take.

[jessetrinity]

Sorry, my comment was definitely vague.

I wouldn't worry about trying to copy the semicolon here. If the original has two semicolons we will already end up with the semicolon from the original variable statement, assuming we just are just replacing the function body:

const b = (a: number) => { 
    return a; 
};|<

const b = (a: number) => a;|<

No semicolons gives no semicolons

const b = (a: number) => { 
    return a
}

const b = (a: number) => a

And then we have two cases of inconsistent semicolons

const b = (a: number) => { 
    return a
};

const b = (a: number) => { 
    return a;
}

Which do we pick? I don't think we need to think too hard about making complex rules to decide which one to use. If the original code has inconsistent semicolon usage, we don't need to be consistent in providing/not providing a semicolon.

[jessetrinity]

It seems like you should be able to stop after copying the comments above. What case(s) is this trying to address?

[jessetrinity]

We probably also want to have something more general to do this like copyComments since we will probably have to do this in a bunch of places.

[jsjoeio]

Looks like this was to address the following cases. When I remove that functiona

refactorAddOrRemoveBracesToArrowFunction25.ts

Expected:
const a = (a: number) => a; /* trailing */

Actual:
const a = (a: number) => a;

refactorAddOrRemoveBracesToArrowFunction26.ts

Expected:
const a = (a: number) => /* leading */ a; /* trailing */

Actual:
const a = (a: number) => /* leading */ a;

refactorAddOrRemoveBracesToArrowFunction27.ts

Expected:
const b = (a: number) => /* leading */ a /* trailing */

Actual:
const b = (a: number) => /* leading */ a

refactorAddOrRemoveBracesToArrowFunction28.ts

Expected:
const a = (a: number) => a; /* trailing */ /* trailing */

Actual:
const a = (a: number) => a;

[jsjoeio]

We probably also want to have something more general to do this like copyComments since we will probably have to do this in a bunch of places.

Agreed! Open to suggestions. How should we modify this?

[jessetrinity]

So if you just make the changes you have on 81-84 but copy the comments to returnStatement, so

copyTrailingAsLeadingComments(returnStatement, body, file, SyntaxKind.MultiLineCommentTrivia, /* hasTrailingNewLine */ false);
copyLeadingComments(returnStatement, body, file, SyntaxKind.MultiLineCommentTrivia, /* hasTrailingNewLine */ false);
copyTrailingComments(returnStatement, body, file, SyntaxKind.MultiLineCommentTrivia, /* hasTrailingNewLine */ false);

we would take this:

const f = (a: number) /* a */ => /* b */ { /* c */
    /* d */
    return a /* e */; /* f */
};

and make this:

const f = (a: number) /* a */ => /* b */ /* c */ /* d */ a /* f */;

The /* e */ comment gets eaten but probably because it belongs to the expression a rather than the ReturnStatement, but you can probably still copy with it one of the above methods. I wouldn't be heartbroken if that comment was just left out though because

1. We don't have to be perfect and the above is already much better than what we have, and
2. I think the placement of /* e */ is ugly in the first place.

A change to refactoring generally involves changes to the AST manipulation, or changes to emitter.ts. Doing the above keeps us in the first which is probably the way you want to go in this PR.

I think if you still need to make some changes to how the comment formatting is handled, I think you would want to that in emitter.ts (rather than copying its code here), and in a separate PR.

[jsjoeio]

Thanks for the review @jessetrinity ! I've responded to your comments.

To be honest, it's been four months since I submitted this so I don't remember every single detail. If you have specific feedback/suggestions, I'm happy to continue working on this.

[jsjoeio]

Looks like this was to address the following cases. When I remove that functiona

refactorAddOrRemoveBracesToArrowFunction25.ts

Expected:
const a = (a: number) => a; /* trailing */

Actual:
const a = (a: number) => a;

refactorAddOrRemoveBracesToArrowFunction26.ts

Expected:
const a = (a: number) => /* leading */ a; /* trailing */

Actual:
const a = (a: number) => /* leading */ a;

refactorAddOrRemoveBracesToArrowFunction27.ts

Expected:
const b = (a: number) => /* leading */ a /* trailing */

Actual:
const b = (a: number) => /* leading */ a

refactorAddOrRemoveBracesToArrowFunction28.ts

Expected:
const a = (a: number) => a; /* trailing */ /* trailing */

Actual:
const a = (a: number) => a;

[jsjoeio]

We probably also want to have something more general to do this like copyComments since we will probably have to do this in a bunch of places.

Agreed! Open to suggestions. How should we modify this?

[jsjoeio]

It's hard to remember why this was happening, but if there is a semicolon, it wasn't being included as part of the refactor when grabbing the trailing comments, hence why I added this.

If I jog my memory, I think it was for the semicolon that come at the end of the function body but before the trailing comment:

const a = (a: number) => a; /* trailing */

I'm not sure what action you want me to take.

[jsjoeio]

So I don't forget, here's how you can run a specific fourslash test:

yarn gulp runtests --tests=tests/cases/fourslash/refactorAddOrRemoveBracesToArrowFunction28.ts

[jessetrinity]

So if you just make the changes you have on 81-84 but copy the comments to returnStatement, so

copyTrailingAsLeadingComments(returnStatement, body, file, SyntaxKind.MultiLineCommentTrivia, /* hasTrailingNewLine */ false);
copyLeadingComments(returnStatement, body, file, SyntaxKind.MultiLineCommentTrivia, /* hasTrailingNewLine */ false);
copyTrailingComments(returnStatement, body, file, SyntaxKind.MultiLineCommentTrivia, /* hasTrailingNewLine */ false);

we would take this:

const f = (a: number) /* a */ => /* b */ { /* c */
    /* d */
    return a /* e */; /* f */
};

and make this:

const f = (a: number) /* a */ => /* b */ /* c */ /* d */ a /* f */;

The /* e */ comment gets eaten but probably because it belongs to the expression a rather than the ReturnStatement, but you can probably still copy with it one of the above methods. I wouldn't be heartbroken if that comment was just left out though because

1. We don't have to be perfect and the above is already much better than what we have, and
2. I think the placement of /* e */ is ugly in the first place.

A change to refactoring generally involves changes to the AST manipulation, or changes to emitter.ts. Doing the above keeps us in the first which is probably the way you want to go in this PR.

I think if you still need to make some changes to how the comment formatting is handled, I think you would want to that in emitter.ts (rather than copying its code here), and in a separate PR.

[jessetrinity]

Sorry, my comment was definitely vague.

I wouldn't worry about trying to copy the semicolon here. If the original has two semicolons we will already end up with the semicolon from the original variable statement, assuming we just are just replacing the function body:

const b = (a: number) => { 
    return a; 
};|<

const b = (a: number) => a;|<

No semicolons gives no semicolons

const b = (a: number) => { 
    return a
}

const b = (a: number) => a

And then we have two cases of inconsistent semicolons

const b = (a: number) => { 
    return a
};

const b = (a: number) => { 
    return a;
}

Which do we pick? I don't think we need to think too hard about making complex rules to decide which one to use. If the original code has inconsistent semicolon usage, we don't need to be consistent in providing/not providing a semicolon.

[jsjoeio]

I wouldn't be heartbroken if that comment was just left out though because

We don't have to be perfect and the above is already much better than what we have, and
I think the placement of /* e */ is ugly in the first place.

I went with this approach! I agree with you. The code was ugly too and it's a weird edge case anyway. I removed the hasSemiColon function and the other code i copied from the emitter. I don't plan to open a PR just to fix that edge case (I don't have the motivation or time 😂).

Thanks a ton for the thorough explanations and helping me work through this! I updated the tests to match the new expected functionality and things.

I'm running the tests locally. Everything should pass still I think.

[jsjoeio]

Nooo. I broke it

Here's the actual code:

/// <reference path='fourslash.ts' />

//// const foo = /*a*/a/*b*/ => {
////     // return comment
////     return a;
//// };

goTo.select("a", "b");
edit.applyRefactor({
    refactorName: "Add or remove braces in an arrow function",
    actionName: "Remove braces from arrow function",
    actionDescription: "Remove braces from arrow function",
    newContent: `const foo = a => /* return comment*/ a;`,
});

[jsjoeio]

Okay, I think I found the issue...It looks like I was calling copyLeadingComments twice. Oops! Glad the test caught that. Running all tests again locally before pushing.

[jsjoeio]

Passing locally!

Pushing up now

[jessetrinity]

Okay, I think I found the issue...It looks like I was calling copyLeadingComments twice. Oops! Glad the test caught that. Running all tests again locally before pushing.

It looks like you delete the wrong copyLeadingComments duplicate, but otherwise looks good.

[jsjoeio]

It looks like you delete the wrong copyLeadingComments duplicate, but otherwise looks good.

Oops! Okay, I added a new commit from the browser switching those to the correct order. Let's see if that still passes

[jessetrinity]

@jsjoeio awesome, thank you and congrats on your first contribution to TypeScript!

[jsjoeio]

jessetrinity approved these changes