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.

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

Conversation

a-tarasyuk
Copy link
Contributor

@a-tarasyuk a-tarasyuk commented Jan 1, 2025

Fixes #60887

@typescript-bot typescript-bot added For Uncommitted Bug PR for untriaged, rejected, closed or missing bug For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 1, 2025
@@ -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

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

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

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

@a-tarasyuk a-tarasyuk requested a review from jakebailey January 17, 2025 21:05
@jakebailey
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 18, 2025

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
pack this ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 18, 2025

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/164566/artifacts?artifactName=tgz&fileId=B9186007CBA01B4F34C25858EFDA94F805D5C7AC878A1E404ABE28F5E4F85F1902&fileName=/typescript-5.8.0-insiders.20250118.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@jakebailey
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 22, 2025

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
pack this ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 22, 2025

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/164856/artifacts?artifactName=tgz&fileId=064C6DAC8161A217161351EE9E2DAD4C68B41E3B1FBFC831AF72E3A943D5378302&fileName=/typescript-5.9.0-insiders.20250222.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@jakebailey jakebailey merged commit 2c3be44 into microsoft:main Feb 24, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Incorrect Formatting on if (a) try {} finally {}
3 participants