GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Go to Implementation by riknoll · Pull Request #10482 · microsoft/TypeScript

Conversation

@riknoll

@riknoll Richard Knoll (riknoll) commented Aug 22, 2016

Copy link
Copy Markdown
Member

Fixes #6209

This PR adds "Go To Implementation" functionality to the typescript language service.

The code works as follows:

First, if a symbol's definition is the same as its implementation, then "Go to Definition" is invoked. This covers most symbols, but not classes, interfaces, or unions/intersections and their members.

For symbols where the implementation is not the same as the definition, it performs "Find all References" and then filters the results down to those that are implementations. If anything is not caught by "Find all References", it won't show up in the results.

For example:

interface Foo {
    hello(): void;
}

class Bar implements Foo {
    hello () {}
}

class Baz {
    hello () {}
}

Both Bar and Baz implement Foo, but only Bar will show up in the results of "Go to Implementation" when invoked on Foo. You'll get the same results if you use "Go to Implementation" on Foo.hello.

"Find all References" is based on a text search, so if something implements a given interface but does not do so explicitly (i.e. using the name of the thing being referenced), it won't show up in the results. In general, it's more useful to invoke "Go to Implementation" on a member of the interface instead of the interface itself, since the names of members always show up in the implementation.

Here are some examples that illustrate that:

// This implementation will show up if invoked on "Foo" or "Foo.hello" because "Foo" is in the type reference
let x: Foo = { hello: () => { console.log("Hello!"); } };

// This one will not show up if invoked on "Foo", because the string "Foo" isn't in it (but "Foo.hello" will work)
x = { hello: () => { console.log("Hi!"); } };

// This one will also not work with "Foo", but will work with "Foo.hello"
someFunctionThatTakesAFoo({ hello: () => { console.log("Aloha!"); } })

Still, I tried to capture most the usual patterns for creating instances of an interface. All of the following will work if invoked on Foo:

// Class declarations and expressions that implement the interface
class Bar implements Foo {
    hello() {}
}

// Variable-like assignments with type references to the interface
const x: Foo = { hello: () => {} };

// Object literals in return statements of functions with the correct return type
function createFoo(): Foo {
    return { hello: () => {} };
}

// Type assertion expressions
const y = <Foo> { hello: () => {} };

Another point of interest is the members of classes. If "Go to Implementation" is invoked on the member of a class, the results will also contain implementations in sub-classes. For example, if "Go to Implementation" was invoked on the reference x.hello below:

function someFunction(x: Bar) {
    x.hello();
}

class Bar extends BaseBar {}

// This implementation would show up, because it is the one used by Bar at runtime
class BaseBar {
    public hello() {}
}

// This implementation will also show up, because SubBar extends Bar
class SubBar extends Bar {
    public hello() {}
}

And there it is! I'm sure I missed some edge cases somewhere along the way, but they'll hopefully be caught once people start trying out the code. I tried the current implementation out on the TypeScript code base, and it seems to work pretty well. Implementations in JS files are not supported.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tinganho reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@riknoll
Go to Implementation
111e509
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Aug 22, 2016
This was referenced Aug 22, 2016
[tsserver] Support for "Search Hierarchy Type" #8268
Closed
Ctrl+Shift+T is not working for finding types in TypeScript projects angelozerr/typescript.java#29
Closed
yuit
yuit reviewed Aug 22, 2016
View reviewed changes
Comment thread src/server/protocol.d.ts
* Go to implementation request; value of command field is
* "implementation". Return response giving the file locations that
* implement the symbol found in file at location line, col.
*/

@yuit Yui (yuit) Aug 22, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

question: why you make this empty interface?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@riknoll Richard Knoll (riknoll) Aug 22, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I was following what TypeDefinitionRequest and DefinitionRequest did (see above).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@riknoll
PR Feedback
457b67f
mhegazy
mhegazy reviewed Aug 26, 2016
View reviewed changes
Comment thread src/services/services.ts Outdated
}

const referenceParentDeclarations = referenceSymbol.parent.getDeclarations();
if (referenceParentDeclarations.length) {

@mhegazy Mohamed Hegazy (mhegazy) Aug 26, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

you should be able to check referenceSymbol.parent.valueDeclaration here instead of length and first declaration

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mhegazy Mohamed Hegazy (mhegazy) Aug 26, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nevermind, you could just get the type for the getTypeOfSymbolAtLocation

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Richard Knoll (riknoll) added 3 commits August 29, 2016 16:12
@riknoll
Refine the search set instead of filtering to implementations
051c7b0
@riknoll
Also check inheritance for union and intersection types
115141c
@riknoll
Merge remote-tracking branch 'origin/master' into go_to_implementatio…
b0eff90
…n_pr
@riknoll

Richard Knoll (riknoll) commented Aug 30, 2016

Copy link
Copy Markdown
Member Author

Updated the PR to remove the filtering and instead filter the search set for find all references

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhegazy
mhegazy reviewed Sep 2, 2016
View reviewed changes
Comment thread src/services/services.ts Outdated
return true;
}

