Skip to content

Commit

Permalink
Clean up FAR aggregation (#48619)
Browse files Browse the repository at this point in the history
* Clean up FAR and RenameLocations

This change had two goals:

1. Make the code easier to understand, primarily by simplifying the callback structure and minimizing side-effects
2. Improve performance by reducing repeated work, both FAR searches of individual projects and default tsconfig searches

This implementation attempts to preserve the merging order found in the original code (someone less relevant in the present state of using syntactic isDefinition).

* Stop enforcing search and aggregation order

...in preparation for implementing isDefinition explicitly.

Also restore convention of referring to `DocumentPosition`s as "locations".

* Introduce LanguageService.updateIsDefinitionOfReferencedSymbols

...to allow use of the checker when computing isDefinition across projects.

* Update baselines

* Tidy diff

* De-dup simplified results

* Baseline cross-project isDefinition results

* Move de-duping upstream to fix Full output

* Add server baseline test to confirm searches are not repeated

* Manually merge #48758

* Update baseline for newer fix to #48963
  • Loading branch information
amcasey authored May 19, 2022
1 parent e56a067 commit 12ed012
Show file tree
Hide file tree
Showing 36 changed files with 4,226 additions and 221 deletions.
4 changes: 4 additions & 0 deletions src/harness/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,10 @@ namespace ts.server {
throw new Error("Program objects are not serializable through the server protocol.");
}

updateIsDefinitionOfReferencedSymbols(_referencedSymbols: readonly ReferencedSymbol[], _knownSymbolSpans: Set<DocumentSpan>): boolean {
return notImplemented();
}

getNonBoundSourceFile(_fileName: string): SourceFile {
throw new Error("SourceFile objects are not serializable through the server protocol.");
}
Expand Down
3 changes: 3 additions & 0 deletions src/harness/harnessLanguageService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,9 @@ namespace Harness.LanguageService {
getAutoImportProvider(): ts.Program | undefined {
throw new Error("Program can not be marshaled across the shim layer.");
}
updateIsDefinitionOfReferencedSymbols(_referencedSymbols: readonly ts.ReferencedSymbol[], _knownSymbolSpans: ts.Set<ts.DocumentSpan>): boolean {
return ts.notImplemented();
}
getNonBoundSourceFile(): ts.SourceFile {
throw new Error("SourceFile can not be marshaled across the shim layer.");
}
Expand Down
431 changes: 262 additions & 169 deletions src/server/session.ts

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/services/findAllReferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,7 @@ namespace ts.FindAllReferences {
}

/** Whether a reference, `node`, is a definition of the `target` symbol */
function isDeclarationOfSymbol(node: Node, target: Symbol | undefined): boolean {
export function isDeclarationOfSymbol(node: Node, target: Symbol | undefined): boolean {
if (!target) return false;
const source = getDeclarationFromName(node) ||
(node.kind === SyntaxKind.DefaultKeyword ? node.parent
Expand Down
57 changes: 57 additions & 0 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1601,6 +1601,62 @@ namespace ts {
return host.getPackageJsonAutoImportProvider?.();
}

function updateIsDefinitionOfReferencedSymbols(referencedSymbols: readonly ReferencedSymbol[], knownSymbolSpans: Set<DocumentSpan>): boolean {
const checker = program.getTypeChecker();
const symbol = getSymbolForProgram();

if (!symbol) return false;

for (const referencedSymbol of referencedSymbols) {
for (const ref of referencedSymbol.references) {
const refNode = getNodeForSpan(ref);
Debug.assertIsDefined(refNode);
if (knownSymbolSpans.has(ref) || FindAllReferences.isDeclarationOfSymbol(refNode, symbol)) {
knownSymbolSpans.add(ref);
ref.isDefinition = true;
const mappedSpan = getMappedDocumentSpan(ref, sourceMapper, maybeBind(host, host.fileExists));
if (mappedSpan) {
knownSymbolSpans.add(mappedSpan);
}
}
else {
ref.isDefinition = false;
}
}
}

return true;

function getSymbolForProgram(): Symbol | undefined {
for (const referencedSymbol of referencedSymbols) {
for (const ref of referencedSymbol.references) {
if (knownSymbolSpans.has(ref)) {
const refNode = getNodeForSpan(ref);
Debug.assertIsDefined(refNode);
return checker.getSymbolAtLocation(refNode);
}
const mappedSpan = getMappedDocumentSpan(ref, sourceMapper, maybeBind(host, host.fileExists));
if (mappedSpan && knownSymbolSpans.has(mappedSpan)) {
const refNode = getNodeForSpan(mappedSpan);
if (refNode) {
return checker.getSymbolAtLocation(refNode);
}
}
}
}

return undefined;
}

function getNodeForSpan(docSpan: DocumentSpan): Node | undefined {
const sourceFile = program.getSourceFile(docSpan.fileName);
if (!sourceFile) return undefined;
const rawNode = getTouchingPropertyName(sourceFile, docSpan.textSpan.start);
const adjustedNode = FindAllReferences.Core.getAdjustedNode(rawNode, { use: FindAllReferences.FindReferencesUse.References });
return adjustedNode;
}
}

function cleanupSemanticCache(): void {
program = undefined!; // TODO: GH#18217
}
Expand Down Expand Up @@ -2716,6 +2772,7 @@ namespace ts {
getNonBoundSourceFile,
getProgram,
getAutoImportProvider,
updateIsDefinitionOfReferencedSymbols,
getApplicableRefactors,
getEditsForRefactor,
toLineColumnOffset,
Expand Down
5 changes: 5 additions & 0 deletions src/services/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,11 @@ namespace ts {
/* @internal */ getNonBoundSourceFile(fileName: string): SourceFile;
/* @internal */ getAutoImportProvider(): Program | undefined;

/// Returns true if a suitable symbol was found in the project.
/// May set isDefinition properties in `referencedSymbols` to false.
/// May add elements to `knownSymbolSpans`.
/* @internal */ updateIsDefinitionOfReferencedSymbols(referencedSymbols: readonly ReferencedSymbol[], knownSymbolSpans: Set<DocumentSpan>): boolean;

toggleLineComment(fileName: string, textRange: TextRange): TextChange[];
toggleMultilineComment(fileName: string, textRange: TextRange): TextChange[];
commentSelection(fileName: string, textRange: TextRange): TextChange[];
Expand Down
42 changes: 42 additions & 0 deletions src/services/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2117,6 +2117,48 @@ namespace ts {
return true;
}

export function getMappedLocation(location: DocumentPosition, sourceMapper: SourceMapper, fileExists: ((path: string) => boolean) | undefined): DocumentPosition | undefined {
const mapsTo = sourceMapper.tryGetSourcePosition(location);
return mapsTo && (!fileExists || fileExists(normalizePath(mapsTo.fileName)) ? mapsTo : undefined);
}

export function getMappedDocumentSpan(documentSpan: DocumentSpan, sourceMapper: SourceMapper, fileExists?: (path: string) => boolean): DocumentSpan | undefined {
const { fileName, textSpan } = documentSpan;
const newPosition = getMappedLocation({ fileName, pos: textSpan.start }, sourceMapper, fileExists);
if (!newPosition) return undefined;
const newEndPosition = getMappedLocation({ fileName, pos: textSpan.start + textSpan.length }, sourceMapper, fileExists);
const newLength = newEndPosition
? newEndPosition.pos - newPosition.pos
: textSpan.length; // This shouldn't happen
return {
fileName: newPosition.fileName,
textSpan: {
start: newPosition.pos,
length: newLength,
},
originalFileName: documentSpan.fileName,
originalTextSpan: documentSpan.textSpan,
contextSpan: getMappedContextSpan(documentSpan, sourceMapper, fileExists),
originalContextSpan: documentSpan.contextSpan
};
}

export function getMappedContextSpan(documentSpan: DocumentSpan, sourceMapper: SourceMapper, fileExists?: (path: string) => boolean): TextSpan | undefined {
const contextSpanStart = documentSpan.contextSpan && getMappedLocation(
{ fileName: documentSpan.fileName, pos: documentSpan.contextSpan.start },
sourceMapper,
fileExists
);
const contextSpanEnd = documentSpan.contextSpan && getMappedLocation(
{ fileName: documentSpan.fileName, pos: documentSpan.contextSpan.start + documentSpan.contextSpan.length },
sourceMapper,
fileExists
);
return contextSpanStart && contextSpanEnd ?
{ start: contextSpanStart.pos, length: contextSpanEnd.pos - contextSpanStart.pos } :
undefined;
}

// #endregion

// Display-part writer helpers
Expand Down
4 changes: 2 additions & 2 deletions src/testRunner/unittests/tsserver/declarationFileMaps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ namespace ts.projectSystem {

const response = executeSessionRequest<protocol.ReferencesRequest, protocol.ReferencesResponse>(session, protocol.CommandTypes.References, protocolFileLocationFromSubstring(userTs, "fnA()"));
assert.deepEqual<protocol.ReferencesResponseBody | undefined>(response, {
refs: [...referencesUserTs(userTs, /*isDefinition*/ undefined), referenceATs(aTs, /*isDefinition*/ true)], // Presently inconsistent across projects
refs: [...referencesUserTs(userTs, /*isDefinition*/ undefined), referenceATs(aTs, /*isDefinition*/ undefined)],
symbolName: "fnA",
symbolStartOffset: protocolLocationFromSubstring(userTs.content, "fnA()").offset,
symbolDisplayString: "function fnA(): void",
Expand Down Expand Up @@ -455,7 +455,7 @@ namespace ts.projectSystem {
},
references: [
makeReferencedSymbolEntry({ file: userTs, text: "fnA" }),
makeReferencedSymbolEntry({ file: aTs, text: "fnA", isDefinition: true, isWriteAccess: true, contextText: "export function fnA() {}" }),
makeReferencedSymbolEntry({ file: aTs, text: "fnA", isWriteAccess: true, contextText: "export function fnA() {}" }),
],
},
]);
Expand Down
120 changes: 120 additions & 0 deletions src/testRunner/unittests/tsserver/projectReferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,126 @@ testCompositeFunction('why hello there', 42);`
baselineTsserverLogs("projectReferences", `finding local reference doesnt load ancestor/sibling projects`, session);
});

it("when finding references in overlapping projects", () => {
const solutionLocation = "/user/username/projects/solution";
const solutionConfig: File = {
path: `${solutionLocation}/tsconfig.json`,
content: JSON.stringify({
files: [],
include: [],
references: [
{ path: "./a" },
{ path: "./b" },
{ path: "./c" },
{ path: "./d" },
]
})
};
const aConfig: File = {
path: `${solutionLocation}/a/tsconfig.json`,
content: JSON.stringify({
compilerOptions: {
composite: true,
module: "none"
},
files: ["./index.ts"]
})
};
const aFile: File = {
path: `${solutionLocation}/a/index.ts`,
content: `
export interface I {
M(): void;
}`
};

const bConfig: File = {
path: `${solutionLocation}/b/tsconfig.json`,
content: JSON.stringify({
compilerOptions: {
composite: true
},
files: ["./index.ts"],
references: [
{ path: "../a" }
]
})
};
const bFile: File = {
path: `${solutionLocation}/b/index.ts`,
content: `
import { I } from "../a";
export class B implements I {
M() {}
}`
};

const cConfig: File = {
path: `${solutionLocation}/c/tsconfig.json`,
content: JSON.stringify({
compilerOptions: {
composite: true
},
files: ["./index.ts"],
references: [
{ path: "../b" }
]
})
};
const cFile: File = {
path: `${solutionLocation}/c/index.ts`,
content: `
import { I } from "../a";
import { B } from "../b";
export const C: I = new B();
`
};

const dConfig: File = {
path: `${solutionLocation}/d/tsconfig.json`,
content: JSON.stringify({
compilerOptions: {
composite: true
},
files: ["./index.ts"],
references: [
{ path: "../c" }
]
})
};
const dFile: File = {
path: `${solutionLocation}/d/index.ts`,
content: `
import { I } from "../a";
import { C } from "../c";
export const D: I = C;
`
};

const files = [libFile, solutionConfig, aConfig, aFile, bConfig, bFile, cConfig, cFile, dConfig, dFile, libFile];
const host = createServerHost(files);
const session = createSession(host, { logger: createLoggerWithInMemoryLogs() });
openFilesForSession([bFile], session);

// The first search will trigger project loads
session.executeCommandSeq<protocol.ReferencesRequest>({
command: protocol.CommandTypes.References,
arguments: protocolFileLocationFromSubstring(bFile, "I", { index: 1 })
});

// The second search starts with the projects already loaded
// Formerly, this would search some projects multiple times
session.executeCommandSeq<protocol.ReferencesRequest>({
command: protocol.CommandTypes.References,
arguments: protocolFileLocationFromSubstring(bFile, "I", { index: 1 })
});

baselineTsserverLogs("projectReferences", `finding references in overlapping projects`, session);
});

describe("special handling of localness of the definitions for findAllRefs", () => {
function verify(scenario: string, definition: string, usage: string, referenceTerm: string) {
it(scenario, () => {
Expand Down
Loading

0 comments on commit 12ed012

Please sign in to comment.