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.

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

Added DefinitionAndBoundSpan command #19175

Merged
merged 15 commits into from
Oct 31, 2017
Merged

Added DefinitionAndBoundSpan command #19175

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1cb2d24
Added DefinitionAndBoundSpan command
armanio123 Oct 13, 2017
c6a8a32
Fixed api reference tests
armanio123 Oct 13, 2017
b86153d
Changed command designed based on review input
armanio123 Oct 17, 2017
8004fec
Addressed PR comments: added simplified/full version, changed design
armanio123 Oct 18, 2017
16c3255
Updated an incredible amount of tests.
armanio123 Oct 18, 2017
051da11
Removed custom guard and added isArray
armanio123 Oct 23, 2017
f8ccde5
Renamed a couple of methods, refactored code for reusability
armanio123 Oct 24, 2017
ae266f6
Merge branch 'master' into AddDefinitionAndBoundSpan
armanio123 Oct 24, 2017
7f577dc
Added triple slash support
armanio123 Oct 24, 2017
c62f288
Merge branch 'AddDefinitionAndBoundSpan' of https://github.com/armani…
armanio123 Oct 24, 2017
7fd9fe6
Rollback spread operator changes
armanio123 Oct 24, 2017
5b7abf2
Fixed tsserverlibrary.d.ts tests
armanio123 Oct 24, 2017
f3059ce
Addressed PR comments.
armanio123 Oct 24, 2017
c4a675e
Removed double tests of gotoDefinition
armanio123 Oct 24, 2017
d5c18a6
Merge branch 'master' into AddDefinitionAndBoundSpan
armanio123 Oct 31, 2017
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
53 changes: 45 additions & 8 deletions src/harness/fourslash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,18 +584,23 @@ namespace FourSlash {

public verifyGoToDefinition(arg0: any, endMarkerNames?: string | string[]) {
this.verifyGoToX(arg0, endMarkerNames, () => this.getGoToDefinition());
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why we need two calls here. getGoToDefinitionAndBoundSpan subsumes the other one, so would be sufficient to do here.

Copy link
Contributor

Choose a reason for hiding this comment

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

i would say always use getGoToDefinitionAndBoundSpan and check the span only if the marker name was provided.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed as suggested.

this.verifyGoToX(arg0, endMarkerNames, () => this.getGoToDefinitionAndBoundSpan());
}

private getGoToDefinition(): ts.DefinitionInfo[] {
return this.languageService.getDefinitionAtPosition(this.activeFile.fileName, this.currentCaretPosition);
}

private getGoToDefinitionAndBoundSpan(): ts.DefinitionInfoAndBoundSpan {
return this.languageService.getDefinitionAndBoundSpan(this.activeFile.fileName, this.currentCaretPosition);
}

public verifyGoToType(arg0: any, endMarkerNames?: string | string[]) {
this.verifyGoToX(arg0, endMarkerNames, () =>
this.languageService.getTypeDefinitionAtPosition(this.activeFile.fileName, this.currentCaretPosition));
}

private verifyGoToX(arg0: any, endMarkerNames: string | string[] | undefined, getDefs: () => ts.DefinitionInfo[] | undefined) {
private verifyGoToX(arg0: any, endMarkerNames: string | string[] | undefined, getDefs: () => ts.DefinitionInfo[] | ts.DefinitionInfoAndBoundSpan | undefined) {
if (endMarkerNames) {
this.verifyGoToXPlain(arg0, endMarkerNames, getDefs);
}
Expand All @@ -615,7 +620,7 @@ namespace FourSlash {
}
}

private verifyGoToXPlain(startMarkerNames: string | string[], endMarkerNames: string | string[], getDefs: () => ts.DefinitionInfo[] | undefined) {
private verifyGoToXPlain(startMarkerNames: string | string[], endMarkerNames: string | string[], getDefs: () => ts.DefinitionInfo[] | ts.DefinitionInfoAndBoundSpan | undefined) {
for (const start of toArray(startMarkerNames)) {
this.verifyGoToXSingle(start, endMarkerNames, getDefs);
}
Expand All @@ -627,26 +632,57 @@ namespace FourSlash {
}
}

private verifyGoToXSingle(startMarkerName: string, endMarkerNames: string | string[], getDefs: () => ts.DefinitionInfo[] | undefined) {
private verifyGoToXSingle(startMarkerName: string, endMarkerNames: string | string[], getDefs: () => ts.DefinitionInfo[] | ts.DefinitionInfoAndBoundSpan | undefined) {
this.goToMarker(startMarkerName);
this.verifyGoToXWorker(toArray(endMarkerNames), getDefs);
this.verifyGoToXWorker(toArray(endMarkerNames), getDefs, startMarkerName);
}

private verifyGoToXWorker(endMarkers: string[], getDefs: () => ts.DefinitionInfo[] | undefined) {
const definitions = getDefs() || [];
private verifyGoToXWorker(endMarkers: string[], getDefs: () => ts.DefinitionInfo[] | ts.DefinitionInfoAndBoundSpan | undefined, startMarkerName?: string) {
const defs = getDefs();
let definitions: ts.DefinitionInfo[] | ReadonlyArray<ts.DefinitionInfo>;
let testName: string;
Copy link
Member

@amcasey amcasey Oct 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

testName [](start = 16, length = 8)

It seems like this is actually the name of the API under test. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Which one is reported when the argument is undefined? Does it matter?

In reply to: 146340127 [](ancestors = 146340127)

Copy link
Member Author

Choose a reason for hiding this comment

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

The argument is not going to be undefined only the response from the argument function. It happens when the token doesn't contain a definition.


if (!defs || Array.isArray(defs)) {
definitions = defs as ts.DefinitionInfo[] || [];
testName = "goToDefinitions";
}
else {
this.verifyDefinitionTextSpan(defs, startMarkerName);

definitions = defs.definitions;
testName = "goToDefinitionsAndBoundSpan";
}

if (endMarkers.length !== definitions.length) {
this.raiseError(`goToDefinitions failed - expected to find ${endMarkers.length} definitions but got ${definitions.length}`);
this.raiseError(`${testName} failed - expected to find ${endMarkers.length} definitions but got ${definitions.length}`);
}

ts.zipWith(endMarkers, definitions, (endMarker, definition, i) => {
const marker = this.getMarkerByName(endMarker);
if (marker.fileName !== definition.fileName || marker.position !== definition.textSpan.start) {
this.raiseError(`goToDefinition failed for definition ${endMarker} (${i}): expected ${marker.fileName} at ${marker.position}, got ${definition.fileName} at ${definition.textSpan.start}`);
this.raiseError(`${testName} failed for definition ${endMarker} (${i}): expected ${marker.fileName} at ${marker.position}, got ${definition.fileName} at ${definition.textSpan.start}`);
}
});
}

private verifyDefinitionTextSpan(defs: ts.DefinitionInfoAndBoundSpan, startMarkerName: string) {
const range = this.testData.ranges.find(range => this.markerName(range.marker) === startMarkerName);

if (!range && !defs.textSpan) {
return;
}

if (!range) {
this.raiseError(`goToDefinitionsAndBoundSpan failed - found a TextSpan ${JSON.stringify(defs.textSpan)} when it wasn't expected.`);
}
else if (defs.textSpan.start !== range.start || defs.textSpan.length !== range.end - range.start) {
const expected: ts.TextSpan = {
start: range.start, length: range.end - range.start
};
this.raiseError(`goToDefinitionsAndBoundSpan failed - expected to find TextSpan ${JSON.stringify(expected)} but got ${JSON.stringify(defs.textSpan)}`);
}
}

public verifyGetEmitOutputForCurrentFile(expected: string): void {
const emit = this.languageService.getEmitOutput(this.activeFile.fileName);
if (emit.outputFiles.length !== 1) {
Expand Down Expand Up @@ -3895,6 +3931,7 @@ namespace FourSlashInterface {
}

public goToDefinition(startMarkerName: string | string[], endMarkerName: string | string[]): void;
public goToDefinition(startMarkerName: string | string[], endMarkerName: string | string[], range: FourSlash.Range): void;
public goToDefinition(startsAndEnds: [string | string[], string | string[]][]): void;
public goToDefinition(startsAndEnds: { [startMarkerName: string]: string | string[] }): void;
public goToDefinition(arg0: any, endMarkerName?: string | string[]) {
Expand Down
3 changes: 3 additions & 0 deletions src/harness/harnessLanguageService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,9 @@ namespace Harness.LanguageService {
getDefinitionAtPosition(fileName: string, position: number): ts.DefinitionInfo[] {
return unwrapJSONCallResult(this.shim.getDefinitionAtPosition(fileName, position));
}
getDefinitionAndBoundSpan(fileName: string, position: number): ts.DefinitionInfoAndBoundSpan {
return unwrapJSONCallResult(this.shim.getDefinitionAndBoundSpan(fileName, position));
}
getTypeDefinitionAtPosition(fileName: string, position: number): ts.DefinitionInfo[] {
return unwrapJSONCallResult(this.shim.getTypeDefinitionAtPosition(fileName, position));
}
Expand Down
6 changes: 4 additions & 2 deletions src/harness/unittests/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ namespace ts.server {
body: undefined
});
});
it ("should handle literal types in request", () => {
it("should handle literal types in request", () => {
const configureRequest: protocol.ConfigureRequest = {
command: CommandNames.Configure,
seq: 0,
Expand Down Expand Up @@ -175,6 +175,8 @@ namespace ts.server {
CommandNames.Configure,
CommandNames.Definition,
CommandNames.DefinitionFull,
CommandNames.DefinitionAndBoundSpan,
Copy link
Member

@amcasey amcasey Oct 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I notice that the regular Definition command has a separate Full version. Does the new command need one as well? #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Offline, I think we established that VS Code will probably not consume the new API. If this API is just for VS, should it have "Full" in the name? I think that's how we usually distinguish.

In reply to: 144675978 [](ancestors = 144675978)

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on a discussion with @mhegazy, I have added the method for the simplified, hence, the "full" has been updated.

CommandNames.DefinitionAndBoundSpanFull,
CommandNames.Implementation,
CommandNames.ImplementationFull,
CommandNames.Exit,
Expand Down Expand Up @@ -341,7 +343,7 @@ namespace ts.server {
session.addProtocolHandler(command, () => resp);

expect(() => session.addProtocolHandler(command, () => resp))
.to.throw(`Protocol handler already exists for command "${command}"`);
.to.throw(`Protocol handler already exists for command "${command}"`);
});
});

Expand Down
21 changes: 20 additions & 1 deletion src/server/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,25 @@ namespace ts.server {
}));
}

getDefinitionAndBoundSpan(fileName: string, position: number): DefinitionInfoAndBoundSpan {
const args: protocol.FileLocationRequestArgs = this.createFileLocationRequestArgs(fileName, position);

const request = this.processRequest<protocol.DefinitionRequest>(CommandNames.DefinitionAndBoundSpan, args);
const response = this.processResponse<protocol.DefinitionInfoAndBoundSpanReponse>(request);

return {
definitions: response.body.definitions.map(entry => ({
containerKind: ScriptElementKind.unknown,
containerName: "",
fileName: entry.file,
textSpan: this.decodeSpan(entry),
kind: ScriptElementKind.unknown,
name: ""
})),
textSpan: this.decodeSpan(response.body.textSpan, request.arguments.file)
};
}

getTypeDefinitionAtPosition(fileName: string, position: number): DefinitionInfo[] {
const args: protocol.FileLocationRequestArgs = this.createFileLocationRequestArgs(fileName, position);

Expand Down Expand Up @@ -324,7 +343,7 @@ namespace ts.server {
}

getSyntacticDiagnostics(file: string): Diagnostic[] {
const args: protocol.SyntacticDiagnosticsSyncRequestArgs = { file, includeLinePosition: true };
const args: protocol.SyntacticDiagnosticsSyncRequestArgs = { file, includeLinePosition: true };

const request = this.processRequest<protocol.SyntacticDiagnosticsSyncRequest>(CommandNames.SyntacticDiagnosticsSync, args);
const response = this.processResponse<protocol.SyntacticDiagnosticsSyncResponse>(request);
Expand Down
12 changes: 12 additions & 0 deletions src/server/protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ namespace ts.server.protocol {
Definition = "definition",
/* @internal */
DefinitionFull = "definition-full",
DefinitionAndBoundSpan = "definitionAndBoundSpan",
/* @internal */
DefinitionAndBoundSpanFull = "definitionAndBoundSpan-full",
Implementation = "implementation",
/* @internal */
ImplementationFull = "implementation-full",
Expand Down Expand Up @@ -708,13 +711,22 @@ namespace ts.server.protocol {
file: string;
}

export interface DefinitionInfoAndBoundSpan {
definitions: ReadonlyArray<FileSpan>;
textSpan: TextSpan;
}

/**
* Definition response message. Gives text range for definition.
*/
export interface DefinitionResponse extends Response {
body?: FileSpan[];
}

export interface DefinitionInfoAndBoundSpanReponse extends Response {
body?: DefinitionInfoAndBoundSpan;
}

/**
* Definition response message. Gives text range for definition.
*/
Expand Down
Loading