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

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

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

Conversation

kirkwaiblinger
Copy link

@kirkwaiblinger kirkwaiblinger commented Jan 1, 2025

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 added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jan 1, 2025
@typescript-bot
Copy link
Collaborator

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
Author

Relates to #37332

@kirkwaiblinger 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
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
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
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
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
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
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!

@jakebailey jakebailey reopened this Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Status: Not started
Development

Successfully merging this pull request may close these issues.

3 participants