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

Go to Implementation #10482

Merged
merged 19 commits into from
Sep 15, 2016
Merged

Go to Implementation #10482

merged 19 commits into from
Sep 15, 2016

Conversation

riknoll
Copy link
Member

@riknoll riknoll commented Aug 22, 2016

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.

* "implementation". Return response giving the file locations that
* implement the symbol found in file at location line, col.
*/
export interface ImplementationRequest extends FileLocationRequest {
Copy link
Contributor

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?

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 was following what TypeDefinitionRequest and DefinitionRequest did (see above).

}

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

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

Copy link
Contributor

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

@riknoll
Copy link
Member Author

riknoll commented Aug 30, 2016

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

return true;
}

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

@mhegazy mhegazy Sep 2, 2016

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);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

forget about this.

@riknoll
Copy link
Member Author

riknoll commented Sep 7, 2016

@mhegazy I've updated the branch with your last round of feedback

}
}

function isClassOrInterfaceReference(toCheck: Symbol) {
Copy link
Contributor

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.

// 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[];
Copy link

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.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 13, 2016

Other than comments from @andy-ms, no comments on my side.

@ghost
Copy link

ghost commented Sep 14, 2016

👍

@riknoll
Copy link
Member Author

riknoll commented Sep 14, 2016

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.

@riknoll
Copy link
Member Author

riknoll commented Sep 15, 2016

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.

Copy link
Contributor

@mhegazy mhegazy left a comment

Choose a reason for hiding this comment

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

👍

@riknoll riknoll merged commit 50d243e into master Sep 15, 2016
@mhegazy mhegazy deleted the go_to_implementation_pr branch September 20, 2016 22:34
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants