Skip to content

Commit

Permalink
Hierarchical refactorings (#41975)
Browse files Browse the repository at this point in the history
* add hierarchical refactoring strings

* fourslash tests

* extractSymbol filters returned actions

* move refactorKind check to utilities

* rename parameters

* messaging for addOrRemoveBracesToArrowFunction

* fix up inferFunctionReturnType

* fix up convertArrowFunctionOrFunctionExpression

* add preferences to fourslash method

* fix up convert string

* fix up moveToNewFile

* fix lint errors

* remove extra arrow braces diagnostics

* break out tests

* add refactor helpers

* refactor refactors

* keep list of actions

* address PR comments

* response protocol

* address more comments
  • Loading branch information
Jesse Trinity authored Dec 23, 2020
1 parent d1ac451 commit 8bbef81
Show file tree
Hide file tree
Showing 37 changed files with 627 additions and 325 deletions.
28 changes: 28 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -6143,6 +6143,34 @@
"category": "Message",
"code": 95148
},
"Return type must be inferred from a function": {
"category": "Message",
"code": 95149
},
"Could not determine function return type": {
"category": "Message",
"code": 95150
},
"Could not convert to arrow function": {
"category": "Message",
"code": 95151
},
"Could not convert to named function": {
"category": "Message",
"code": 95152
},
"Could not convert to anonymous function": {
"category": "Message",
"code": 95153
},
"Can only convert string concatenation": {
"category": "Message",
"code": 95154
},
"Selection is not a valid statement or statements": {
"category": "Message",
"code": 95155
},

"No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
"category": "Error",
Expand Down
18 changes: 12 additions & 6 deletions src/harness/fourslashImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3420,6 +3420,12 @@ namespace FourSlash {
}
}

public verifyRefactorKindsAvailable(kind: string, expected: string[], preferences = ts.emptyOptions) {
const refactors = this.getApplicableRefactorsAtSelection("invoked", kind, preferences);
const availableKinds = ts.flatMap(refactors, refactor => refactor.actions).map(action => action.kind);
assert.deepEqual(availableKinds.sort(), expected.sort(), `Expected kinds to be equal`);
}

public verifyRefactorsAvailable(names: readonly string[]): void {
assert.deepEqual(unique(this.getApplicableRefactorsAtSelection(), r => r.name), names);
}
Expand Down Expand Up @@ -3833,14 +3839,14 @@ namespace FourSlash {
test(renameKeys(newFileContents, key => pathUpdater(key) || key), "with file moved");
}

private getApplicableRefactorsAtSelection(triggerReason: ts.RefactorTriggerReason = "implicit") {
return this.getApplicableRefactorsWorker(this.getSelection(), this.activeFile.fileName, ts.emptyOptions, triggerReason);
private getApplicableRefactorsAtSelection(triggerReason: ts.RefactorTriggerReason = "implicit", kind?: string, preferences = ts.emptyOptions) {
return this.getApplicableRefactorsWorker(this.getSelection(), this.activeFile.fileName, preferences, triggerReason, kind);
}
private getApplicableRefactors(rangeOrMarker: Range | Marker, preferences = ts.emptyOptions, triggerReason: ts.RefactorTriggerReason = "implicit"): readonly ts.ApplicableRefactorInfo[] {
return this.getApplicableRefactorsWorker("position" in rangeOrMarker ? rangeOrMarker.position : rangeOrMarker, rangeOrMarker.fileName, preferences, triggerReason); // eslint-disable-line no-in-operator
private getApplicableRefactors(rangeOrMarker: Range | Marker, preferences = ts.emptyOptions, triggerReason: ts.RefactorTriggerReason = "implicit", kind?: string): readonly ts.ApplicableRefactorInfo[] {
return this.getApplicableRefactorsWorker("position" in rangeOrMarker ? rangeOrMarker.position : rangeOrMarker, rangeOrMarker.fileName, preferences, triggerReason, kind); // eslint-disable-line no-in-operator
}
private getApplicableRefactorsWorker(positionOrRange: number | ts.TextRange, fileName: string, preferences = ts.emptyOptions, triggerReason: ts.RefactorTriggerReason): readonly ts.ApplicableRefactorInfo[] {
return this.languageService.getApplicableRefactors(fileName, positionOrRange, preferences, triggerReason) || ts.emptyArray;
private getApplicableRefactorsWorker(positionOrRange: number | ts.TextRange, fileName: string, preferences = ts.emptyOptions, triggerReason: ts.RefactorTriggerReason, kind?: string): readonly ts.ApplicableRefactorInfo[] {
return this.languageService.getApplicableRefactors(fileName, positionOrRange, preferences, triggerReason, kind) || ts.emptyArray;
}

public configurePlugin(pluginName: string, configuration: any): void {
Expand Down
4 changes: 4 additions & 0 deletions src/harness/fourslashInterfaceImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,10 @@ namespace FourSlashInterface {
this.state.verifyRefactorAvailable(this.negative, triggerReason, name, actionName);
}

public refactorKindAvailable(kind: string, expected: string[], preferences = ts.emptyOptions) {
this.state.verifyRefactorKindsAvailable(kind, expected, preferences);
}

public toggleLineComment(newFileContent: string) {
this.state.toggleLineComment(newFileContent);
}
Expand Down
8 changes: 7 additions & 1 deletion src/server/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,8 @@ namespace ts.server.protocol {
arguments: GetApplicableRefactorsRequestArgs;
}
export type GetApplicableRefactorsRequestArgs = FileLocationOrRangeRequestArgs & {
triggerReason?: RefactorTriggerReason
triggerReason?: RefactorTriggerReason;
kind?: string;
};

export type RefactorTriggerReason = "implicit" | "invoked";
Expand Down Expand Up @@ -626,6 +627,11 @@ namespace ts.server.protocol {
* the current context.
*/
notApplicableReason?: string;

/**
* The hierarchical dotted name of the refactor action.
*/
kind?: string;
}

export interface GetEditsForRefactorRequest extends Request {
Expand Down
2 changes: 1 addition & 1 deletion src/server/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2129,7 +2129,7 @@ namespace ts.server {
private getApplicableRefactors(args: protocol.GetApplicableRefactorsRequestArgs): protocol.ApplicableRefactorInfo[] {
const { file, project } = this.getFileAndProject(args);
const scriptInfo = project.getScriptInfoForNormalizedPath(file)!;
return project.getLanguageService().getApplicableRefactors(file, this.extractPositionOrRange(args, scriptInfo), this.getPreferences(file), args.triggerReason);
return project.getLanguageService().getApplicableRefactors(file, this.extractPositionOrRange(args, scriptInfo), this.getPreferences(file), args.triggerReason, args.kind);
}

private getEditsForRefactor(args: protocol.GetEditsForRefactorRequestArgs, simplifiedResult: boolean): RefactorEditInfo | protocol.RefactorEditInfo {
Expand Down
37 changes: 14 additions & 23 deletions src/services/codefixes/generateAccessors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ namespace ts.codefix {
type AcceptedNameType = Identifier | StringLiteral;
type ContainerDeclaration = ClassLikeDeclaration | ObjectLiteralExpression;

interface Info {
type Info = AccessorInfo | refactor.RefactorErrorInfo;
interface AccessorInfo {
readonly container: ContainerDeclaration;
readonly isStatic: boolean;
readonly isReadonly: boolean;
Expand All @@ -16,20 +17,12 @@ namespace ts.codefix {
readonly renameAccessor: boolean;
}

type InfoOrError = {
info: Info,
error?: never
} | {
info?: never,
error: string
};

export function generateAccessorFromProperty(file: SourceFile, program: Program, start: number, end: number, context: textChanges.TextChangesContext, _actionName: string): FileTextChanges[] | undefined {
const fieldInfo = getAccessorConvertiblePropertyAtPosition(file, program, start, end);
if (!fieldInfo || !fieldInfo.info) return undefined;
if (!fieldInfo || refactor.isRefactorErrorInfo(fieldInfo)) return undefined;

const changeTracker = textChanges.ChangeTracker.fromContext(context);
const { isStatic, isReadonly, fieldName, accessorName, originalName, type, container, declaration } = fieldInfo.info;
const { isStatic, isReadonly, fieldName, accessorName, originalName, type, container, declaration } = fieldInfo;

suppressLeadingAndTrailingTrivia(fieldName);
suppressLeadingAndTrailingTrivia(accessorName);
Expand Down Expand Up @@ -112,7 +105,7 @@ namespace ts.codefix {
return modifierFlags;
}

export function getAccessorConvertiblePropertyAtPosition(file: SourceFile, program: Program, start: number, end: number, considerEmptySpans = true): InfoOrError | undefined {
export function getAccessorConvertiblePropertyAtPosition(file: SourceFile, program: Program, start: number, end: number, considerEmptySpans = true): Info | undefined {
const node = getTokenAtPosition(file, start);
const cursorRequest = start === end && considerEmptySpans;
const declaration = findAncestor(node.parent, isAcceptedDeclaration);
Expand Down Expand Up @@ -142,17 +135,15 @@ namespace ts.codefix {
const fieldName = createPropertyName(startWithUnderscore ? name : getUniqueName(`_${name}`, file), declaration.name);
const accessorName = createPropertyName(startWithUnderscore ? getUniqueName(name.substring(1), file) : name, declaration.name);
return {
info: {
isStatic: hasStaticModifier(declaration),
isReadonly: hasEffectiveReadonlyModifier(declaration),
type: getDeclarationType(declaration, program),
container: declaration.kind === SyntaxKind.Parameter ? declaration.parent.parent : declaration.parent,
originalName: (<AcceptedNameType>declaration.name).text,
declaration,
fieldName,
accessorName,
renameAccessor: startWithUnderscore
}
isStatic: hasStaticModifier(declaration),
isReadonly: hasEffectiveReadonlyModifier(declaration),
type: getDeclarationType(declaration, program),
container: declaration.kind === SyntaxKind.Parameter ? declaration.parent.parent : declaration.parent,
originalName: (<AcceptedNameType>declaration.name).text,
declaration,
fieldName,
accessorName,
renameAccessor: startWithUnderscore
};
}

Expand Down
4 changes: 3 additions & 1 deletion src/services/refactorProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ namespace ts.refactor {

export function getApplicableRefactors(context: RefactorContext): ApplicableRefactorInfo[] {
return arrayFrom(flatMapIterator(refactors.values(), refactor =>
context.cancellationToken && context.cancellationToken.isCancellationRequested() ? undefined : refactor.getAvailableActions(context)));
context.cancellationToken && context.cancellationToken.isCancellationRequested() ||
!refactor.kinds?.some(kind => refactorKindBeginsWith(kind, context.kind)) ? undefined :
refactor.getAvailableActions(context)));
}

export function getEditsForRefactor(context: RefactorContext, refactorName: string, actionName: string): RefactorEditInfo | undefined {
Expand Down
85 changes: 31 additions & 54 deletions src/services/refactors/addOrRemoveBracesToArrowFunction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,45 +2,40 @@
namespace ts.refactor.addOrRemoveBracesToArrowFunction {
const refactorName = "Add or remove braces in an arrow function";
const refactorDescription = Diagnostics.Add_or_remove_braces_in_an_arrow_function.message;
const addBracesActionName = "Add braces to arrow function";
const removeBracesActionName = "Remove braces from arrow function";
const addBracesActionDescription = Diagnostics.Add_braces_to_arrow_function.message;
const removeBracesActionDescription = Diagnostics.Remove_braces_from_arrow_function.message;
registerRefactor(refactorName, { getEditsForAction, getAvailableActions });

interface Info {
const addBracesAction = {
name: "Add braces to arrow function",
description: Diagnostics.Add_braces_to_arrow_function.message,
kind: "refactor.rewrite.arrow.braces.add",
};
const removeBracesAction = {
name: "Remove braces from arrow function",
description: Diagnostics.Remove_braces_from_arrow_function.message,
kind: "refactor.rewrite.arrow.braces.remove"
};
registerRefactor(refactorName, {
kinds: [removeBracesAction.kind],
getEditsForAction,
getAvailableActions });

interface FunctionBracesInfo {
func: ArrowFunction;
expression: Expression | undefined;
returnStatement?: ReturnStatement;
addBraces: boolean;
}

type InfoOrError = {
info: Info,
error?: never
} | {
info?: never,
error: string
};

function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] {
const { file, startPosition, triggerReason } = context;
const info = getConvertibleArrowFunctionAtPosition(file, startPosition, triggerReason === "invoked");
if (!info) return emptyArray;

if (info.error === undefined) {
if (!isRefactorErrorInfo(info)) {
return [{
name: refactorName,
description: refactorDescription,
actions: [
info.info.addBraces ?
{
name: addBracesActionName,
description: addBracesActionDescription
} : {
name: removeBracesActionName,
description: removeBracesActionDescription
}
info.addBraces ? addBracesAction : removeBracesAction
]
}];
}
Expand All @@ -49,15 +44,10 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
return [{
name: refactorName,
description: refactorDescription,
actions: [{
name: addBracesActionName,
description: addBracesActionDescription,
notApplicableReason: info.error
}, {
name: removeBracesActionName,
description: removeBracesActionDescription,
notApplicableReason: info.error
}]
actions: [
{ ...addBracesAction, notApplicableReason: info.error },
{ ...removeBracesAction, notApplicableReason: info.error },
]
}];
}

Expand All @@ -67,19 +57,19 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
function getEditsForAction(context: RefactorContext, actionName: string): RefactorEditInfo | undefined {
const { file, startPosition } = context;
const info = getConvertibleArrowFunctionAtPosition(file, startPosition);
if (!info || !info.info) return undefined;
Debug.assert(info && !isRefactorErrorInfo(info), "Expected applicable refactor info");

const { expression, returnStatement, func } = info.info;
const { expression, returnStatement, func } = info;

let body: ConciseBody;

if (actionName === addBracesActionName) {
if (actionName === addBracesAction.name) {
const returnStatement = factory.createReturnStatement(expression);
body = factory.createBlock([returnStatement], /* multiLine */ true);
suppressLeadingAndTrailingTrivia(body);
copyLeadingComments(expression!, returnStatement, file, SyntaxKind.MultiLineCommentTrivia, /* hasTrailingNewLine */ true);
}
else if (actionName === removeBracesActionName && returnStatement) {
else if (actionName === removeBracesAction.name && returnStatement) {
const actualExpression = expression || factory.createVoidZero();
body = needsParentheses(actualExpression) ? factory.createParenthesizedExpression(actualExpression) : actualExpression;
suppressLeadingAndTrailingTrivia(body);
Expand All @@ -98,7 +88,7 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
return { renameFilename: undefined, renameLocation: undefined, edits };
}

function getConvertibleArrowFunctionAtPosition(file: SourceFile, startPosition: number, considerFunctionBodies = true): InfoOrError | undefined {
function getConvertibleArrowFunctionAtPosition(file: SourceFile, startPosition: number, considerFunctionBodies = true, kind?: string): FunctionBracesInfo | RefactorErrorInfo | undefined {
const node = getTokenAtPosition(file, startPosition);
const func = getContainingFunction(node);

Expand All @@ -118,26 +108,13 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
return undefined;
}

if (isExpression(func.body)) {
return {
info: {
func,
addBraces: true,
expression: func.body
}
};
if (refactorKindBeginsWith(addBracesAction.kind, kind) && isExpression(func.body)) {
return { func, addBraces: true, expression: func.body };
}
else if (func.body.statements.length === 1) {
else if (refactorKindBeginsWith(removeBracesAction.kind, kind) && isBlock(func.body) && func.body.statements.length === 1) {
const firstStatement = first(func.body.statements);
if (isReturnStatement(firstStatement)) {
return {
info: {
func,
addBraces: false,
expression: firstStatement.expression,
returnStatement: firstStatement
}
};
return { func, addBraces: false, expression: firstStatement.expression, returnStatement: firstStatement };
}
}
return undefined;
Expand Down
Loading

0 comments on commit 8bbef81

Please sign in to comment.