Skip to content

When testing references, also test documentHighlights respects filesToSearch #23306

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.

Sign up for GitHub

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

Merged
3 commits merged into from
Apr 11, 2018
Merged

When testing references, also test documentHighlights respects filesToSearch #23306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e3e7e60
When testing references, also test documentHighlights respects filesT…
Apr 10, 2018
fb1eb47
Fix handling for redirects and move assertion inside getDocumentHighl…
Apr 10, 2018
765d00e
Add another assert
Apr 11, 2018
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
18 changes: 17 additions & 1 deletion src/harness/fourslash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1081,8 +1081,20 @@ namespace FourSlash {
}
}

private verifyDocumentHighlightsRespectFilesList(files: ReadonlyArray<string>): void {
const startFile = this.activeFile.fileName;
for (const fileName of files) {
const searchFileNames = startFile === fileName ? [startFile] : [startFile, fileName];
const highlights = this.getDocumentHighlightsAtCurrentPosition(searchFileNames);
if (!highlights.every(dh => ts.contains(searchFileNames, dh.fileName))) {
this.raiseError(`When asking for document highlights only in files ${searchFileNames}, got document highlights in ${unique(highlights, dh => dh.fileName)}`);
}
}
}

public verifyReferencesOf(range: Range, references: Range[]) {
this.goToRangeStart(range);
this.verifyDocumentHighlightsRespectFilesList(unique(references, e => e.fileName));
this.verifyReferencesAre(references);
}

Expand All @@ -1094,7 +1106,7 @@ namespace FourSlash {
}
}

public verifyReferenceGroups(starts: string | string[] | Range | Range[], parts: FourSlashInterface.ReferenceGroup[]): void {
public verifyReferenceGroups(starts: string | string[] | Range | Range[], parts: FourSlashInterface.ReferenceGroup[] | undefined): void {
interface ReferenceGroupJson {
definition: string | { text: string, range: ts.TextSpan };
references: ts.ReferenceEntry[];
Expand Down Expand Up @@ -1128,6 +1140,10 @@ namespace FourSlash {
};
});
this.assertObjectsEqual(fullActual, fullExpected);

if (parts) {
this.verifyDocumentHighlightsRespectFilesList(unique(ts.flatMap(parts, p => p.ranges), r => r.fileName));
}
}
}

Expand Down
14 changes: 12 additions & 2 deletions src/services/documentHighlights.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,20 @@ namespace ts.DocumentHighlights {
}

function getSemanticDocumentHighlights(position: number, node: Node, program: Program, cancellationToken: CancellationToken, sourceFilesToSearch: ReadonlyArray<SourceFile>): DocumentHighlights[] | undefined {
const referenceEntries = FindAllReferences.getReferenceEntriesForNode(position, node, program, sourceFilesToSearch, cancellationToken);
const sourceFilesSet = arrayToSet(sourceFilesToSearch, f => f.fileName);
const referenceEntries = FindAllReferences.getReferenceEntriesForNode(position, node, program, sourceFilesToSearch, cancellationToken, /*options*/ undefined, sourceFilesSet);
if (!referenceEntries) return undefined;
const map = arrayToMultiMap(referenceEntries.map(FindAllReferences.toHighlightSpan), e => e.fileName, e => e.span);
return arrayFrom(map.entries(), ([fileName, highlightSpans]) => ({ fileName, highlightSpans }));
return arrayFrom(map.entries(), ([fileName, highlightSpans]) => {
if (!sourceFilesSet.has(fileName)) {
Debug.assert(program.redirectTargetsSet.has(fileName));
const redirectTarget = program.getSourceFile(fileName);
const redirect = find(sourceFilesToSearch, f => f.redirectInfo && f.redirectInfo.redirectTarget === redirectTarget)!;
fileName = redirect.fileName;
Copy link
Contributor

Choose a reason for hiding this comment

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

we are not sure this fileName is in the list either.. we should probally assert that it is.

Debug.assert(sourceFilesSet.has(fileName));
}
return { fileName, highlightSpans };
});
}

function getSyntacticDocumentHighlights(node: Node, sourceFile: SourceFile): DocumentHighlights[] {
Expand Down
32 changes: 16 additions & 16 deletions src/services/findAllReferences.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ namespace ts.FindAllReferences {

export function findReferencedSymbols(program: Program, cancellationToken: CancellationToken, sourceFiles: ReadonlyArray<SourceFile>, sourceFile: SourceFile, position: number): ReferencedSymbol[] | undefined {
const node = getTouchingPropertyName(sourceFile, position, /*includeJsDocComment*/ true);
const referencedSymbols = Core.getReferencedSymbolsForNode(position, node, program, sourceFiles, cancellationToken, /*options*/ {});
const referencedSymbols = Core.getReferencedSymbolsForNode(position, node, program, sourceFiles, cancellationToken);
const checker = program.getTypeChecker();
return !referencedSymbols || !referencedSymbols.length ? undefined : mapDefined<SymbolAndEntries, ReferencedSymbol>(referencedSymbols, ({ definition, references }) =>
// Only include referenced symbols that have a valid definition.
Expand Down Expand Up @@ -88,8 +88,8 @@ namespace ts.FindAllReferences {
return map(flattenEntries(Core.getReferencedSymbolsForNode(position, node, program, sourceFiles, cancellationToken, options)), toReferenceEntry);
}

export function getReferenceEntriesForNode(position: number, node: Node, program: Program, sourceFiles: ReadonlyArray<SourceFile>, cancellationToken: CancellationToken, options: Options = {}): Entry[] | undefined {
return flattenEntries(Core.getReferencedSymbolsForNode(position, node, program, sourceFiles, cancellationToken, options));
export function getReferenceEntriesForNode(position: number, node: Node, program: Program, sourceFiles: ReadonlyArray<SourceFile>, cancellationToken: CancellationToken, options: Options = {}, sourceFilesSet: ReadonlyMap<true> = arrayToSet(sourceFiles, f => f.fileName)): Entry[] | undefined {
return flattenEntries(Core.getReferencedSymbolsForNode(position, node, program, sourceFiles, cancellationToken, options, sourceFilesSet));
}

function flattenEntries(referenceSymbols: SymbolAndEntries[]): Entry[] {
Expand Down Expand Up @@ -231,10 +231,10 @@ namespace ts.FindAllReferences {
/* @internal */
namespace ts.FindAllReferences.Core {
/** Core find-all-references algorithm. Handles special cases before delegating to `getReferencedSymbolsForSymbol`. */
export function getReferencedSymbolsForNode(position: number, node: Node, program: Program, sourceFiles: ReadonlyArray<SourceFile>, cancellationToken: CancellationToken, options: Options = {}): SymbolAndEntries[] | undefined {
export function getReferencedSymbolsForNode(position: number, node: Node, program: Program, sourceFiles: ReadonlyArray<SourceFile>, cancellationToken: CancellationToken, options: Options = {}, sourceFilesSet: ReadonlyMap<true> = arrayToSet(sourceFiles, f => f.fileName)): SymbolAndEntries[] | undefined {
if (isSourceFile(node)) {
const reference = GoToDefinition.getReferenceAtPosition(node, position, program);
return reference && getReferencedSymbolsForModule(program, program.getTypeChecker().getMergedSymbol(reference.file.symbol), sourceFiles);
return reference && getReferencedSymbolsForModule(program, program.getTypeChecker().getMergedSymbol(reference.file.symbol), sourceFiles, sourceFilesSet);
}

if (!options.implementations) {
Expand All @@ -254,10 +254,10 @@ namespace ts.FindAllReferences.Core {
}

if (symbol.flags & SymbolFlags.Module && isModuleReferenceLocation(node)) {
return getReferencedSymbolsForModule(program, symbol, sourceFiles);
return getReferencedSymbolsForModule(program, symbol, sourceFiles, sourceFilesSet);
}

return getReferencedSymbolsForSymbol(symbol, node, sourceFiles, checker, cancellationToken, options);
return getReferencedSymbolsForSymbol(symbol, node, sourceFiles, sourceFilesSet, checker, cancellationToken, options);
}

function isModuleReferenceLocation(node: Node): boolean {
Expand All @@ -277,7 +277,7 @@ namespace ts.FindAllReferences.Core {
}
}

function getReferencedSymbolsForModule(program: Program, symbol: Symbol, sourceFiles: ReadonlyArray<SourceFile>): SymbolAndEntries[] {
function getReferencedSymbolsForModule(program: Program, symbol: Symbol, sourceFiles: ReadonlyArray<SourceFile>, sourceFilesSet: ReadonlyMap<true>): SymbolAndEntries[] {
Debug.assert(!!symbol.valueDeclaration);

const references = findModuleReferences(program, sourceFiles, symbol).map<Entry>(reference => {
Expand All @@ -299,7 +299,9 @@ namespace ts.FindAllReferences.Core {
// Don't include the source file itself. (This may not be ideal behavior, but awkward to include an entire file as a reference.)
break;
case SyntaxKind.ModuleDeclaration:
references.push({ type: "node", node: (decl as ModuleDeclaration).name });
if (sourceFilesSet.has(decl.getSourceFile().fileName)) {
references.push({ type: "node", node: (decl as ModuleDeclaration).name });
}
break;
default:
Debug.fail("Expected a module symbol to be declared by a SourceFile or ModuleDeclaration.");
Expand Down Expand Up @@ -339,14 +341,14 @@ namespace ts.FindAllReferences.Core {
}

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

// Compute the meaning from the location and the symbol it references
const searchMeaning = getIntersectingMeaningFromDeclarations(node, symbol);

const result: SymbolAndEntries[] = [];
const state = new State(sourceFiles, getSpecialSearchKind(node), checker, cancellationToken, searchMeaning, options, result);
const state = new State(sourceFiles, sourceFilesSet, getSpecialSearchKind(node), checker, cancellationToken, searchMeaning, options, result);

if (node.kind === SyntaxKind.DefaultKeyword) {
addReference(node, symbol, state);
Expand Down Expand Up @@ -469,28 +471,26 @@ namespace ts.FindAllReferences.Core {
*/
readonly markSeenReExportRHS = nodeSeenTracker();

private readonly includedSourceFiles: Map<true>;

constructor(
readonly sourceFiles: ReadonlyArray<SourceFile>,
readonly sourceFilesSet: ReadonlyMap<true>,
/** True if we're searching for constructor references. */
readonly specialSearchKind: SpecialSearchKind,
readonly checker: TypeChecker,
readonly cancellationToken: CancellationToken,
readonly searchMeaning: SemanticMeaning,
readonly options: Options,
private readonly result: Push<SymbolAndEntries>) {
this.includedSourceFiles = arrayToSet(sourceFiles, s => s.fileName);
}

includesSourceFile(sourceFile: SourceFile): boolean {
return this.includedSourceFiles.has(sourceFile.fileName);
return this.sourceFilesSet.has(sourceFile.fileName);
}

private importTracker: ImportTracker | undefined;
/** Gets every place to look for references of an exported symbols. See `ImportsResult` in `importTracker.ts` for more documentation. */
getImportSearches(exportSymbol: Symbol, exportInfo: ExportInfo): ImportsResult {
if (!this.importTracker) this.importTracker = createImportTracker(this.sourceFiles, this.checker, this.cancellationToken);
if (!this.importTracker) this.importTracker = createImportTracker(this.sourceFiles, this.sourceFilesSet, this.checker, this.cancellationToken);
return this.importTracker(exportSymbol, exportInfo, this.options.isForRename);
}

Expand Down
7 changes: 4 additions & 3 deletions src/services/importTracker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ namespace ts.FindAllReferences {
export type ImportTracker = (exportSymbol: Symbol, exportInfo: ExportInfo, isForRename: boolean) => ImportsResult;

/** Creates the imports map and returns an ImportTracker that uses it. Call this lazily to avoid calling `getDirectImportsMap` unnecessarily. */
export function createImportTracker(sourceFiles: ReadonlyArray<SourceFile>, checker: TypeChecker, cancellationToken: CancellationToken): ImportTracker {
export function createImportTracker(sourceFiles: ReadonlyArray<SourceFile>, sourceFilesSet: ReadonlyMap<true>, checker: TypeChecker, cancellationToken: CancellationToken): ImportTracker {
const allDirectImports = getDirectImportsMap(sourceFiles, checker, cancellationToken);
return (exportSymbol, exportInfo, isForRename) => {
const { directImports, indirectUsers } = getImportersForExport(sourceFiles, allDirectImports, exportInfo, checker, cancellationToken);
const { directImports, indirectUsers } = getImportersForExport(sourceFiles, sourceFilesSet, allDirectImports, exportInfo, checker, cancellationToken);
return { indirectUsers, ...getSearchesFromDirectImports(directImports, exportSymbol, exportInfo.exportKind, checker, isForRename) };
};
}
Expand All @@ -39,6 +39,7 @@ namespace ts.FindAllReferences {
/** Returns import statements that directly reference the exporting module, and a list of files that may access the module through a namespace. */
function getImportersForExport(
sourceFiles: ReadonlyArray<SourceFile>,
sourceFilesSet: ReadonlyMap<true>,
allDirectImports: Map<ImporterOrCallExpression[]>,
{ exportingModuleSymbol, exportKind }: ExportInfo,
checker: TypeChecker,
Expand All @@ -62,7 +63,7 @@ namespace ts.FindAllReferences {

// Module augmentations may use this module's exports without importing it.
for (const decl of exportingModuleSymbol.declarations) {
if (isExternalModuleAugmentation(decl)) {
if (isExternalModuleAugmentation(decl) && sourceFilesSet.has(decl.getSourceFile().fileName)) {
addIndirectUser(decl);
}
}
Expand Down
19 changes: 8 additions & 11 deletions src/services/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1697,20 +1697,17 @@ namespace ts {

/// References and Occurrences
function getOccurrencesAtPosition(fileName: string, position: number): ReferenceEntry[] {
const canonicalFileName = getCanonicalFileName(normalizeSlashes(fileName));
return flatMap(getDocumentHighlights(fileName, position, [fileName]), entry => entry.highlightSpans.map<ReferenceEntry>(highlightSpan => {
Debug.assert(getCanonicalFileName(normalizeSlashes(entry.fileName)) === canonicalFileName); // Get occurrences only supports reporting occurrences for the file queried.
return {
fileName: entry.fileName,
textSpan: highlightSpan.textSpan,
isWriteAccess: highlightSpan.kind === HighlightSpanKind.writtenReference,
isDefinition: false,
isInString: highlightSpan.isInString,
};
}));
return flatMap(getDocumentHighlights(fileName, position, [fileName]), entry => entry.highlightSpans.map<ReferenceEntry>(highlightSpan => ({
fileName: entry.fileName,
textSpan: highlightSpan.textSpan,
isWriteAccess: highlightSpan.kind === HighlightSpanKind.writtenReference,
isDefinition: false,
isInString: highlightSpan.isInString,
})));
}

function getDocumentHighlights(fileName: string, position: number, filesToSearch: ReadonlyArray<string>): DocumentHighlights[] {
Debug.assert(contains(filesToSearch, fileName));
synchronizeHostData();
const sourceFilesToSearch = map(filesToSearch, f => Debug.assertDefined(program.getSourceFile(f)));
const sourceFile = getValidSourceFile(fileName);
Expand Down
6 changes: 2 additions & 4 deletions tests/cases/fourslash/duplicatePackageServices.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
////}

// @Filename: /node_modules/b/node_modules/x/package.json
////{ "name": "x", "version": "1.2./*bVersionPatch*/3" }
////{ "name": "x", "version": "1.2.3" }

// @Filename: /src/a.ts
////import { a } from "a";
Expand All @@ -40,7 +40,5 @@ const aImport = { definition: "(alias) class X\nimport X", ranges: [r0, r1] };
const def = { definition: "class X", ranges: [r2] };
const bImport = { definition: "(alias) class X\nimport X", ranges: [r3, r4] };
verify.referenceGroups([r0, r1], [aImport, def, bImport]);
verify.referenceGroups([r2], [def, aImport, bImport]);
verify.referenceGroups([r2, r5], [def, aImport, bImport]);
verify.referenceGroups([r3, r4], [bImport, def, aImport]);

verify.referenceGroups(r5, [def, aImport, bImport]);