Added DefinitionAndBoundSpan command #19175

armanio123 · 2017-10-13T22:55:17Z

Adds a new command DefinitionAndBoundSpan that will return all definitions for a given position plus the TextSpan.

The purpose of this is for allowing editors to highlight the token being defined.

mhegazy · 2017-10-13T23:22:00Z

src/services/services.ts

+
+            const sourceFile = getValidSourceFile(fileName);
+            const node = getTouchingPropertyName(sourceFile, position, /*includeJsDocCcomment*/ false);
+


what if this position is within a comment? do you want to verify that it is at least a token? identifier? can you elaborate on the scenario here?

Added JsDoc for comments, based on our discussion I had changed the design of the command. This is no longer valid.

mhegazy · 2017-10-13T23:22:54Z

src/services/services.ts

+
+            const sourceFile = getValidSourceFile(fileName);
+            const node = getTouchingPropertyName(sourceFile, position, /*includeJsDocCcomment*/ false);
+


also SpanForPosition seems a bit vauge to me.. is this TokenSpanAtPosition or NodeSpanAtPosition or IdentiiferTokenSpanAtPosition?

No longer valid as the method has been removed.

amcasey · 2017-10-13T23:31:29Z

src/harness/harnessLanguageService.ts

@@ -490,6 +490,9 @@ namespace Harness.LanguageService {
        getSpanOfEnclosingComment(fileName: string, position: number, onlyMultiLine: boolean): ts.TextSpan {
            return unwrapJSONCallResult(this.shim.getSpanOfEnclosingComment(fileName, position, onlyMultiLine));
        }
+        getSpanForPosition(): ts.TextSpan {
+            throw new Error("Not supportred on the shim.");


Typo: "supported" #Closed

amcasey · 2017-10-13T23:32:40Z

src/harness/unittests/session.ts

@@ -175,6 +175,7 @@ namespace ts.server {
                CommandNames.Configure,
                CommandNames.Definition,
                CommandNames.DefinitionFull,
+                CommandNames.DefinitionAndBoundSpan,


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

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)

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

amcasey · 2017-10-14T00:58:09Z

src/services/services.ts

@@ -1807,6 +1807,15 @@ namespace ts {
            return range && createTextSpanFromRange(range);
        }

+        function getSpanForPosition(fileName: string, position: number): TextSpan {
+            synchronizeHostData();


synchronizeHostData(); [](start = 12, length = 22)

Why synchronize? #Closed

No longer valid as the method has been removed.

I still find the synchronization surprising, but I see that the existing GTD code path does it. #Closed

amcasey · 2017-10-14T00:58:36Z

src/services/services.ts

+            synchronizeHostData();
+
+            const sourceFile = getValidSourceFile(fileName);
+            const node = getTouchingPropertyName(sourceFile, position, /*includeJsDocCcomment*/ false);


false [](start = 96, length = 5)

Do we support GTD in doc comments? (I don't know, but I believe C# does.) #Closed

We do support it, fixed.

amcasey

🕐

amcasey · 2017-10-23T17:28:06Z

src/server/session.ts

+            };
+        }
+
+        private getSimplifiedTextSpan(scriptInfo: ScriptInfo, textSpan: TextSpan): protocol.TextSpan {


getSimplifiedTextSpan [](start = 16, length = 21)

Should this be called in more places? #Closed

Maybe, there must be some other places where a "simplified textSpan" might be required and this method could be reused. I didn't check for that thou.

In any case, this was refactored to reuse in two different places. line 628 and line 641.

Refactored for reusability. Also based on a conversation with @mhegazy, changed a couple of method names.

amcasey · 2017-10-23T17:30:25Z

src/services/goToDefinition.ts

+        const comment = findReferenceInPosition(sourceFile.referencedFiles, position);
+        if (comment && tryResolveScriptReference(program, sourceFile, comment) || findReferenceInPosition(sourceFile.typeReferenceDirectives, position)) {
+            return { definitions, textSpan: undefined };
+        }


What does this block do? Is it for ctrl-click on the file path in a triple-slash reference? Is the TODO still relevant? #Closed

This block checks if the position we're searching is within a comment and if that comment is a script reference (triple slash). If true we don't search for a textSpan as the method for that involves a comment parser and complicates a little bit more the process, so undefined is returned instead for now.

The TODO is relevant because of this. Ctrl + Click is going to be enabled with definitions but no textSpan for underlining it is implemented yet.

Added bug #19447 for tracking this.

amcasey · 2017-10-23T17:36:13Z

src/harness/fourslash.ts

+            const defs = getDefs();
+            let definitions: ts.DefinitionInfo[] | ReadonlyArray<ts.DefinitionInfo>;
+            let testName: string;
+            if (this.isDefinitionInfoAndBoundSpan(defs)) {


Would it be easier to just check isArray? #Closed

It would be the same IMO, if anything maybe a little bit harder to read.

First, we need to compare against undefined as well making the comparison a little bit bigger. Second, we'll need to use a type assertion instead of a type guard.

isArray has the advantage of already existing. And I believe (but don't know) that code flow type inference will make type assertions unnecessary (essentially, any[] & (foo[] | bar) => foo[]). #Closed

Yes, isArray will narrow the type, but due to the undefined check it will not be able to do it. In any case, I have added the modification as it's no big deal on readability and like you mentioned already exists and no custom guards are necessary.

amcasey · 2017-10-23T17:37:35Z

src/harness/fourslash.ts

+        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;


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

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

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

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

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.

amcasey

Still have questions.

mhegazy · 2017-10-24T20:30:39Z

src/harness/fourslash.ts

@@ -584,18 +584,23 @@ namespace FourSlash {

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


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

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

Changed as suggested.

mhegazy · 2017-10-24T20:33:59Z

src/server/session.ts

+            return definitionAndBoundSpan;
+        }
+
+        private mapFileSpan(definitions: ReadonlyArray<DefinitionInfo>, project: Project): ReadonlyArray<protocol.FileSpan> {


consider renaming to mapDefinitionInfo

mhegazy · 2017-10-24T20:35:25Z

src/server/session.ts

+
+            return {
+                file: fileName,
+                ...this.toLocationTextSpan(textSpan, scriptInfo)


get the location first, then add the fileName. this way you avoid the extra object allocation per entry.

Changed to add the textspan inline

mhegazy · 2017-10-24T20:36:23Z

src/server/session.ts

+            return definitions.map(def => this.getFileSpan(def.fileName, def.textSpan, project));
+        }
+
+        private getFileSpan(fileName: string, textSpan: TextSpan, project: Project): protocol.FileSpan {


consider renaming it to toFileSpan

mhegazy · 2017-10-24T20:37:16Z

src/server/session.ts

                    file: fileName,
                    isWriteAccess,
+                    ...this.toLocationTextSpan(textSpan, scriptInfo)


this will cause an extra object allocation for every occurrence. we use this on every hover, and do not want to increase the amount of garbage we create.

Rollback to be inline.

mhegazy · 2017-10-24T20:39:03Z

src/server/session.ts

-                    end: scriptInfo.positionToLineOffset(textSpanEnd(edit.span)),
-                    newText: edit.newText ? edit.newText : ""
+                    newText: edit.newText ? edit.newText : "",
+                    ...this.toLocationTextSpan(edit.span, scriptInfo)


this one too.

Rollback to be inline.

mhegazy · 2017-10-24T20:39:19Z

src/server/session.ts

                            const bakedItem: protocol.NavtoItem = {
                                name: navItem.name,
                                kind: navItem.kind,
                                file: navItem.fileName,
-                                start,
-                                end,
+                                ...this.toLocationTextSpan(navItem.textSpan, scriptInfo)


Rollback to be inline.

mhegazy · 2017-10-24T20:39:42Z

src/services/goToDefinition.ts

+        // TODO: Add textSpan for triple slash references (file and type).
+        const comment = findReferenceInPosition(sourceFile.referencedFiles, position);
+        if (comment && tryResolveScriptReference(program, sourceFile, comment) || findReferenceInPosition(sourceFile.typeReferenceDirectives, position)) {
+            return { definitions, textSpan: undefined };


i would make the textSpan the full line.

The code has changed so this is no longer valid. The new one has it full line.

mhegazy · 2017-10-24T20:44:31Z

src/services/goToDefinition.ts

+        }
+
+        // TODO: Add textSpan for triple slash references (file and type).
+        const comment = findReferenceInPosition(sourceFile.referencedFiles, position);


i think this should be:

const comment = findReferenceInPosition(sourceFile.referencedFiles, position) || findReferenceInPosition(sourceFile.typeReferenceDirectives, position);

mhegazy · 2017-10-24T20:45:59Z

src/services/goToDefinition.ts

+
+        // TODO: Add textSpan for triple slash references (file and type).
+        const comment = findReferenceInPosition(sourceFile.referencedFiles, position);
+        if (comment && tryResolveScriptReference(program, sourceFile, comment) || findReferenceInPosition(sourceFile.typeReferenceDirectives, position)) {


do not think you need to call tryResolveScriptReference here. just make sure definitions is not empty, which you already checked earlier.

…o123/TypeScript into AddDefinitionAndBoundSpan

mhegazy · 2017-10-24T21:24:26Z

src/services/goToDefinition.ts

+        }
+
+        let comment = findReferenceInPosition(sourceFile.referencedFiles, position);
+        if (!comment || !tryResolveScriptReference(program, sourceFile, comment)) {


why do you need the resolve call?

No need. Removed the resolve call.

amcasey

mhegazy · 2017-10-31T22:28:36Z

//cc @mjbvz

Added DefinitionAndBoundSpan command

1cb2d24

mhegazy reviewed Oct 13, 2017

View reviewed changes

amcasey reviewed Oct 13, 2017

View reviewed changes

Fixed api reference tests

c6a8a32

amcasey reviewed Oct 14, 2017

View reviewed changes

amcasey suggested changes Oct 14, 2017

View reviewed changes

armanio123 added 3 commits October 16, 2017 17:50

Changed command designed based on review input

b86153d

Addressed PR comments: added simplified/full version, changed design

8004fec

Updated an incredible amount of tests.

16c3255

amcasey reviewed Oct 23, 2017

View reviewed changes

armanio123 added 2 commits October 23, 2017 16:03

Removed custom guard and added isArray

051da11

Renamed a couple of methods, refactored code for reusability

f8ccde5

armanio123 mentioned this pull request Oct 24, 2017

Ctrl+Click (CommandNames.DefinitionAndBoundSpan) doesn't return a location TextSpan for triple slash references #19447

Closed

Merge branch 'master' into AddDefinitionAndBoundSpan

ae266f6

mhegazy suggested changes Oct 24, 2017

View reviewed changes

armanio123 added 2 commits October 24, 2017 14:02

Added triple slash support

7f577dc

Merge branch 'AddDefinitionAndBoundSpan' of https://github.com/armani…

c62f288

…o123/TypeScript into AddDefinitionAndBoundSpan

mhegazy reviewed Oct 24, 2017

View reviewed changes

armanio123 added 4 commits October 24, 2017 15:58

Rollback spread operator changes

7fd9fe6

Fixed tsserverlibrary.d.ts tests

5b7abf2

Addressed PR comments.

f3059ce

Removed double tests of gotoDefinition

c4a675e

mhegazy mentioned this pull request Oct 27, 2017

Add sourceRange to TS Server definition response #19518

Closed

amcasey approved these changes Oct 30, 2017

View reviewed changes

mhegazy approved these changes Oct 30, 2017

View reviewed changes

Merge branch 'master' into AddDefinitionAndBoundSpan

d5c18a6

armanio123 merged commit cf2bdf8 into microsoft:master Oct 31, 2017

mjbvz mentioned this pull request Oct 31, 2017

Extend language API to allow sourceRange in Go to Definition microsoft/vscode#10037

Closed

microsoft locked and limited conversation to collaborators Jun 14, 2018


		const sourceFile = getValidSourceFile(fileName);
		const node = getTouchingPropertyName(sourceFile, position, /includeJsDocCcomment/ false);

		@@ -584,18 +584,23 @@ namespace FourSlash {

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

Added DefinitionAndBoundSpan command #19175

Added DefinitionAndBoundSpan command #19175

Conversation

armanio123 commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

amcasey Oct 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

amcasey Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

mhegazy commented Oct 31, 2017

amcasey Oct 13, 2017 •

edited

Loading

amcasey Oct 13, 2017 •

edited

Loading

amcasey Oct 14, 2017 •

edited

Loading

amcasey Oct 23, 2017 •

edited

Loading

amcasey Oct 14, 2017 •

edited

Loading

amcasey Oct 23, 2017 •

edited

Loading

amcasey Oct 23, 2017 •

edited

Loading

amcasey Oct 23, 2017 •

edited

Loading

amcasey Oct 23, 2017 •

edited

Loading

amcasey Oct 23, 2017 •

edited

Loading