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

Jump to bottom

Add containerName to CallHierarchyItem #38997

Merged
merged 3 commits into from
Jun 10, 2020
Merged

Add containerName to CallHierarchyItem #38997

merged 3 commits into from
Jun 10, 2020

Conversation

andrewbranch
Copy link
Member

Adds a containerName to CallHierarchyItems:

  • For class methods and accessors (static or instance), uses the class declaration/expression’s name (if it exists)
  • For object literal methods and accessors, uses the assigned name of the object literal (if it exists)
  • For top-level function- and class-like declarations in a module declaration, uses the module name (if it is an identifier)

For all other CallHierarchyItems, containerName will be undefined. Note that fileName is already included separately on all items.

Fixes #37061

@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

@andrewbranch andrewbranch requested a review from mjbvz June 9, 2020 18:58
weswigham
weswigham approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Do we know what the ux displaying this info in vscode looks like yet?

src/services/callHierarchy.ts
@@ -129,6 +129,31 @@ namespace ts.CallHierarchy {
return { text, pos: declName.getStart(), end: declName.getEnd() };
}

function getCallHierarchItemContainerName(node: CallHierarchyDeclaration): string | undefined {
if (isConstNamedExpression(node)) {
if (isModuleBlock(node.parent.parent.parent.parent) && isIdentifier(node.parent.parent.parent.parent.parent.name)) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to do this many unguarded parent accesses?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes—isConstNamedExpression guarantees that the third-to-last .parent is a VariableDeclarationList or CatchClause, so the second-to-last is VariableStatement | ForStatement | ForOfStatement | ForInStatement | TryStatement, so one more .parent is safe from there.

@andrewbranch
Copy link
Member Author

Do we know what the ux displaying this info in vscode looks like yet?

I personally don’t—@mjbvz or @jrieken might.

@weswigham
Copy link
Member

Should we have @typescript-bot pack this and let @mjbvz see if this is sufficient for him?

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 9, 2020 •
edited
Loading

Heya @weswigham, I've started to run the tarball bundle task on this PR at 6e0e065. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 9, 2020 •
edited
Loading

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/75992/artifacts?artifactName=tgz&fileId=93734AFBAEBEA15867056B43628A1D184D6537936F3F5C109503EEAC4CFC556F02&fileName=/typescript-4.0.0-insiders.20200609.tgz"
    }
}

and then running npm install.

There is also a playground for this build.

@mjbvz
Copy link
Contributor

mjbvz commented Jun 9, 2020

Here's a quick example of how VS Code could render this:

Screen Shot 2020-06-09 at 4 28 10 PM

The container name would be shown where src is in this example screenshot

mjbvz
mjbvz approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

Protocol changes look good to me. Thanks for looking into this!

@andrewbranch andrewbranch merged commit 852e7a0 into microsoft:master Jun 10, 2020
@andrewbranch andrewbranch deleted the feature/37061 branch June 10, 2020 18:56
cangSDARM added a commit to cangSDARM/TypeScript that referenced this pull request Jun 15, 2020
@cangSDARM
Merge remote-tracking branch 'upstream/master'
09801d4 
* upstream/master: (22 commits)
  Small fix in `getIsContextSensitiveAssignmentOrContextType`
  Simplify `visitObjectLiteralExpression`
  Fix handling of `aruments` in the emitter
  Fix declaration emit for property references of imported object literal types (microsoft#39055)
  Fix casing for wild card keys for implicit globs to get wild card directories to watch (microsoft#39049)
  DOM update 2020-06-12
  fix(a11y): make ISSUE_TEMPLATE/Bug_report.md more accessible for folks with screen readers (microsoft#39013)
  Update request-pr-review script to latest version of octokit (microsoft#39031)
  pin version of octokit
  skip implements types with no symbols
  isDynamicName skips parentheses for element access
  Allow `e: unknown` in `catch` arguments
  Serialize (noncontextual) keyword named namespace members with export declarations in both declaration emitters (microsoft#38982)
  Patch to use this.timeout() > 0 rather than this.enableTimeout() to work with mocha 8+
  Handle missing return type nodes and nested type references missing type arguments in existing jsdoc node serialization (microsoft#39011)
  Add containerName to CallHierarchyItem (microsoft#38997)
  Fix isSameEntityName (microsoft#38999)
  Fix typo for 'blocklist' (microsoft#39001)
  remove errant tab
  Switch to isSymbolAccessible for both.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sheetalkamat sheetalkamat sheetalkamat left review comments

@weswigham weswigham weswigham approved these changes

@mjbvz mjbvz mjbvz approved these changes

@rbuckton rbuckton Awaiting requested review from rbuckton

Assignees

@andrewbranch andrewbranch

Labels
Author: Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add container information to CallHierarchyItem
5 participants
@andrewbranch @typescript-bot @weswigham @mjbvz @sheetalkamat