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.

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

fix(60887): Incorrect Formatting on if (a) try {} finally {} #60898

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(60887): Incorrect Formatting on if (a) try {} finally {} #60898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions src/services/formatting/smartIndenter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import {
SourceFileLike,
SyntaxKind,
TextRange,
TryStatement,
TypeAliasDeclaration,
TypeLiteralNode,
TypeReferenceNode,
Expand Down Expand Up @@ -729,6 +730,12 @@ export namespace SmartIndenter {
return false;
}
break;
case SyntaxKind.TryStatement:
const tryStatement = parent as TryStatement;
if (tryStatement.finallyBlock && tryStatement.finallyBlock === child) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm suspicious about this just because all of the other code in this switch case are only checking syntax kind or position info, not node equality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jakebailey thanks for the feedback. I found that in this case, the finallyBlock should not be handled as a child of the try block to ensure the correct delta calculation

TypeScript/src/services/formatting/formatting.ts

Line 624 in 5170645

const delta = SmartIndenter.shouldIndentChildNode(options, node) ? options.indentSize! : 0;

There are other places where we handle the finallyBlock in a similar way

TypeScript/src/services/outliningElementsCollector.ts

Lines 259 to 261 in 5170645

else if (tryStatement.finallyBlock === n) {
const node = findChildOfKind(tryStatement, SyntaxKind.FinallyKeyword, sourceFile);
if (node) return spanForNode(node);

I hope that makes sense., if you have any thoughts or suggestions, I’d be grateful to hear them. thanks

Copy link
Member

Choose a reason for hiding this comment

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

How does this bug not affect the try block or the catch block, then? Maybe I'm missing something...

return false;
}
break;
}
// No explicit rule for given nodes so the result will follow the default value argument
return indentByDefault;
Expand Down
15 changes: 15 additions & 0 deletions tests/cases/fourslash/formatTryFinally.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/// <reference path="fourslash.ts"/>

////if (true) try {
//// // ...
////} finally {
//// // ...
////}

format.document();
verify.currentFileContentIs(
`if (true) try {
// ...
} finally {
// ...
}`);
Loading