add support for add or remove braces to arrow function #23423

Kingwl · 2018-04-16T06:32:23Z

mhegazy · 2018-04-17T00:13:19Z

src/compiler/diagnosticMessages.json

@@ -4165,5 +4165,17 @@
    "Generate 'get' and 'set' accessors": {
        "category": "Message",
        "code": 95046
+    },
+    "Convert arrow function": {


nit. Though no one sees this message, i think it should this be Add or remove braces in an arrow function

mhegazy · 2018-04-17T00:14:08Z

src/services/refactors/convertArrowFunction.ts

+        const info = getConvertibleArrowFunctionAtPosition(file, startPosition);
+        if (!info) return undefined;
+
+        const actions: RefactorActionInfo[] = [


consider inlining this into the return statement

mhegazy · 2018-04-17T00:16:59Z

src/services/refactors/convertArrowFunction.ts

+                expression: container.body
+            };
+        }
+        else if (container.body.statements.length === 1 && isReturnStatement(first(container.body.statements))) {


consider extracting first(container.body.statements) out and reusing it, will save you the cast later.

mhegazy

few nits.

mhegazy · 2018-04-17T00:18:21Z

@andy-ms can you please review

amcasey · 2018-04-17T00:34:40Z

src/services/refactors/convertArrowFunction.ts

@@ -0,0 +1,90 @@
+/* @internal */
+namespace ts.refactor.convertArrowFunction {


I would have thought this would be a suggestion diagnostic/code-fix so that it could be discovered via the lightbulb.

would not that make it a lint feature at this point?

never mind.. all suggestions are lint features anyways..

why this could be a diagnostic/code-fix...
I don't want to see this lightbulb in the code except when I want to convert

and it also could convert back 😂

amcasey · 2018-04-17T00:38:18Z

src/services/refactors/convertArrowFunction.ts

+        const info = getConvertibleArrowFunctionAtPosition(file, startPosition);
+        if (!info) return undefined;
+
+        const { addBraces, expression, container } = info;


Is it possible addBraces could disagree with _actionName. If that does happen, should we bail?

amcasey · 2018-04-17T00:43:56Z

src/services/refactors/convertArrowFunction.ts

+            return {
+                container,
+                addBraces: false,
+                expression: (<ReturnStatement>first(container.body.statements)).expression


What happens if the expression is {}?

Or a comma expression. Feels like there might be some hidden complications here.

amcasey · 2018-04-17T00:44:51Z

src/services/refactors/convertArrowFunction.ts

+    }
+
+    function updateBraces(changeTracker: textChanges.ChangeTracker, file: SourceFile, container: ArrowFunction, expression: Expression, addBraces: boolean) {
+        const body = addBraces ? createBlock([createReturn(expression)]) : expression;


Is there any value in preserving any comments that might have been attached to the return statement?

amcasey

Mostly, I'm concerned about the fact that this is a refactoring. Secondarily, I think there are cases where we'll synthesize syntactically invalid code (good to document with tests, but not the end of the world).

Kingwl · 2018-04-17T03:38:14Z

need some help:
how could i move the comment which attach to the return statement...

ghost · 2018-04-17T15:16:47Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

+        }];
+    }
+
+    function getEditsForAction(context: RefactorContext, _actionName: string): RefactorEditInfo | undefined {


Don't begin a name with _ unless it's unused.

ghost · 2018-04-17T15:27:11Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

+    }
+
+    function removeBraces(changeTracker: textChanges.ChangeTracker, file: SourceFile, container: ArrowFunction, expression: Expression) {
+        if (!isLiteralExpression(expression) && !isIdentifier(expression) && !isParenthesizedExpression(expression) && expression.kind !== SyntaxKind.NullKeyword) {


Should have a needsParentheses utility function that uses a switch statement. There are probably some unhandled cases here -- a property access shouldn't need parentheses, nor should a function call. Nor should true or false or undefined.
(By the way, I don't think it would be worth it to add a test for every single one of these cases. I notice there are a lot of tests related to parenthesizing already -- maybe they should be combined into one file.)

ghost · 2018-04-17T15:41:06Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

+        if (!info) return undefined;
+
+        const { expression, container } = info;
+        const changeTracker = textChanges.ChangeTracker.fromContext(context);


Both branches end up calling the same method on the change tracker. So this can be written as:

function getEditsForAction(context: RefactorContext, _actionName: string): RefactorEditInfo | undefined { const { file, startPosition } = context; const info = getConvertibleArrowFunctionAtPosition(file, startPosition); if (!info) return undefined; const { expression, container } = info; const newBody = _actionName === addBracesActionName ? createBlock([createReturn(expression)]) : _actionName === removeBracesActionName ? needsParentheses(expression) ? createParen(expression) : expression : Debug.fail("invalid action"); const edits = textChanges.ChangeTracker.with(context, t => t.replaceNode(file, container, updateBody(container, newBody))); return { renameFilename: undefined, renameLocation: undefined, edits }; } function updateBody(node: ArrowFunction, body: ConciseBody): ArrowFunction { return updateArrowFunction(node, node.modifiers, node.typeParameters, node.parameters, node.type, body); } function needsParentheses(expression: Expression): boolean { if (isLiteralExpression(expression)) { return false; } switch (expression.kind) { case SyntaxKind.Identifier: case SyntaxKind.NullKeyword: case SyntaxKind.TrueKeyword: case SyntaxKind.FalseKeyword: case SyntaxKind.UndefinedKeyword: case SyntaxKind.PropertyAccessExpression: case SyntaxKind.ElementAccessExpression: case SyntaxKind.CallExpression: case SyntaxKind.NewExpression: return false; default: return true; } }

Maybe needsParentheses should be written backwards -- what are the expressions that do need parentheses? BinaryExpression where the operator is ,, ObjectLiteralExpression, any others?

ghost · 2018-04-17T15:43:04Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

+    registerRefactor(refactorName, { getEditsForAction, getAvailableActions });
+
+    interface Info {
+        container: ArrowFunction;


container seems like the wrong name. It comes from getContainingFunction, but that's irrelevant to the rest of the code ... might just call it fun.

ghost · 2018-04-17T15:53:32Z

If we do this, we'll presumably want to implement add/remove braces for if, for, and while.

A problem with this being implemented as a code fix is performance -- we would have to add a suggestion diagnostic to every single arrow function. I looked at how IntelliJ does this and they only show the suggestion when you're at a particular position, which is more like a refactor in our design.

(Contrast with the underscore on true which is always visible, since that is considered a problem.)

I think the criteria we currently use for this is overly broad though -- getContainingFunction will return the arrow function if we're anywhere inside its body. I would recommend only providing the refactor if we're at one of its tokens -- the parentheses (or first parameter if no parentheses), the arrow, or the braces (if they already exist).

ghost · 2018-04-17T15:56:24Z

@Kingwl There is a copyComments function in convertFunctionToEs6Class which could be moved to utilities.

Kingwl · 2018-04-18T07:37:43Z

something need to consider:
what should follow code been convert to?

const a = () => /* test */ a + 1

A:

const a = () => { /* test */ return a + 1; }

B:

const a = () => {
  // test
  return a + 1; 
}

C:

const a = () => {
  /* test */
  return a + 1; 
}

the question is:

should the code add braces multi line
what kind of comment should be generate after convert

ghost · 2018-04-24T17:05:12Z

My opinion would be that adding braces implies an intent to make it multiline, so option C. But it's not a big deal if that would be hard to implement.

Kingwl · 2018-04-25T10:27:51Z

@andy-ms need some help!😢
how could i remove expression comment with suppressLeadingAndTrailingTrivia
i tried returnStatement body expression and they have no effect

Kingwl · 2018-04-25T10:58:39Z

#23611

ghost · 2018-04-25T15:31:24Z

It looks like in services, we're using a function forEachLeadingCommentRange, but the emitter is using a completely different function forEachLeadingCommentToEmit, so it's possible that those aren't aligned with each other. CC @rbuckton

ghost · 2018-04-25T15:33:07Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

+        if (!info) return undefined;
+
+        const { expression, func } = info;
+        const changeTracker = textChanges.ChangeTracker.fromContext(context);


This could be moved closer to its use:

const edits = textChanges.ChangeTracker.with(context, t => updateBody(t, file, func, body)); return { renameFilename: undefined, renameLocation: undefined, edits };

Also, we should consider replacing only the body node instead of the entire arrow function? Though that doesn't fix the duplicated comments issue.

ghost · 2018-04-25T15:33:17Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

+    }
+
+    function needsParentheses(expression: Expression) {
+        if (isBinaryExpression(expression) && expression.operatorToken.kind === SyntaxKind.CommaToken) return true;


if (a) return true; if (b) return true; return false; is just a || b.

Kingwl · 2018-04-26T02:44:24Z

update body only and the duplicated comments increased again😂

Kingwl · 2018-04-27T05:46:12Z

ping @weswigham
does those two duplicated comments is the TrailingComments for the => token? 😢

weswigham · 2018-04-27T08:20:29Z

Indeed it is. Setting EmitFlags.NoComments on the arrow function should, I believe, also disable the comments in the inner tokens? We had a similar issue before with another refactoring @amcasey maybe remembers the specifics? All the modifications to comment traversal you've got here just seem a tad excessive.

Kingwl · 2018-05-22T07:54:12Z

any progress here?
I manually performed dozens of conversions ~~and forget the return statement~~ 😅

amcasey · 2018-05-22T17:38:18Z

@Kingwl I know it's not your fault that this has been sitting for so long, but could you possibly push an update that resolves the merge conflicts and build issues (which might just be expirations?) before we take another look? Thanks!

mhegazy · 2018-05-23T23:26:09Z

@Kingwl a few tests need to be updated.

amcasey

I still think it would be more natural, both in code and for users, to express this as a code fix. For example, that would allow it to be suppressed or applied automatically at multiple locations. It would also make it discoverable and eliminate possible confusion about what range needs to be selected to have the change offered.

amcasey · 2018-05-24T18:14:50Z

src/compiler/scanner.ts

@@ -727,7 +727,7 @@ namespace ts {
    export function forEachLeadingCommentRange<U>(text: string, pos: number, cb: (pos: number, end: number, kind: CommentKind, hasTrailingNewLine: boolean) => U): U | undefined;
    export function forEachLeadingCommentRange<T, U>(text: string, pos: number, cb: (pos: number, end: number, kind: CommentKind, hasTrailingNewLine: boolean, state: T) => U, state: T): U | undefined;
    export function forEachLeadingCommentRange<T, U>(text: string, pos: number, cb: (pos: number, end: number, kind: CommentKind, hasTrailingNewLine: boolean, state: T) => U, state?: T): U | undefined {
-        return iterateCommentRanges(/*reduce*/ false, text, pos, /*trailing*/ false, cb, state);


What changed here? Is this a merge artifact?

amcasey · 2018-05-24T18:15:51Z

src/services/utilities.ts

@@ -1649,4 +1649,20 @@ namespace ts {
        Debug.assert(lastPos >= 0);
        return lastPos;
    }
+
+    export function copyComments(sourceNode: Node, targetNode: Node, sourceFile: SourceFile, explicitKind?: CommentKind, explicitHtnl?: boolean) {


What is "htnl"?

amcasey · 2018-05-24T18:17:51Z

tests/cases/fourslash/refactorAddOrRemoveBracesToArrowFunction9.ts

@@ -0,0 +1,13 @@
+/// <reference path='fourslash.ts' />
+
+//// const foo = /*a*/a/*b*/ => (1, 2, 3);


All of these tests seem to use the same selection range. What if the parameter has more than one character and only part of it is selected? What if a different part of the arrow function is selected?

Does it matter if the selection is empty?

amcasey · 2018-05-24T18:20:14Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

+
+    interface Info {
+        func: ArrowFunction;
+        expression: Expression | undefined;


I'm not sure I understand why expression is required, but can be undefined, whereas returnStatement is optional. Should they follow the same pattern?

amcasey · 2018-05-24T18:21:52Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

+        const { expression, returnStatement, func } = info;
+
+        let body: ConciseBody;
+        if (actionName === addBracesActionName) {


Does it matter whether this agrees with info.addBraces?

What happened with this?

amcasey · 2018-05-24T18:23:37Z

src/services/refactors/addOrRemoveBracesToArrowFunction.ts

+        return isBinaryExpression(expression) && expression.operatorToken.kind === SyntaxKind.CommaToken || isObjectLiteralExpression(expression);
+    }
+
+    function updateBody(changeTracker: textChanges.ChangeTracker, file: SourceFile, container: ArrowFunction, body: ConciseBody) {


Any reason not to inline this?

amcasey · 2018-05-24T18:28:33Z

I don't object strongly enough to block merging.

mhegazy · 2018-05-24T19:52:33Z

I will take on the codefix vs refactoring discussion with the VSCode team. there are some usability issues we need to figure out.. once i have that concluded i can convert the refactor to a code fix..

@Kingwl please address the remaining comments from @amcasey and we should be good to go.

mhegazy · 2018-05-25T22:55:55Z

@amcasey one more look?

mhegazy · 2018-05-25T23:04:34Z

@Kingwl can you look at the remaining comments from @amcasey

Kingwl · 2018-06-08T17:03:49Z

🆙

amcasey

I've re-raised the only comment that I thought might be important. The others were about consistency.

DanielRosenwasser · 2018-06-15T03:42:17Z

Thank you!

mhegazy reviewed Apr 17, 2018

View reviewed changes

mhegazy approved these changes Apr 17, 2018

View reviewed changes

mhegazy requested review from amcasey and a user April 17, 2018 00:18

amcasey reviewed Apr 17, 2018

View reviewed changes

amcasey suggested changes Apr 17, 2018

View reviewed changes

Kingwl force-pushed the add-braces branch from 4e7b31b to 07d1c86 Compare April 17, 2018 03:37

ghost reviewed Apr 17, 2018

View reviewed changes

ghost reviewed Apr 25, 2018

View reviewed changes

Kingwl added 2 commits May 23, 2018 10:16

add support for add or remove braces to arrow function

5a69d9c

add tests and fix

32be0c7

Kingwl added 5 commits May 23, 2018 10:20

stash

bd9a8b5

fix converter

0c06126

update body only

1a59eb3

revert wrong inline parameter

b6669c9

fix void return statement

de75f14

Kingwl force-pushed the add-braces branch from f7bef1e to de75f14 Compare May 23, 2018 02:50

Kingwl added 3 commits May 24, 2018 18:00

remove some case

5497b42

Merge branch 'master' into add-braces

d470f32

remove failed test

3d9a6ab

amcasey reviewed May 24, 2018

View reviewed changes

add more test and fix others

590476b

amcasey approved these changes Jun 8, 2018

View reviewed changes

Merge branch 'master' into add-braces

0d730c0

mhegazy merged commit e07e2e0 into microsoft:master Jun 11, 2018

Kingwl deleted the add-braces branch June 11, 2018 23:10

linde12 mentioned this pull request Aug 30, 2021

add or remove braces to arrow function neoclide/coc-tsserver#309

Closed

		@@ -0,0 +1,90 @@
		/* @internal */
		namespace ts.refactor.convertArrowFunction {

		@@ -0,0 +1,13 @@
		/// <reference path='fourslash.ts' />

		//// const foo = /a/a/b/ => (1, 2, 3);

add support for add or remove braces to arrow function #23423

add support for add or remove braces to arrow function #23423

Conversation

Kingwl commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy left a comment

Choose a reason for hiding this comment

mhegazy commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

Kingwl commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 17, 2018

ghost commented Apr 17, 2018

Kingwl commented Apr 18, 2018

ghost commented Apr 24, 2018 • edited by ghost Loading

Kingwl commented Apr 25, 2018

Kingwl commented Apr 25, 2018

ghost commented Apr 25, 2018

Choose a reason for hiding this comment

ghost Apr 25, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Apr 25, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Kingwl commented Apr 26, 2018

Kingwl commented Apr 27, 2018

weswigham commented Apr 27, 2018

Kingwl commented May 22, 2018 • edited Loading

amcasey commented May 22, 2018

mhegazy commented May 23, 2018

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey commented May 24, 2018

mhegazy commented May 24, 2018

mhegazy commented May 25, 2018

mhegazy commented May 25, 2018

Kingwl commented Jun 8, 2018

amcasey left a comment

Choose a reason for hiding this comment

DanielRosenwasser commented Jun 15, 2018

ghost commented Apr 24, 2018 •

edited by ghost

Loading

ghost Apr 25, 2018 •

edited by ghost

Loading

ghost Apr 25, 2018 •

edited by ghost

Loading

Kingwl commented May 22, 2018 •

edited

Loading