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 opt-in user preference for prefix and suffix text on renames #29314

Merged
merged 21 commits into from
Jan 16, 2019
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
1 change: 1 addition & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5860,6 +5860,7 @@ namespace ts {
/** Determines whether we import `foo/index.ts` as "foo", "foo/index", or "foo/index.js" */
readonly importModuleSpecifierEnding?: "minimal" | "index" | "js";
readonly allowTextChangesInNewFiles?: boolean;
readonly usePrefixAndSuffixTextForRename?: boolean;
uniqueiniquity marked this conversation as resolved.
Show resolved Hide resolved
}

/** Represents a bigint literal value without requiring bigint support */
Expand Down
5 changes: 3 additions & 2 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1170,7 +1170,7 @@ Actual: ${stringify(fullActual)}`);
}

public verifyRenameLocations(startRanges: ArrayOrSingle<Range>, options: FourSlashInterface.RenameLocationsOptions) {
const { findInStrings = false, findInComments = false, ranges = this.getRanges() } = ts.isArray(options) ? { findInStrings: false, findInComments: false, ranges: options } : options;
const { findInStrings = false, findInComments = false, ranges = this.getRanges(), usePrefixAndSuffixTextForRename = true } = ts.isArray(options) ? { findInStrings: false, findInComments: false, ranges: options, usePrefixAndSuffixTextForRename: true } : options;

for (const startRange of toArray(startRanges)) {
this.goToRangeStart(startRange);
Expand All @@ -1182,7 +1182,7 @@ Actual: ${stringify(fullActual)}`);
}

const references = this.languageService.findRenameLocations(
this.activeFile.fileName, this.currentCaretPosition, findInStrings, findInComments);
this.activeFile.fileName, this.currentCaretPosition, findInStrings, findInComments, usePrefixAndSuffixTextForRename);

const sort = (locations: ReadonlyArray<ts.RenameLocation> | undefined) =>
locations && ts.sort(locations, (r1, r2) => ts.compareStringsCaseSensitive(r1.fileName, r2.fileName) || r1.textSpan.start - r2.textSpan.start);
Expand Down Expand Up @@ -5086,6 +5086,7 @@ namespace FourSlashInterface {
readonly findInStrings?: boolean;
readonly findInComments?: boolean;
readonly ranges: ReadonlyArray<RenameLocationOptions>;
readonly usePrefixAndSuffixTextForRename?: boolean;
};
export type RenameLocationOptions = FourSlash.Range | { readonly range: FourSlash.Range, readonly prefixText?: string, readonly suffixText?: string };
}
1 change: 1 addition & 0 deletions src/server/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2905,6 +2905,7 @@ namespace ts.server.protocol {
readonly importModuleSpecifierPreference?: "relative" | "non-relative";
readonly allowTextChangesInNewFiles?: boolean;
readonly lazyConfiguredProjectsFromExternalProject?: boolean;
readonly usePrefixAndSuffixTextForRename?: boolean;
}

