From 76871d2841659bdc603315963219a327fb0f538f Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Mon, 23 Apr 2018 15:02:04 -0700 Subject: [PATCH 01/12] Add 'move to new file' refactor --- src/compiler/core.ts | 4 +- src/compiler/diagnosticMessages.json | 4 + src/harness/fourslash.ts | 61 ++- src/harness/tsconfig.json | 1 + src/server/tsconfig.json | 1 + src/server/tsconfig.library.json | 1 + src/services/codefixes/convertToEs6Module.ts | 9 - .../codefixes/fixInvalidImportSyntax.ts | 2 +- src/services/codefixes/useDefaultImport.ts | 2 +- src/services/importTracker.ts | 15 +- src/services/refactorProvider.ts | 4 +- src/services/refactors/extractSymbol.ts | 4 +- src/services/refactors/moveToNewFile.ts | 359 ++++++++++++++++++ src/services/textChanges.ts | 20 +- src/services/tsconfig.json | 1 + src/services/utilities.ts | 34 ++ tests/cases/fourslash/fourslash.ts | 5 +- tests/cases/fourslash/moveToNewFile.ts | 23 ++ .../moveToNewFile_declarationKinds.ts | 38 ++ .../fourslash/moveToNewFile_defaultExport.ts | 25 ++ .../fourslash/moveToNewFile_defaultImport.ts | 17 + tests/cases/fourslash/moveToNewFile_global.ts | 16 + .../cases/fourslash/moveToNewFile_multiple.ts | 28 ++ .../moveToNewFile_newModuleNameUnique.ts | 20 + .../fourslash/moveToNewFile_onlyStatements.ts | 16 + .../fourslash/moveToNewFile_rangeInvalid.ts | 13 + .../fourslash/moveToNewFile_rangeSemiValid.ts | 19 + .../fourslash/moveToNewFile_updateUses.ts | 27 ++ 28 files changed, 724 insertions(+), 45 deletions(-) create mode 100644 src/services/refactors/moveToNewFile.ts create mode 100644 tests/cases/fourslash/moveToNewFile.ts create mode 100644 tests/cases/fourslash/moveToNewFile_declarationKinds.ts create mode 100644 tests/cases/fourslash/moveToNewFile_defaultExport.ts create mode 100644 tests/cases/fourslash/moveToNewFile_defaultImport.ts create mode 100644 tests/cases/fourslash/moveToNewFile_global.ts create mode 100644 tests/cases/fourslash/moveToNewFile_multiple.ts create mode 100644 tests/cases/fourslash/moveToNewFile_newModuleNameUnique.ts create mode 100644 tests/cases/fourslash/moveToNewFile_onlyStatements.ts create mode 100644 tests/cases/fourslash/moveToNewFile_rangeInvalid.ts create mode 100644 tests/cases/fourslash/moveToNewFile_rangeSemiValid.ts create mode 100644 tests/cases/fourslash/moveToNewFile_updateUses.ts diff --git a/src/compiler/core.ts b/src/compiler/core.ts index f0220fb8c93a5..034725bf0d0f2 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -311,8 +311,8 @@ namespace ts { } /** Works like Array.prototype.findIndex, returning `-1` if no element satisfying the predicate is found. */ - export function findIndex(array: ReadonlyArray, predicate: (element: T, index: number) => boolean): number { - for (let i = 0; i < array.length; i++) { + export function findIndex(array: ReadonlyArray, predicate: (element: T, index: number) => boolean, startIndex = 0): number { + for (let i = startIndex; i < array.length; i++) { if (predicate(array[i], i)) { return i; } diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index e81cd4fbc221a..d08d81ea1e4be 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -4174,5 +4174,9 @@ "Generate 'get' and 'set' accessors": { "category": "Message", "code": 95046 + }, + "Move to new file": { + "category": "Message", + "code": 95047 } } diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 7dfff7dd18995..ca0051e0bb978 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2950,9 +2950,7 @@ Actual: ${stringify(fullActual)}`); } public verifyApplicableRefactorAvailableAtMarker(negative: boolean, markerName: string) { - const marker = this.getMarkerByName(markerName); - const applicableRefactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, marker.position, ts.defaultPreferences); - const isAvailable = applicableRefactors && applicableRefactors.length > 0; + const isAvailable = this.getApplicableRefactors(this.getMarkerByName(markerName).position).length > 0; if (negative && isAvailable) { this.raiseError(`verifyApplicableRefactorAvailableAtMarker failed - expected no refactor at marker ${markerName} but found some.`); } @@ -2969,9 +2967,7 @@ Actual: ${stringify(fullActual)}`); } public verifyRefactorAvailable(negative: boolean, name: string, actionName?: string) { - const selection = this.getSelection(); - - let refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, selection, ts.defaultPreferences) || []; + let refactors = this.getApplicableRefactors(this.getSelection()); refactors = refactors.filter(r => r.name === name && (actionName === undefined || r.actions.some(a => a.name === actionName))); const isAvailable = refactors.length > 0; @@ -2991,10 +2987,7 @@ Actual: ${stringify(fullActual)}`); } public verifyRefactor({ name, actionName, refactors }: FourSlashInterface.VerifyRefactorOptions) { - const selection = this.getSelection(); - - const actualRefactors = (this.languageService.getApplicableRefactors(this.activeFile.fileName, selection, ts.defaultPreferences) || ts.emptyArray) - .filter(r => r.name === name && r.actions.some(a => a.name === actionName)); + const actualRefactors = this.getApplicableRefactors(this.getSelection()).filter(r => r.name === name && r.actions.some(a => a.name === actionName)); this.assertObjectsEqual(actualRefactors, refactors); } @@ -3004,8 +2997,7 @@ Actual: ${stringify(fullActual)}`); throw new Error("Exactly one refactor range is allowed per test."); } - const applicableRefactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, ts.first(ranges), ts.defaultPreferences); - const isAvailable = applicableRefactors && applicableRefactors.length > 0; + const isAvailable = this.getApplicableRefactors(ts.first(ranges)).length > 0; if (negative && isAvailable) { this.raiseError(`verifyApplicableRefactorAvailableForRange failed - expected no refactor but found some.`); } @@ -3016,7 +3008,7 @@ Actual: ${stringify(fullActual)}`); public applyRefactor({ refactorName, actionName, actionDescription, newContent: newContentWithRenameMarker }: FourSlashInterface.ApplyRefactorOptions) { const range = this.getSelection(); - const refactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, range, ts.defaultPreferences); + const refactors = this.getApplicableRefactors(range); const refactorsWithName = refactors.filter(r => r.name === refactorName); if (refactorsWithName.length === 0) { this.raiseError(`The expected refactor: ${refactorName} is not available at the marker location.\nAvailable refactors: ${refactors.map(r => r.name)}`); @@ -3062,7 +3054,38 @@ Actual: ${stringify(fullActual)}`); return { renamePosition, newContent }; } } + } + + public moveToNewFile(options: FourSlashInterface.MoveToNewFileOptions): void { + assert(this.getRanges().length === 1); + const range = this.getRanges()[0]; + const refactor = ts.find(this.getApplicableRefactors(range), r => r.name === "Move to new file"); + assert(refactor.actions.length === 1); + const action = ts.first(refactor.actions); + assert(action.name === "Move to new file" && action.description === "Move to new file"); + + const editInfo = this.languageService.getEditsForRefactor(this.activeFile.fileName, this.formatCodeSettings, range, refactor.name, action.name, ts.defaultPreferences); + for (const edit of editInfo.edits) { + const newContent = options.newFileContents[edit.fileName]; + if (newContent === undefined) { + this.raiseError(`There was an edit in ${edit.fileName} but new content was not specified.`); + } + if (this.testData.files.some(f => f.fileName === edit.fileName)) { + this.applyEdits(edit.fileName, edit.textChanges, /*isFormattingEdit*/ false); + this.openFile(edit.fileName); + this.verifyCurrentFileContent(newContent); + } + else { + assert(edit.textChanges.length === 1); + const change = ts.first(edit.textChanges); + assert.deepEqual(change.span, ts.createTextSpan(0, 0)); + assert.equal(change.newText, newContent, `Content for ${edit.fileName}`); + } + } + for (const fileName in options.newFileContents) { + assert(editInfo.edits.some(e => e.fileName === fileName)); + } } public verifyFileAfterApplyingRefactorAtMarker( @@ -3278,6 +3301,10 @@ Actual: ${stringify(fullActual)}`); this.verifyCurrentFileContent(options.newFileContents[fileName]); } } + + private getApplicableRefactors(positionOrRange: number | ts.TextRange): ReadonlyArray { + return this.languageService.getApplicableRefactors(this.activeFile.fileName, positionOrRange, ts.defaultPreferences) || ts.emptyArray; + } } export function runFourSlashTest(basePath: string, testType: FourSlashTestType, fileName: string) { @@ -4373,6 +4400,10 @@ namespace FourSlashInterface { public getEditsForFileRename(options: GetEditsForFileRenameOptions) { this.state.getEditsForFileRename(options); } + + public moveToNewFile(options: MoveToNewFileOptions): void { + this.state.moveToNewFile(options); + } } export class Edit { @@ -4721,4 +4752,8 @@ namespace FourSlashInterface { readonly newPath: string; readonly newFileContents: { readonly [fileName: string]: string }; } + + export interface MoveToNewFileOptions { + readonly newFileContents: { readonly [fileName: string]: string }; + } } diff --git a/src/harness/tsconfig.json b/src/harness/tsconfig.json index a4ad8cb37975f..44b83f257bbe6 100644 --- a/src/harness/tsconfig.json +++ b/src/harness/tsconfig.json @@ -115,6 +115,7 @@ "../services/codefixes/useDefaultImport.ts", "../services/refactors/extractSymbol.ts", "../services/refactors/generateGetAccessorAndSetAccessor.ts", + "../services/refactors/moveToNewFile.ts", "../services/sourcemaps.ts", "../services/services.ts", "../services/breakpoints.ts", diff --git a/src/server/tsconfig.json b/src/server/tsconfig.json index e6768f4edeed1..7ab06cf0666a0 100644 --- a/src/server/tsconfig.json +++ b/src/server/tsconfig.json @@ -111,6 +111,7 @@ "../services/codefixes/useDefaultImport.ts", "../services/refactors/extractSymbol.ts", "../services/refactors/generateGetAccessorAndSetAccessor.ts", + "../services/refactors/moveToNewFile.ts", "../services/sourcemaps.ts", "../services/services.ts", "../services/breakpoints.ts", diff --git a/src/server/tsconfig.library.json b/src/server/tsconfig.library.json index a167d3b39a2ff..a57331f7d1550 100644 --- a/src/server/tsconfig.library.json +++ b/src/server/tsconfig.library.json @@ -117,6 +117,7 @@ "../services/codefixes/useDefaultImport.ts", "../services/refactors/extractSymbol.ts", "../services/refactors/generateGetAccessorAndSetAccessor.ts", + "../services/refactors/moveToNewFile.ts", "../services/sourcemaps.ts", "../services/services.ts", "../services/breakpoints.ts", diff --git a/src/services/codefixes/convertToEs6Module.ts b/src/services/codefixes/convertToEs6Module.ts index a8df21dc96d2d..39e8f1059dfc5 100644 --- a/src/services/codefixes/convertToEs6Module.ts +++ b/src/services/codefixes/convertToEs6Module.ts @@ -507,15 +507,6 @@ namespace ts.codefix { : makeImport(/*name*/ undefined, [makeImportSpecifier(propertyName, localName)], moduleSpecifier); } - function makeImport(name: Identifier | undefined, namedImports: ReadonlyArray | undefined, moduleSpecifier: StringLiteralLike): ImportDeclaration { - return makeImportDeclaration(name, namedImports, moduleSpecifier); - } - - export function makeImportDeclaration(name: Identifier, namedImports: ReadonlyArray | undefined, moduleSpecifier: Expression) { - const importClause = (name || namedImports) && createImportClause(name, namedImports && createNamedImports(namedImports)); - return createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, importClause, moduleSpecifier); - } - function makeImportSpecifier(propertyName: string | undefined, name: string): ImportSpecifier { return createImportSpecifier(propertyName !== undefined && propertyName !== name ? createIdentifier(propertyName) : undefined, createIdentifier(name)); } diff --git a/src/services/codefixes/fixInvalidImportSyntax.ts b/src/services/codefixes/fixInvalidImportSyntax.ts index 50bf41360bce3..89a9aca461cb5 100644 --- a/src/services/codefixes/fixInvalidImportSyntax.ts +++ b/src/services/codefixes/fixInvalidImportSyntax.ts @@ -28,7 +28,7 @@ namespace ts.codefix { const variations: CodeFixAction[] = []; // import Bluebird from "bluebird"; - variations.push(createAction(context, sourceFile, node, makeImportDeclaration(namespace.name, /*namedImports*/ undefined, node.moduleSpecifier))); + variations.push(createAction(context, sourceFile, node, makeImport(namespace.name, /*namedImports*/ undefined, node.moduleSpecifier))); if (getEmitModuleKind(opts) === ModuleKind.CommonJS) { // import Bluebird = require("bluebird"); diff --git a/src/services/codefixes/useDefaultImport.ts b/src/services/codefixes/useDefaultImport.ts index 632cfc97e0075..99fc145b5630a 100644 --- a/src/services/codefixes/useDefaultImport.ts +++ b/src/services/codefixes/useDefaultImport.ts @@ -37,6 +37,6 @@ namespace ts.codefix { } function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, info: Info): void { - changes.replaceNode(sourceFile, info.importNode, makeImportDeclaration(info.name, /*namedImports*/ undefined, info.moduleSpecifier)); + changes.replaceNode(sourceFile, info.importNode, makeImport(info.name, /*namedImports*/ undefined, info.moduleSpecifier)); } } diff --git a/src/services/importTracker.ts b/src/services/importTracker.ts index 1a4b2a9f75195..5a4bc20676a68 100644 --- a/src/services/importTracker.ts +++ b/src/services/importTracker.ts @@ -248,7 +248,7 @@ namespace ts.FindAllReferences { const { name } = importClause; // If a default import has the same name as the default export, allow to rename it. // Given `import f` and `export default function f`, we will rename both, but for `import g` we will rename just that. - if (name && (!isForRename || name.escapedText === symbolName(exportSymbol))) { + if (name && (!isForRename || name.escapedText === symbolEscapedNameNoDefault(exportSymbol))) { const defaultImportAlias = checker.getSymbolAtLocation(name); addSearch(name, defaultImportAlias); } @@ -534,7 +534,7 @@ namespace ts.FindAllReferences { // If the import has a different name than the export, do not continue searching. // If `importedName` is undefined, do continue searching as the export is anonymous. // (All imports returned from this function will be ignored anyway if we are in rename and this is a not a named export.) - const importedName = symbolName(importedSymbol); + const importedName = symbolEscapedNameNoDefault(importedSymbol); if (importedName === undefined || importedName === InternalSymbolName.Default || importedName === symbol.escapedName) { return { kind: ImportExport.Import, symbol: importedSymbol, ...isImport }; } @@ -606,17 +606,6 @@ namespace ts.FindAllReferences { return isExternalModuleSymbol(exportingModuleSymbol) ? { exportingModuleSymbol, exportKind } : undefined; } - function symbolName(symbol: Symbol): __String | undefined { - if (symbol.escapedName !== InternalSymbolName.Default) { - return symbol.escapedName; - } - - return forEach(symbol.declarations, decl => { - const name = getNameOfDeclaration(decl); - return name && name.kind === SyntaxKind.Identifier && name.escapedText; - }); - } - /** If at an export specifier, go to the symbol it refers to. */ function skipExportSpecifierSymbol(symbol: Symbol, checker: TypeChecker): Symbol { // For `export { foo } from './bar", there's nothing to skip, because it does not create a new alias. But `export { foo } does. diff --git a/src/services/refactorProvider.ts b/src/services/refactorProvider.ts index f3504ed89b82d..adb3af3ea7adc 100644 --- a/src/services/refactorProvider.ts +++ b/src/services/refactorProvider.ts @@ -38,7 +38,7 @@ namespace ts { } } - export function getRefactorContextLength(context: RefactorContext): number { - return context.endPosition === undefined ? 0 : context.endPosition - context.startPosition; + export function getRefactorContextSpan({ startPosition, endPosition }: RefactorContext): TextSpan { + return createTextSpanFromBounds(startPosition, endPosition === undefined ? startPosition : endPosition); } } diff --git a/src/services/refactors/extractSymbol.ts b/src/services/refactors/extractSymbol.ts index 5805a9861801e..354c200217943 100644 --- a/src/services/refactors/extractSymbol.ts +++ b/src/services/refactors/extractSymbol.ts @@ -8,7 +8,7 @@ namespace ts.refactor.extractSymbol { * Exported for tests. */ export function getAvailableActions(context: RefactorContext): ApplicableRefactorInfo[] | undefined { - const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length: getRefactorContextLength(context) }); + const rangeToExtract = getRangeToExtract(context.file, getRefactorContextSpan(context)); const targetRange: TargetRange = rangeToExtract.targetRange; if (targetRange === undefined) { @@ -87,7 +87,7 @@ namespace ts.refactor.extractSymbol { /* Exported for tests */ export function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined { - const rangeToExtract = getRangeToExtract(context.file, { start: context.startPosition, length: getRefactorContextLength(context) }); + const rangeToExtract = getRangeToExtract(context.file, getRefactorContextSpan(context)); const targetRange: TargetRange = rangeToExtract.targetRange; const parsedFunctionIndexMatch = /^function_scope_(\d+)$/.exec(actionName); diff --git a/src/services/refactors/moveToNewFile.ts b/src/services/refactors/moveToNewFile.ts new file mode 100644 index 0000000000000..ad461d880d9e9 --- /dev/null +++ b/src/services/refactors/moveToNewFile.ts @@ -0,0 +1,359 @@ +/* @internal */ +namespace ts.refactor { + const refactorName = "Move to new file"; + registerRefactor(refactorName, { + getAvailableActions(context): ApplicableRefactorInfo[] { + if (getStatementsToMove(context) === undefined) return undefined; + const description = getLocaleSpecificMessage(Diagnostics.Move_to_new_file); + return [{ name: refactorName, description, actions: [{ name: refactorName, description }] }]; + }, + getEditsForAction(context, actionName): RefactorEditInfo { + Debug.assert(actionName === refactorName); + const statements = Debug.assertDefined(getStatementsToMove(context)); + const edits = textChanges.ChangeTracker.with(context, t => doChange(context.file, context.program, statements, t, context.host)); + return { edits, renameFilename: undefined, renameLocation: undefined }; + } + }); + + function getStatementsToMove(context: RefactorContext): ReadonlyArray | undefined { + const { file } = context; + const range = createTextRangeFromSpan(getRefactorContextSpan(context)); + const { statements } = file; + + const startNodeIndex = findIndex(statements, s => s.end > range.pos); + if (startNodeIndex === -1) return undefined; + // Can't only partially include the start node or be partially into the next node + const okStart = range.pos <= statements[startNodeIndex].getStart(file); + const afterEndNodeIndex = findIndex(statements, s => s.end > range.end, startNodeIndex); + // Can't be partially into the next node + const okEnd = afterEndNodeIndex === -1 || afterEndNodeIndex !== 0 && statements[afterEndNodeIndex].getStart(file) >= range.end; + return okStart && okEnd ? statements.slice(startNodeIndex, afterEndNodeIndex === -1 ? statements.length : afterEndNodeIndex) : undefined; + } + + function doChange(oldFile: SourceFile, program: Program, toMove: ReadonlyArray, changes: textChanges.ChangeTracker, host: LanguageServiceHost): void { + const checker = program.getTypeChecker(); + const usage = getUsageInfo(oldFile, toMove, checker); + + const currentDirectory = getDirectoryPath(oldFile.fileName); + const extension = extensionFromPath(oldFile.fileName); + const newModuleName = makeUniqueModuleName(getNewModuleName(usage.movedSymbols), extension, currentDirectory, host); + const newFileNameWithExtension = newModuleName + extension; + + // If previous file was global, this is easy. + changes.createNewFile(oldFile, combinePaths(currentDirectory, newFileNameWithExtension), getNewStatements(oldFile, usage, changes, toMove, program, newModuleName)); + } + + function getNewStatements( + oldFile: SourceFile, usage: UsageInfo, changes: textChanges.ChangeTracker, toMove: ReadonlyArray, program: Program, newModuleName: string, + ): ReadonlyArray { + const checker = program.getTypeChecker(); + + if (!oldFile.externalModuleIndicator) { + changes.deleteNodeRange(oldFile, first(toMove), last(toMove)); + return toMove; + } + + const importsFromNewFile = createOldFileImportsFromNewFile(usage.oldFileImportsFromNewFile, newModuleName); + if (importsFromNewFile) { + changes.insertNodeBefore(oldFile, oldFile.statements[0], importsFromNewFile, /*blankLineBetween*/ true); + } + + deleteUnusedOldImports(oldFile, toMove, changes, usage.unusedImportsFromOldFile, checker); + changes.deleteNodeRange(oldFile, first(toMove), last(toMove)); + + updateImportsInOtherFiles(changes, program, oldFile, usage.movedSymbols, newModuleName); + + return [ + ...getNewFileImportsAndAddExportInOldFile(oldFile, usage.oldImportsNeededByNewFile, usage.newFileImportsFromOldFile, changes, checker), + ...addExports(toMove, usage.oldFileImportsFromNewFile), + ]; + } + + function deleteUnusedOldImports(oldFile: SourceFile, toMove: ReadonlyArray, changes: textChanges.ChangeTracker, toDelete: ReadonlySymbolSet, checker: TypeChecker) { + for (const statement of oldFile.statements) { + if (!contains(toMove, statement) && isImportDeclaration(statement)) { + deleteUnusedImports(oldFile, statement, changes, name => toDelete.has(checker.getSymbolAtLocation(name))); + } + } + } + + function updateImportsInOtherFiles(changes: textChanges.ChangeTracker, program: Program, oldFile: SourceFile, movedSymbols: ReadonlySymbolSet, newModuleName: string): void { + const checker = program.getTypeChecker(); + for (const sourceFile of program.getSourceFiles()) { + if (sourceFile === oldFile) continue; + for (const statement of sourceFile.statements) { + if (!isImportDeclaration(statement) || !isStringLiteral(statement.moduleSpecifier)) continue; + + const shouldMove = (name: Identifier): boolean => movedSymbols.has(skipAlias(checker.getSymbolAtLocation(name), checker)); + deleteUnusedImports(sourceFile, statement, changes, shouldMove); + const newModuleSpecifier = combinePaths(getDirectoryPath(statement.moduleSpecifier.text), newModuleName); + const newImportDeclaration = filterImport(statement, createLiteral(newModuleSpecifier), shouldMove); + if (newImportDeclaration) changes.insertNodeAfter(sourceFile, statement, newImportDeclaration); + } + } + } + + function createOldFileImportsFromNewFile(newFileNeedExport: ReadonlySymbolSet, newFileNameWithExtension: string): ImportDeclaration | undefined { + let defaultImport: Identifier | undefined; + const imports: ImportSpecifier[] = []; + newFileNeedExport.forEach(symbol => { + if (symbol.escapedName === InternalSymbolName.Default) { + defaultImport = createIdentifier(symbolNameNoDefault(symbol)); + } + else { + imports.push(createImportSpecifier(undefined, createIdentifier(symbol.name))); + } + }); + return makeImportIfNecessary(defaultImport, imports, ensurePathIsRelative(newFileNameWithExtension)); + } + + function addExports(toMove: ReadonlyArray, needExport: ReadonlySymbolSet): ReadonlyArray { + return toMove.map(statement => { + return !hasModifier(statement, ModifierFlags.Export) && forEachTopLevelDeclaration(statement, d => needExport.has(Debug.assertDefined(d.symbol))) + ? addExport(statement as TopLevelDeclarationStatement) + : statement; + }); + } + + function deleteUnusedImports(sourceFile: SourceFile, importDecl: ImportDeclaration, changes: textChanges.ChangeTracker, isUnused: (name: Identifier) => boolean): void { + if (!importDecl.importClause) return; + const { name, namedBindings } = importDecl.importClause; + const defaultUnused = !name || isUnused(name); + const namedBindingsUnused = !namedBindings || + (namedBindings.kind === SyntaxKind.NamespaceImport ? isUnused(namedBindings.name) : namedBindings.elements.every(e => isUnused(e.name))); + if (defaultUnused && namedBindingsUnused) { + changes.deleteNode(sourceFile, importDecl); + } + else { + if (name && defaultUnused) { + changes.deleteNode(sourceFile, name); + } + if (namedBindings) { + if (namedBindingsUnused) { + changes.deleteNode(sourceFile, namedBindings); + } + else if (namedBindings.kind === SyntaxKind.NamedImports) { + for (const element of namedBindings.elements) { + if (isUnused(element.name)) changes.deleteNodeInList(sourceFile, element); + } + } + } + } + } + + function getNewFileImportsAndAddExportInOldFile( + oldFile: SourceFile, + importsToCopy: ReadonlySymbolSet, + newFileImportsFromOldFile: ReadonlySymbolSet, + changes: textChanges.ChangeTracker, + checker: TypeChecker, + ): ReadonlyArray { + const copiedOldImports = mapDefined(oldFile.statements, oldStatement => + isImportDeclaration(oldStatement) + ? filterImport(oldStatement, oldStatement.moduleSpecifier, name => importsToCopy.has(checker.getSymbolAtLocation(name))) + : undefined); + + // Also, import things used from the old file, and insert 'export' modifiers as necessary in the old file. + let oldFileDefault: Identifier | undefined; + const oldFileNamedImports: ImportSpecifier[] = []; + const markSeenTop = nodeSeenTracker(); // Needed because multiple declarations may appear in `const x = 0, y = 1;`. + newFileImportsFromOldFile.forEach(symbol => { + for (const decl of symbol.declarations) { + if (!isTopLevelDeclaration(decl) || !isIdentifier(decl.name)) continue; + + const top = getTopLevelDeclarationStatement(decl); + if (markSeenTop(top) && !hasModifier(top, ModifierFlags.Export)) { + changes.insertExportModifier(oldFile, top); + } + if (hasModifier(decl, ModifierFlags.Default)) { + oldFileDefault = decl.name; + } + else { + oldFileNamedImports.push(createImportSpecifier(undefined, decl.name)); + } + } + }); + + const oldFileImport = makeImportIfNecessary(oldFileDefault, oldFileNamedImports, `./${removeFileExtension(getBaseFileName(oldFile.fileName))}`); + return [...copiedOldImports, ...(oldFileImport ? [oldFileImport] : emptyArray)]; + } + + function makeUniqueModuleName(moduleName: string, extension: string, inDirectory: string, host: LanguageServiceHost): string { + while (true) { + const name = combinePaths(inDirectory, moduleName + extension); + if (!host.fileExists(name)) return moduleName; + moduleName += "0"; + } + } + + function getNewModuleName(movedSymbols: ReadonlySymbolSet): string { + let name: string | undefined; + movedSymbols.forEach(s => { if (name === undefined) name = symbolNameNoDefault(s); }); + return name === undefined ? "newFile" : name; + } + + interface UsageInfo { + // Symbols whose declarations are moved from the old file to the new file. + readonly movedSymbols: ReadonlySymbolSet; + + // Symbols declared in the old file that must be imported by the new file. (May not already be exported.) + readonly newFileImportsFromOldFile: ReadonlySymbolSet; + // Subset of movedSymbols that are still used elsewhere in the old file and must be imported back. + readonly oldFileImportsFromNewFile: ReadonlySymbolSet; + + readonly oldImportsNeededByNewFile: ReadonlySymbolSet; + // Subset of oldImportsNeededByNewFile that are will no longer be used in the old file. + readonly unusedImportsFromOldFile: ReadonlySymbolSet; + } + function getUsageInfo(oldFile: SourceFile, toMove: ReadonlyArray, checker: TypeChecker): UsageInfo { + const movedSymbols = new SymbolSet(); + const oldImportsNeededByNewFile = new SymbolSet(); + const newFileImportsFromOldFile = new SymbolSet(); + + for (const statement of toMove) { + forEachTopLevelDeclaration(statement, decl => { + movedSymbols.add(Debug.assertDefined(decl.symbol)); + }); + + forEachReference(statement, checker, symbol => { + if (!symbol.declarations) return; + for (const decl of symbol.declarations) { + if (isImportSpecifier(decl) || isImportClause(decl.parent)) { + oldImportsNeededByNewFile.add(symbol); + } + else if (isTopLevelDeclaration(decl) && !movedSymbols.has(symbol)) { + newFileImportsFromOldFile.add(symbol); + } + } + }); + } + + const unusedImportsFromOldFile = oldImportsNeededByNewFile.clone(); + + const oldFileImportsFromNewFile = new SymbolSet(); + for (const statement of oldFile.statements) { + if (contains(toMove, statement)) continue; + + forEachReference(statement, checker, symbol => { + if (movedSymbols.has(symbol)) oldFileImportsFromNewFile.add(symbol); + unusedImportsFromOldFile.delete(symbol); + }); + } + + return { movedSymbols, newFileImportsFromOldFile, oldFileImportsFromNewFile, oldImportsNeededByNewFile, unusedImportsFromOldFile }; + } + + // Below should all be utilities + + function filterImport(i: ImportDeclaration, moduleSpecifier: Expression, keep: (name: Identifier) => boolean): ImportDeclaration | undefined { + const clause = i.importClause; + const defaultImport = clause.name && keep(clause.name) ? clause.name : undefined; + const namedBindings = clause.namedBindings && filterNamedBindings(clause.namedBindings, keep); + return defaultImport || namedBindings ? createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, createImportClause(defaultImport, namedBindings), moduleSpecifier) : undefined; + } + function filterNamedBindings(namedBindings: NamedImportBindings, keep: (name: Identifier) => boolean): NamedImportBindings | undefined { + if (namedBindings.kind === SyntaxKind.NamespaceImport) { + return keep(namedBindings.name) ? namedBindings : undefined; + } + else { + const newElements = namedBindings.elements.filter(e => keep(e.name)); + return newElements.length ? createNamedImports(newElements) : undefined; + } + } + + function forEachReference(node: Node, checker: TypeChecker, onReference: (s: Symbol) => void) { + node.forEachChild(function cb(node) { + if (isIdentifier(node) && !isDeclarationName(node)) { + const sym = checker.getSymbolAtLocation(node); + if (sym) onReference(sym); + } + else { + node.forEachChild(cb); + } + }); + } + + interface ReadonlySymbolSet { + has(symbol: Symbol): boolean; + forEach(cb: (symbol: Symbol) => void): void; + } + class SymbolSet implements ReadonlySymbolSet { + private map = createMap(); + add(symbol: Symbol): void { + this.map.set(String(getSymbolId(symbol)), symbol); + } + has(symbol: Symbol): boolean { + return this.map.has(String(getSymbolId(symbol))); + } + delete(symbol: Symbol): void { + this.map.delete(String(getSymbolId(symbol))); + } + forEach(cb: (symbol: Symbol) => void): void { + this.map.forEach(cb); + } + clone(): SymbolSet { + const clone = new SymbolSet(); + copyEntries(this.map, clone.map); + return clone; + } + } + + type NonVariableTopLevelDeclaration = FunctionDeclaration | ClassDeclaration | EnumDeclaration | TypeAliasDeclaration | InterfaceDeclaration | ModuleDeclaration; + type TopLevelDeclarationStatement = NonVariableTopLevelDeclaration | VariableStatement; + interface TopLevelVariableDeclaration extends VariableDeclaration { parent: VariableDeclarationList & { parent: VariableStatement; }; } + type TopLevelDeclaration = NonVariableTopLevelDeclaration | TopLevelVariableDeclaration; + function isTopLevelDeclaration(node: Node): node is TopLevelDeclaration { + switch (node.kind) { + case SyntaxKind.FunctionDeclaration: + case SyntaxKind.ClassDeclaration: + case SyntaxKind.ModuleDeclaration: + case SyntaxKind.EnumDeclaration: + case SyntaxKind.TypeAliasDeclaration: + case SyntaxKind.InterfaceDeclaration: + return isSourceFile(node.parent); + case SyntaxKind.VariableDeclaration: + return isSourceFile((node as VariableDeclaration).parent.parent.parent); + } + } + + function forEachTopLevelDeclaration(statement: Statement, cb: (node: TopLevelDeclaration) => T): T { + switch (statement.kind) { + case SyntaxKind.FunctionDeclaration: + case SyntaxKind.ClassDeclaration: + case SyntaxKind.ModuleDeclaration: + case SyntaxKind.EnumDeclaration: + case SyntaxKind.TypeAliasDeclaration: + case SyntaxKind.InterfaceDeclaration: + return cb(statement as FunctionDeclaration | ClassDeclaration | EnumDeclaration | ModuleDeclaration | TypeAliasDeclaration | InterfaceDeclaration); + + case SyntaxKind.VariableStatement: + return forEach((statement as VariableStatement).declarationList.declarations as ReadonlyArray, cb); + } + } + + function getTopLevelDeclarationStatement(d: TopLevelDeclaration): TopLevelDeclarationStatement { + return isVariableDeclaration(d) ? d.parent.parent : d; + } + + function addExport(d: TopLevelDeclarationStatement): TopLevelDeclarationStatement { + const modifiers = concatenate([createModifier(SyntaxKind.ExportKeyword)], d.modifiers); + switch (d.kind) { + case SyntaxKind.FunctionDeclaration: + return updateFunctionDeclaration(d, d.decorators, modifiers, d.asteriskToken, d.name, d.typeParameters, d.parameters, d.type, d.body); + case SyntaxKind.ClassDeclaration: + return updateClassDeclaration(d, d.decorators, modifiers, d.name, d.typeParameters, d.heritageClauses, d.members); + case SyntaxKind.VariableStatement: + return updateVariableStatement(d, modifiers, d.declarationList); + case SyntaxKind.ModuleDeclaration: + return updateModuleDeclaration(d, d.decorators, modifiers, d.name, d.body); + case SyntaxKind.EnumDeclaration: + return updateEnumDeclaration(d, d.decorators, modifiers, d.name, d.members); + case SyntaxKind.TypeAliasDeclaration: + return updateTypeAliasDeclaration(d, d.decorators, modifiers, d.name, d.typeParameters, d.type); + case SyntaxKind.InterfaceDeclaration: + return updateInterfaceDeclaration(d, d.decorators, modifiers, d.name, d.typeParameters, d.heritageClauses, d.members); + default: + Debug.assertNever(d); + } + } +} diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 648ab77a922e9..7a566ccd71929 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -211,6 +211,7 @@ namespace ts.textChanges { export class ChangeTracker { private readonly changes: Change[] = []; + private readonly newFiles: { readonly oldFile: SourceFile, readonly fileName: string, readonly statements: ReadonlyArray }[] = []; private readonly deletedNodesInLists: true[] = []; // Stores ids of nodes in lists that we already deleted. Used to avoid deleting `, ` twice in `a, b`. private readonly classesWithNodesInsertedAtStart = createMap(); // Set implemented as Map @@ -517,6 +518,10 @@ namespace ts.textChanges { } } + public insertExportModifier(sourceFile: SourceFile, node: DeclarationStatement | VariableStatement): void { + this.insertText(sourceFile, node.getStart(sourceFile), "export "); + } + /** * This function should be used to insert nodes in lists when nodes don't carry separators as the part of the node range, * i.e. arguments in arguments lists, parameters in parameter lists etc. @@ -654,7 +659,15 @@ namespace ts.textChanges { */ public getChanges(validate?: ValidateNonFormattedText): FileTextChanges[] { this.finishClassesWithNodesInsertedAtStart(); - return changesToText.getTextChangesFromChanges(this.changes, this.newLineCharacter, this.formatContext, validate); + const changes = changesToText.getTextChangesFromChanges(this.changes, this.newLineCharacter, this.formatContext, validate); + for (const { oldFile, fileName, statements } of this.newFiles) { + changes.push(changesToText.newFileChanges(oldFile, fileName, statements, this.newLineCharacter)); + } + return changes; + } + + public createNewFile(oldFile: SourceFile, fileName: string, statements: ReadonlyArray) { + this.newFiles.push({ oldFile, fileName, statements }); } } @@ -681,6 +694,11 @@ namespace ts.textChanges { }); } + export function newFileChanges(oldFile: SourceFile, fileName: string, statements: ReadonlyArray, newLineCharacter: string): FileTextChanges { + const text = statements.map(s => getNonformattedText(s, oldFile, newLineCharacter).text).join(newLineCharacter); + return { fileName, textChanges: [createTextChange(createTextSpan(0, 0), text)] }; + } + function computeNewText(change: Change, sourceFile: SourceFile, newLineCharacter: string, formatContext: formatting.FormatContext, validate: ValidateNonFormattedText): string { if (change.kind === ChangeKind.Remove) { return ""; diff --git a/src/services/tsconfig.json b/src/services/tsconfig.json index 75b46a5c49bbe..2236c89300a25 100644 --- a/src/services/tsconfig.json +++ b/src/services/tsconfig.json @@ -108,6 +108,7 @@ "codefixes/useDefaultImport.ts", "refactors/extractSymbol.ts", "refactors/generateGetAccessorAndSetAccessor.ts", + "refactors/moveToNewFile.ts", "sourcemaps.ts", "services.ts", "breakpoints.ts", diff --git a/src/services/utilities.ts b/src/services/utilities.ts index 78f18869b372c..4fcda5be1bd15 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1149,6 +1149,10 @@ namespace ts { return createTextSpanFromBounds(range.pos, range.end); } + export function createTextRangeFromSpan(span: TextSpan): TextRange { + return createTextRange(span.start, span.start + span.length); + } + export function createTextChangeFromStartLength(start: number, length: number, newText: string): TextChange { return createTextChange(createTextSpan(start, length), newText); } @@ -1228,6 +1232,36 @@ namespace ts { export function hostGetCanonicalFileName(host: LanguageServiceHost): GetCanonicalFileName { return createGetCanonicalFileName(hostUsesCaseSensitiveFileNames(host)); } + + export function makeImportIfNecessary(defaultImport: Identifier | undefined, namedImports: ReadonlyArray | undefined, moduleSpecifier: string): ImportDeclaration | undefined { + return defaultImport || namedImports && namedImports.length ? makeImport(defaultImport, namedImports, moduleSpecifier) : undefined; + } + + export function makeImport(defaultImport: Identifier | undefined, namedImports: ReadonlyArray | undefined, moduleSpecifier: string | Expression): ImportDeclaration { + return createImportDeclaration( + /*decorators*/ undefined, + /*modifiers*/ undefined, + defaultImport || namedImports + ? createImportClause(defaultImport, namedImports && namedImports.length ? createNamedImports(namedImports) : undefined) + : undefined, + typeof moduleSpecifier === "string" ? createLiteral(moduleSpecifier) : moduleSpecifier); + } + + export function symbolNameNoDefault(symbol: Symbol): string | undefined { + const escaped = symbolEscapedNameNoDefault(symbol); + return escaped === undefined ? undefined : unescapeLeadingUnderscores(escaped); + } + + export function symbolEscapedNameNoDefault(symbol: Symbol): __String | undefined { + if (symbol.escapedName !== InternalSymbolName.Default) { + return symbol.escapedName; + } + + return firstDefined(symbol.declarations, decl => { + const name = getNameOfDeclaration(decl); + return name && name.kind === SyntaxKind.Identifier ? name.escapedText : undefined; + }); + } } // Display-part writer helpers diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index 42dee74bb03e3..9d7af75f7fb25 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -349,7 +349,10 @@ declare namespace FourSlashInterface { oldPath: string; newPath: string; newFileContents: { [fileName: string]: string }; - }); + }): void; + moveToNewFile(options: { + readonly newFileContents: { readonly [fileName: string]: string }; + }): void; } class edit { backspace(count?: number): void; diff --git a/tests/cases/fourslash/moveToNewFile.ts b/tests/cases/fourslash/moveToNewFile.ts new file mode 100644 index 0000000000000..ad61803c04f51 --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile.ts @@ -0,0 +1,23 @@ +/// + +// @Filename: /a.ts +////import { a, b } from "./other"; +////const p = 0; +////[|const y = p + b;|] +////y; + +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +`import { y } from "./y"; + +import { a } from "./other"; +export const p = 0; +y;`, + + "/y.ts": +`import { b } from "./other"; +import { p } from "./a"; +export const y = p + b;`, + }, +}); diff --git a/tests/cases/fourslash/moveToNewFile_declarationKinds.ts b/tests/cases/fourslash/moveToNewFile_declarationKinds.ts new file mode 100644 index 0000000000000..6420a9e8ce153 --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_declarationKinds.ts @@ -0,0 +1,38 @@ +/// + +// @Filename: /a.ts +////export {}; // make this a module +////[|const x = 0; +////function f() {} +////class C {} +////enum E {} +////namespace N { export const x = 0; } +////type T = number; +////interface I {}|] +////x; f; C; E; N; +////type U = T; type V = I; + +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +`import { x, f, C, E, N, T, I } from "./x"; + +export {}; // make this a module +x; f; C; E; N; +type U = T; type V = I;`, + + "/x.ts": +`export const x = 0; +export function f() { } +export class C { +} +export enum E { +} +export namespace N { + export const x = 0; +} +export type T = number; +export interface I { +}`, + }, +}); diff --git a/tests/cases/fourslash/moveToNewFile_defaultExport.ts b/tests/cases/fourslash/moveToNewFile_defaultExport.ts new file mode 100644 index 0000000000000..2a7e3e47a67c9 --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_defaultExport.ts @@ -0,0 +1,25 @@ +/// + +// @Filename: /a.ts +////[|export default function f() { }|] +////f(); + +// @Filename: /user.ts +////import f from "./a"; +////f(); + +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +`import f from "./f"; + +f();`, + + "/f.ts": +`export default function f() { }`, + + "/user.ts": +`import f from "./f"; +f();`, + }, +}); diff --git a/tests/cases/fourslash/moveToNewFile_defaultImport.ts b/tests/cases/fourslash/moveToNewFile_defaultImport.ts new file mode 100644 index 0000000000000..031da380885eb --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_defaultImport.ts @@ -0,0 +1,17 @@ +/// + +// @Filename: /a.ts +////export default function f() { } +////[|const x = f();|] + +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +`export default function f() { } +`, + + "/x.ts": +`import f from "./a"; +const x = f();`, + }, +}); diff --git a/tests/cases/fourslash/moveToNewFile_global.ts b/tests/cases/fourslash/moveToNewFile_global.ts new file mode 100644 index 0000000000000..04443c50cb272 --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_global.ts @@ -0,0 +1,16 @@ +/// + +// @Filename: /a.ts +////const x = y; +////[|const y = x;|] + +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +`const x = y; +`, + + "/y.ts": +`const y = x;`, + }, +}); diff --git a/tests/cases/fourslash/moveToNewFile_multiple.ts b/tests/cases/fourslash/moveToNewFile_multiple.ts new file mode 100644 index 0000000000000..2a235ad9d8f70 --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_multiple.ts @@ -0,0 +1,28 @@ +/// + +// @Filename: /a.ts +////export {}; // make this a module +////const a = 0, b = 0; +////[|const x = 0; +////a; +////const y = 1; +////b;|] +////x; y; + +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +`import { x, y } from "./x"; + +export {}; // make this a module +export const a = 0, b = 0; +x; y;`, + + "/x.ts": +`import { a, b } from "./a"; +export const x = 0; +a; +export const y = 1; +b;`, + }, +}); diff --git a/tests/cases/fourslash/moveToNewFile_newModuleNameUnique.ts b/tests/cases/fourslash/moveToNewFile_newModuleNameUnique.ts new file mode 100644 index 0000000000000..f05861ae5ba44 --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_newModuleNameUnique.ts @@ -0,0 +1,20 @@ +/// + +// @Filename: /a.ts +////[|export const x = 0;|] + +// @Filename: /x.ts +//// + +// @Filename: /x0.ts +//// + +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +``, + + "/x00.ts": +`export const x = 0;`, + }, +}); diff --git a/tests/cases/fourslash/moveToNewFile_onlyStatements.ts b/tests/cases/fourslash/moveToNewFile_onlyStatements.ts new file mode 100644 index 0000000000000..fe019d5e9afaf --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_onlyStatements.ts @@ -0,0 +1,16 @@ +/// + +// @Filename: /a.ts +////console.log("hello"); +////[|console.log("goodbye");|] + +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +`console.log("hello"); +`, + + "/newFile.ts": +`console.log("goodbye");`, + }, +}); diff --git a/tests/cases/fourslash/moveToNewFile_rangeInvalid.ts b/tests/cases/fourslash/moveToNewFile_rangeInvalid.ts new file mode 100644 index 0000000000000..cd7ec49cd04d5 --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_rangeInvalid.ts @@ -0,0 +1,13 @@ +/// + +// @Filename: /a.ts +////[|const x = 0; +////const|] y = 0; +////function f() { +//// [|function inner() {}|] +////} + +for (const range of test.ranges()) { + goTo.selectRange(range); + verify.not.refactorAvailable("Move to new file") +} diff --git a/tests/cases/fourslash/moveToNewFile_rangeSemiValid.ts b/tests/cases/fourslash/moveToNewFile_rangeSemiValid.ts new file mode 100644 index 0000000000000..0f5636f33cff8 --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_rangeSemiValid.ts @@ -0,0 +1,19 @@ +/// + +// @Filename: /a.ts +////[|const x = 0; +//// +/////** Comm|]ent */ +////const y = 0; + +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +` +/** Comment */ +const y = 0;`, + + "/x.ts": +`const x = 0;`, + }, +}); diff --git a/tests/cases/fourslash/moveToNewFile_updateUses.ts b/tests/cases/fourslash/moveToNewFile_updateUses.ts new file mode 100644 index 0000000000000..b067889625eda --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_updateUses.ts @@ -0,0 +1,27 @@ +/// + +// @Filename: /a.ts +////export const x = 0; +////[|export const y = 0;|] + +// @Filename: /user.ts +////import { x, y } from "./a"; +//// + +// TODO: GH#23728 Shouldn't need `////` above + +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +`export const x = 0; +`, + + "/y.ts": +`export const y = 0;`, + + "/user.ts": +`import { x } from "./a"; +import { y } from "./y"; +`, + }, +}); From 2bb076018b65d17287cf07f491c0ba89e208e746 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Mon, 30 Apr 2018 15:59:01 -0700 Subject: [PATCH 02/12] Code review, and support commonjs --- src/compiler/diagnosticMessages.json | 2 +- src/compiler/factory.ts | 12 +- src/harness/fourslash.ts | 4 +- src/services/findAllReferences.ts | 16 +- src/services/refactors/moveToNewFile.ts | 353 +++++++++++++++--- src/services/textChanges.ts | 15 +- src/services/utilities.ts | 11 + tests/cases/fourslash/moveToNewFile.ts | 4 +- .../fourslash/moveToNewFile_importEquals.ts | 18 + tests/cases/fourslash/moveToNewFile_js.ts | 39 ++ .../moveToNewFile_newModuleNameUnique.ts | 4 +- .../fourslash/moveToNewFile_rangeInvalid.ts | 2 +- .../fourslash/moveToNewFile_updateUses_js.ts | 30 ++ 13 files changed, 426 insertions(+), 84 deletions(-) create mode 100644 tests/cases/fourslash/moveToNewFile_importEquals.ts create mode 100644 tests/cases/fourslash/moveToNewFile_js.ts create mode 100644 tests/cases/fourslash/moveToNewFile_updateUses_js.ts diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index d08d81ea1e4be..46de24d8f856b 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -4175,7 +4175,7 @@ "category": "Message", "code": 95046 }, - "Move to new file": { + "Move to a new file": { "category": "Message", "code": 95047 } diff --git a/src/compiler/factory.ts b/src/compiler/factory.ts index e3191930c0e5e..bc735ead14f2a 100644 --- a/src/compiler/factory.ts +++ b/src/compiler/factory.ts @@ -1504,6 +1504,13 @@ namespace ts { return block; } + /* @internal */ + export function createExpressionStatement(expression: Expression): ExpressionStatement { + const node = createSynthesizedNode(SyntaxKind.ExpressionStatement); + node.expression = expression; + return node; + } + export function updateBlock(node: Block, statements: ReadonlyArray) { return node.statements !== statements ? updateNode(createBlock(statements, node.multiLine), node) @@ -1530,9 +1537,8 @@ namespace ts { } export function createStatement(expression: Expression) { - const node = createSynthesizedNode(SyntaxKind.ExpressionStatement); - node.expression = parenthesizeExpressionForExpressionStatement(expression); - return node; + return createExpressionStatement(parenthesizeExpressionForExpressionStatement(expression)); + } export function updateStatement(node: ExpressionStatement, expression: Expression) { diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index ca0051e0bb978..8ccb493191639 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -3059,10 +3059,10 @@ Actual: ${stringify(fullActual)}`); public moveToNewFile(options: FourSlashInterface.MoveToNewFileOptions): void { assert(this.getRanges().length === 1); const range = this.getRanges()[0]; - const refactor = ts.find(this.getApplicableRefactors(range), r => r.name === "Move to new file"); + const refactor = ts.find(this.getApplicableRefactors(range), r => r.name === "Move to a new file"); assert(refactor.actions.length === 1); const action = ts.first(refactor.actions); - assert(action.name === "Move to new file" && action.description === "Move to new file"); + assert(action.name === "Move to a new file" && action.description === "Move to a new file"); const editInfo = this.languageService.getEditsForRefactor(this.activeFile.fileName, this.formatCodeSettings, range, refactor.name, action.name, ts.defaultPreferences); for (const edit of editInfo.edits) { diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index 2be21476fabca..be46b10c1abf2 100644 --- a/src/services/findAllReferences.ts +++ b/src/services/findAllReferences.ts @@ -612,27 +612,19 @@ namespace ts.FindAllReferences.Core { checker.getPropertySymbolOfDestructuringAssignment(location); } - function getObjectBindingElementWithoutPropertyName(symbol: Symbol): BindingElement | undefined { + function getObjectBindingElementWithoutPropertyName(symbol: Symbol): BindingElement & { name: Identifier } | undefined { const bindingElement = getDeclarationOfKind(symbol, SyntaxKind.BindingElement); if (bindingElement && bindingElement.parent.kind === SyntaxKind.ObjectBindingPattern && + isIdentifier(bindingElement.name) && !bindingElement.propertyName) { - return bindingElement; + return bindingElement as BindingElement & { name: Identifier }; } } function getPropertySymbolOfObjectBindingPatternWithoutPropertyName(symbol: Symbol, checker: TypeChecker): Symbol | undefined { const bindingElement = getObjectBindingElementWithoutPropertyName(symbol); - if (!bindingElement) return undefined; - - const typeOfPattern = checker.getTypeAtLocation(bindingElement.parent); - const propSymbol = typeOfPattern && checker.getPropertyOfType(typeOfPattern, (bindingElement.name).text); - if (propSymbol && propSymbol.flags & SymbolFlags.Accessor) { - // See GH#16922 - Debug.assert(!!(propSymbol.flags & SymbolFlags.Transient)); - return (propSymbol as TransientSymbol).target; - } - return propSymbol; + return bindingElement && getPropertySymbolFromBindingElement(checker, bindingElement); } /** diff --git a/src/services/refactors/moveToNewFile.ts b/src/services/refactors/moveToNewFile.ts index ad461d880d9e9..59ca94f260416 100644 --- a/src/services/refactors/moveToNewFile.ts +++ b/src/services/refactors/moveToNewFile.ts @@ -1,10 +1,10 @@ /* @internal */ namespace ts.refactor { - const refactorName = "Move to new file"; + const refactorName = "Move to a new file"; registerRefactor(refactorName, { getAvailableActions(context): ApplicableRefactorInfo[] { if (getStatementsToMove(context) === undefined) return undefined; - const description = getLocaleSpecificMessage(Diagnostics.Move_to_new_file); + const description = getLocaleSpecificMessage(Diagnostics.Move_to_a_new_file); return [{ name: refactorName, description, actions: [{ name: refactorName, description }] }]; }, getEditsForAction(context, actionName): RefactorEditInfo { @@ -48,12 +48,13 @@ namespace ts.refactor { ): ReadonlyArray { const checker = program.getTypeChecker(); - if (!oldFile.externalModuleIndicator) { + if (!oldFile.externalModuleIndicator && !oldFile.commonJsModuleIndicator) { changes.deleteNodeRange(oldFile, first(toMove), last(toMove)); return toMove; } - const importsFromNewFile = createOldFileImportsFromNewFile(usage.oldFileImportsFromNewFile, newModuleName); + const useEs6ModuleSyntax = !!oldFile.externalModuleIndicator; + const importsFromNewFile = createOldFileImportsFromNewFile(usage.oldFileImportsFromNewFile, newModuleName, useEs6ModuleSyntax); if (importsFromNewFile) { changes.insertNodeBefore(oldFile, oldFile.statements[0], importsFromNewFile, /*blankLineBetween*/ true); } @@ -64,16 +65,15 @@ namespace ts.refactor { updateImportsInOtherFiles(changes, program, oldFile, usage.movedSymbols, newModuleName); return [ - ...getNewFileImportsAndAddExportInOldFile(oldFile, usage.oldImportsNeededByNewFile, usage.newFileImportsFromOldFile, changes, checker), - ...addExports(toMove, usage.oldFileImportsFromNewFile), + ...getNewFileImportsAndAddExportInOldFile(oldFile, usage.oldImportsNeededByNewFile, usage.newFileImportsFromOldFile, changes, checker, useEs6ModuleSyntax), + ...addExports(oldFile, toMove, usage.oldFileImportsFromNewFile, useEs6ModuleSyntax), ]; } function deleteUnusedOldImports(oldFile: SourceFile, toMove: ReadonlyArray, changes: textChanges.ChangeTracker, toDelete: ReadonlySymbolSet, checker: TypeChecker) { for (const statement of oldFile.statements) { - if (!contains(toMove, statement) && isImportDeclaration(statement)) { - deleteUnusedImports(oldFile, statement, changes, name => toDelete.has(checker.getSymbolAtLocation(name))); - } + if (contains(toMove, statement)) continue; + forEachImportInStatement(statement, i => deleteUnusedImports(oldFile, i, changes, name => toDelete.has(checker.getSymbolAtLocation(name)))); } } @@ -82,40 +82,122 @@ namespace ts.refactor { for (const sourceFile of program.getSourceFiles()) { if (sourceFile === oldFile) continue; for (const statement of sourceFile.statements) { - if (!isImportDeclaration(statement) || !isStringLiteral(statement.moduleSpecifier)) continue; + forEachImportInStatement(statement, importNode => { + const shouldMove = (name: Identifier): boolean => { + const symbol = isBindingElement(name.parent) + ? getPropertySymbolFromBindingElement(checker, name.parent as BindingElement & { name: Identifier }) + : skipAlias(checker.getSymbolAtLocation(name), checker); + return !!symbol && movedSymbols.has(symbol); + }; + deleteUnusedImports(sourceFile, importNode, changes, shouldMove); // These will be changed to imports from the new file + const newModuleSpecifier = combinePaths(getDirectoryPath(moduleSpecifierFromImport(importNode).text), newModuleName); + const newImportDeclaration = filterImport(importNode, createLiteral(newModuleSpecifier), shouldMove); + if (newImportDeclaration) changes.insertNodeAfter(sourceFile, statement, newImportDeclaration); + }); + } + } + } - const shouldMove = (name: Identifier): boolean => movedSymbols.has(skipAlias(checker.getSymbolAtLocation(name), checker)); - deleteUnusedImports(sourceFile, statement, changes, shouldMove); - const newModuleSpecifier = combinePaths(getDirectoryPath(statement.moduleSpecifier.text), newModuleName); - const newImportDeclaration = filterImport(statement, createLiteral(newModuleSpecifier), shouldMove); - if (newImportDeclaration) changes.insertNodeAfter(sourceFile, statement, newImportDeclaration); + function moduleSpecifierFromImport(i: SupportedImport): StringLiteralLike { + return (i.kind === SyntaxKind.ImportDeclaration ? i.moduleSpecifier + : i.kind === SyntaxKind.ImportEqualsDeclaration ? i.moduleReference.expression + : i.initializer.arguments[0]); + } + + function forEachImportInStatement(statement: Statement, cb: (importNode: SupportedImport) => void): void { + if (isImportDeclaration(statement)) { + if (isStringLiteral(statement.moduleSpecifier)) cb(statement as SupportedImport); + } + else if (isImportEqualsDeclaration(statement)) { + if (isExternalModuleReference(statement.moduleReference) && isStringLiteralLike(statement.moduleReference.expression)) { + cb(statement as SupportedImport); + } + } + else if (isVariableStatement(statement)) { + for (const decl of statement.declarationList.declarations) { + if (decl.initializer && isRequireCall(decl.initializer, /*checkArgumentIsStringLiteralLike*/ true)) { + cb(decl as SupportedImport); + } } } } - function createOldFileImportsFromNewFile(newFileNeedExport: ReadonlySymbolSet, newFileNameWithExtension: string): ImportDeclaration | undefined { + type SupportedImport = + | ImportDeclaration & { moduleSpecifier: StringLiteralLike } + | ImportEqualsDeclaration & { moduleReference: ExternalModuleReference & { expression: StringLiteralLike } } + | VariableDeclaration & { initializer: RequireOrImportCall }; + type SupportedImportStatement = + | ImportDeclaration + | ImportEqualsDeclaration + | VariableStatement; + + function createOldFileImportsFromNewFile(newFileNeedExport: ReadonlySymbolSet, newFileNameWithExtension: string, useEs6Imports: boolean): Statement | undefined { let defaultImport: Identifier | undefined; - const imports: ImportSpecifier[] = []; + const imports: string[] = []; newFileNeedExport.forEach(symbol => { if (symbol.escapedName === InternalSymbolName.Default) { defaultImport = createIdentifier(symbolNameNoDefault(symbol)); } else { - imports.push(createImportSpecifier(undefined, createIdentifier(symbol.name))); + imports.push(symbol.name); } }); - return makeImportIfNecessary(defaultImport, imports, ensurePathIsRelative(newFileNameWithExtension)); + return makeImportOrRequire(defaultImport, imports, newFileNameWithExtension, useEs6Imports); + } + + function makeImportOrRequire(defaultImport: Identifier | undefined, imports: ReadonlyArray, path: string, useEs6Imports: boolean): Statement | undefined { + path = ensurePathIsRelative(path); + if (useEs6Imports) { + const specifiers = imports.map(i => createImportSpecifier(/*propertyName*/ undefined, createIdentifier(i))); + return makeImportIfNecessary(defaultImport, specifiers, path); + } + else { + Debug.assert(!defaultImport); // If there's a default export, it should have been an es6 module. + const bindingElements = imports.map(i => createBindingElement(/*dotDotDotToken*/ undefined, /*propertyName*/ undefined, i)); + return bindingElements.length + ? makeVariableStatement(createObjectBindingPattern(bindingElements), /*type*/ undefined, createRequireCall(createLiteral(path))) + : undefined; + } + } + + function makeVariableStatement(name: BindingName, type: TypeNode | undefined, initializer: Expression | undefined, flags: NodeFlags = NodeFlags.Const) { + return createVariableStatement(/*modifiers*/ undefined, createVariableDeclarationList([createVariableDeclaration(name, type, initializer)], flags)); + } + + function createRequireCall(moduleSpecifier: StringLiteralLike): CallExpression { + return createCall(createIdentifier("require"), /*typeArguments*/ undefined, [moduleSpecifier]); } - function addExports(toMove: ReadonlyArray, needExport: ReadonlySymbolSet): ReadonlyArray { - return toMove.map(statement => { - return !hasModifier(statement, ModifierFlags.Export) && forEachTopLevelDeclaration(statement, d => needExport.has(Debug.assertDefined(d.symbol))) - ? addExport(statement as TopLevelDeclarationStatement) - : statement; + function addExports(sourceFile: SourceFile, toMove: ReadonlyArray, needExport: ReadonlySymbolSet, useEs6Exports: boolean): ReadonlyArray { + return flatMap(toMove, statement => { + if (isTopLevelDeclarationStatement(statement) && + !isExported(sourceFile, statement, useEs6Exports) && + forEachTopLevelDeclaration(statement, d => needExport.has(Debug.assertDefined(d.symbol)))) { + const exports = addExport(statement, useEs6Exports); + if (exports) return exports; + } + return statement; }); } - function deleteUnusedImports(sourceFile: SourceFile, importDecl: ImportDeclaration, changes: textChanges.ChangeTracker, isUnused: (name: Identifier) => boolean): void { + function deleteUnusedImports(sourceFile: SourceFile, importDecl: SupportedImport, changes: textChanges.ChangeTracker, isUnused: (name: Identifier) => boolean): void { + switch (importDecl.kind) { + case SyntaxKind.ImportDeclaration: + deleteUnusedImportsInDeclaration(sourceFile, importDecl, changes, isUnused); + break; + case SyntaxKind.ImportEqualsDeclaration: + if (isUnused(importDecl.name)) { + changes.deleteNode(sourceFile, importDecl); + } + break; + case SyntaxKind.VariableDeclaration: + deleteUnusedImportsInVariableDeclaration(sourceFile, importDecl, changes, isUnused); + break; + default: + Debug.assertNever(importDecl); + } + } + function deleteUnusedImportsInDeclaration(sourceFile: SourceFile, importDecl: ImportDeclaration, changes: textChanges.ChangeTracker, isUnused: (name: Identifier) => boolean): void { if (!importDecl.importClause) return; const { name, namedBindings } = importDecl.importClause; const defaultUnused = !name || isUnused(name); @@ -140,6 +222,31 @@ namespace ts.refactor { } } } + function deleteUnusedImportsInVariableDeclaration(sourceFile: SourceFile, varDecl: VariableDeclaration, changes: textChanges.ChangeTracker, isUnused: (name: Identifier) => boolean) { + const { name } = varDecl; + switch (name.kind) { + case SyntaxKind.Identifier: + if (isUnused(name)) { + changes.deleteNode(sourceFile, name); + } + break; + case SyntaxKind.ArrayBindingPattern: + break; + case SyntaxKind.ObjectBindingPattern: + if (name.elements.every(e => isIdentifier(e.name) && isUnused(e.name))) { + changes.deleteNode(sourceFile, + isVariableDeclarationList(varDecl.parent) && varDecl.parent.declarations.length === 1 ? varDecl.parent.parent : varDecl); + } + else { + for (const element of name.elements) { + if (isIdentifier(element.name) && isUnused(element.name)) { + changes.deleteNode(sourceFile, element.name); + } + } + } + break; + } + } function getNewFileImportsAndAddExportInOldFile( oldFile: SourceFile, @@ -147,49 +254,53 @@ namespace ts.refactor { newFileImportsFromOldFile: ReadonlySymbolSet, changes: textChanges.ChangeTracker, checker: TypeChecker, - ): ReadonlyArray { - const copiedOldImports = mapDefined(oldFile.statements, oldStatement => - isImportDeclaration(oldStatement) - ? filterImport(oldStatement, oldStatement.moduleSpecifier, name => importsToCopy.has(checker.getSymbolAtLocation(name))) - : undefined); + useEs6ModuleSyntax: boolean, + ): ReadonlyArray { + const copiedOldImports: SupportedImportStatement[] = []; + for (const oldStatement of oldFile.statements) { + forEachImportInStatement(oldStatement, i => { + append(copiedOldImports, filterImport(i, moduleSpecifierFromImport(i), name => importsToCopy.has(checker.getSymbolAtLocation(name)))); + }); + } // Also, import things used from the old file, and insert 'export' modifiers as necessary in the old file. let oldFileDefault: Identifier | undefined; - const oldFileNamedImports: ImportSpecifier[] = []; + const oldFileNamedImports: string[] = []; const markSeenTop = nodeSeenTracker(); // Needed because multiple declarations may appear in `const x = 0, y = 1;`. newFileImportsFromOldFile.forEach(symbol => { for (const decl of symbol.declarations) { - if (!isTopLevelDeclaration(decl) || !isIdentifier(decl.name)) continue; + if (!isTopLevelDeclaration(decl)) continue; + const name = nameOfTopLevelDeclaration(decl); + if (!name) continue; const top = getTopLevelDeclarationStatement(decl); - if (markSeenTop(top) && !hasModifier(top, ModifierFlags.Export)) { - changes.insertExportModifier(oldFile, top); + if (markSeenTop(top)) { + addExportToChanges(oldFile, top, changes, useEs6ModuleSyntax); } if (hasModifier(decl, ModifierFlags.Default)) { - oldFileDefault = decl.name; + oldFileDefault = name; } else { - oldFileNamedImports.push(createImportSpecifier(undefined, decl.name)); + oldFileNamedImports.push(name.text); } } }); - const oldFileImport = makeImportIfNecessary(oldFileDefault, oldFileNamedImports, `./${removeFileExtension(getBaseFileName(oldFile.fileName))}`); - return [...copiedOldImports, ...(oldFileImport ? [oldFileImport] : emptyArray)]; + append(copiedOldImports, makeImportOrRequire(oldFileDefault, oldFileNamedImports, removeFileExtension(getBaseFileName(oldFile.fileName)), useEs6ModuleSyntax)); + return copiedOldImports; } function makeUniqueModuleName(moduleName: string, extension: string, inDirectory: string, host: LanguageServiceHost): string { - while (true) { - const name = combinePaths(inDirectory, moduleName + extension); - if (!host.fileExists(name)) return moduleName; - moduleName += "0"; + let newModuleName = moduleName; + for (let i = 1; ; i++) { + const name = combinePaths(inDirectory, newModuleName + extension); + if (!host.fileExists(name)) return newModuleName; + newModuleName = `${moduleName}.${i}`; } } function getNewModuleName(movedSymbols: ReadonlySymbolSet): string { - let name: string | undefined; - movedSymbols.forEach(s => { if (name === undefined) name = symbolNameNoDefault(s); }); - return name === undefined ? "newFile" : name; + return movedSymbols.forEachEntry(symbolNameNoDefault) || "newFile"; } interface UsageInfo { @@ -212,13 +323,13 @@ namespace ts.refactor { for (const statement of toMove) { forEachTopLevelDeclaration(statement, decl => { - movedSymbols.add(Debug.assertDefined(decl.symbol)); + movedSymbols.add(Debug.assertDefined(isExpressionStatement(decl) ? checker.getSymbolAtLocation(decl.expression.left) : decl.symbol)); }); forEachReference(statement, checker, symbol => { if (!symbol.declarations) return; for (const decl of symbol.declarations) { - if (isImportSpecifier(decl) || isImportClause(decl.parent)) { + if (isInImport(decl)) { oldImportsNeededByNewFile.add(symbol); } else if (isTopLevelDeclaration(decl) && !movedSymbols.has(symbol)) { @@ -245,11 +356,44 @@ namespace ts.refactor { // Below should all be utilities - function filterImport(i: ImportDeclaration, moduleSpecifier: Expression, keep: (name: Identifier) => boolean): ImportDeclaration | undefined { - const clause = i.importClause; - const defaultImport = clause.name && keep(clause.name) ? clause.name : undefined; - const namedBindings = clause.namedBindings && filterNamedBindings(clause.namedBindings, keep); - return defaultImport || namedBindings ? createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, createImportClause(defaultImport, namedBindings), moduleSpecifier) : undefined; + function isInImport(decl: Declaration) { + switch (decl.kind) { + case SyntaxKind.ImportEqualsDeclaration: + case SyntaxKind.ImportSpecifier: + case SyntaxKind.ImportClause: + return true; + case SyntaxKind.VariableDeclaration: + return isVariableDeclarationInImport(decl as VariableDeclaration); + case SyntaxKind.BindingElement: + return isVariableDeclaration(decl.parent.parent) && isVariableDeclarationInImport(decl.parent.parent); + default: + return false; + } + } + function isVariableDeclarationInImport(decl: VariableDeclaration) { + return isSourceFile(decl.parent.parent.parent) && + isRequireCall(decl.initializer, /*checkArgumentIsStringLiteralLike*/ true); + } + + function filterImport(i: SupportedImport, moduleSpecifier: StringLiteralLike, keep: (name: Identifier) => boolean): SupportedImportStatement | undefined { + switch (i.kind) { + case SyntaxKind.ImportDeclaration: { + const clause = i.importClause; + const defaultImport = clause.name && keep(clause.name) ? clause.name : undefined; + const namedBindings = clause.namedBindings && filterNamedBindings(clause.namedBindings, keep); + return defaultImport || namedBindings + ? createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, createImportClause(defaultImport, namedBindings), moduleSpecifier) + : undefined; + } + case SyntaxKind.ImportEqualsDeclaration: + return keep(i.name) ? i : undefined; + case SyntaxKind.VariableDeclaration: { + const name = filterBindingName(i.name, keep); + return name ? makeVariableStatement(name, i.type, createRequireCall(moduleSpecifier), i.parent.flags) : undefined; + } + default: + return Debug.assertNever(i); + } } function filterNamedBindings(namedBindings: NamedImportBindings, keep: (name: Identifier) => boolean): NamedImportBindings | undefined { if (namedBindings.kind === SyntaxKind.NamespaceImport) { @@ -260,6 +404,19 @@ namespace ts.refactor { return newElements.length ? createNamedImports(newElements) : undefined; } } + function filterBindingName(name: BindingName, keep: (name: Identifier) => boolean): BindingName | undefined { + switch (name.kind) { + case SyntaxKind.Identifier: + return keep(name) ? name : undefined; + case SyntaxKind.ArrayBindingPattern: + return name; + case SyntaxKind.ObjectBindingPattern: { + // We can't handle nested destructurings or property names well here, so just copy them all. + const newElements = name.elements.filter(prop => prop.propertyName || !isIdentifier(prop.name) || keep(prop.name)); + return newElements.length ? createObjectBindingPattern(newElements) : undefined; + } + } + } function forEachReference(node: Node, checker: TypeChecker, onReference: (s: Symbol) => void) { node.forEachChild(function cb(node) { @@ -276,6 +433,7 @@ namespace ts.refactor { interface ReadonlySymbolSet { has(symbol: Symbol): boolean; forEach(cb: (symbol: Symbol) => void): void; + forEachEntry(cb: (symbol: Symbol) => T | undefined): T | undefined; } class SymbolSet implements ReadonlySymbolSet { private map = createMap(); @@ -291,6 +449,9 @@ namespace ts.refactor { forEach(cb: (symbol: Symbol) => void): void { this.map.forEach(cb); } + forEachEntry(cb: (symbol: Symbol) => T | undefined): T | undefined { + return forEachEntry(this.map, cb); + } clone(): SymbolSet { const clone = new SymbolSet(); copyEntries(this.map, clone.map); @@ -298,11 +459,21 @@ namespace ts.refactor { } } - type NonVariableTopLevelDeclaration = FunctionDeclaration | ClassDeclaration | EnumDeclaration | TypeAliasDeclaration | InterfaceDeclaration | ModuleDeclaration; + type TopLevelExpressionStatement = ExpressionStatement & { expression: BinaryExpression & { left: PropertyAccessExpression } }; // 'exports.x = ...' + type NonVariableTopLevelDeclaration = FunctionDeclaration | ClassDeclaration | EnumDeclaration | TypeAliasDeclaration | InterfaceDeclaration | ModuleDeclaration | TopLevelExpressionStatement; type TopLevelDeclarationStatement = NonVariableTopLevelDeclaration | VariableStatement; interface TopLevelVariableDeclaration extends VariableDeclaration { parent: VariableDeclarationList & { parent: VariableStatement; }; } type TopLevelDeclaration = NonVariableTopLevelDeclaration | TopLevelVariableDeclaration; function isTopLevelDeclaration(node: Node): node is TopLevelDeclaration { + return isNonVariableTopLevelDeclaration(node) || isVariableDeclaration(node) && isSourceFile(node.parent.parent.parent); + } + + function isTopLevelDeclarationStatement(node: Node): node is TopLevelDeclarationStatement { + Debug.assert(isSourceFile(node.parent)); + return isNonVariableTopLevelDeclaration(node) || isVariableStatement(node); + } + + function isNonVariableTopLevelDeclaration(node: Node): node is NonVariableTopLevelDeclaration { switch (node.kind) { case SyntaxKind.FunctionDeclaration: case SyntaxKind.ClassDeclaration: @@ -310,9 +481,9 @@ namespace ts.refactor { case SyntaxKind.EnumDeclaration: case SyntaxKind.TypeAliasDeclaration: case SyntaxKind.InterfaceDeclaration: - return isSourceFile(node.parent); - case SyntaxKind.VariableDeclaration: - return isSourceFile((node as VariableDeclaration).parent.parent.parent); + return true; + default: + return false; } } @@ -328,14 +499,48 @@ namespace ts.refactor { case SyntaxKind.VariableStatement: return forEach((statement as VariableStatement).declarationList.declarations as ReadonlyArray, cb); + + case SyntaxKind.ExpressionStatement: { + const { expression } = statement as ExpressionStatement; + return isBinaryExpression(expression) && getSpecialPropertyAssignmentKind(expression) === SpecialPropertyAssignmentKind.ExportsProperty + ? cb(statement as TopLevelExpressionStatement) + : undefined; + } } } + function nameOfTopLevelDeclaration(d: TopLevelDeclaration): Identifier | undefined { + return d.kind === SyntaxKind.ExpressionStatement ? d.expression.left.name : tryCast(d.name, isIdentifier); + } + function getTopLevelDeclarationStatement(d: TopLevelDeclaration): TopLevelDeclarationStatement { return isVariableDeclaration(d) ? d.parent.parent : d; } - function addExport(d: TopLevelDeclarationStatement): TopLevelDeclarationStatement { + function addExportToChanges(sourceFile: SourceFile, decl: TopLevelDeclarationStatement, changes: textChanges.ChangeTracker, useEs6Exports: boolean): void { + if (isExported(sourceFile, decl, useEs6Exports)) return; + if (useEs6Exports) { + if (!isExpressionStatement(decl)) changes.insertExportModifier(sourceFile, decl); + } + else { + const names = getNamesToExportInCommonJS(decl); + if (names.length !== 0) changes.insertNodesAfter(sourceFile, decl, names.map(createExportAssignment)); + } + } + + function isExported(sourceFile: SourceFile, decl: TopLevelDeclarationStatement, useEs6Exports: boolean): boolean { + if (useEs6Exports) { + return !isExpressionStatement(decl) && hasModifier(decl, ModifierFlags.Export); + } + else { + return getNamesToExportInCommonJS(decl).some(name => sourceFile.symbol.exports.has(escapeLeadingUnderscores(name))); + } + } + + function addExport(decl: TopLevelDeclarationStatement, useEs6Exports: boolean): ReadonlyArray | undefined { + return useEs6Exports ? [addEs6Export(decl)] : addCommonjsExport(decl); + } + function addEs6Export(d: TopLevelDeclarationStatement): TopLevelDeclarationStatement { const modifiers = concatenate([createModifier(SyntaxKind.ExportKeyword)], d.modifiers); switch (d.kind) { case SyntaxKind.FunctionDeclaration: @@ -352,8 +557,40 @@ namespace ts.refactor { return updateTypeAliasDeclaration(d, d.decorators, modifiers, d.name, d.typeParameters, d.type); case SyntaxKind.InterfaceDeclaration: return updateInterfaceDeclaration(d, d.decorators, modifiers, d.name, d.typeParameters, d.heritageClauses, d.members); + case SyntaxKind.ExpressionStatement: + return Debug.fail(); // Shouldn't try to add 'export' keyword to `exports.x = ...` default: - Debug.assertNever(d); + return Debug.assertNever(d); } } + function addCommonjsExport(decl: TopLevelDeclarationStatement): ReadonlyArray | undefined { + return [decl, ...getNamesToExportInCommonJS(decl).map(createExportAssignment)]; + } + function getNamesToExportInCommonJS(decl: TopLevelDeclarationStatement): ReadonlyArray { + switch (decl.kind) { + case SyntaxKind.FunctionDeclaration: + case SyntaxKind.ClassDeclaration: + return [decl.name.text]; + case SyntaxKind.VariableStatement: + return mapDefined(decl.declarationList.declarations, d => isIdentifier(d.name) ? d.name.text : undefined); + case SyntaxKind.ModuleDeclaration: + case SyntaxKind.EnumDeclaration: + case SyntaxKind.TypeAliasDeclaration: + case SyntaxKind.InterfaceDeclaration: + return undefined; + case SyntaxKind.ExpressionStatement: + return Debug.fail(); // Shouldn't try to add 'export' keyword to `exports.x = ...` + default: + Debug.assertNever(decl); + } + } + + /** Creates `exports.x = x;` */ + function createExportAssignment(name: string): Statement { + return createExpressionStatement( + createBinary( + createPropertyAccess(createIdentifier("exports"), createIdentifier(name)), + SyntaxKind.EqualsToken, + createIdentifier(name))); + } } diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 7a566ccd71929..4aa130e2fed82 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -458,7 +458,17 @@ namespace ts.textChanges { this.insertNodeAt(sourceFile, cls.members.pos, newElement, { indentation, prefix, suffix }); } - public insertNodeAfter(sourceFile: SourceFile, after: Node, newNode: Node): this { + public insertNodeAfter(sourceFile: SourceFile, after: Node, newNode: Node): void { + const endPosition = this.insertNodeAfterWorker(sourceFile, after, newNode); + this.insertNodeAt(sourceFile, endPosition, newNode, this.getInsertNodeAfterOptions(after)); + } + + public insertNodesAfter(sourceFile: SourceFile, after: Node, newNodes: ReadonlyArray): void { + const endPosition = this.insertNodeAfterWorker(sourceFile, after, first(newNodes)); + this.insertNodesAt(sourceFile, endPosition, newNodes, this.getInsertNodeAfterOptions(after)); + } + + private insertNodeAfterWorker(sourceFile: SourceFile, after: Node, newNode: Node): number { if (needSemicolonBetween(after, newNode)) { // check if previous statement ends with semicolon // if not - insert semicolon to preserve the code from changing the meaning due to ASI @@ -466,8 +476,7 @@ namespace ts.textChanges { this.replaceRange(sourceFile, createTextRange(after.end), createToken(SyntaxKind.SemicolonToken)); } } - const endPosition = getAdjustedEndPosition(sourceFile, after, {}); - return this.replaceRange(sourceFile, createTextRange(endPosition), newNode, this.getInsertNodeAfterOptions(after)); + return getAdjustedEndPosition(sourceFile, after, {}); } private getInsertNodeAfterOptions(node: Node): InsertNodeOptions { diff --git a/src/services/utilities.ts b/src/services/utilities.ts index 4fcda5be1bd15..8c77f96c7a4bd 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1262,6 +1262,17 @@ namespace ts { return name && name.kind === SyntaxKind.Identifier ? name.escapedText : undefined; }); } + + export function getPropertySymbolFromBindingElement(checker: TypeChecker, bindingElement: BindingElement & { name: Identifier }) { + const typeOfPattern = checker.getTypeAtLocation(bindingElement.parent); + const propSymbol = typeOfPattern && checker.getPropertyOfType(typeOfPattern, bindingElement.name.text); + if (propSymbol && propSymbol.flags & SymbolFlags.Accessor) { + // See GH#16922 + Debug.assert(!!(propSymbol.flags & SymbolFlags.Transient)); + return (propSymbol as TransientSymbol).target; + } + return propSymbol; + } } // Display-part writer helpers diff --git a/tests/cases/fourslash/moveToNewFile.ts b/tests/cases/fourslash/moveToNewFile.ts index ad61803c04f51..074e341a778b4 100644 --- a/tests/cases/fourslash/moveToNewFile.ts +++ b/tests/cases/fourslash/moveToNewFile.ts @@ -4,7 +4,7 @@ ////import { a, b } from "./other"; ////const p = 0; ////[|const y = p + b;|] -////y; +////a; y; verify.moveToNewFile({ newFileContents: { @@ -13,7 +13,7 @@ verify.moveToNewFile({ import { a } from "./other"; export const p = 0; -y;`, +a; y;`, "/y.ts": `import { b } from "./other"; diff --git a/tests/cases/fourslash/moveToNewFile_importEquals.ts b/tests/cases/fourslash/moveToNewFile_importEquals.ts new file mode 100644 index 0000000000000..a87c84c00046a --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_importEquals.ts @@ -0,0 +1,18 @@ +/// + +// @Filename: /a.ts +////import i = require("./i"); +////import j = require("./j"); +////[|const y = i;|] +////j; + +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +`import j = require("./j"); +j;`, + "/y.ts": +`import i = require("./i"); +const y = i;`, + }, +}); diff --git a/tests/cases/fourslash/moveToNewFile_js.ts b/tests/cases/fourslash/moveToNewFile_js.ts new file mode 100644 index 0000000000000..fd977bc0ef762 --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_js.ts @@ -0,0 +1,39 @@ +/// + +// @allowJs: true + +// @Filename: /a.js +////const { a, b } = require("./other"); +////const p = 0; +//// +////[|const y = p + b; +////const z = 0; +////exports.z = 0;|] +////a; y; z; + +// @Filename: /user.ts +////const { x, y } = require("./a"); + +// TODO: GH#23793 This will crash if the blank line between `const p` and `const y` is removed + +verify.moveToNewFile({ + newFileContents: { + "/a.js": +// TODO: GH#22330 +`const { y, z } = require("./y"); + +const { a, } = require("./other"); +const p = 0; +exports.p = p; + +a; y; z;`, + + "/y.js": +`const { b } = require("./other"); +const { p } = require("./a"); +const y = p + b; +exports.y = y; +const z = 0; +exports.z = 0;`, + }, +}); diff --git a/tests/cases/fourslash/moveToNewFile_newModuleNameUnique.ts b/tests/cases/fourslash/moveToNewFile_newModuleNameUnique.ts index f05861ae5ba44..58cebf993daf2 100644 --- a/tests/cases/fourslash/moveToNewFile_newModuleNameUnique.ts +++ b/tests/cases/fourslash/moveToNewFile_newModuleNameUnique.ts @@ -6,7 +6,7 @@ // @Filename: /x.ts //// -// @Filename: /x0.ts +// @Filename: /x.1.ts //// verify.moveToNewFile({ @@ -14,7 +14,7 @@ verify.moveToNewFile({ "/a.ts": ``, - "/x00.ts": + "/x.2.ts": `export const x = 0;`, }, }); diff --git a/tests/cases/fourslash/moveToNewFile_rangeInvalid.ts b/tests/cases/fourslash/moveToNewFile_rangeInvalid.ts index cd7ec49cd04d5..e217828122256 100644 --- a/tests/cases/fourslash/moveToNewFile_rangeInvalid.ts +++ b/tests/cases/fourslash/moveToNewFile_rangeInvalid.ts @@ -9,5 +9,5 @@ for (const range of test.ranges()) { goTo.selectRange(range); - verify.not.refactorAvailable("Move to new file") + verify.not.refactorAvailable("Move to a new file") } diff --git a/tests/cases/fourslash/moveToNewFile_updateUses_js.ts b/tests/cases/fourslash/moveToNewFile_updateUses_js.ts new file mode 100644 index 0000000000000..09fbf32a31335 --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_updateUses_js.ts @@ -0,0 +1,30 @@ +/// + +// @allowJs: true + +// @Filename: /a.js +////exports.x = 0; +////[|exports.y = 0;|] + +// @Filename: /user.js +////const { x, y } = require("./a"); +//// + +// TODO: GH#23728 Shouldn't need `////` above + +verify.moveToNewFile({ + newFileContents: { + "/a.js": +`exports.x = 0; +`, + + "/y.js": +`exports.y = 0;`, + + "/user.js": +// TODO: GH#22330 +`const { x, } = require("./a"); +const { y } = require("./y"); +`, + }, +}); From 8c46867b440770eb7cbc46136a2b9b0e95d070ec Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Tue, 1 May 2018 08:12:29 -0700 Subject: [PATCH 03/12] Compute movedSymbols completely before using, and support `export import` --- src/services/refactors/moveToNewFile.ts | 20 ++++++++++++++--- .../fourslash/moveToNewFile_exportImport.ts | 22 +++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) create mode 100644 tests/cases/fourslash/moveToNewFile_exportImport.ts diff --git a/src/services/refactors/moveToNewFile.ts b/src/services/refactors/moveToNewFile.ts index 59ca94f260416..09ed4199151be 100644 --- a/src/services/refactors/moveToNewFile.ts +++ b/src/services/refactors/moveToNewFile.ts @@ -325,7 +325,8 @@ namespace ts.refactor { forEachTopLevelDeclaration(statement, decl => { movedSymbols.add(Debug.assertDefined(isExpressionStatement(decl) ? checker.getSymbolAtLocation(decl.expression.left) : decl.symbol)); }); - + } + for (const statement of toMove) { forEachReference(statement, checker, symbol => { if (!symbol.declarations) return; for (const decl of symbol.declarations) { @@ -460,7 +461,15 @@ namespace ts.refactor { } type TopLevelExpressionStatement = ExpressionStatement & { expression: BinaryExpression & { left: PropertyAccessExpression } }; // 'exports.x = ...' - type NonVariableTopLevelDeclaration = FunctionDeclaration | ClassDeclaration | EnumDeclaration | TypeAliasDeclaration | InterfaceDeclaration | ModuleDeclaration | TopLevelExpressionStatement; + type NonVariableTopLevelDeclaration = + | FunctionDeclaration + | ClassDeclaration + | EnumDeclaration + | TypeAliasDeclaration + | InterfaceDeclaration + | ModuleDeclaration + | TopLevelExpressionStatement + | ImportEqualsDeclaration; type TopLevelDeclarationStatement = NonVariableTopLevelDeclaration | VariableStatement; interface TopLevelVariableDeclaration extends VariableDeclaration { parent: VariableDeclarationList & { parent: VariableStatement; }; } type TopLevelDeclaration = NonVariableTopLevelDeclaration | TopLevelVariableDeclaration; @@ -481,6 +490,7 @@ namespace ts.refactor { case SyntaxKind.EnumDeclaration: case SyntaxKind.TypeAliasDeclaration: case SyntaxKind.InterfaceDeclaration: + case SyntaxKind.ImportEqualsDeclaration: return true; default: return false; @@ -495,7 +505,8 @@ namespace ts.refactor { case SyntaxKind.EnumDeclaration: case SyntaxKind.TypeAliasDeclaration: case SyntaxKind.InterfaceDeclaration: - return cb(statement as FunctionDeclaration | ClassDeclaration | EnumDeclaration | ModuleDeclaration | TypeAliasDeclaration | InterfaceDeclaration); + case SyntaxKind.ImportEqualsDeclaration: + return cb(statement as FunctionDeclaration | ClassDeclaration | EnumDeclaration | ModuleDeclaration | TypeAliasDeclaration | InterfaceDeclaration | ImportEqualsDeclaration); case SyntaxKind.VariableStatement: return forEach((statement as VariableStatement).declarationList.declarations as ReadonlyArray, cb); @@ -557,6 +568,8 @@ namespace ts.refactor { return updateTypeAliasDeclaration(d, d.decorators, modifiers, d.name, d.typeParameters, d.type); case SyntaxKind.InterfaceDeclaration: return updateInterfaceDeclaration(d, d.decorators, modifiers, d.name, d.typeParameters, d.heritageClauses, d.members); + case SyntaxKind.ImportEqualsDeclaration: + return updateImportEqualsDeclaration(d, d.decorators, modifiers, d.name, d.moduleReference); case SyntaxKind.ExpressionStatement: return Debug.fail(); // Shouldn't try to add 'export' keyword to `exports.x = ...` default: @@ -577,6 +590,7 @@ namespace ts.refactor { case SyntaxKind.EnumDeclaration: case SyntaxKind.TypeAliasDeclaration: case SyntaxKind.InterfaceDeclaration: + case SyntaxKind.ImportEqualsDeclaration: return undefined; case SyntaxKind.ExpressionStatement: return Debug.fail(); // Shouldn't try to add 'export' keyword to `exports.x = ...` diff --git a/tests/cases/fourslash/moveToNewFile_exportImport.ts b/tests/cases/fourslash/moveToNewFile_exportImport.ts new file mode 100644 index 0000000000000..a8ac3a66a0124 --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_exportImport.ts @@ -0,0 +1,22 @@ +/// + +// @Filename: /a.ts +////namespace N { export const x = 0; } +////[|import M = N; +////export import O = N;|] +////M; + +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +`import { M } from "./M"; + +export namespace N { export const x = 0; } +M;`, + + "/M.ts": +`import { N } from "./a"; +export import M = N; +export import O = N;`, + }, +}); From 1884fe9aeef4ab89c963aeff2c6a8af7e230334c Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Wed, 2 May 2018 09:34:25 -0700 Subject: [PATCH 04/12] Fix assertion error: sort empty change before non-empty change --- src/services/textChanges.ts | 3 ++- tests/cases/fourslash/moveToNewFile_js.ts | 4 ---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 4aa130e2fed82..f29a92b7a37f2 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -691,7 +691,8 @@ namespace ts.textChanges { return group(changes, c => c.sourceFile.path).map(changesInFile => { const sourceFile = changesInFile[0].sourceFile; // order changes by start position - const normalized = stableSort(changesInFile, (a, b) => a.range.pos - b.range.pos); + // If the start position is the same, put the shorter range first, since an empty range (x, x) may precede (x, y) but not vice-versa. + const normalized = stableSort(changesInFile, (a, b) => (a.range.pos - b.range.pos) || (a.range.end - b.range.end)); // verify that change intervals do not overlap, except possibly at end points. for (let i = 0; i < normalized.length - 1; i++) { Debug.assert(normalized[i].range.end <= normalized[i + 1].range.pos, "Changes overlap", () => diff --git a/tests/cases/fourslash/moveToNewFile_js.ts b/tests/cases/fourslash/moveToNewFile_js.ts index fd977bc0ef762..e70bfa5f430c2 100644 --- a/tests/cases/fourslash/moveToNewFile_js.ts +++ b/tests/cases/fourslash/moveToNewFile_js.ts @@ -5,7 +5,6 @@ // @Filename: /a.js ////const { a, b } = require("./other"); ////const p = 0; -//// ////[|const y = p + b; ////const z = 0; ////exports.z = 0;|] @@ -14,8 +13,6 @@ // @Filename: /user.ts ////const { x, y } = require("./a"); -// TODO: GH#23793 This will crash if the blank line between `const p` and `const y` is removed - verify.moveToNewFile({ newFileContents: { "/a.js": @@ -25,7 +22,6 @@ verify.moveToNewFile({ const { a, } = require("./other"); const p = 0; exports.p = p; - a; y; z;`, "/y.js": From 67c61ff620a4156c3d355de7cb519c298758687e Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Wed, 2 May 2018 10:47:02 -0700 Subject: [PATCH 05/12] Remove extra newline --- src/compiler/factory.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/compiler/factory.ts b/src/compiler/factory.ts index bc735ead14f2a..64d86c9657772 100644 --- a/src/compiler/factory.ts +++ b/src/compiler/factory.ts @@ -1538,7 +1538,6 @@ namespace ts { export function createStatement(expression: Expression) { return createExpressionStatement(parenthesizeExpressionForExpressionStatement(expression)); - } export function updateStatement(node: ExpressionStatement, expression: Expression) { From 0f2b6b92773065246ff02f87193d7f952bce6540 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 4 May 2018 10:19:27 -0700 Subject: [PATCH 06/12] Add allowTextChangesInNewFiles preference --- src/harness/fourslash.ts | 6 +++--- src/services/refactors/moveToNewFile.ts | 2 +- src/services/types.ts | 1 + 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 4f0898d274bcc..a6b5b0e78b600 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -3061,7 +3061,7 @@ Actual: ${stringify(fullActual)}`); public moveToNewFile(options: FourSlashInterface.MoveToNewFileOptions): void { assert(this.getRanges().length === 1); const range = this.getRanges()[0]; - const refactor = ts.find(this.getApplicableRefactors(range), r => r.name === "Move to a new file"); + const refactor = ts.find(this.getApplicableRefactors(range, { allowTextChangesInNewFiles: true }), r => r.name === "Move to a new file"); assert(refactor.actions.length === 1); const action = ts.first(refactor.actions); assert(action.name === "Move to a new file" && action.description === "Move to a new file"); @@ -3304,8 +3304,8 @@ Actual: ${stringify(fullActual)}`); } } - private getApplicableRefactors(positionOrRange: number | ts.TextRange): ReadonlyArray { - return this.languageService.getApplicableRefactors(this.activeFile.fileName, positionOrRange, ts.defaultPreferences) || ts.emptyArray; + private getApplicableRefactors(positionOrRange: number | ts.TextRange, preferences = ts.defaultPreferences): ReadonlyArray { + return this.languageService.getApplicableRefactors(this.activeFile.fileName, positionOrRange, preferences) || ts.emptyArray; } } diff --git a/src/services/refactors/moveToNewFile.ts b/src/services/refactors/moveToNewFile.ts index 09ed4199151be..7edc8516f2d5d 100644 --- a/src/services/refactors/moveToNewFile.ts +++ b/src/services/refactors/moveToNewFile.ts @@ -3,7 +3,7 @@ namespace ts.refactor { const refactorName = "Move to a new file"; registerRefactor(refactorName, { getAvailableActions(context): ApplicableRefactorInfo[] { - if (getStatementsToMove(context) === undefined) return undefined; + if (getStatementsToMove(context) === undefined || !context.preferences.allowTextChangesInNewFiles) return undefined; const description = getLocaleSpecificMessage(Diagnostics.Move_to_a_new_file); return [{ name: refactorName, description, actions: [{ name: refactorName, description }] }]; }, diff --git a/src/services/types.ts b/src/services/types.ts index 2369db8a0ce04..442e7784dd4c0 100644 --- a/src/services/types.ts +++ b/src/services/types.ts @@ -233,6 +233,7 @@ namespace ts { readonly includeCompletionsForModuleExports?: boolean; readonly includeCompletionsWithInsertText?: boolean; readonly importModuleSpecifierPreference?: "relative" | "non-relative"; + readonly allowTextChangesInNewFiles?: boolean; } /* @internal */ export const defaultPreferences: UserPreferences = {}; From 497343b48541d18c38388009fe2aecbb751cca13 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 4 May 2018 10:46:26 -0700 Subject: [PATCH 07/12] Add the new file to 'files' in tsconfig --- src/services/refactors/moveToNewFile.ts | 11 ++++++++ src/services/textChanges.ts | 3 +-- .../cases/fourslash/moveToNewFile_tsconfig.ts | 26 +++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 tests/cases/fourslash/moveToNewFile_tsconfig.ts diff --git a/src/services/refactors/moveToNewFile.ts b/src/services/refactors/moveToNewFile.ts index 115d0ecd5b5d5..1dc9b8d74bf2c 100644 --- a/src/services/refactors/moveToNewFile.ts +++ b/src/services/refactors/moveToNewFile.ts @@ -41,6 +41,17 @@ namespace ts.refactor { // If previous file was global, this is easy. changes.createNewFile(oldFile, combinePaths(currentDirectory, newFileNameWithExtension), getNewStatements(oldFile, usage, changes, toMove, program, newModuleName)); + + addNewFileToTsconfig(program, changes, normalizePath(combinePaths(oldFile.fileName, "..", newFileNameWithExtension))); + } + + function addNewFileToTsconfig(program: Program, changes: textChanges.ChangeTracker, newFilePath: string): void { + const cfg = program.getCompilerOptions().configFile; + const filesProp = cfg && cfg.jsonObject && find(cfg.jsonObject.properties, (prop): prop is PropertyAssignment => + isPropertyAssignment(prop) && isStringLiteral(prop.name) && prop.name.text === "files"); + if (filesProp && isArrayLiteralExpression(filesProp.initializer)) { + changes.insertNodeInListAfter(cfg, last(filesProp.initializer.elements), createLiteral(newFilePath), filesProp.initializer.elements); + } } function getNewStatements( diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index c2d91b18f1f6d..9e731dfdf2dbd 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -544,8 +544,7 @@ namespace ts.textChanges { * i.e. arguments in arguments lists, parameters in parameter lists etc. * Note that separators are part of the node in statements and class elements. */ - public insertNodeInListAfter(sourceFile: SourceFile, after: Node, newNode: Node) { - const containingList = formatting.SmartIndenter.getContainingList(after, sourceFile); + public insertNodeInListAfter(sourceFile: SourceFile, after: Node, newNode: Node, containingList = formatting.SmartIndenter.getContainingList(after, sourceFile)) { if (!containingList) { Debug.fail("node is not a list element"); return this; diff --git a/tests/cases/fourslash/moveToNewFile_tsconfig.ts b/tests/cases/fourslash/moveToNewFile_tsconfig.ts new file mode 100644 index 0000000000000..f58d5225f3af1 --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_tsconfig.ts @@ -0,0 +1,26 @@ +/// + +// @Filename: /a.ts +////0; +////[|1;|] + +// @Filename: /tsconfig.json +////{ +//// "files": ["/a.ts"] +////} + +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +`0; +`, + + "/newFile.ts": +`1;`, + + "/tsconfig.json": +`{ + "files": ["/a.ts", "/newFile.ts"] +}`, + }, +}); From 9900c4bc676eb91b9732aa29f957bac735e1e065 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 4 May 2018 11:18:51 -0700 Subject: [PATCH 08/12] Avoid parameter initializer --- src/compiler/core.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 283e33a561dca..d482cb4fc0ecb 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -311,8 +311,8 @@ namespace ts { } /** Works like Array.prototype.findIndex, returning `-1` if no element satisfying the predicate is found. */ - export function findIndex(array: ReadonlyArray, predicate: (element: T, index: number) => boolean, startIndex = 0): number { - for (let i = startIndex; i < array.length; i++) { + export function findIndex(array: ReadonlyArray, predicate: (element: T, index: number) => boolean, startIndex?: number): number { + for (let i = startIndex || 0; i < array.length; i++) { if (predicate(array[i], i)) { return i; } From 233640be21a1945ce03a21a7c79cf010f1796903 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Fri, 4 May 2018 11:21:12 -0700 Subject: [PATCH 09/12] Update API baselines --- tests/baselines/reference/api/tsserverlibrary.d.ts | 1 + tests/baselines/reference/api/typescript.d.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index c52960b574b39..87f0e992ea05a 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -4400,6 +4400,7 @@ declare namespace ts { readonly includeCompletionsForModuleExports?: boolean; readonly includeCompletionsWithInsertText?: boolean; readonly importModuleSpecifierPreference?: "relative" | "non-relative"; + readonly allowTextChangesInNewFiles?: boolean; } interface LanguageService { cleanupSemanticCache(): void; diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts index 9c2639305df0f..cddd9be7cd228 100644 --- a/tests/baselines/reference/api/typescript.d.ts +++ b/tests/baselines/reference/api/typescript.d.ts @@ -4400,6 +4400,7 @@ declare namespace ts { readonly includeCompletionsForModuleExports?: boolean; readonly includeCompletionsWithInsertText?: boolean; readonly importModuleSpecifierPreference?: "relative" | "non-relative"; + readonly allowTextChangesInNewFiles?: boolean; } interface LanguageService { cleanupSemanticCache(): void; From 7051e32557c297fc11492c27a28083ce092cd024 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Tue, 8 May 2018 10:49:45 -0700 Subject: [PATCH 10/12] Use path relative to tsconfig.json --- src/compiler/core.ts | 14 ++++--- src/harness/unittests/paths.ts | 40 +++++++++---------- src/harness/vpath.ts | 2 +- src/services/codefixes/importFixes.ts | 8 ++-- src/services/getEditsForFileRename.ts | 2 +- src/services/refactors/moveToNewFile.ts | 11 +++-- .../cases/fourslash/moveToNewFile_tsconfig.ts | 16 ++++---- 7 files changed, 52 insertions(+), 41 deletions(-) diff --git a/src/compiler/core.ts b/src/compiler/core.ts index d482cb4fc0ecb..08e3d6bbe7aa8 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -2289,20 +2289,24 @@ namespace ts { return ["", ...relative, ...components]; } + export function getRelativePathFromFile(from: string, to: string, getCanonicalFileName: GetCanonicalFileName) { + return ensurePathIsNonModuleName(getRelativePathFromDirectory(ts.getDirectoryPath(from), to, getCanonicalFileName)); + } + /** * Gets a relative path that can be used to traverse between `from` and `to`. */ - export function getRelativePath(from: string, to: string, ignoreCase: boolean): string; + export function getRelativePathFromDirectory(from: string, to: string, ignoreCase: boolean): string; /** * Gets a relative path that can be used to traverse between `from` and `to`. */ // tslint:disable-next-line:unified-signatures - export function getRelativePath(from: string, to: string, getCanonicalFileName: GetCanonicalFileName): string; - export function getRelativePath(from: string, to: string, getCanonicalFileNameOrIgnoreCase: GetCanonicalFileName | boolean) { - Debug.assert((getRootLength(from) > 0) === (getRootLength(to) > 0), "Paths must either both be absolute or both be relative"); + export function getRelativePathFromDirectory(fromDirectory: string, to: string, getCanonicalFileName: GetCanonicalFileName): string; + export function getRelativePathFromDirectory(fromDirectory: string, to: string, getCanonicalFileNameOrIgnoreCase: GetCanonicalFileName | boolean) { + Debug.assert((getRootLength(fromDirectory) > 0) === (getRootLength(to) > 0), "Paths must either both be absolute or both be relative"); const getCanonicalFileName = typeof getCanonicalFileNameOrIgnoreCase === "function" ? getCanonicalFileNameOrIgnoreCase : identity; const ignoreCase = typeof getCanonicalFileNameOrIgnoreCase === "boolean" ? getCanonicalFileNameOrIgnoreCase : false; - const pathComponents = getPathComponentsRelativeTo(from, to, ignoreCase ? equateStringsCaseInsensitive : equateStringsCaseSensitive, getCanonicalFileName); + const pathComponents = getPathComponentsRelativeTo(fromDirectory, to, ignoreCase ? equateStringsCaseInsensitive : equateStringsCaseSensitive, getCanonicalFileName); return getPathFromPathComponents(pathComponents); } diff --git a/src/harness/unittests/paths.ts b/src/harness/unittests/paths.ts index 0cd90be0483d0..0b28bedc929af 100644 --- a/src/harness/unittests/paths.ts +++ b/src/harness/unittests/paths.ts @@ -268,25 +268,25 @@ describe("core paths", () => { assert.strictEqual(ts.resolvePath("a", "b", "../c"), "a/c"); }); it("getPathRelativeTo", () => { - assert.strictEqual(ts.getRelativePath("/", "/", /*ignoreCase*/ false), ""); - assert.strictEqual(ts.getRelativePath("/a", "/a", /*ignoreCase*/ false), ""); - assert.strictEqual(ts.getRelativePath("/a/", "/a", /*ignoreCase*/ false), ""); - assert.strictEqual(ts.getRelativePath("/a", "/", /*ignoreCase*/ false), ".."); - assert.strictEqual(ts.getRelativePath("/a", "/b", /*ignoreCase*/ false), "../b"); - assert.strictEqual(ts.getRelativePath("/a/b", "/b", /*ignoreCase*/ false), "../../b"); - assert.strictEqual(ts.getRelativePath("/a/b/c", "/b", /*ignoreCase*/ false), "../../../b"); - assert.strictEqual(ts.getRelativePath("/a/b/c", "/b/c", /*ignoreCase*/ false), "../../../b/c"); - assert.strictEqual(ts.getRelativePath("/a/b/c", "/a/b", /*ignoreCase*/ false), ".."); - assert.strictEqual(ts.getRelativePath("c:", "d:", /*ignoreCase*/ false), "d:/"); - assert.strictEqual(ts.getRelativePath("file:///", "file:///", /*ignoreCase*/ false), ""); - assert.strictEqual(ts.getRelativePath("file:///a", "file:///a", /*ignoreCase*/ false), ""); - assert.strictEqual(ts.getRelativePath("file:///a/", "file:///a", /*ignoreCase*/ false), ""); - assert.strictEqual(ts.getRelativePath("file:///a", "file:///", /*ignoreCase*/ false), ".."); - assert.strictEqual(ts.getRelativePath("file:///a", "file:///b", /*ignoreCase*/ false), "../b"); - assert.strictEqual(ts.getRelativePath("file:///a/b", "file:///b", /*ignoreCase*/ false), "../../b"); - assert.strictEqual(ts.getRelativePath("file:///a/b/c", "file:///b", /*ignoreCase*/ false), "../../../b"); - assert.strictEqual(ts.getRelativePath("file:///a/b/c", "file:///b/c", /*ignoreCase*/ false), "../../../b/c"); - assert.strictEqual(ts.getRelativePath("file:///a/b/c", "file:///a/b", /*ignoreCase*/ false), ".."); - assert.strictEqual(ts.getRelativePath("file:///c:", "file:///d:", /*ignoreCase*/ false), "file:///d:/"); + assert.strictEqual(ts.getRelativePathFromDirectory("/", "/", /*ignoreCase*/ false), ""); + assert.strictEqual(ts.getRelativePathFromDirectory("/a", "/a", /*ignoreCase*/ false), ""); + assert.strictEqual(ts.getRelativePathFromDirectory("/a/", "/a", /*ignoreCase*/ false), ""); + assert.strictEqual(ts.getRelativePathFromDirectory("/a", "/", /*ignoreCase*/ false), ".."); + assert.strictEqual(ts.getRelativePathFromDirectory("/a", "/b", /*ignoreCase*/ false), "../b"); + assert.strictEqual(ts.getRelativePathFromDirectory("/a/b", "/b", /*ignoreCase*/ false), "../../b"); + assert.strictEqual(ts.getRelativePathFromDirectory("/a/b/c", "/b", /*ignoreCase*/ false), "../../../b"); + assert.strictEqual(ts.getRelativePathFromDirectory("/a/b/c", "/b/c", /*ignoreCase*/ false), "../../../b/c"); + assert.strictEqual(ts.getRelativePathFromDirectory("/a/b/c", "/a/b", /*ignoreCase*/ false), ".."); + assert.strictEqual(ts.getRelativePathFromDirectory("c:", "d:", /*ignoreCase*/ false), "d:/"); + assert.strictEqual(ts.getRelativePathFromDirectory("file:///", "file:///", /*ignoreCase*/ false), ""); + assert.strictEqual(ts.getRelativePathFromDirectory("file:///a", "file:///a", /*ignoreCase*/ false), ""); + assert.strictEqual(ts.getRelativePathFromDirectory("file:///a/", "file:///a", /*ignoreCase*/ false), ""); + assert.strictEqual(ts.getRelativePathFromDirectory("file:///a", "file:///", /*ignoreCase*/ false), ".."); + assert.strictEqual(ts.getRelativePathFromDirectory("file:///a", "file:///b", /*ignoreCase*/ false), "../b"); + assert.strictEqual(ts.getRelativePathFromDirectory("file:///a/b", "file:///b", /*ignoreCase*/ false), "../../b"); + assert.strictEqual(ts.getRelativePathFromDirectory("file:///a/b/c", "file:///b", /*ignoreCase*/ false), "../../../b"); + assert.strictEqual(ts.getRelativePathFromDirectory("file:///a/b/c", "file:///b/c", /*ignoreCase*/ false), "../../../b/c"); + assert.strictEqual(ts.getRelativePathFromDirectory("file:///a/b/c", "file:///a/b", /*ignoreCase*/ false), ".."); + assert.strictEqual(ts.getRelativePathFromDirectory("file:///c:", "file:///d:", /*ignoreCase*/ false), "file:///d:/"); }); }); \ No newline at end of file diff --git a/src/harness/vpath.ts b/src/harness/vpath.ts index 9b42ba2a28db8..cee41d9df9154 100644 --- a/src/harness/vpath.ts +++ b/src/harness/vpath.ts @@ -18,7 +18,7 @@ namespace vpath { export import dirname = ts.getDirectoryPath; export import basename = ts.getBaseFileName; export import extname = ts.getAnyExtensionFromPath; - export import relative = ts.getRelativePath; + export import relative = ts.getRelativePathFromDirectory; export import beneath = ts.containsPath; export import changeExtension = ts.changeAnyExtension; export import isTypeScript = ts.hasTypeScriptFileExtension; diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 68b6e9a856f62..43fd56ae0b6d9 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -266,7 +266,7 @@ namespace ts.codefix { return [global]; } - const relativePath = removeExtensionAndIndexPostFix(ensurePathIsNonModuleName(getRelativePath(sourceDirectory, moduleFileName, getCanonicalFileName)), moduleResolutionKind, addJsExtension); + const relativePath = removeExtensionAndIndexPostFix(ensurePathIsNonModuleName(getRelativePathFromDirectory(sourceDirectory, moduleFileName, getCanonicalFileName)), moduleResolutionKind, addJsExtension); if (!baseUrl || preferences.importModuleSpecifierPreference === "relative") { return [relativePath]; } @@ -321,7 +321,7 @@ namespace ts.codefix { 1 < 2 = true In this case we should prefer using the relative path "../a" instead of the baseUrl path "foo/a". */ - const pathFromSourceToBaseUrl = ensurePathIsNonModuleName(getRelativePath(sourceDirectory, baseUrl, getCanonicalFileName)); + const pathFromSourceToBaseUrl = ensurePathIsNonModuleName(getRelativePathFromDirectory(sourceDirectory, baseUrl, getCanonicalFileName)); const relativeFirst = getRelativePathNParents(relativePath) < getRelativePathNParents(pathFromSourceToBaseUrl); return relativeFirst ? [relativePath, importRelativeToBaseUrl] : [importRelativeToBaseUrl, relativePath]; }); @@ -390,7 +390,7 @@ namespace ts.codefix { } const normalizedSourcePath = getPathRelativeToRootDirs(sourceDirectory, rootDirs, getCanonicalFileName); - const relativePath = normalizedSourcePath !== undefined ? ensurePathIsNonModuleName(getRelativePath(normalizedSourcePath, normalizedTargetPath, getCanonicalFileName)) : normalizedTargetPath; + const relativePath = normalizedSourcePath !== undefined ? ensurePathIsNonModuleName(getRelativePathFromDirectory(normalizedSourcePath, normalizedTargetPath, getCanonicalFileName)) : normalizedTargetPath; return removeFileExtension(relativePath); } @@ -473,7 +473,7 @@ namespace ts.codefix { return path.substring(parts.topLevelPackageNameIndex + 1); } else { - return ensurePathIsNonModuleName(getRelativePath(sourceDirectory, path, getCanonicalFileName)); + return ensurePathIsNonModuleName(getRelativePathFromDirectory(sourceDirectory, path, getCanonicalFileName)); } } } diff --git a/src/services/getEditsForFileRename.ts b/src/services/getEditsForFileRename.ts index 741ba977184cf..23193e902c0ef 100644 --- a/src/services/getEditsForFileRename.ts +++ b/src/services/getEditsForFileRename.ts @@ -63,7 +63,7 @@ namespace ts { function getPathUpdater(oldFilePath: string, newFilePath: string, host: LanguageServiceHost): (oldPath: string) => string | undefined { // Get the relative path from old to new location, and append it on to the end of imports and normalize. - const rel = ensurePathIsNonModuleName(getRelativePath(getDirectoryPath(oldFilePath), newFilePath, createGetCanonicalFileName(hostUsesCaseSensitiveFileNames(host)))); + const rel = getRelativePathFromFile(oldFilePath, newFilePath, createGetCanonicalFileName(hostUsesCaseSensitiveFileNames(host))); return oldPath => { if (!pathIsRelative(oldPath)) return; return ensurePathIsNonModuleName(normalizePath(combinePaths(getDirectoryPath(oldPath), rel))); diff --git a/src/services/refactors/moveToNewFile.ts b/src/services/refactors/moveToNewFile.ts index 1dc9b8d74bf2c..07df9edb6607e 100644 --- a/src/services/refactors/moveToNewFile.ts +++ b/src/services/refactors/moveToNewFile.ts @@ -42,12 +42,17 @@ namespace ts.refactor { // If previous file was global, this is easy. changes.createNewFile(oldFile, combinePaths(currentDirectory, newFileNameWithExtension), getNewStatements(oldFile, usage, changes, toMove, program, newModuleName)); - addNewFileToTsconfig(program, changes, normalizePath(combinePaths(oldFile.fileName, "..", newFileNameWithExtension))); + addNewFileToTsconfig(program, changes, oldFile.fileName, newFileNameWithExtension, hostGetCanonicalFileName(host)); } - function addNewFileToTsconfig(program: Program, changes: textChanges.ChangeTracker, newFilePath: string): void { + function addNewFileToTsconfig(program: Program, changes: textChanges.ChangeTracker, oldFileName: string, newFileNameWithExtension: string, getCanonicalFileName: GetCanonicalFileName): void { const cfg = program.getCompilerOptions().configFile; - const filesProp = cfg && cfg.jsonObject && find(cfg.jsonObject.properties, (prop): prop is PropertyAssignment => + if (!cfg) return; + + const newFileAbsolutePath = normalizePath(combinePaths(oldFileName, "..", newFileNameWithExtension)); + const newFilePath = getRelativePathFromFile(cfg.fileName, newFileAbsolutePath, getCanonicalFileName); + + const filesProp = cfg.jsonObject && find(cfg.jsonObject.properties, (prop): prop is PropertyAssignment => isPropertyAssignment(prop) && isStringLiteral(prop.name) && prop.name.text === "files"); if (filesProp && isArrayLiteralExpression(filesProp.initializer)) { changes.insertNodeInListAfter(cfg, last(filesProp.initializer.elements), createLiteral(newFilePath), filesProp.initializer.elements); diff --git a/tests/cases/fourslash/moveToNewFile_tsconfig.ts b/tests/cases/fourslash/moveToNewFile_tsconfig.ts index f58d5225f3af1..264f2a85ef22f 100644 --- a/tests/cases/fourslash/moveToNewFile_tsconfig.ts +++ b/tests/cases/fourslash/moveToNewFile_tsconfig.ts @@ -1,26 +1,28 @@ /// -// @Filename: /a.ts +// @Filename: /src/a.ts ////0; ////[|1;|] -// @Filename: /tsconfig.json +// @Filename: /src/tsconfig.json ////{ -//// "files": ["/a.ts"] +//// "files": ["./a.ts"] ////} +verify.noErrors(); + verify.moveToNewFile({ newFileContents: { - "/a.ts": + "/src/a.ts": `0; `, - "/newFile.ts": + "/src/newFile.ts": `1;`, - "/tsconfig.json": + "/src/tsconfig.json": `{ - "files": ["/a.ts", "/newFile.ts"] + "files": ["./a.ts", "./newFile.ts"] }`, }, }); From 886a937ba093fefb1d6fefb889517483d682e66e Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Tue, 8 May 2018 11:17:16 -0700 Subject: [PATCH 11/12] Code review --- src/compiler/core.ts | 2 +- src/harness/fourslash.ts | 13 +++++++++++++ src/services/refactors/moveToNewFile.ts | 9 +++++---- tests/cases/fourslash/fourslash.ts | 1 + tests/cases/fourslash/moveToNewFile.ts | 4 ++-- .../fourslash/moveToNewFile_moveImport.ts | 18 ++++++++++++++++++ .../fourslash/moveToNewFile_rangeInvalid.ts | 5 +---- 7 files changed, 41 insertions(+), 11 deletions(-) create mode 100644 tests/cases/fourslash/moveToNewFile_moveImport.ts diff --git a/src/compiler/core.ts b/src/compiler/core.ts index 08e3d6bbe7aa8..2be4f95eb8059 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -2290,7 +2290,7 @@ namespace ts { } export function getRelativePathFromFile(from: string, to: string, getCanonicalFileName: GetCanonicalFileName) { - return ensurePathIsNonModuleName(getRelativePathFromDirectory(ts.getDirectoryPath(from), to, getCanonicalFileName)); + return ensurePathIsNonModuleName(getRelativePathFromDirectory(getDirectoryPath(from), to, getCanonicalFileName)); } /** diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 4d75ccf706fa2..d8d6eaed7be03 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -3155,6 +3155,16 @@ Actual: ${stringify(fullActual)}`); } } + public noMoveToNewFile() { + for (const range of this.getRanges()) { + for (const refactor of this.getApplicableRefactors(range, { allowTextChangesInNewFiles: true })) { + if (refactor.name === "Move to a new file") { + ts.Debug.fail("Did not expect to get 'move to a new file' refactor"); + } + } + } + } + public moveToNewFile(options: FourSlashInterface.MoveToNewFileOptions): void { assert(this.getRanges().length === 1); const range = this.getRanges()[0]; @@ -4509,6 +4519,9 @@ namespace FourSlashInterface { public moveToNewFile(options: MoveToNewFileOptions): void { this.state.moveToNewFile(options); } + public noMoveToNewFile(): void { + this.state.noMoveToNewFile(); + } } export class Edit { diff --git a/src/services/refactors/moveToNewFile.ts b/src/services/refactors/moveToNewFile.ts index 07df9edb6607e..f75d9ce884236 100644 --- a/src/services/refactors/moveToNewFile.ts +++ b/src/services/refactors/moveToNewFile.ts @@ -3,7 +3,7 @@ namespace ts.refactor { const refactorName = "Move to a new file"; registerRefactor(refactorName, { getAvailableActions(context): ApplicableRefactorInfo[] { - if (getStatementsToMove(context) === undefined || !context.preferences.allowTextChangesInNewFiles) return undefined; + if (!context.preferences.allowTextChangesInNewFiles || getStatementsToMove(context) === undefined) return undefined; const description = getLocaleSpecificMessage(Diagnostics.Move_to_a_new_file); return [{ name: refactorName, description, actions: [{ name: refactorName, description }] }]; }, @@ -23,11 +23,12 @@ namespace ts.refactor { const startNodeIndex = findIndex(statements, s => s.end > range.pos); if (startNodeIndex === -1) return undefined; // Can't only partially include the start node or be partially into the next node - const okStart = range.pos <= statements[startNodeIndex].getStart(file); + if (range.pos > statements[startNodeIndex].getStart(file)) return undefined; const afterEndNodeIndex = findIndex(statements, s => s.end > range.end, startNodeIndex); // Can't be partially into the next node - const okEnd = afterEndNodeIndex === -1 || afterEndNodeIndex !== 0 && statements[afterEndNodeIndex].getStart(file) >= range.end; - return okStart && okEnd ? statements.slice(startNodeIndex, afterEndNodeIndex === -1 ? statements.length : afterEndNodeIndex) : undefined; + if (afterEndNodeIndex !== -1 && (afterEndNodeIndex === 0 || statements[afterEndNodeIndex].getStart(file) < range.end)) return undefined; + + return statements.slice(startNodeIndex, afterEndNodeIndex === -1 ? statements.length : afterEndNodeIndex); } function doChange(oldFile: SourceFile, program: Program, toMove: ReadonlyArray, changes: textChanges.ChangeTracker, host: LanguageServiceHost): void { diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index a20e6ecd06aee..3832fe074d457 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -362,6 +362,7 @@ declare namespace FourSlashInterface { moveToNewFile(options: { readonly newFileContents: { readonly [fileName: string]: string }; }): void; + noMoveToNewFile(): void; } class edit { backspace(count?: number): void; diff --git a/tests/cases/fourslash/moveToNewFile.ts b/tests/cases/fourslash/moveToNewFile.ts index 074e341a778b4..cd2bd4ff8d62a 100644 --- a/tests/cases/fourslash/moveToNewFile.ts +++ b/tests/cases/fourslash/moveToNewFile.ts @@ -1,7 +1,7 @@ /// // @Filename: /a.ts -////import { a, b } from "./other"; +////import { a, b, alreadyUnused } from "./other"; ////const p = 0; ////[|const y = p + b;|] ////a; y; @@ -11,7 +11,7 @@ verify.moveToNewFile({ "/a.ts": `import { y } from "./y"; -import { a } from "./other"; +import { a, alreadyUnused } from "./other"; export const p = 0; a; y;`, diff --git a/tests/cases/fourslash/moveToNewFile_moveImport.ts b/tests/cases/fourslash/moveToNewFile_moveImport.ts new file mode 100644 index 0000000000000..a96a187f633f6 --- /dev/null +++ b/tests/cases/fourslash/moveToNewFile_moveImport.ts @@ -0,0 +1,18 @@ +/// + +// @Filename: /a.ts +////[|import { a, b } from "m"; +////a;|] +////b; + +//verify.noMoveToNewFile(); +verify.moveToNewFile({ + newFileContents: { + "/a.ts": +`b;`, + "/newFile.ts": +`import { a } from "m"; +import { a, b } from "m"; +a;`, + } +}); diff --git a/tests/cases/fourslash/moveToNewFile_rangeInvalid.ts b/tests/cases/fourslash/moveToNewFile_rangeInvalid.ts index e217828122256..dc0ba661be48b 100644 --- a/tests/cases/fourslash/moveToNewFile_rangeInvalid.ts +++ b/tests/cases/fourslash/moveToNewFile_rangeInvalid.ts @@ -7,7 +7,4 @@ //// [|function inner() {}|] ////} -for (const range of test.ranges()) { - goTo.selectRange(range); - verify.not.refactorAvailable("Move to a new file") -} +verify.noMoveToNewFile(); From 662e93c45c8360a1cd25d8fdf41f9daf7712ae76 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Tue, 8 May 2018 12:00:03 -0700 Subject: [PATCH 12/12] Fix error where node in tsconfig file was missing a source file --- src/services/breakpoints.ts | 4 ++-- src/services/formatting/smartIndenter.ts | 2 +- src/services/refactors/moveToNewFile.ts | 3 ++- src/services/types.ts | 2 +- src/services/utilities.ts | 20 +++++++++---------- .../reference/api/tsserverlibrary.d.ts | 2 +- tests/baselines/reference/api/typescript.d.ts | 2 +- 7 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/services/breakpoints.ts b/src/services/breakpoints.ts index faab2b08db67c..6815be9863d1a 100644 --- a/src/services/breakpoints.ts +++ b/src/services/breakpoints.ts @@ -41,7 +41,7 @@ namespace ts.BreakpointResolver { } function textSpanEndingAtNextToken(startNode: Node, previousTokenToFindNextEndToken: Node): TextSpan { - return textSpan(startNode, findNextToken(previousTokenToFindNextEndToken, previousTokenToFindNextEndToken.parent)); + return textSpan(startNode, findNextToken(previousTokenToFindNextEndToken, previousTokenToFindNextEndToken.parent, sourceFile)); } function spanInNodeIfStartsOnSameLine(node: Node, otherwiseOnNode?: Node): TextSpan { @@ -60,7 +60,7 @@ namespace ts.BreakpointResolver { } function spanInNextNode(node: Node): TextSpan { - return spanInNode(findNextToken(node, node.parent)); + return spanInNode(findNextToken(node, node.parent, sourceFile)); } function spanInNode(node: Node): TextSpan { diff --git a/src/services/formatting/smartIndenter.ts b/src/services/formatting/smartIndenter.ts index 0e89be425328c..a75781b8da0e4 100644 --- a/src/services/formatting/smartIndenter.ts +++ b/src/services/formatting/smartIndenter.ts @@ -269,7 +269,7 @@ namespace ts.formatting { } function nextTokenIsCurlyBraceOnSameLineAsCursor(precedingToken: Node, current: Node, lineAtPosition: number, sourceFile: SourceFile): NextTokenKind { - const nextToken = findNextToken(precedingToken, current); + const nextToken = findNextToken(precedingToken, current, sourceFile); if (!nextToken) { return NextTokenKind.Unknown; } diff --git a/src/services/refactors/moveToNewFile.ts b/src/services/refactors/moveToNewFile.ts index f75d9ce884236..2f5a27f87f781 100644 --- a/src/services/refactors/moveToNewFile.ts +++ b/src/services/refactors/moveToNewFile.ts @@ -53,7 +53,8 @@ namespace ts.refactor { const newFileAbsolutePath = normalizePath(combinePaths(oldFileName, "..", newFileNameWithExtension)); const newFilePath = getRelativePathFromFile(cfg.fileName, newFileAbsolutePath, getCanonicalFileName); - const filesProp = cfg.jsonObject && find(cfg.jsonObject.properties, (prop): prop is PropertyAssignment => + const cfgObject = cfg.statements[0] && tryCast(cfg.statements[0].expression, isObjectLiteralExpression); + const filesProp = cfgObject && find(cfgObject.properties, (prop): prop is PropertyAssignment => isPropertyAssignment(prop) && isStringLiteral(prop.name) && prop.name.text === "files"); if (filesProp && isArrayLiteralExpression(filesProp.initializer)) { changes.insertNodeInListAfter(cfg, last(filesProp.initializer.elements), createLiteral(newFilePath), filesProp.initializer.elements); diff --git a/src/services/types.ts b/src/services/types.ts index 785d3f3f05297..cf2e58a3712a6 100644 --- a/src/services/types.ts +++ b/src/services/types.ts @@ -13,7 +13,7 @@ namespace ts { getStart(sourceFile?: SourceFileLike, includeJsDocComment?: boolean): number; getFullStart(): number; getEnd(): number; - getWidth(sourceFile?: SourceFile): number; + getWidth(sourceFile?: SourceFileLike): number; getFullWidth(): number; getLeadingTriviaWidth(sourceFile?: SourceFile): number; getFullText(sourceFile?: SourceFile): string; diff --git a/src/services/utilities.ts b/src/services/utilities.ts index fac577b3597f0..55b5f4f0ff910 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -707,7 +707,7 @@ namespace ts { return findPrecedingToken(position, file); } - export function findNextToken(previousToken: Node, parent: Node): Node { + export function findNextToken(previousToken: Node, parent: Node, sourceFile: SourceFile): Node { return find(parent); function find(n: Node): Node { @@ -724,7 +724,7 @@ namespace ts { // previous token ends exactly at the beginning of child (child.pos === previousToken.end); - if (shouldDiveInChildNode && nodeHasTokens(child)) { + if (shouldDiveInChildNode && nodeHasTokens(child, sourceFile)) { return find(child); } } @@ -759,12 +759,12 @@ namespace ts { const start = child.getStart(sourceFile, includeJsDoc); const lookInPreviousChild = (start >= position) || // cursor in the leading trivia - !nodeHasTokens(child) || + !nodeHasTokens(child, sourceFile) || isWhiteSpaceOnlyJsxText(child); if (lookInPreviousChild) { // actual start of the node is past the position - previous token should be at the end of previous child - const candidate = findRightmostChildNodeWithTokens(children, /*exclusiveStartPosition*/ i); + const candidate = findRightmostChildNodeWithTokens(children, /*exclusiveStartPosition*/ i, sourceFile); return candidate && findRightmostToken(candidate, sourceFile); } else { @@ -781,7 +781,7 @@ namespace ts { // Try to find the rightmost token in the file without filtering. // Namely we are skipping the check: 'position < node.end' if (children.length) { - const candidate = findRightmostChildNodeWithTokens(children, /*exclusiveStartPosition*/ children.length); + const candidate = findRightmostChildNodeWithTokens(children, /*exclusiveStartPosition*/ children.length, sourceFile); return candidate && findRightmostToken(candidate, sourceFile); } } @@ -797,21 +797,21 @@ namespace ts { } const children = n.getChildren(sourceFile); - const candidate = findRightmostChildNodeWithTokens(children, /*exclusiveStartPosition*/ children.length); + const candidate = findRightmostChildNodeWithTokens(children, /*exclusiveStartPosition*/ children.length, sourceFile); return candidate && findRightmostToken(candidate, sourceFile); } /** * Finds the rightmost child to the left of `children[exclusiveStartPosition]` which is a non-all-whitespace token or has constituent tokens. */ - function findRightmostChildNodeWithTokens(children: Node[], exclusiveStartPosition: number): Node | undefined { + function findRightmostChildNodeWithTokens(children: Node[], exclusiveStartPosition: number, sourceFile: SourceFile): Node | undefined { for (let i = exclusiveStartPosition - 1; i >= 0; i--) { const child = children[i]; if (isWhiteSpaceOnlyJsxText(child)) { Debug.assert(i > 0, "`JsxText` tokens should not be the first child of `JsxElement | JsxSelfClosingElement`"); } - else if (nodeHasTokens(children[i])) { + else if (nodeHasTokens(children[i], sourceFile)) { return children[i]; } } @@ -1022,10 +1022,10 @@ namespace ts { } } - function nodeHasTokens(n: Node): boolean { + function nodeHasTokens(n: Node, sourceFile: SourceFileLike): boolean { // If we have a token or node that has a non-zero width, it must have tokens. // Note: getWidth() does not take trivia into account. - return n.getWidth() !== 0; + return n.getWidth(sourceFile) !== 0; } export function getNodeModifiers(node: Node): string { diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index d405866b6d314..8cccebc9ffbc8 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -4326,7 +4326,7 @@ declare namespace ts { getStart(sourceFile?: SourceFile, includeJsDocComment?: boolean): number; getFullStart(): number; getEnd(): number; - getWidth(sourceFile?: SourceFile): number; + getWidth(sourceFile?: SourceFileLike): number; getFullWidth(): number; getLeadingTriviaWidth(sourceFile?: SourceFile): number; getFullText(sourceFile?: SourceFile): string; diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts index 965c7a048b752..a760b1816ae97 100644 --- a/tests/baselines/reference/api/typescript.d.ts +++ b/tests/baselines/reference/api/typescript.d.ts @@ -4326,7 +4326,7 @@ declare namespace ts { getStart(sourceFile?: SourceFile, includeJsDocComment?: boolean): number; getFullStart(): number; getEnd(): number; - getWidth(sourceFile?: SourceFile): number; + getWidth(sourceFile?: SourceFileLike): number; getFullWidth(): number; getLeadingTriviaWidth(sourceFile?: SourceFile): number; getFullText(sourceFile?: SourceFile): string;