Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'move to new file' refactor #23726

Merged
15 commits merged into from
May 10, 2018
4 changes: 2 additions & 2 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -311,8 +311,8 @@ namespace ts {
}

/** Works like Array.prototype.findIndex, returning `-1` if no element satisfying the predicate is found. */
export function findIndex<T>(array: ReadonlyArray<T>, predicate: (element: T, index: number) => boolean): number {
for (let i = 0; i < array.length; i++) {
export function findIndex<T>(array: ReadonlyArray<T>, predicate: (element: T, index: number) => boolean, startIndex = 0): number {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you instead make it startIndex?: number and use let i = startIndex || 0 ?

for (let i = startIndex; i < array.length; i++) {
if (predicate(array[i], i)) {
return i;
}
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -4174,5 +4174,9 @@
"Generate 'get' and 'set' accessors": {
"category": "Message",
"code": 95046
},
"Move to a new file": {
"category": "Message",
"code": 95047
}
}
12 changes: 9 additions & 3 deletions src/compiler/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1504,6 +1504,13 @@ namespace ts {
return block;
}

/* @internal */
export function createExpressionStatement(expression: Expression): ExpressionStatement {
const node = <ExpressionStatement>createSynthesizedNode(SyntaxKind.ExpressionStatement);
node.expression = expression;
return node;
}

export function updateBlock(node: Block, statements: ReadonlyArray<Statement>) {
return node.statements !== statements
? updateNode(createBlock(statements, node.multiLine), node)
Expand All @@ -1530,9 +1537,8 @@ namespace ts {
}

export function createStatement(expression: Expression) {
const node = <ExpressionStatement>createSynthesizedNode(SyntaxKind.ExpressionStatement);
node.expression = parenthesizeExpressionForExpressionStatement(expression);
return node;
return createExpressionStatement(parenthesizeExpressionForExpressionStatement(expression));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: extra newline.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider linting for no-padding.

}

export function updateStatement(node: ExpressionStatement, expression: Expression) {
Expand Down
61 changes: 48 additions & 13 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.`);
}
Expand All @@ -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;

Expand All @@ -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);
}

Expand All @@ -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.`);
}
Expand All @@ -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)}`);
Expand Down Expand Up @@ -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 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");

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(
Expand Down Expand Up @@ -3278,6 +3301,10 @@ Actual: ${stringify(fullActual)}`);
this.verifyCurrentFileContent(options.newFileContents[fileName]);
}
}

private getApplicableRefactors(positionOrRange: number | ts.TextRange): ReadonlyArray<ts.ApplicableRefactorInfo> {
return this.languageService.getApplicableRefactors(this.activeFile.fileName, positionOrRange, ts.defaultPreferences) || ts.emptyArray;
}
}

export function runFourSlashTest(basePath: string, testType: FourSlashTestType, fileName: string) {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -4721,4 +4752,8 @@ namespace FourSlashInterface {
readonly newPath: string;
readonly newFileContents: { readonly [fileName: string]: string };
}

export interface MoveToNewFileOptions {
readonly newFileContents: { readonly [fileName: string]: string };
}
}
1 change: 1 addition & 0 deletions src/harness/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions src/server/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions src/server/tsconfig.library.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
9 changes: 0 additions & 9 deletions src/services/codefixes/convertToEs6Module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -507,15 +507,6 @@ namespace ts.codefix {
: makeImport(/*name*/ undefined, [makeImportSpecifier(propertyName, localName)], moduleSpecifier);
}

function makeImport(name: Identifier | undefined, namedImports: ReadonlyArray<ImportSpecifier> | undefined, moduleSpecifier: StringLiteralLike): ImportDeclaration {
return makeImportDeclaration(name, namedImports, moduleSpecifier);
}

export function makeImportDeclaration(name: Identifier, namedImports: ReadonlyArray<ImportSpecifier> | 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));
}
Expand Down
2 changes: 1 addition & 1 deletion src/services/codefixes/fixInvalidImportSyntax.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
2 changes: 1 addition & 1 deletion src/services/codefixes/useDefaultImport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
16 changes: 4 additions & 12 deletions src/services/findAllReferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -612,27 +612,19 @@ namespace ts.FindAllReferences.Core {
checker.getPropertySymbolOfDestructuringAssignment(<Identifier>location);
}

function getObjectBindingElementWithoutPropertyName(symbol: Symbol): BindingElement | undefined {
function getObjectBindingElementWithoutPropertyName(symbol: Symbol): BindingElement & { name: Identifier } | undefined {
const bindingElement = getDeclarationOfKind<BindingElement>(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, (<Identifier>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);
}

/**
Expand Down
15 changes: 2 additions & 13 deletions src/services/importTracker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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 };
}
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions src/services/refactorProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
4 changes: 2 additions & 2 deletions src/services/refactors/extractSymbol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down