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 20 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 @@ -5864,6 +5864,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 providePrefixAndSuffixTextForRename?: boolean;
}

/** 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(), providePrefixAndSuffixTextForRename = true } = ts.isArray(options) ? { findInStrings: false, findInComments: false, ranges: options, providePrefixAndSuffixTextForRename: 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, providePrefixAndSuffixTextForRename);

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 @@ -5087,6 +5087,7 @@ namespace FourSlashInterface {
readonly findInStrings?: boolean;
readonly findInComments?: boolean;
readonly ranges: ReadonlyArray<RenameLocationOptions>;
readonly providePrefixAndSuffixTextForRename?: boolean;
};
export type RenameLocationOptions = FourSlash.Range | { readonly range: FourSlash.Range, readonly prefixText?: string, readonly suffixText?: string };
}
4 changes: 2 additions & 2 deletions src/harness/harnessLanguageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -472,8 +472,8 @@ namespace Harness.LanguageService {
getRenameInfo(fileName: string, position: number, options?: ts.RenameInfoOptions): ts.RenameInfo {
return unwrapJSONCallResult(this.shim.getRenameInfo(fileName, position, options));
}
findRenameLocations(fileName: string, position: number, findInStrings: boolean, findInComments: boolean): ts.RenameLocation[] {
return unwrapJSONCallResult(this.shim.findRenameLocations(fileName, position, findInStrings, findInComments));
findRenameLocations(fileName: string, position: number, findInStrings: boolean, findInComments: boolean, providePrefixAndSuffixTextForRename: boolean): ts.RenameLocation[] {
return unwrapJSONCallResult(this.shim.findRenameLocations(fileName, position, findInStrings, findInComments, providePrefixAndSuffixTextForRename));
}
getDefinitionAtPosition(fileName: string, position: number): ts.DefinitionInfo[] {
return unwrapJSONCallResult(this.shim.getDefinitionAtPosition(fileName, position));
Expand Down
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 providePrefixAndSuffixTextForRename?: boolean;
readonly allowRenameOfImportPath?: boolean;
}

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,
hostPreferences: UserPreferences
): 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, hostPreferences.providePrefixAndSuffixTextForRename || false) || emptyArray) {
if (!contains(outputs, output, documentSpansEqual) && !tryAddToTodo(project, documentSpanLocation(output))) {
outputs.push(output);
}
Expand Down Expand Up @@ -1232,7 +1233,8 @@ namespace ts.server {
this.getDefaultProject(args),
{ fileName: args.file, pos: position },
!!args.findInStrings,
!!args.findInComments
!!args.findInComments,
this.getHostPreferences()
);
if (!simplifiedResult) return locations;

Expand Down
63 changes: 48 additions & 15 deletions src/services/findAllReferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ 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;
/**
* True to opt in for enhanced renaming of shorthand properties and import/export specifiers.
* Default is false for backwards compatibility.
*/
readonly providePrefixAndSuffixTextForRename?: boolean;
}

export function findReferencedSymbols(program: Program, cancellationToken: CancellationToken, sourceFiles: ReadonlyArray<SourceFile>, sourceFile: SourceFile, position: number): ReferencedSymbol[] | undefined {
Expand Down Expand Up @@ -157,8 +162,8 @@ namespace ts.FindAllReferences {
return { displayParts, kind: symbolKind };
}

export function toRenameLocation(entry: Entry, originalNode: Node, checker: TypeChecker): RenameLocation {
return { ...entryToDocumentSpan(entry), ...getPrefixAndSuffixText(entry, originalNode, checker) };
export function toRenameLocation(entry: Entry, originalNode: Node, checker: TypeChecker, providePrefixAndSuffixText: boolean): RenameLocation {
return { ...entryToDocumentSpan(entry), ...(providePrefixAndSuffixText && getPrefixAndSuffixText(entry, originalNode, checker)) };
}

export function toReferenceEntry(entry: Entry): ReferenceEntry {
Expand Down Expand Up @@ -484,15 +489,16 @@ 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 isForRenameWithPrefixAndSuffixText = options.isForRename && options.providePrefixAndSuffixTextForRename;
const symbol = node && skipPastExportOrImportSpecifierOrUnion(originalSymbol, node, checker, /*useLocalSymbolForExportSpecifier*/ !isForRenameWithPrefixAndSuffixText) || originalSymbol;

// Compute the meaning from the location and the symbol it references
const searchMeaning = node ? getIntersectingMeaningFromDeclarations(node, symbol) : SemanticMeaning.All;

const result: SymbolAndEntries[] = [];
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);
const exportSpecifier = !isForRenameWithPrefixAndSuffixText ? undefined : find(symbol.declarations, isExportSpecifier);
if (exportSpecifier) {
// 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 +508,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.providePrefixAndSuffixTextForRename, !!options.implementations) : [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 +544,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, useLocalSymbolForExportSpecifier: boolean): Symbol | undefined {
const { parent } = node;
if (isExportSpecifier(parent) && !isForRename) {
if (isExportSpecifier(parent) && useLocalSymbolForExportSpecifier) {
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 @@ -1071,6 +1077,9 @@ namespace ts.FindAllReferences.Core {
addReferencesHere: boolean,
alwaysGetReferences?: boolean,
): void {
Debug.assert(!alwaysGetReferences || !!state.options.providePrefixAndSuffixTextForRename, "If alwaysGetReferences is true, then prefix/suffix text must be enabled");
const isForRenameWithPrefixAndSuffixText = state.options.isForRename && state.options.providePrefixAndSuffixTextForRename;

const { parent, propertyName, name } = exportSpecifier;
const exportDeclaration = parent.parent;
const localSymbol = getLocalSymbolForExportSpecifier(referenceLocation, referenceSymbol, exportSpecifier, state.checker);
Expand Down Expand Up @@ -1102,15 +1111,15 @@ namespace ts.FindAllReferences.Core {
}

// For `export { foo as bar }`, rename `foo`, but not `bar`.
if (!state.options.isForRename || alwaysGetReferences) {
if (!isForRenameWithPrefixAndSuffixText || 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));
searchForImportsOfExport(referenceLocation, exportSymbol, exportInfo, state);
}

// At `export { x } from "foo"`, also search for the imported symbol `"foo".x`.
if (search.comingFrom !== ImportExport.Export && exportDeclaration.moduleSpecifier && !propertyName && !state.options.isForRename) {
if (search.comingFrom !== ImportExport.Export && exportDeclaration.moduleSpecifier && !propertyName && !isForRenameWithPrefixAndSuffixText) {
const imported = state.checker.getExportSpecifierLocalTargetSymbol(exportSpecifier);
if (imported) searchForImportedSymbol(imported, state);
}
Expand Down Expand Up @@ -1145,7 +1154,7 @@ namespace ts.FindAllReferences.Core {
const { symbol } = importOrExport;

if (importOrExport.kind === ImportExport.Import) {
if (!state.options.isForRename) {
if (!(state.options.isForRename && state.options.providePrefixAndSuffixTextForRename)) {
uniqueiniquity marked this conversation as resolved.
Show resolved Hide resolved
searchForImportedSymbol(symbol, state);
}
}
Expand Down Expand Up @@ -1514,16 +1523,16 @@ 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, providePrefixAndSuffixText: boolean, implementations: boolean): Symbol[] {
const result: Symbol[] = [];
forEachRelatedSymbol<void>(symbol, location, checker, isForRename,
forEachRelatedSymbol<void>(symbol, location, checker, isForRename, !(isForRename && providePrefixAndSuffixText),
(sym, root, base) => { result.push(base || root || sym); },
/*allowBaseTypes*/ () => !implementations);
return result;
}

function forEachRelatedSymbol<T>(
symbol: Symbol, location: Node, checker: TypeChecker, isForRenamePopulateSearchSymbolSet: boolean,
symbol: Symbol, location: Node, checker: TypeChecker, isForRenamePopulateSearchSymbolSet: boolean, onlyIncludeBindingElementAtReferenceLocation: boolean,
cbSymbol: (symbol: Symbol, rootSymbol?: Symbol, baseSymbol?: Symbol, kind?: NodeEntryKind) => T | undefined,
allowBaseTypes: (rootSymbol: Symbol) => boolean,
): T | undefined {
Expand Down Expand Up @@ -1577,9 +1586,25 @@ namespace ts.FindAllReferences.Core {
}

// 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.
// Don't do this when populating search set for a rename when prefix and suffix text are available -- just rename the local.
uniqueiniquity marked this conversation as resolved.
Show resolved Hide resolved
if (!isForRenamePopulateSearchSymbolSet) {
const bindingElementPropertySymbol = isObjectBindingElementWithoutPropertyName(location.parent) ? getPropertySymbolFromBindingElement(checker, location.parent) : undefined;
let bindingElementPropertySymbol: Symbol | undefined;
if (onlyIncludeBindingElementAtReferenceLocation) {
bindingElementPropertySymbol = isObjectBindingElementWithoutPropertyName(location.parent) ? getPropertySymbolFromBindingElement(checker, location.parent) : undefined;
}
else {
bindingElementPropertySymbol = getPropertySymbolOfObjectBindingPatternWithoutPropertyName(symbol, checker);
}
return bindingElementPropertySymbol && fromRoot(bindingElementPropertySymbol, EntryKind.SearchedPropertyFoundLocal);
}

Debug.assert(isForRenamePopulateSearchSymbolSet);
// due to the above assert and the arguments at the uses of this function,
// (onlyIncludeBindingElementAtReferenceLocation <=> !providePrefixAndSuffixTextForRename) holds
const includeOriginalSymbolOfBindingElement = onlyIncludeBindingElementAtReferenceLocation;

if (includeOriginalSymbolOfBindingElement) {
const bindingElementPropertySymbol = getPropertySymbolOfObjectBindingPatternWithoutPropertyName(symbol, checker);
return bindingElementPropertySymbol && fromRoot(bindingElementPropertySymbol, EntryKind.SearchedPropertyFoundLocal);
}

Expand All @@ -1597,6 +1622,13 @@ namespace ts.FindAllReferences.Core {
? getPropertySymbolsFromBaseTypes(rootSymbol.parent, rootSymbol.name, checker, base => cbSymbol(sym, rootSymbol, base, kind))
: undefined));
}

function getPropertySymbolOfObjectBindingPatternWithoutPropertyName(symbol: Symbol, checker: TypeChecker): Symbol | undefined {
const bindingElement = getDeclarationOfKind<BindingElement>(symbol, SyntaxKind.BindingElement);
if (bindingElement && isObjectBindingElementWithoutPropertyName(bindingElement)) {
return getPropertySymbolFromBindingElement(checker, bindingElement);
}
}
}

interface RelatedSymbol {
Expand All @@ -1606,6 +1638,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,
/*onlyIncludeBindingElementAtReferenceLocation*/ !state.options.isForRename || !!state.options.providePrefixAndSuffixTextForRename,
(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, providePrefixAndSuffixTextForRename: 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, providePrefixAndSuffixTextForRename, isForRename: true },
(entry, originalNode, checker) => FindAllReferences.toRenameLocation(entry, originalNode, checker, providePrefixAndSuffixTextForRename));
}
}

Expand Down
Loading