Skip to content

chore: don't fix TS code actions on save in vscode - #60897

Closed
Kirk Waiblinger (kirkwaiblinger) wants to merge 1 commit into
microsoft:mainfrom
kirkwaiblinger:dont-fix-ts-on-save
Closed

Kirk Waiblinger (kirkwaiblinger) wants to merge 1 commit into
microsoft:mainfrom
kirkwaiblinger:dont-fix-ts-on-save

Conversation

@kirkwaiblinger

@kirkwaiblinger Kirk Waiblinger (kirkwaiblinger) commented Jan 1, 2025 •

Copy link
Copy Markdown

For contributors who have "source.fixAll" enabled at a user level, spurious changes occur when saving files. For example, when I save the checker.ts, I get this diff:

iff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 80b61edd36..0a279c773a 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -9869,7 +9869,6 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
                         }
                         // We don't know how to serialize this (nested?) binding element
                         Debug.failBadSyntaxKind(node.parent?.parent || node, "Unhandled binding element grandparent kind in declaration serialization");
-                        break;
                     case SyntaxKind.ShorthandPropertyAssignment:
                         if (node.parent?.parent?.kind === SyntaxKind.BinaryExpression) {
                             // module.exports = { SomeClass }
@@ -10889,7 +10888,6 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
             case TypeSystemPropertyName.ParameterInitializerContainsUndefined:
                 return getNodeLinks(target as ParameterDeclaration).parameterInitializerContainsUndefined !== undefined;
         }
-        return Debug.assertNever(propertyName);
     }
 
     /**
@@ -36974,7 +36972,6 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
             case SyntaxKind.BinaryExpression:
                 return resolveInstanceofExpression(node, candidatesOutArray, checkMode);
         }
-        Debug.assertNever(node, "Branch in 'resolveSignature' should be unreachable.");
     }
 
     /**
@@ -39337,7 +39334,6 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
                 }
                 return getUnaryResultType(operandType);
         }
-        return errorType;
     }
 
     function checkPostfixUnaryExpression(node: PostfixUnaryExpression): Type {

Therefore, explicitly setting "source.fixAll.ts": "never" would be beneficial to contributors

@typescript-bot TypeScript Bot (typescript-bot) added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jan 1, 2025
@typescript-bot

Copy link
Copy Markdown
Contributor

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@kirkwaiblinger

Copy link
Copy Markdown
Author

Relates to #37332

@kirkwaiblinger Kirk Waiblinger (kirkwaiblinger) changed the title chore: don't fix TS errors on save in vscode chore: don't fix TS code actions on save in vscode Jan 1, 2025
@jakebailey

Copy link
Copy Markdown
Member

Honestly I don't understand why this quick fix is even eligible for an on save action... If it were me, I'd probably fix that instead

@kirkwaiblinger

Copy link
Copy Markdown
Author

Honestly I don't understand why this quick fix is even eligible for an on save action... If it were me, I'd probably fix that instead

Do you mean dead code branch deletion in general?

@jakebailey

Copy link
Copy Markdown
Member

Yes, though probably there are others?

I use auto fix for high quality lints (e.g. let -> var in eslint), but I can't imagine wanting my code deleted on save

@kirkwaiblinger

Copy link
Copy Markdown
Author

Yes, though probably there are others?

I use auto fix for high quality lints (e.g. let -> var in eslint), but I can't imagine wanting my code deleted on save

Well, this quick fix is coming from TS itself, not from the linter. I don't really have any context to speak to TS's intentions around quick fix, I can only speak for (typescript-)eslint, so I can't really chime in with anything intelligent here. Closing this PR.

@jakebailey

Copy link
Copy Markdown
Member

Oh, I don't think the PR was a problem, I was just looking past it to "if this is a good behavior, shouldn't it be the default too".

@kirkwaiblinger

Copy link
Copy Markdown
Author

Oh, I don't think the PR was a problem, I was just looking past it to "if this is a good behavior, shouldn't it be the default too".

Oh, I see - my motivation is completely agnostic as to whether it's a good behavior. I just think it's categorically best to avoid applying autofixes in the editor that aren't validated in CI, in order to keep diffs free of unrelated changes 🤷‍♂️.

Feel free to reopen if you like! I've changed my .vscode/settings.json so this changeset doesn't impact me; it's just a suggestion for your consideration of what I think would help a few contributors like myself who have customized their vscode settings in a way that appears to differ from the TS team's user-level settings 🤷‍♂️. Not a big deal one way or another!

@kirkwaiblinger

Copy link
Copy Markdown
Author

Doesn't seem like this is going anywhere, and I'd like to clear out my open PRs

@github-project-automation github-project-automation Bot moved this from Not started to Done in PR Backlog Dec 26, 2025
@kirkwaiblinger
Kirk Waiblinger (kirkwaiblinger) deleted the dont-fix-ts-on-save branch December 26, 2025 23:38
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Jun 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants