Skip to content

Fix formatting of parameter list of a multiple-line call expression #14955

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

Closed
wants to merge 4 commits into from
Closed

Fix formatting of parameter list of a multiple-line call expression #14955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5270c33
Add test
zhengbli Mar 30, 2017
36f2cf4
WIP
zhengbli Mar 31, 2017
adeb9a6
Fix it
zhengbli Mar 31, 2017
eb6038d
Add ElementAccessExpression case
zhengbli Apr 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 40 additions & 3 deletions src/services/formatting/formatting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ namespace ts.formatting {
}

/* @internal */
export function formatNode(node: Node, sourceFileLike: SourceFileLike, languageVariant: LanguageVariant, initialIndentation: number, delta: number, rulesProvider: RulesProvider): TextChange[] {
export function formatNode(node: Node, sourceFileLike: SourceFileLike, languageVariant: LanguageVariant, initialIndentation: number, delta: number, rulesProvider: RulesProvider): TextChange[] {
const range = { pos: 0, end: sourceFileLike.text.length };
return formatSpanWorker(
range,
Expand Down Expand Up @@ -536,7 +536,7 @@ namespace ts.formatting {
}
case SyntaxKind.OpenBracketToken:
case SyntaxKind.CloseBracketToken: {
if (container.kind !== SyntaxKind.MappedType) {
if (container.kind !== SyntaxKind.MappedType && container.kind !== SyntaxKind.ElementAccessExpression) {
return indentation;
}
break;
Expand Down Expand Up @@ -691,6 +691,31 @@ namespace ts.formatting {
return inheritedIndentation;
}

/**
* In some case even when a node list did not start at the same line with its parent, it should be treated specially because
* of the "effective parent line" was indented. E.g.
*
* Promise
* .then( // this is the "lastIndentedLine"
* a, <- here should be indented
* )
*
* Here the parent of argument list is the call expression itself, however the indentation of the argument list should carry
* over the indentation in line 1 (".then").
*/
function shouldListInheritIndentationFromLastIndentedLine(listStartLine: number, parent: Node, nodes: NodeArray<Node>) {
Copy link
Contributor

@yuit yuit Apr 5, 2017

Choose a reason for hiding this comment

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

Could you comment with an above example what is the value of "listStartLine"?

if (lastIndentedLine !== listStartLine) {
return false;
}

switch (parent.kind) {
case SyntaxKind.CallExpression:
return (<CallExpression>parent).arguments === nodes;
default:
return false;
Copy link
Contributor

@yuit yuit Apr 5, 2017

Choose a reason for hiding this comment

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

nit: would merge this with the if clause above.

}
}

function processChildNodes(nodes: NodeArray<Node>,
parent: Node,
parentStartLine: number,
Expand All @@ -713,8 +738,13 @@ namespace ts.formatting {
else if (tokenInfo.token.kind === listStartToken) {
// consume list start token
startLine = sourceFile.getLineAndCharacterOfPosition(tokenInfo.token.pos).line;

if (shouldListInheritIndentationFromLastIndentedLine(startLine, parent, nodes)) {
parentStartLine = lastIndentedLine;
}

const indentation =
computeIndentation(tokenInfo.token, startLine, Constants.Unknown, parent, parentDynamicIndentation, parentStartLine);
computeIndentation(tokenInfo.token, startLine, /*inheritedIndentation*/ Constants.Unknown, parent, parentDynamicIndentation, parentStartLine);

listDynamicIndentation = getDynamicIndentation(parent, parentStartLine, indentation.indentation, indentation.delta);
consumeTokenAndAdvanceScanner(tokenInfo, parent, listDynamicIndentation, parent);
Expand All @@ -732,6 +762,13 @@ namespace ts.formatting {
inheritedIndentation = processChildNode(child, inheritedIndentation, node, listDynamicIndentation, startLine, startLine, /*isListItem*/ true, /*isFirstListItem*/ i === 0);
}

if (nodes.hasTrailingComma && formattingScanner.isOnToken()) {
Copy link
Contributor

@yuit yuit Apr 5, 2017

Choose a reason for hiding this comment

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

Is this clause is so that we will indent trailing comma appropriately?

Copy link
Contributor Author

@zhengbli zhengbli Apr 5, 2017

Choose a reason for hiding this comment

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

Yes. The current issue is the formatter does not expect a trailing comma; it checks after all children nodes if the the current token is not the expected list ending token, it will do nothing. And then the list end token will be formatted later as part of the parent node using the parent's dynamic indentations. It worked fine before most of the time because normally the list uses the same indentation with its parent node; however, in this particular case they are different, therefore the trailing comma is causing problems. This change is to ensure that the list end token is processed with the list not its parent.

const tokenInfo = formattingScanner.readTokenInfo(parent);
if (tokenInfo.token.kind === SyntaxKind.CommaToken) {
consumeTokenAndAdvanceScanner(tokenInfo, parent, listDynamicIndentation, parent);
}
}

if (listEndToken !== SyntaxKind.Unknown) {
if (formattingScanner.isOnToken()) {
const tokenInfo = formattingScanner.readTokenInfo(parent);
Expand Down
34 changes: 34 additions & 0 deletions tests/cases/fourslash/formattingOnChainedCallbacks2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/// <reference path='fourslash.ts' />

////Promise
//// .then(
/////*then1cb1*/cb
//// /*then1*/);

////Promise
//// .then(
/////*then2cb1*/cb,
//// /*then2*/);

////Promise
//// ["then"](/*then3_1*/
/////*then3cb1*/cb,
//// /*then3*/);

format.document();
goTo.marker('then1cb1');
verify.currentLineContentIs(" cb");
goTo.marker('then1');
verify.currentLineContentIs(" );");

goTo.marker('then2cb1');
verify.currentLineContentIs(" cb,");
goTo.marker('then2');
verify.currentLineContentIs(" );");

goTo.marker('then3_1');
verify.currentLineContentIs(` ["then"](`);
goTo.marker('then3cb1');
verify.currentLineContentIs(" cb,");
goTo.marker('then3');
verify.currentLineContentIs(" );");
4 changes: 2 additions & 2 deletions tests/cases/fourslash/formattingOptionsChange.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
/////*insertSpaceBeforeAndAfterBinaryOperators*/1+2- 3
/////*insertSpaceAfterKeywordsInControlFlowStatements*/if (true) { }
/////*insertSpaceAfterFunctionKeywordForAnonymousFunctions*/(function () { })
/////*insertSpaceAfterOpeningAndBeforeClosingNonemptyParenthesis*/(1 )
/////*insertSpaceAfterOpeningAndBeforeClosingNonemptyParenthesis*/(1 );
/////*insertSpaceAfterOpeningAndBeforeClosingNonemptyBrackets*/[1 ]; [ ]; []; [,];
/////*insertSpaceAfterOpeningAndBeforeClosingTemplateStringBraces*/`${1}`;`${ 1 }`
/////*insertSpaceAfterTypeAssertion*/const bar = <Bar> Thing.getFoo();
Expand All @@ -22,7 +22,7 @@ runTest("insertSpaceAfterSemicolonInForStatements", "for (i = 0; i; i++);", "for
runTest("insertSpaceBeforeAndAfterBinaryOperators", "1 + 2 - 3", "1+2-3");
runTest("insertSpaceAfterKeywordsInControlFlowStatements", "if (true) { }", "if(true) { }");
runTest("insertSpaceAfterFunctionKeywordForAnonymousFunctions", "(function () { })", "(function() { })");
runTest("insertSpaceAfterOpeningAndBeforeClosingNonemptyParenthesis", " ( 1 )", " (1)");
runTest("insertSpaceAfterOpeningAndBeforeClosingNonemptyParenthesis", " ( 1 );", " (1);");
runTest("insertSpaceAfterOpeningAndBeforeClosingNonemptyBrackets", "[ 1 ];[];[];[ , ];", "[1];[];[];[,];");
runTest("insertSpaceAfterOpeningAndBeforeClosingTemplateStringBraces", "`${ 1 }`; `${ 1 }`", "`${1}`; `${1}`");
runTest("insertSpaceAfterTypeAssertion", "const bar = <Bar> Thing.getFoo();", "const bar = <Bar>Thing.getFoo();");
Expand Down