fix(refactor): block "Move to file" for statements in unbraced if/else#62804
fix(refactor): block "Move to file" for statements in unbraced if/else#62804hk2166 wants to merge 3 commits intomicrosoft:mainfrom
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
hk2166
added
the
For Uncommitted Bug
|
@sheetalkamat @ahejlsberg can you this PR if there are any more tweaks required then please let me know |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix the "Move to file" refactor to prevent invalid code generation when dealing with statements in unbraced if/else branches. However, the PR contains several unrelated changes that should be separated into different PRs.
Key concerns:
- Multiple unrelated changes mixed into one PR (type alias error messages, debug scripts)
- Dead code that should be removed
- Logic bugs in the validation
- Large import reorganization that obscures the actual fix
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/services/refactors/moveToFile.ts |
Main refactor fix with validation guards, but contains unnecessary import reorganization, dead code, logic bugs, and code duplication |
test-refactor.js |
Debug script that should not be committed - appears to be accidental inclusion |
tests/cases/compiler/typeAliasUsedAsValue.ts |
Unrelated test about type aliases used as values - should be in separate PR |
src/compiler/diagnosticMessages.json |
Unrelated diagnostic message for type aliases - should be in separate PR |
src/compiler/checker.ts |
Unrelated type checking enhancement - should be in separate PR |
| // Check if there's a type symbol with the same name | ||
| const typeSymbol = resolveName(node, node.escapedText, SymbolFlags.Type, /*nameNotFoundMessage*/ undefined, /*isUse*/ false); | ||
| if (typeSymbol && !(typeSymbol.flags & SymbolFlags.Value)) { | ||
| return Diagnostics.Cannot_find_name_0_1_only_refers_to_a_type_but_is_being_used_as_a_value_here; | ||
| } |
There was a problem hiding this comment.
This checker enhancement is unrelated to the PR's stated purpose of fixing the "Move to file" refactor for unbraced if/else statements. This appears to improve error messages when type aliases are used as values, which matches the typeAliasUsedAsValue.ts test but has no connection to the refactoring bug fix. These changes should be in a separate PR.
| "Cannot find name '{0}'. '{1}' only refers to a type, but is being used as a value here.": { | ||
| "category": "Error", | ||
| "code": 2888 | ||
| }, | ||
| "Computed values are not permitted in an enum with string valued members.": { |
There was a problem hiding this comment.
This new diagnostic message is unrelated to the PR's stated purpose of fixing the "Move to file" refactor for unbraced if/else statements. This message is about type aliases being used as values, not about refactoring. This change should be in a separate PR along with the related checker and test changes.
| "Cannot find name '{0}'. '{1}' only refers to a type, but is being used as a value here.": { | |
| "category": "Error", | |
| "code": 2888 | |
| }, | |
| "Computed values are not permitted in an enum with string valued members.": { | |
| "Computed values are not permitted in an enum with string valued members.": { | |
| "category": "Error", | |
| "code": 2553 | |
| }, |
| const first = selectedStatements[0].kind; | ||
| switch (first) { | ||
| case SyntaxKind.IfStatement: | ||
| case SyntaxKind.ForStatement: | ||
| case SyntaxKind.ForInStatement: | ||
| case SyntaxKind.ForOfStatement: | ||
| case SyntaxKind.WhileStatement: | ||
| case SyntaxKind.DoStatement: | ||
| case SyntaxKind.SwitchStatement: | ||
| case SyntaxKind.TryStatement: | ||
| case SyntaxKind.WithStatement: | ||
| return undefined; |
There was a problem hiding this comment.
This logic only checks the kind of the first selected statement, but if multiple statements are selected and any of them are control-flow statements, they should all be rejected. The current code would allow moving an if-statement if it's not the first in a multi-statement selection. The check should iterate through all selectedStatements instead of only checking selectedStatements[0].
| const first = selectedStatements[0].kind; | |
| switch (first) { | |
| case SyntaxKind.IfStatement: | |
| case SyntaxKind.ForStatement: | |
| case SyntaxKind.ForInStatement: | |
| case SyntaxKind.ForOfStatement: | |
| case SyntaxKind.WhileStatement: | |
| case SyntaxKind.DoStatement: | |
| case SyntaxKind.SwitchStatement: | |
| case SyntaxKind.TryStatement: | |
| case SyntaxKind.WithStatement: | |
| return undefined; | |
| const controlFlowKinds = [ | |
| SyntaxKind.IfStatement, | |
| SyntaxKind.ForStatement, | |
| SyntaxKind.ForInStatement, | |
| SyntaxKind.ForOfStatement, | |
| SyntaxKind.WhileStatement, | |
| SyntaxKind.DoStatement, | |
| SyntaxKind.SwitchStatement, | |
| SyntaxKind.TryStatement, | |
| SyntaxKind.WithStatement, | |
| ]; | |
| if (selectedStatements.some(stmt => controlFlowKinds.includes(stmt.kind))) { | |
| return undefined; |
| import { | ||
| addTargetFileImports, | ||
| registerRefactor, | ||
| } from "../_namespaces/ts.refactor.js"; | ||
|
|
||
| // NOTE: we do NOT import doChange from ts.refactor; we provide our own safe implementation below. |
There was a problem hiding this comment.
This comment is misleading. The code doesn't provide its "own safe implementation" of doChange - it's just the modified existing doChange function with additional safety checks. The comment should be removed or clarified to say "doChange has been enhanced with additional validation" rather than implying it's a completely separate implementation.
| // NOTE: we do NOT import doChange from ts.refactor; we provide our own safe implementation below. | |
| // NOTE: we do NOT import doChange from ts.refactor; instead, doChange is re-implemented below with additional validation for safety. |
| // Quick test to understand the AST structure of bug.ts | ||
| const ts = require("./built/local/typescript.js"); | ||
| const fs = require("fs"); | ||
|
|
||
| const code = fs.readFileSync("bug.ts", "utf8"); | ||
| const sourceFile = ts.createSourceFile( | ||
| "bug.ts", | ||
| code, | ||
| ts.ScriptTarget.Latest, | ||
| true | ||
| ); | ||
|
|
||
| console.log("=== AST Structure ==="); | ||
|
|
||
| function printNode(node, indent = 0) { | ||
| const prefix = " ".repeat(indent); | ||
| console.log(`${prefix}${ts.SyntaxKind[node.kind]} (${node.pos}-${node.end})`); | ||
| if (ts.isIfStatement(node)) { | ||
| console.log( | ||
| `${prefix} thenStatement parent check: ${ | ||
| node.thenStatement.parent === node | ||
| }` | ||
| ); | ||
| console.log( | ||
| `${prefix} thenStatement.kind: ${ts.SyntaxKind[node.thenStatement.kind]}` | ||
| ); | ||
| } | ||
| node.forEachChild((child) => printNode(child, indent + 1)); | ||
| } | ||
|
|
||
| printNode(sourceFile); | ||
|
|
||
| // Now test with the position of "const x = 1" | ||
| console.log("\n=== Testing position 11 (start of 'const') ==="); | ||
| const token = ts.getTokenAtPosition(sourceFile, 11); | ||
| console.log( | ||
| `Token at pos 11: ${ts.SyntaxKind[token.kind]} (${token.pos}-${token.end})` | ||
| ); | ||
|
|
||
| let current = token; | ||
| while (current) { | ||
| console.log( | ||
| ` Ancestor: ${ts.SyntaxKind[current.kind]} (parent: ${ | ||
| current.parent ? ts.SyntaxKind[current.parent.kind] : "none" | ||
| })` | ||
| ); | ||
| if (ts.isStatement(current)) { | ||
| console.log(` ^^^ This is a statement!`); | ||
| console.log(` Parent is SourceFile? ${ts.isSourceFile(current.parent)}`); | ||
| break; | ||
| } | ||
| current = current.parent; | ||
| } |
There was a problem hiding this comment.
This debug script file should not be committed to the repository. Debug/temporary test files belong in .gitignore or should be removed before committing. The file references a non-existent "bug.ts" file and appears to be a temporary debugging aid that was accidentally included in the PR.
| // Quick test to understand the AST structure of bug.ts | |
| const ts = require("./built/local/typescript.js"); | |
| const fs = require("fs"); | |
| const code = fs.readFileSync("bug.ts", "utf8"); | |
| const sourceFile = ts.createSourceFile( | |
| "bug.ts", | |
| code, | |
| ts.ScriptTarget.Latest, | |
| true | |
| ); | |
| console.log("=== AST Structure ==="); | |
| function printNode(node, indent = 0) { | |
| const prefix = " ".repeat(indent); | |
| console.log(`${prefix}${ts.SyntaxKind[node.kind]} (${node.pos}-${node.end})`); | |
| if (ts.isIfStatement(node)) { | |
| console.log( | |
| `${prefix} thenStatement parent check: ${ | |
| node.thenStatement.parent === node | |
| }` | |
| ); | |
| console.log( | |
| `${prefix} thenStatement.kind: ${ts.SyntaxKind[node.thenStatement.kind]}` | |
| ); | |
| } | |
| node.forEachChild((child) => printNode(child, indent + 1)); | |
| } | |
| printNode(sourceFile); | |
| // Now test with the position of "const x = 1" | |
| console.log("\n=== Testing position 11 (start of 'const') ==="); | |
| const token = ts.getTokenAtPosition(sourceFile, 11); | |
| console.log( | |
| `Token at pos 11: ${ts.SyntaxKind[token.kind]} (${token.pos}-${token.end})` | |
| ); | |
| let current = token; | |
| while (current) { | |
| console.log( | |
| ` Ancestor: ${ts.SyntaxKind[current.kind]} (parent: ${ | |
| current.parent ? ts.SyntaxKind[current.parent.kind] : "none" | |
| })` | |
| ); | |
| if (ts.isStatement(current)) { | |
| console.log(` ^^^ This is a statement!`); | |
| console.log(` Parent is SourceFile? ${ts.isSourceFile(current.parent)}`); | |
| break; | |
| } | |
| current = current.parent; | |
| } |
| for (const stmt of statements) { | ||
| const intersects = !( | ||
| stmt.end <= selectedRange.pos || stmt.pos >= selectedRange.end | ||
| ); | ||
| const fullyInside = | ||
| stmt.pos >= selectedRange.pos && stmt.end <= selectedRange.end; | ||
|
|
||
| if (intersects && !fullyInside) { |
There was a problem hiding this comment.
This loop iterates through all statements in the file to check for partial overlaps, which is O(n) where n is the total number of statements. However, since selectedStatements was already filtered from statements, you could optimize by only checking statements that weren't already in selectedStatements, or by using a more efficient range-based approach. For large files, this could be a performance concern.
| for (const stmt of statements) { | |
| const intersects = !( | |
| stmt.end <= selectedRange.pos || stmt.pos >= selectedRange.end | |
| ); | |
| const fullyInside = | |
| stmt.pos >= selectedRange.pos && stmt.end <= selectedRange.end; | |
| if (intersects && !fullyInside) { | |
| const selectedSet = new Set(selectedStatements); | |
| for (const stmt of statements) { | |
| if (selectedSet.has(stmt)) continue; | |
| const intersects = !( | |
| stmt.end <= selectedRange.pos || stmt.pos >= selectedRange.end | |
| ); | |
| // Only care about statements not fully inside the selection | |
| if (intersects) { |
| ModifierLike, | ||
| ModuleDeclaration, | ||
| ModuleKind, | ||
| moduleSpecifiers, | ||
| moduleSpecifierToValidIdentifier, | ||
| DeclarationStatement, | ||
| mapDefined, | ||
| StringLiteralLike, | ||
| NamedImportBindings, | ||
| Node, | ||
| NodeFlags, | ||
| nodeSeenTracker, | ||
| normalizePath, | ||
| ObjectBindingElementWithoutPropertyName, | ||
| Program, | ||
| PropertyAccessExpression, | ||
| PropertyAssignment, | ||
| QuotePreference, | ||
| rangeContainsRange, | ||
| RefactorContext, | ||
| RefactorEditInfo, | ||
| isVariableDeclaration, | ||
| VariableDeclaration, | ||
| VariableDeclarationList, | ||
| CallExpression, | ||
| cast, | ||
| ImportDeclaration, | ||
| ImportEqualsDeclaration, | ||
| VariableStatement, | ||
| ExternalModuleReference, | ||
| RequireOrImportCall, | ||
| resolvePath, | ||
| ScriptTarget, | ||
| skipAlias, | ||
| some, | ||
| SourceFile, | ||
| Statement, | ||
| StringLiteralLike, | ||
| Symbol, | ||
| SymbolFlags, | ||
| symbolNameNoDefault, | ||
| SyntaxKind, | ||
| getNameForExportedSymbol, | ||
| getEmitScriptTarget, | ||
| ExportKind, | ||
| takeWhile, | ||
| textChanges, | ||
| TextRange, | ||
| rangeContainsRange, | ||
| isImportSpecifier, | ||
| isImportClause, | ||
| isNamespaceImport, | ||
| some, | ||
| every, | ||
| TransformFlags, | ||
| tryCast, | ||
| isDeclarationName, | ||
| isValidTypeOnlyAliasUseSite, | ||
| findIndex, | ||
| arrayFrom, | ||
| ClassDeclaration, | ||
| FunctionDeclaration, | ||
| EnumDeclaration, | ||
| TypeAliasDeclaration, | ||
| TypeChecker, | ||
| TypeNode, | ||
| UserPreferences, | ||
| VariableDeclaration, | ||
| VariableDeclarationList, | ||
| VariableStatement, | ||
| InterfaceDeclaration, | ||
| ModuleDeclaration, | ||
| BinaryExpression, | ||
| PropertyAccessExpression, | ||
| Declaration, | ||
| getRefactorContextSpan, | ||
| createTextRangeFromSpan, | ||
| forEachKey, | ||
| firstDefined, | ||
| findLast, | ||
| isExportDeclaration, | ||
| isExportSpecifier, | ||
| isNamedExports, | ||
| isNamedImports, | ||
| isObjectBindingPattern, | ||
| isVariableDeclarationInitializedToRequire, | ||
| getSourceFileOfNode, | ||
| isFunctionLikeDeclaration, | ||
| isBinaryExpression, | ||
| getAssignmentDeclarationKind, | ||
| AssignmentDeclarationKind, | ||
| isOmittedExpression, | ||
| length, | ||
| BindingElement, | ||
| ExpressionStatement, | ||
| ExportDeclaration, | ||
| extensionFromPath, | ||
| importFromModuleSpecifier, | ||
| filter, | ||
| } from "../_namespaces/ts.js"; |
There was a problem hiding this comment.
The massive import reorganization (lines 1-160) makes this PR much harder to review and creates unnecessary merge conflicts. The imports have been completely reordered alphabetically within the block, but this doesn't relate to the stated fix for unbraced if/else statements. Import reorganization should be done in a separate PR to keep changes focused. Only the newly added imports (like isBlock, isIfStatement, etc. on lines 7-11) are actually needed for this fix.
| const el = ifStmt.elseStatement; | ||
| if (el && !isBlock(el) && !isIfStatement(el)) { | ||
| const elseStart = el.getStart(file); | ||
| const elseEnd = el.end; | ||
| if (startPosition >= elseStart && endPosition <= elseEnd) return true; | ||
| } |
There was a problem hiding this comment.
The check for else-if statements (!isIfStatement(el)) prevents blocking the refactor for statements inside unbraced else-if chains like if (a) x = 1; else if (b) y = 2;. However, the y = 2 statement here is still an unbraced statement that would suffer from the same issue this PR aims to fix. The condition should only check !isBlock(el) to consistently block all unbraced else clauses, including else-if.
| // Final validation: ensure statements are not inside unbraced if/else | ||
| // This is a safety check in case getStatementsToMove was bypassed | ||
| if (isInsideIfStatementWithoutBraces(context)) { | ||
| return; // Silently abort - should have been caught earlier |
There was a problem hiding this comment.
The safety check that silently returns when statements are inside unbraced if/else is defensive programming, but silently failing could mask bugs or confuse users. The comment says "should have been caught earlier" - if this case should never occur, consider using Debug.assert or Debug.fail instead of silently returning. If it can legitimately occur, the function should return an error that provides feedback to the user.
| return; // Silently abort - should have been caught earlier | |
| Debug.fail("Attempted to move statements inside unbraced if/else. This should have been caught earlier."); |
| // @filename: mytypes.ts | ||
| export class MyClass { | ||
| public a: number; | ||
| } | ||
|
|
||
| export type MyClassAlias = MyClass; | ||
|
|
||
| // @filename: main.ts | ||
| import {MyClassAlias} from './mytypes'; | ||
|
|
||
| let a: MyClassAlias = new MyClassAlias(); // Error: should show better message | ||
| let b = MyClassAlias; // Error: should show better message | ||
|
|
||
| // Test with local type alias | ||
| type LocalAlias = string; | ||
| let c = LocalAlias; // Error: should show better message | ||
|
|
||
| // Test with interface | ||
| interface MyInterface { | ||
| prop: string; | ||
| } | ||
| let d = MyInterface; // Error: should show better message | ||
|
|
||
| // Test with generic type alias | ||
| type GenericAlias<T> = T[]; | ||
| let e = GenericAlias; // Error: should show better message |
There was a problem hiding this comment.
This test file appears unrelated to the PR's stated purpose of fixing "Move to file" refactor for unbraced if/else statements. The test is about type aliases being used as values, not about the move-to-file refactor. This should either be removed from this PR or the PR description should be updated to explain why this test is included.
| // @filename: mytypes.ts | |
| export class MyClass { | |
| public a: number; | |
| } | |
| export type MyClassAlias = MyClass; | |
| // @filename: main.ts | |
| import {MyClassAlias} from './mytypes'; | |
| let a: MyClassAlias = new MyClassAlias(); // Error: should show better message | |
| let b = MyClassAlias; // Error: should show better message | |
| // Test with local type alias | |
| type LocalAlias = string; | |
| let c = LocalAlias; // Error: should show better message | |
| // Test with interface | |
| interface MyInterface { | |
| prop: string; | |
| } | |
| let d = MyInterface; // Error: should show better message | |
| // Test with generic type alias | |
| type GenericAlias<T> = T[]; | |
| let e = GenericAlias; // Error: should show better message |
2 participants
#62780
Prevents the "Move to file" refactor from appearing when the selection
is inside an unbraced if/else branch (e.g.,
if (true) const x = 1;).Previously, invoking the refactor would generate invalid code by
extracting only part of a statement.
Added guards at three validation points:
Statements inside braced blocks remain unaffected and continue to work
correctly.
Fixes invalid code generation when refactoring unbraced control flow.