export interface CompilerOptions {
Expand Down
8 changes: 5 additions & 3 deletions src/server/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,8 @@ namespace ts.server {
defaultProject: Project,
initialLocation: DocumentPosition,
findInStrings: boolean,
findInComments: boolean
findInComments: boolean,
usePrefixAndSuffixTextForRename: boolean
): ReadonlyArray<RenameLocation> {
const outputs: RenameLocation[] = [];

Expand All @@ -323,7 +324,7 @@ namespace ts.server {
defaultProject,
initialLocation,
({ project, location }, tryAddToTodo) => {
for (const output of project.getLanguageService().findRenameLocations(location.fileName, location.pos, findInStrings, findInComments) || emptyArray) {
for (const output of project.getLanguageService().findRenameLocations(location.fileName, location.pos, findInStrings, findInComments, usePrefixAndSuffixTextForRename) || emptyArray) {
if (!contains(outputs, output, documentSpansEqual) && !tryAddToTodo(project, documentSpanLocation(output))) {
outputs.push(output);
}
Expand Down Expand Up @@ -1231,7 +1232,8 @@ namespace ts.server {
this.getDefaultProject(args),
{ fileName: args.file, pos: position },
!!args.findInStrings,
!!args.findInComments
!!args.findInComments,
this.getHostPreferences().usePrefixAndSuffixTextForRename || false
Copy link
Member

Choose a reason for hiding this comment

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

this.getHostPreferences().usePrefixAndSuffixTextForRename || false [](start = 16, length = 66)

Since there may be multiple preferences affecting rename behavior (e.g. #28677), would it make more sense to pass in the entire preferences object (e.g. as in getEditsForFileRename)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched to passing entire preferences object.
However I only passed it as deep as this call; any deeper would require an API change, so I didn't want to do that unless absolutely necessary.

);
if (!simplifiedResult) return locations;

Expand Down
31 changes: 22 additions & 9 deletions src/services/findAllReferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ namespace ts.FindAllReferences {
readonly isForRename?: boolean;
/** True if we are searching for implementations. We will have a different method of adding references if so. */
readonly implementations?: boolean;
/** User setting to allow backwards compatibility for renaming shorthand properties and import/export specifiers */
uniqueiniquity marked this conversation as resolved.
Show resolved Hide resolved
readonly usePrefixAndSuffixTextForRename?: boolean;
}

export function findReferencedSymbols(program: Program, cancellationToken: CancellationToken, sourceFiles: ReadonlyArray<SourceFile>, sourceFile: SourceFile, position: number): ReferencedSymbol[] | undefined {
Expand Down Expand Up @@ -178,7 +180,7 @@ namespace ts.FindAllReferences {
};
}

function entryToDocumentSpan(entry: Entry): DocumentSpan {
export function entryToDocumentSpan(entry: Entry): DocumentSpan {
if (entry.kind === EntryKind.Span) {
return { textSpan: entry.textSpan, fileName: entry.fileName };
}
Expand Down Expand Up @@ -484,7 +486,7 @@ namespace ts.FindAllReferences.Core {

/** Core find-all-references algorithm for a normal symbol. */
function getReferencedSymbolsForSymbol(originalSymbol: Symbol, node: Node | undefined, sourceFiles: ReadonlyArray<SourceFile>, sourceFilesSet: ReadonlyMap<true>, checker: TypeChecker, cancellationToken: CancellationToken, options: Options): SymbolAndEntries[] {
const symbol = node && skipPastExportOrImportSpecifierOrUnion(originalSymbol, node, checker, !!options.isForRename) || originalSymbol;
const symbol = node && skipPastExportOrImportSpecifierOrUnion(originalSymbol, node, checker, !options.usePrefixAndSuffixTextForRename || !options.isForRename) || originalSymbol;

// Compute the meaning from the location and the symbol it references
const searchMeaning = node ? getIntersectingMeaningFromDeclarations(node, symbol) : SemanticMeaning.All;
Expand All @@ -493,7 +495,7 @@ namespace ts.FindAllReferences.Core {
const state = new State(sourceFiles, sourceFilesSet, node ? getSpecialSearchKind(node) : SpecialSearchKind.None, checker, cancellationToken, searchMeaning, options, result);

const exportSpecifier = !options.isForRename ? undefined : find(symbol.declarations, isExportSpecifier);
if (exportSpecifier) {
if (exportSpecifier && (exportSpecifier.propertyName || options.usePrefixAndSuffixTextForRename)) {
// When renaming at an export specifier, rename the export and not the thing being exported.
getReferencesAtExportSpecifier(exportSpecifier.name, symbol, exportSpecifier, state.createSearch(node, originalSymbol, /*comingFrom*/ undefined), state, /*addReferencesHere*/ true, /*alwaysGetReferences*/ true);
}
Expand All @@ -502,7 +504,7 @@ namespace ts.FindAllReferences.Core {
searchForImportsOfExport(node, symbol, { exportingModuleSymbol: Debug.assertDefined(symbol.parent, "Expected export symbol to have a parent"), exportKind: ExportKind.Default }, state);
}
else {
const search = state.createSearch(node, symbol, /*comingFrom*/ undefined, { allSearchSymbols: node ? populateSearchSymbolSet(symbol, node, checker, !!options.isForRename, !!options.implementations) : [symbol] });
const search = state.createSearch(node, symbol, /*comingFrom*/ undefined, { allSearchSymbols: node ? populateSearchSymbolSet(symbol, node, checker, !!options.isForRename, !!options.implementations, !!options.usePrefixAndSuffixTextForRename) : [symbol] });

// Try to get the smallest valid scope that we can limit our search to;
// otherwise we'll need to search globally (i.e. include each file).
Expand Down Expand Up @@ -538,9 +540,9 @@ namespace ts.FindAllReferences.Core {
}

/** Handle a few special cases relating to export/import specifiers. */
function skipPastExportOrImportSpecifierOrUnion(symbol: Symbol, node: Node, checker: TypeChecker, isForRename: boolean): Symbol | undefined {
function skipPastExportOrImportSpecifierOrUnion(symbol: Symbol, node: Node, checker: TypeChecker, handleExportSpecifier: boolean): Symbol | undefined {
const { parent } = node;
if (isExportSpecifier(parent) && !isForRename) {
if (isExportSpecifier(parent) && handleExportSpecifier) {
return getLocalSymbolForExportSpecifier(node as Identifier, symbol, parent, checker);
}
// If the symbol is declared as part of a declaration like `{ type: "a" } | { type: "b" }`, use the property on the union type to get more references.
Expand Down Expand Up @@ -1102,7 +1104,7 @@ namespace ts.FindAllReferences.Core {
}

// For `export { foo as bar }`, rename `foo`, but not `bar`.
if (!state.options.isForRename || alwaysGetReferences) {
if ((!state.options.usePrefixAndSuffixTextForRename && referenceLocation !== propertyName) || !state.options.isForRename || alwaysGetReferences) {
const exportKind = referenceLocation.originalKeywordKind === SyntaxKind.DefaultKeyword ? ExportKind.Default : ExportKind.Named;
const exportSymbol = Debug.assertDefined(exportSpecifier.symbol);
const exportInfo = Debug.assertDefined(getExportInfo(exportSymbol, exportKind, state.checker));
Expand Down Expand Up @@ -1145,7 +1147,7 @@ namespace ts.FindAllReferences.Core {
const { symbol } = importOrExport;

if (importOrExport.kind === ImportExport.Import) {
if (!state.options.isForRename) {
if (!state.options.usePrefixAndSuffixTextForRename || !state.options.isForRename) {
searchForImportedSymbol(symbol, state);
}
}
Expand Down Expand Up @@ -1514,16 +1516,18 @@ namespace ts.FindAllReferences.Core {

// For certain symbol kinds, we need to include other symbols in the search set.
// This is not needed when searching for re-exports.
function populateSearchSymbolSet(symbol: Symbol, location: Node, checker: TypeChecker, isForRename: boolean, implementations: boolean): Symbol[] {
function populateSearchSymbolSet(symbol: Symbol, location: Node, checker: TypeChecker, isForRename: boolean, implementations: boolean, usePrefixAndSuffixTextForRename: boolean): Symbol[] {
const result: Symbol[] = [];
forEachRelatedSymbol<void>(symbol, location, checker, isForRename,
!isForRename || usePrefixAndSuffixTextForRename,
(sym, root, base) => { result.push(base || root || sym); },
/*allowBaseTypes*/ () => !implementations);
return result;
}

function forEachRelatedSymbol<T>(
symbol: Symbol, location: Node, checker: TypeChecker, isForRenamePopulateSearchSymbolSet: boolean,
excludeReferenceToShorthandBinding: boolean,
cbSymbol: (symbol: Symbol, rootSymbol?: Symbol, baseSymbol?: Symbol, kind?: NodeEntryKind) => T | undefined,
allowBaseTypes: (rootSymbol: Symbol) => boolean,
): T | undefined {
Expand Down Expand Up @@ -1576,6 +1580,14 @@ namespace ts.FindAllReferences.Core {
return fromRoot(symbol.flags & SymbolFlags.FunctionScopedVariable ? paramProps[1] : paramProps[0]);
}

if (!excludeReferenceToShorthandBinding) {
const bindingElement = getObjectBindingElementWithoutPropertyNameOrUndefined(symbol);
const bindingElementPropertySymbol = bindingElement && getPropertySymbolFromBindingElement(checker, bindingElement);
if (bindingElementPropertySymbol) {
return fromRoot(bindingElementPropertySymbol);
}
}

// symbolAtLocation for a binding element is the local symbol. See if the search symbol is the property.
// Don't do this when populating search set for a rename -- just rename the local.
if (!isForRenamePopulateSearchSymbolSet) {
Expand Down Expand Up @@ -1606,6 +1618,7 @@ namespace ts.FindAllReferences.Core {
function getRelatedSymbol(search: Search, referenceSymbol: Symbol, referenceLocation: Node, state: State): RelatedSymbol | undefined {
const { checker } = state;
return forEachRelatedSymbol(referenceSymbol, referenceLocation, checker, /*isForRenamePopulateSearchSymbolSet*/ false,
!state.options.isForRename || !!state.options.usePrefixAndSuffixTextForRename,
(sym, rootSymbol, baseSymbol, kind): RelatedSymbol | undefined => search.includes(baseSymbol || rootSymbol || sym)
// For a base type, use the symbol for the derived type. For a synthetic (e.g. union) property, use the union symbol.
? { symbol: rootSymbol && !(getCheckFlags(sym) & CheckFlags.Synthetic) ? rootSymbol : sym, kind }
Expand Down
5 changes: 3 additions & 2 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1549,7 +1549,7 @@ namespace ts {
return DocumentHighlights.getDocumentHighlights(program, cancellationToken, sourceFile, position, sourceFilesToSearch);
}

function findRenameLocations(fileName: string, position: number, findInStrings: boolean, findInComments: boolean): RenameLocation[] | undefined {
function findRenameLocations(fileName: string, position: number, findInStrings: boolean, findInComments: boolean, usePrefixAndSuffixTextForRename: boolean): RenameLocation[] | undefined {
synchronizeHostData();
const sourceFile = getValidSourceFile(fileName);
const node = getTouchingPropertyName(sourceFile, position);
Expand All @@ -1559,7 +1559,8 @@ namespace ts {
({ fileName: sourceFile.fileName, textSpan: createTextSpanFromNode(node.tagName, sourceFile) }));
}
else {
return getReferencesWorker(node, position, { findInStrings, findInComments, isForRename: true }, FindAllReferences.toRenameLocation);
return getReferencesWorker(node, position, { findInStrings, findInComments, usePrefixAndSuffixTextForRename, isForRename: true },
usePrefixAndSuffixTextForRename ? FindAllReferences.toRenameLocation : FindAllReferences.entryToDocumentSpan);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/services/shims.ts
Original file line number Diff line number Diff line change
Expand Up @@ -841,7 +841,7 @@ namespace ts {
public findRenameLocations(fileName: string, position: number, findInStrings: boolean, findInComments: boolean): string {
return this.forwardJSONCall(
`findRenameLocations('${fileName}', ${position}, ${findInStrings}, ${findInComments})`,
() => this.languageService.findRenameLocations(fileName, position, findInStrings, findInComments)
() => this.languageService.findRenameLocations(fileName, position, findInStrings, findInComments, /*usePrefixAndSuffixTextForRename*/ true)
);
}

Expand Down
2 changes: 1 addition & 1 deletion src/services/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ namespace ts {
getSignatureHelpItems(fileName: string, position: number, options: SignatureHelpItemsOptions | undefined): SignatureHelpItems | undefined;

getRenameInfo(fileName: string, position: number): RenameInfo;
findRenameLocations(fileName: string, position: number, findInStrings: boolean, findInComments: boolean): ReadonlyArray<RenameLocation> | undefined;
findRenameLocations(fileName: string, position: number, findInStrings: boolean, findInComments: boolean, usePrefixAndSuffixTextForRename: boolean): ReadonlyArray<RenameLocation> | undefined;

getDefinitionAtPosition(fileName: string, position: number): ReadonlyArray<DefinitionInfo> | undefined;
getDefinitionAndBoundSpan(fileName: string, position: number): DefinitionInfoAndBoundSpan | undefined;
Expand Down
7 changes: 7 additions & 0 deletions src/services/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1357,6 +1357,13 @@ namespace ts {
!bindingElement.propertyName;
}

export function getObjectBindingElementWithoutPropertyNameOrUndefined(symbol: Symbol): BindingElement & { name: Identifier } | undefined {
uniqueiniquity marked this conversation as resolved.
Show resolved Hide resolved
const bindingElement = getDeclarationOfKind<BindingElement>(symbol, SyntaxKind.BindingElement);
if (bindingElement && isObjectBindingElementWithoutPropertyName(bindingElement)) {
return bindingElement as BindingElement & { name: Identifier };
uniqueiniquity marked this conversation as resolved.
Show resolved Hide resolved
}
}

export function getPropertySymbolFromBindingElement(checker: TypeChecker, bindingElement: ObjectBindingElementWithoutPropertyName): Symbol | undefined {
const typeOfPattern = checker.getTypeAtLocation(bindingElement.parent);
return typeOfPattern && checker.getPropertyOfType(typeOfPattern, bindingElement.name.text);
Expand Down
34 changes: 30 additions & 4 deletions src/testRunner/unittests/tsserver/rename.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,39 @@ namespace ts.projectSystem {
});
});

it("works with prefixText and suffixText", () => {
it("works with prefixText and suffixText when enabled", () => {
const aTs: File = { path: "/a.ts", content: "const x = 0; const o = { x };" };
const session = createSession(createServerHost([aTs]));
const host = createServerHost([aTs]);
const session = createSession(host);
openFilesForSession([aTs], session);

const response = executeSessionRequest<protocol.RenameRequest, protocol.RenameResponse>(session, protocol.CommandTypes.Rename, protocolFileLocationFromSubstring(aTs, "x"));
assert.deepEqual<protocol.RenameResponseBody | undefined>(response, {
// rename without prefixText and suffixText enabled
uniqueiniquity marked this conversation as resolved.
Show resolved Hide resolved
const response1 = executeSessionRequest<protocol.RenameRequest, protocol.RenameResponse>(session, protocol.CommandTypes.Rename, protocolFileLocationFromSubstring(aTs, "x"));
assert.deepEqual<protocol.RenameResponseBody | undefined>(response1, {
info: {
canRename: true,
fileToRename: undefined,
displayName: "x",
fullDisplayName: "x",
kind: ScriptElementKind.constElement,
kindModifiers: ScriptElementKindModifier.none,
triggerSpan: protocolTextSpanFromSubstring(aTs.content, "x"),
},
locs: [
{
file: aTs.path,
locs: [
protocolRenameSpanFromSubstring(aTs.content, "x"),
protocolRenameSpanFromSubstring(aTs.content, "x", { index: 1 }),
],
},
],
});

// rename with prefixText and suffixText enabled
session.getProjectService().setHostConfiguration({ preferences: { usePrefixAndSuffixTextForRename: true } });
const response2 = executeSessionRequest<protocol.RenameRequest, protocol.RenameResponse>(session, protocol.CommandTypes.Rename, protocolFileLocationFromSubstring(aTs, "x"));
assert.deepEqual<protocol.RenameResponseBody | undefined>(response2, {
info: {
canRename: true,
fileToRename: undefined,
Expand Down
Loading