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 RefactorTriggerReason #38378

Merged
merged 36 commits into from
Jun 3, 2020
Merged
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
bac0465
add trigger reason to protocol
Apr 14, 2020
02fa39a
enable explicit requests for addOrRemoveBracesToArrowFunction
Apr 15, 2020
c3828ed
loosen convertExport conditions
Apr 22, 2020
1efcffc
convertImport
Apr 28, 2020
0296254
extractType
Apr 28, 2020
55880ee
extractType explicitCursorRequest
Apr 29, 2020
602ee99
fix lint errors
May 5, 2020
f1127e6
extract symbol
May 6, 2020
c5343f1
update refactorConvertImport_partialSelection
May 6, 2020
53e8ed0
accept new baseline
May 6, 2020
23f4dc9
use enum for RefactorTriggerReason
May 13, 2020
57e856b
convert export tests
May 14, 2020
665e434
convert import tests
May 14, 2020
23e0064
accept new baseline
May 14, 2020
f3751fb
change type of RefactorTriggerReason
May 21, 2020
96f210c
arrow function refactor test
May 26, 2020
fbf6737
use verify trigger reason for import export
May 26, 2020
dc363f1
fix some indices
May 26, 2020
85e0d8b
add refactorNotAvailableForTriggerReason
May 26, 2020
df8ff65
extract type test
May 26, 2020
3825d19
extract symbol test
May 27, 2020
673a868
update test names
May 27, 2020
86122c4
convert get set test
May 27, 2020
d37f4c3
accept new baseline
May 27, 2020
e565931
fix up some bools
May 27, 2020
a07a79b
remove unused test method
May 29, 2020
3e1e614
Revert "update refactorConvertImport_partialSelection"
May 29, 2020
4971c7d
remove declaration
May 29, 2020
a7c07d6
convert export cursor only changes
May 29, 2020
c20908a
convert export trigger reason test
May 29, 2020
06d2461
convert import trigger reason only
May 29, 2020
f479279
convert import trigger reason test
May 29, 2020
2db0054
remove outdated tests
May 29, 2020
a86a2fa
polish tests
May 29, 2020
ca58c0e
fix merge conflicts
May 29, 2020
d88ea4e
address PR comments
Jun 3, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/harness/fourslashImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3214,8 +3214,8 @@ namespace FourSlash {
};
}

public verifyRefactorAvailable(negative: boolean, name: string, actionName?: string) {
let refactors = this.getApplicableRefactorsAtSelection();
public verifyRefactorAvailable(negative: boolean, triggerReason: ts.RefactorTriggerReason, name: string, actionName?: string) {
let refactors = this.getApplicableRefactorsAtSelection(triggerReason);
refactors = refactors.filter(r => r.name === name && (actionName === undefined || r.actions.some(a => a.name === actionName)));
const isAvailable = refactors.length > 0;

Expand Down Expand Up @@ -3644,14 +3644,14 @@ namespace FourSlash {
test(renameKeys(newFileContents, key => pathUpdater(key) || key), "with file moved");
}

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

public configurePlugin(pluginName: string, configuration: any): void {
Expand Down
6 changes: 5 additions & 1 deletion src/harness/fourslashInterfaceImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,11 @@ namespace FourSlashInterface {
}

public refactorAvailable(name: string, actionName?: string) {
this.state.verifyRefactorAvailable(this.negative, name, actionName);
this.state.verifyRefactorAvailable(this.negative, "implicit", name, actionName);
}

public refactorAvailableForTriggerReason(triggerReason: ts.RefactorTriggerReason, name: string, actionName?: string) {
this.state.verifyRefactorAvailable(this.negative, triggerReason, name, actionName);
}
}

Expand Down
6 changes: 5 additions & 1 deletion src/server/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,11 @@ namespace ts.server.protocol {
command: CommandTypes.GetApplicableRefactors;
arguments: GetApplicableRefactorsRequestArgs;
}
export type GetApplicableRefactorsRequestArgs = FileLocationOrRangeRequestArgs;
export type GetApplicableRefactorsRequestArgs = FileLocationOrRangeRequestArgs & {
triggerReason?: RefactorTriggerReason
};

export type RefactorTriggerReason = "implicit" | "invoked";

/**
* Response is a list of available refactorings.
Expand Down
2 changes: 1 addition & 1 deletion src/server/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1970,7 +1970,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));
return project.getLanguageService().getApplicableRefactors(file, this.extractPositionOrRange(args, scriptInfo), this.getPreferences(file), args.triggerReason);
}

private getEditsForRefactor(args: protocol.GetEditsForRefactorRequestArgs, simplifiedResult: boolean): RefactorEditInfo | protocol.RefactorEditInfo {
Expand Down
5 changes: 3 additions & 2 deletions src/services/codefixes/generateAccessors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,13 @@ namespace ts.codefix {
return modifierFlags;
}

export function getAccessorConvertiblePropertyAtPosition(file: SourceFile, start: number, end: number): Info | undefined {
export function getAccessorConvertiblePropertyAtPosition(file: SourceFile, start: number, end: number, userRequested = true): Info | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

I find this a little confusing since the default elsewhere is implicit.

const node = getTokenAtPosition(file, start);
const cursorRequest = start === end && userRequested;
const declaration = findAncestor(node.parent, isAcceptedDeclaration);
// make sure declaration have AccessibilityModifier or Static Modifier or Readonly Modifier
const meaning = ModifierFlags.AccessibilityModifier | ModifierFlags.Static | ModifierFlags.Readonly;
if (!declaration || !nodeOverlapsWithStartEnd(declaration.name, file, start, end)
if (!declaration || !(nodeOverlapsWithStartEnd(declaration.name, file, start, end) || cursorRequest)
|| !isConvertibleName(declaration.name) || (getEffectiveModifierFlags(declaration) | meaning) !== meaning) return undefined;

const name = declaration.name.text;
Expand Down
10 changes: 6 additions & 4 deletions src/services/refactors/addOrRemoveBracesToArrowFunction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
}

function getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] {
const { file, startPosition } = context;
const info = getConvertibleArrowFunctionAtPosition(file, startPosition);
const { file, startPosition, triggerReason } = context;
const info = getConvertibleArrowFunctionAtPosition(file, startPosition, triggerReason === "invoked");
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, JS doesn't have named arguments. Instead, when we're passing a boolean, we tend to give the parameter name as a /*comment*/ before the argument (unless the boolean already has a descriptive name, of course).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't my intention to pass a named argument, but to pass whatever triggerReason === "invoked" evaluates to (false if triggerReason is "implicit" or undefined. Did I do that incorrectly?

if (!info) return emptyArray;

return [{
Expand Down Expand Up @@ -70,10 +70,12 @@ namespace ts.refactor.addOrRemoveBracesToArrowFunction {
return { renameFilename: undefined, renameLocation: undefined, edits };
}

function getConvertibleArrowFunctionAtPosition(file: SourceFile, startPosition: number): Info | undefined {
function getConvertibleArrowFunctionAtPosition(file: SourceFile, startPosition: number, userRequested = true): Info | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

I can see the convenience of having this be called userRequested in all the different refactorings, but, personally, I think it would be clearer to give it a name that's specific to each function you've added it to. For example, in this case, the parameter could be called something like considerFunctionBodies - that would be more meaningful both within the function and at the call-sites that aren't part of a flow that can be either implicit or user-requested. As a bonus, it would also address my comment above, that this parameter defaults to explicit, whereas the ones in the protocol default to implicit.

const node = getTokenAtPosition(file, startPosition);
const func = getContainingFunction(node);
if (!func || !isArrowFunction(func) || (!rangeContainsRange(func, node) || rangeContainsRange(func.body, node))) return undefined;
// Only offer a refactor in the function body on explicit refactor requests.
if (!func || !isArrowFunction(func) || (!rangeContainsRange(func, node)
|| (rangeContainsRange(func.body, node) && !userRequested))) return undefined;

if (isExpression(func.body)) {
return {
Expand Down
7 changes: 4 additions & 3 deletions src/services/refactors/convertExport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ namespace ts.refactor {
const actionNameNamedToDefault = "Convert named export to default export";
registerRefactor(refactorName, {
getAvailableActions(context): readonly ApplicableRefactorInfo[] {
const info = getInfo(context);
const info = getInfo(context, context.triggerReason === "invoked");
if (!info) return emptyArray;
const description = info.wasDefault ? Diagnostics.Convert_default_export_to_named_export.message : Diagnostics.Convert_named_export_to_default_export.message;
const actionName = info.wasDefault ? actionNameDefaultToNamed : actionNameNamedToDefault;
Expand All @@ -27,11 +27,12 @@ namespace ts.refactor {
readonly exportingModuleSymbol: Symbol;
}

function getInfo(context: RefactorContext): Info | undefined {
function getInfo(context: RefactorContext, userRequested = true): Info | undefined {
const { file } = context;
const span = getRefactorContextSpan(context);
const token = getTokenAtPosition(file, span.start);
const exportNode = getParentNodeInSpan(token, file, span);
const cursorRequest = userRequested && span;
const exportNode = !!(getSyntacticModifierFlags(token.parent) & ModifierFlags.Export) && cursorRequest ? token.parent : getParentNodeInSpan(token, file, span);
if (!exportNode || (!isSourceFile(exportNode.parent) && !(isModuleBlock(exportNode.parent) && isAmbientModule(exportNode.parent.parent)))) {
return undefined;
}
Expand Down
9 changes: 5 additions & 4 deletions src/services/refactors/convertImport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ namespace ts.refactor {
const actionNameNamedToNamespace = "Convert named imports to namespace import";
registerRefactor(refactorName, {
getAvailableActions(context): readonly ApplicableRefactorInfo[] {
const i = getImportToConvert(context);
const i = getImportToConvert(context, context.triggerReason === "invoked");
if (!i) return emptyArray;
const description = i.kind === SyntaxKind.NamespaceImport ? Diagnostics.Convert_namespace_import_to_named_imports.message : Diagnostics.Convert_named_imports_to_namespace_import.message;
const actionName = i.kind === SyntaxKind.NamespaceImport ? actionNameNamespaceToNamed : actionNameNamedToNamespace;
Expand All @@ -19,12 +19,13 @@ namespace ts.refactor {
});

// Can convert imports of the form `import * as m from "m";` or `import d, { x, y } from "m";`.
function getImportToConvert(context: RefactorContext): NamedImportBindings | undefined {
function getImportToConvert(context: RefactorContext, userRequested = true): NamedImportBindings | undefined {
const { file } = context;
const span = getRefactorContextSpan(context);
const token = getTokenAtPosition(file, span.start);
const importDecl = getParentNodeInSpan(token, file, span);
if (!importDecl || !isImportDeclaration(importDecl)) return undefined;
const cursorRequest = userRequested && span.length === 0;
const importDecl = cursorRequest ? findAncestor(token, isImportDeclaration) : getParentNodeInSpan(token, file, span);
if (!importDecl || !isImportDeclaration(importDecl) || (importDecl.getEnd() < span.start + span.length)) return undefined;
const { importClause } = importDecl;
return importClause && importClause.namedBindings;
}
Expand Down
18 changes: 12 additions & 6 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): readonly ApplicableRefactorInfo[] {
const rangeToExtract = getRangeToExtract(context.file, getRefactorContextSpan(context));
const rangeToExtract = getRangeToExtract(context.file, getRefactorContextSpan(context), context.triggerReason === "invoked");

const targetRange = rangeToExtract.targetRange;
if (targetRange === undefined) {
Expand Down Expand Up @@ -186,18 +186,20 @@ namespace ts.refactor.extractSymbol {
* not shown to the user, but can be used by us diagnostically)
*/
// exported only for tests
export function getRangeToExtract(sourceFile: SourceFile, span: TextSpan): RangeToExtract {
export function getRangeToExtract(sourceFile: SourceFile, span: TextSpan, userRequested = true): RangeToExtract {
const { length } = span;

if (length === 0) {
if (length === 0 && !userRequested) {
return { errors: [createFileDiagnostic(sourceFile, span.start, length, Messages.cannotExtractEmpty)] };
}
const cursorRequest = length === 0 && userRequested;

// Walk up starting from the the start position until we find a non-SourceFile node that subsumes the selected span.
// This may fail (e.g. you select two statements in the root of a source file)
const start = getParentNodeInSpan(getTokenAtPosition(sourceFile, span.start), sourceFile, span);
const startToken = getTokenAtPosition(sourceFile, span.start);
const start = cursorRequest ? getExtractableParent(startToken): getParentNodeInSpan(startToken, sourceFile, span);
// Do the same for the ending position
const end = getParentNodeInSpan(findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span)), sourceFile, span);
const endToken = findTokenOnLeftOfPosition(sourceFile, textSpanEnd(span));
const end = cursorRequest ? start : getParentNodeInSpan(endToken, sourceFile, span);

const declarations: Symbol[] = [];

Expand Down Expand Up @@ -1832,6 +1834,10 @@ namespace ts.refactor.extractSymbol {
}
}

function getExtractableParent(node: Node | undefined): Node | undefined {
return findAncestor(node, node => node.parent && isExtractableExpression(node) && !isBinaryExpression(node.parent));
}

/**
* Computes whether or not a node represents an expression in a position where it could
* be extracted.
Expand Down
10 changes: 6 additions & 4 deletions src/services/refactors/extractType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ namespace ts.refactor {
const extractToTypeDef = "Extract to typedef";
registerRefactor(refactorName, {
getAvailableActions(context): readonly ApplicableRefactorInfo[] {
const info = getRangeToExtract(context);
const info = getRangeToExtract(context, context.triggerReason === "invoked");
if (!info) return emptyArray;

return [{
Expand All @@ -22,7 +22,7 @@ namespace ts.refactor {
}];
},
getEditsForAction(context, actionName): RefactorEditInfo {
const { file } = context;
const { file, } = context;
const info = Debug.checkDefined(getRangeToExtract(context), "Expected to find a range to extract");

const name = getUniqueName("NewType", file);
Expand Down Expand Up @@ -58,13 +58,15 @@ namespace ts.refactor {

type Info = TypeAliasInfo | InterfaceInfo;

function getRangeToExtract(context: RefactorContext): Info | undefined {
function getRangeToExtract(context: RefactorContext, userRequested = true): Info | undefined {
const { file, startPosition } = context;
const isJS = isSourceFileJS(file);
const current = getTokenAtPosition(file, startPosition);
const range = createTextRangeFromSpan(getRefactorContextSpan(context));
const cursorRequest = range.pos === range.end && userRequested;

const selection = findAncestor(current, (node => node.parent && rangeContainsSkipTrivia(range, node, file) && !rangeContainsSkipTrivia(range, node.parent, file)));
const selection = findAncestor(current, (node => node.parent && isTypeNode(node) && !rangeContainsSkipTrivia(range, node.parent, file) &&
(cursorRequest || nodeOverlapsWithStartEnd(current, file, range.pos, range.end))));
if (!selection || !isTypeNode(selection)) return undefined;

const checker = context.program.getTypeChecker();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
},
getAvailableActions(context: RefactorContext): readonly ApplicableRefactorInfo[] {
if (!context.endPosition) return emptyArray;
if (!codefix.getAccessorConvertiblePropertyAtPosition(context.file, context.startPosition, context.endPosition)) return emptyArray;
if (!codefix.getAccessorConvertiblePropertyAtPosition(context.file, context.startPosition, context.endPosition, context.triggerReason === "invoked")) return emptyArray;

return [{
name: actionName,
Expand Down
7 changes: 4 additions & 3 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2140,7 +2140,7 @@ namespace ts {
return Rename.getRenameInfo(program, getValidSourceFile(fileName), position, options);
}

function getRefactorContext(file: SourceFile, positionOrRange: number | TextRange, preferences: UserPreferences, formatOptions?: FormatCodeSettings): RefactorContext {
function getRefactorContext(file: SourceFile, positionOrRange: number | TextRange, preferences: UserPreferences, formatOptions?: FormatCodeSettings, triggerReason?: RefactorTriggerReason): RefactorContext {
const [startPosition, endPosition] = typeof positionOrRange === "number" ? [positionOrRange, undefined] : [positionOrRange.pos, positionOrRange.end];
return {
file,
Expand All @@ -2151,17 +2151,18 @@ namespace ts {
formatContext: formatting.getFormatContext(formatOptions!, host), // TODO: GH#18217
cancellationToken,
preferences,
triggerReason,
};
}

function getSmartSelectionRange(fileName: string, position: number): SelectionRange {
return SmartSelectionRange.getSmartSelectionRange(position, syntaxTreeCache.getCurrentSourceFile(fileName));
}

function getApplicableRefactors(fileName: string, positionOrRange: number | TextRange, preferences: UserPreferences = emptyOptions): ApplicableRefactorInfo[] {
function getApplicableRefactors(fileName: string, positionOrRange: number | TextRange, preferences: UserPreferences = emptyOptions, triggerReason: RefactorTriggerReason): ApplicableRefactorInfo[] {
synchronizeHostData();
const file = getValidSourceFile(fileName);
return refactor.getApplicableRefactors(getRefactorContext(file, positionOrRange, preferences));
return refactor.getApplicableRefactors(getRefactorContext(file, positionOrRange, preferences, emptyOptions, triggerReason));
}

function getEditsForRefactor(
Expand Down
5 changes: 4 additions & 1 deletion src/services/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ namespace ts {
/** @deprecated `fileName` will be ignored */
applyCodeActionCommand(fileName: string, action: CodeActionCommand | CodeActionCommand[]): Promise<ApplyCodeActionCommandResult | ApplyCodeActionCommandResult[]>;

getApplicableRefactors(fileName: string, positionOrRange: number | TextRange, preferences: UserPreferences | undefined): ApplicableRefactorInfo[];
getApplicableRefactors(fileName: string, positionOrRange: number | TextRange, preferences: UserPreferences | undefined, triggerReason?: RefactorTriggerReason): ApplicableRefactorInfo[];
getEditsForRefactor(fileName: string, formatOptions: FormatCodeSettings, positionOrRange: number | TextRange, refactorName: string, actionName: string, preferences: UserPreferences | undefined): RefactorEditInfo | undefined;
organizeImports(scope: OrganizeImportsScope, formatOptions: FormatCodeSettings, preferences: UserPreferences | undefined): readonly FileTextChanges[];
getEditsForFileRename(oldFilePath: string, newFilePath: string, formatOptions: FormatCodeSettings, preferences: UserPreferences | undefined): readonly FileTextChanges[];
Expand Down Expand Up @@ -741,6 +741,8 @@ namespace ts {
commands?: CodeActionCommand[];
}

export type RefactorTriggerReason = "implicit" | "invoked";

export interface TextInsertion {
newText: string;
/** The position in newText the caret should point to after the insertion. */
Expand Down Expand Up @@ -1404,5 +1406,6 @@ namespace ts {
program: Program;
cancellationToken?: CancellationToken;
preferences: UserPreferences;
triggerReason?: RefactorTriggerReason;
}
}
Loading