if (node.parent.kind === SyntaxKind.PropertyAccessExpression || node.parent.kind === SyntaxKind.ElementAccessExpression) {

@mhegazy Mohamed Hegazy (mhegazy) Sep 2, 2016 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

why not:

if (node.parent.kind === SyntaxKind.PropertyAccessExpression || node.parent.kind === SyntaxKind.ElementAccessExpression) {
     return definitionIsImplementation((<ElementAccessExpression | PropertyAccessExpression>node.parent).expression, typeChecker);
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mhegazy Mohamed Hegazy (mhegazy) Sep 2, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

forget about this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Richard Knoll (riknoll) added 5 commits September 6, 2016 14:11
@riknoll
Merge remote-tracking branch 'origin/master' into go_to_implementatio…
3ccc58c
…n_pr
@riknoll
PR feedback
66f30c9
@riknoll
Handle multiple return statements that implement interface
9c562f8
@riknoll
Merge remote-tracking branch 'origin/master' into go_to_implementatio…
b7071c1
…n_pr
@riknoll
Updating method name that changed in master
5913a35
@riknoll

Richard Knoll (riknoll) commented Sep 7, 2016

Copy link
Copy Markdown
Member Author

Mohamed Hegazy (@mhegazy) I've updated the branch with your last round of feedback

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhegazy
mhegazy reviewed Sep 7, 2016
View reviewed changes
Comment thread src/services/services.ts Outdated
}
}

function isClassOrInterfaceReference(toCheck: Symbol) {

@mhegazy Mohamed Hegazy (mhegazy) Sep 7, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

this is used in only one place. consider in lining it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ghost
ghost reviewed Sep 13, 2016
View reviewed changes
Comment thread src/services/services.ts Outdated
// symbol of the local type of the symbol the property is being accessed on. This is because our search
// symbol may have a different parent symbol if the local type's symbol does not declare the property
// being accessed (i.e. it is declared in some parent class or interface)
let parents: Symbol[];

@ghost Deleted user (ghost) Sep 13, 2016

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The compiler doesn't check this yet, but this is Symbol[] | undefined since it's only used if implementations is true, and so is its use in getRelatedSymbol, where it should be documented that it's only set for implementations.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mhegazy

Mohamed Hegazy (mhegazy) commented Sep 13, 2016

Copy link
Copy Markdown
Contributor

Other than comments from Andy (Andrewkraft) (@Andy-MS), no comments on my side.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@riknoll
More PR feedback
4a37fd7
@ghost

Deleted user (ghost) commented Sep 14, 2016

Copy link
Copy Markdown

👍

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@riknoll
More PR feedback
b6f7dd7
@riknoll

Richard Knoll (riknoll) commented Sep 14, 2016

Copy link
Copy Markdown
Member Author

Now that review is through, I'm going to close this PR and open two new ones: One that merges into release-2.0.5 and one that merges into master. Both of those will probably require some rebasing/git wizardry.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Richard Knoll (riknoll) added 3 commits September 14, 2016 17:25
@riknoll
Merge remote-tracking branch 'origin/master' into go_to_implementatio…
ea3752a
…n_pr

Refactored goToImplementation out of services
@riknoll
Fixing formatting
fd93699
@riknoll
Merge remote-tracking branch 'origin/master' into go_to_implementatio…
4eef417
…n_pr
@riknoll

Richard Knoll (riknoll) commented Sep 15, 2016

Copy link
Copy Markdown
Member Author

Okay, strike that. I'm leaving this PR open and have merged in master. This feature is not going to be merged into release-2.0.5. I factored the "go to implementation" stuff into its own file like we've done with the rest of the features, but I wonder if perhaps it should just be part of "find all references" since the majority of the implementation is in that file.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@riknoll
Merge remote-tracking branch 'origin/master' into go_to_implementatio…
29d85cd
…n_pr
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request Sep 15, 2016
Support Go-to-Implementation #9413
Closed
mhegazy
Mohamed Hegazy (mhegazy) approved these changes Sep 15, 2016
View reviewed changes

@mhegazy Mohamed Hegazy (mhegazy) left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@riknoll
Richard Knoll (riknoll) merged commit 50d243e into master Sep 15, 2016
@mhegazy
Mohamed Hegazy (mhegazy) deleted the go_to_implementation_pr branch September 20, 2016 22:34
@kkovalevskiy Kirill Kovalevskiy (kkovalevskiy) mentioned this pull request Dec 15, 2016
Feature Suggestion: Go to implementation microsoft/vscode#10806
Closed
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@mhegazy Mohamed Hegazy (mhegazy) mhegazy approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@riknoll @mhegazy @yuit @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.