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

Call Hierarchy support for TypeScript #31863

Closed
RyanCavanaugh opened this issue Jun 11, 2019 · 10 comments · Fixed by #35176
Closed

Call Hierarchy support for TypeScript #31863

RyanCavanaugh opened this issue Jun 11, 2019 · 10 comments · Fixed by #35176
Assignees
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@RyanCavanaugh
Copy link
Member

See microsoft/vscode#16110

@vvakame
Copy link
Contributor

vvakame commented Oct 25, 2019

what's changes in this issue? not Find All References ?

@mjbvz
Copy link
Contributor

mjbvz commented Oct 28, 2019

@rbuckton When I talked this over this @jrieken from the VS Code team, it sounds like we will need a specialized TypeScript server api to implement this correctly and efficiently. Determining outgoing calls in particular is very difficult using current TSServer apis. Can we please revisit this for 3.8

@DanielRosenwasser DanielRosenwasser added In Discussion Not yet reached consensus and removed Committed The team has roadmapped this issue labels Oct 31, 2019
@DanielRosenwasser
Copy link
Member

Hey @jrieken, what sort of things were insufficient with the current approach? It'd help to get an idea so we can make sure those are tested.

@mjbvz
Copy link
Contributor

mjbvz commented Oct 31, 2019

A few thing that I know of:

  • Determining outgoing calls of a function is difficult
  • Using references, we can't distinguish function calls from other references to that function

@DanielRosenwasser
Copy link
Member

I suppose that it would be hard to understand every call-like behavior (tagged templates, decorators, function calls, new invocations, etc.

@rbuckton
Copy link
Member

rbuckton commented Nov 13, 2019

@mjbvz, @jrieken: I've been working on the TSServer API for call hierarchies, and here's what I've come up with:

export const enum CommandTypes {
    ...
    PrepareCallHierarchy = "prepareCallHierarchy",
    ProvideCallHierarchyIncomingCalls = "provideCallHierarchyIncomingCalls",
    ProvideCallHierarchyOutgoingCalls = "provideCallHierarchyOutgoingCalls",    
}
export interface CallHierarchyItem {
    name: string;
    kind: ScriptElementKind;
    detail: string;
    file: string;
    span: TextSpan;
    selectionSpan: TextSpan;
}

export interface CallHierarchyIncomingCall {
    from: CallHierarchyItem;
    fromSpans: TextSpan[];
}

export interface CallHierarchyOutgoingCall {
    to: CallHierarchyItem;
    fromSpans: TextSpan[];
}

export interface PrepareCallHierarchyRequest extends FileLocationRequest {
    command: CommandTypes.PrepareCallHierarchy;
}

export interface PrepareCallHierarchyResponse extends Response {
    readonly body: CallHierarchyItem;
}

export interface ProvideCallHierarchyIncomingCallsRequest extends FileLocationRequest {
    command: CommandTypes.ProvideCallHierarchyIncomingCalls;
}

export interface ProvideCallHierarchyIncomingCallsResponse extends Response {
    readonly body: CallHierarchyIncomingCall[];
}

export interface ProvideCallHierarchyOutgoingCallsRequest extends FileLocationRequest {
    command: CommandTypes.ProvideCallHierarchyOutgoingCalls;
}

export interface ProvideCallHierarchyOutgoingCallsResponse extends Response {
    readonly body: CallHierarchyOutgoingCall[];
}

Rather than round-tripping a CallHierarchyItem as would be done in the VS Code API, VS Code would send TSServer the file and location of the declaration related to the CallHierarchyItem. We can then use that to determine the declaration from which to find incoming and outgoing calls.

I've built a prototype of both the VS Code support for the TSServer API, as well as the TSServer implementation itself, though I do have a few additional questions:

Files in the Call Hierarchy

How should a SourceFile be represented as a CallHierarchyItem? In TS/JS, its frequently the case that the caller of a function is the source file itself:

function foo() { }
foo();

I am including source files in the prototype I have built, however I'm not certain of the best way to represent the "name" of the call hierarchy item:

image

Call Hierarchy Item Details

The comment for the details property of a CallHierarchyItem indicates it should be used for something like a signature. Given that this is merely a string, what is the intended format? Since TS/JS functions/methods differ only in signature and not in implementation, does providing the signature even make sense here?

Items to show in Call Hierarchy

This has been brought up in microsoft/vscode#84100, but I'll mention it here as well. TS/JS have several different calling conventions we should be aware of:

  • CallExpression (i.e. foo())
  • NewExpression (i.e. new C())
  • TaggedTemplateExpression (i.e. foo`text`)
  • PropertyAccessExpression/ElementAccessExpression (when the reference is an accessor).
  • Decorator (i.e. @dec class C { })
  • JsxElement (i.e. <Foo />)
    image

Method Signatures

Should we support Call Hierarchy on Method Signatures in interfaces? They obviously wouldn't have outgoing calls, but they could have incoming calls.

The VS version of Call Hierarchy does seem to support this (though they also include things such as places that a member is overridden or implemented as well):

image

@rbuckton
Copy link
Member

Also, a CallHierarchyItem can only represent a single declaration, however TypeScript supports merging interfaces and function signatures across disparate files (either through global symbol merging or module augmentation). How should we represent the CallHierarchyItem for the following cases:

// file1.d.ts
interface Shared {
  foo(): void; // merges with `Shared` in file2
}
declare function bar(): void; // merges with `bar` in file2

// file2.d.ts
interface Shared {
  foo(): void; // merges with `Shared` in file1
}
declare function bar(): void; // merges with `bar` in file1

// file3.ts
declare const x: Shared;
x.foo(); // Which `foo` should we pick for the call hierarchy item's range/selection range?
bar(); // Which `bar` should we pick for the call hierarchy item's range/selection range?

@jrieken
Copy link
Member

jrieken commented Nov 14, 2019

Rather than round-tripping a CallHierarchyItem as would be done in the VS Code API, VS Code would send TSServer the file and location of the declaration related to the CallHierarchyItem

We actually do the same - internally each item is associated with an ID and only that goes over the wire. For the API we wanted to be more expressive and give implementor a chance to store data subsequent requests in an item.

I'm not certain of the best way to represent the "name" of the call hierarchy item:

That's a good question. I think the base-filename, e.g main.ts and SymbolKind.File would be a good start. Maybe put the rest of the path as detail?

Call Hierarchy Item Details

Not sure tbh - the details field is a bit of a free form field for extensions to add "something" useful. We have the same for outline and there TS isn't using it, some extension are. Generally, it will be rendered as is, no MD formatting or so.

Should we support Call Hierarchy on Method Signatures in interfaces?

👍 I think that's useful in order to see how an interface is being used.

How should we represent the CallHierarchyItem for the following cases:

🤷‍♂ though question... for calls each item could just be a call but when starting/preparing it's a little more trick... We might need to evolve our API so that prepare can return N items.

@rbuckton
Copy link
Member

That's a good question. I think the base-filename, e.g main.ts and SymbolKind.File would be a good start. Maybe put the rest of the path as detail?

Is there an API I can leverage to format the path in an OS-specific way? As you can see from the screenshot above, it's currently using TypeScript's internal path representation (c:/dev/tests) as opposed to a Windows-specific representation (C:\dev\tests). It would also be nice if this were easy to be made relative to the workspace. I know this is something you already do for resource Urls in the tree view and to deduplicate editor tabs.

@jrieken
Copy link
Member

jrieken commented Nov 15, 2019

Yeah, we have asRelativePath which should be able to handle that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants