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

Merged
merged 4 commits into from
Feb 24, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
53a897a
fix(60887): prevent indenting finally blocks as children of try state…
a-tarasyuk Jan 1, 2025
2667851
Merge branch 'main' of https://github.com/microsoft/TypeScript into f…
a-tarasyuk Feb 21, 2025
382459f
adjust try statement indentation for child blocks
a-tarasyuk Feb 22, 2025
b416547
cleanup
a-tarasyuk Feb 22, 2025
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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've modified changes to rely exclusively on checking the syntax kind

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