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

Add support for Call Hierarchies in language server #35176

Merged
merged 7 commits into from
Dec 22, 2019
Merged

Conversation

rbuckton
Copy link
Member

This adds support for Call Hierarchies in the TypeScript language service and tsserver.

A Call Hierarchy can be requested on any "callable" named declaration:

  • Class Declarations
  • Class Expressions (with a name or assigned to a const variable)
  • Function Declarations
  • Function Expressions (with a name or assigned to a const variable)
  • Arrow Functions (assigned to a const variable)
  • Methods
  • Accessors

The call hierarchy item for a constructor is currently resolved as its containing class.

Incoming and outgoing calls to a Call Hierarchy item are inferred from the following call sites:

  • CallExpression (i.e. foo())
  • NewExpression (i.e. new C())
  • TaggedTemplateExpression (i.e. foo`tmpl`)
  • Property/element access on an accessor (i.e. obj.a)
  • Decorators (i.e. @foo class C { })
  • Jsx Elements (i.e. <MyCustomElement />)

Fixes #31863

@typescript-bot
Copy link
Collaborator

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary

@rbuckton
Copy link
Member Author

@mjbvz, @jrieken: I have an implementation of the TypeScript language feature for this for VS Code as well. I can submit a PR once this is shipping in a nightly.

@uniqueiniquity
Copy link
Contributor

What's the intent of the "prepare call hierarchy" command being distinct from the "callers" and "callees" commands?

@rbuckton
Copy link
Member Author

@uniqueiniquity a few reasons:

  1. This mirrors the VSCode API (including the LSP API).
  2. The language client can resolve the same call hierarchy item from multiple locations (the declaration and any reference to that declaration), without requesting the full set of results. It can use that information to maintain a cache of incoming/outgoing calls without needing to request all of the data for every location.

@uniqueiniquity
Copy link
Contributor

@rbuckton Makes sense to me - I wanted to make sure I understood fully.
My priority is making sure that this API stays aligned/compatible with what's likely to end up being the LSP equivalent, and the current proposal didn't seem to have the same notion of a distinct preparation command.

@rbuckton
Copy link
Member Author

As I understand it, this is the official VS Code API: microsoft/vscode#70231 (comment). There may be some differences with the LSP API

@uniqueiniquity
Copy link
Contributor

Ah, thanks for the pointer. Looks like LSP is appropriately looped in so that everything ends up aligned. Thanks!

@rbuckton
Copy link
Member Author

@uniqueiniquity
Copy link
Contributor

Yeah that's the proposal I linked earlier. It seems like the prepare call hasn't ended up there yet, but seems likely that it will before being finalized based on the input from you and VS Code.

@rbuckton
Copy link
Member Author

The API for the protocol is here: https://github.com/microsoft/vscode-languageserver-node/blob/0c46b297f8d1a6720edff43ba1618498108c098d/protocol/src/protocol.callHierarchy.proposed.ts#L122-L132

export interface CallHierarchyPrepareParams extends TextDocumentPositionParams, WorkDoneProgressParams {
}

export namespace CallHierarchyPrepareRequest {
	export const type = new RequestType<CallHierarchyPrepareParams, CallHierarchyItem | null, void, CallHierarchyRegistrationOptions>('textDocument/prepareCallHierarchy');
	export type HandlerSignature = RequestHandler<CallHierarchyPrepareParams, CallHierarchyItem | null, void>;
}

export interface CallHierarchyIncomingCallsParams extends WorkDoneProgressParams, PartialResultParams {
	item: CallHierarchyItem;
}

@rbuckton
Copy link
Member Author

I think the md file is 8 months out of date compared to the actual API.

# Conflicts:
#	src/harness/fourslashImpl.ts
#	src/services/utilities.ts
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

So the call hierarchy only goes one level up (incoming) and one level down (outgoing), right?

Is your VS Code PR up yet? It would be cool to see this in action.

@@ -259,6 +290,10 @@ namespace ts {
return node && node.parent && node.parent.kind === SyntaxKind.PropertyAccessExpression && (<PropertyAccessExpression>node.parent).name === node;
}

export function isArgumentExpressionOfElementAccess(node: Node) {
return node && node.parent && node.parent.kind === SyntaxKind.ElementAccessExpression && (<ElementAccessExpression>node.parent).argumentExpression === node;
Copy link
Member

Choose a reason for hiding this comment

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

  1. Can node actually be falsey, and if so why isn’t it typed that way?
  2. No optional chain? 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

I started this before optional chaining went in

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, isArgumentExpressionOfElementAccess was derived from the isRightSideOfPropertyAccess function just above it.

Copy link
Member Author

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

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

VS Code PR is actually waiting on this to ship in a nightly.

] = test.ranges();

goTo.marker();
verify.callHierarchy({
Copy link
Member

@weswigham weswigham Dec 17, 2019

Choose a reason for hiding this comment

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

So, hear me out, might it be nicer to baseline a graphical representation of the call heirarchy results, rather than all this explicit, manual shaping?

Something like verify.callHierarchy() which dumps

//// [/tests/cases/fourslash/callHeirarchyFunction.ts]
[r1|function [r2|foo|]() {
    [r3|bar|]();
}|]

[r4|function /**/[r5|bar|]() {
    [r6|baz|]();
    [r7|quxx|]();
    [r8|baz|]();
}|]

[r9|function [r10|baz|]() {
}|]

[r11|function [r12|quxx|]() {
}|]

//// [query result]
┏⮞ (function) foo
┃ name: r2
┃ selection: r1
┃ references: r3
┗┳⮞ (function) bar (selected)
 ┃ name: r5
 ┃ selection: r4
 ┗┳⮞ (function) baz
  ┃ name: r10
  ┃ selection: r9
  ┃ references: r6, r8
  ┗⮞ (function) quxx
    name: r12
    selection: r11
    references: r7
 

or something else like what the editor would show for the query into a baseline file? So the test file can just be

/// <reference path="fourslash.ts" />

//// function foo() {
////     bar();
//// }
////
//// function /**/bar() {
////     baz();
////     quxx();
////     baz();
//// }
////
//// function baz() {
//// }
////
//// function quxx() {
//// }

goTo.marker();
verify.callHierarchy();

Copy link
Member Author

Choose a reason for hiding this comment

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

I've switched the tests to baselines. Let me know what you think of the output format. Since I'm not using them anymore, should I just go ahead and get rid of the old verifyCallHierarchy... methods?

Copy link
Member

@weswigham weswigham Dec 19, 2019

Choose a reason for hiding this comment

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

Yeah, I guess so. I like the baseline format, but it could do with being a bit more... compact/diffable. I liked what I suggested above because the query result is pretty close to what the editor displays for the query (albeit with text instead of an icon for the kind), plus a bit of markup to link it back to the generated spans.

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 arrows don't show well in beyond compare, unfortunately. I also think the way I'm highlighting spans is easier to read than [r1|function|] etc.

src/server/session.ts Outdated Show resolved Hide resolved
@rbuckton
Copy link
Member Author

@uniqueiniquity, @weswigham: Any other comments? I'd like to get this in soon so I can put up a PR for VS Code as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call Hierarchy support for TypeScript
5 participants