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

Create a new GoToTypeDefinition endpoint #2315

Merged
merged 15 commits into from
Mar 5, 2022

Conversation

jtschuster
Copy link
Contributor

@jtschuster jtschuster commented Dec 21, 2021

Creates a new endpoint to go the definition of the type of a local variable, parameter, field, or property. The implementation borrows heavily from the GotoDefinition endpoint. Fixes issue #2297

This endpoint should be helpful in C# workflow, especially when using implicitly typed variables that don't have the name of the type anywhere in the file. This also provides an endpoint to use with the VS Code command editor.action.goToTypeDefinition.

Example:
file1.cs:

class C {
    C NamedConstructor() { return new C(); }
}

file2.cs:

class D {
    void method() 
    {
        var variableName = NamedConstructor();
    }
}

GoToTypeDefinition at the position of variableName would return the definition of the class C in file1.cs.

@jtschuster
Copy link
Contributor Author

@filipw @JoeRobich You seem to be the most active in this repository, could you take a look or let me know if there's anything I need to do for a PR?

@JoeRobich
Copy link
Member

@filipw @JoeRobich You seem to be the most active in this repository, could you take a look or let me know if there's anything I need to do for a PR?

I will take a look at this next week.

@jtschuster
Copy link
Contributor Author

@JoeRobich Just checking back in. No rush, just don't want this to get lost.

Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@@ -77,6 +77,8 @@ public static class V2
public const string Highlight = "/v2/highlight";

public const string GotoDefinition = "/v2/gotodefinition";

public const string GotoTypeDefinition = "/v2/gototypedefinition";
Copy link
Member

Choose a reason for hiding this comment

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

@filipw Since this is the first iteration of this endpoint, is it ok to be under V2?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should just be /gototypedefinition, we do not version at API level but on endpoint level.
for example /completion is the modern version of intellisense and it was not versioned as v2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, it's updated accordingly

[ImportingConstructor]
public GotoTypeDefinitionV2Handler(
OmniSharpWorkspace workspace,
MetadataExternalSourceService metadataExternalSourceService)
Copy link
Member

@filipw filipw Mar 2, 2022

Choose a reason for hiding this comment

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

I think this should use ExternalSourceServiceFactory to also cover cases when decompilation is enabled

Copy link
Member

@JoeRobich JoeRobich Mar 2, 2022

Choose a reason for hiding this comment

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

This file seems to be mostly a cut and paste of the GotoDefinitionV2Handler. Perhaps we can update all the Navigation handlers as a follow up.

Copy link
Contributor Author

@jtschuster jtschuster Mar 4, 2022

Choose a reason for hiding this comment

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

This file seems to be mostly a cut and paste of the GotoDefinitionV2Handler.

Yeah, it was a copy and paste. I've updated to use the ExternalSourceServiceFactory.

continue;
}

var aliasLocations = await GotoTypeDefinitionHandlerHelper.GetAliasFromMetadataAsync(
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure you can assume that metadata will be used, because the user might have enabled decompilation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip, I've updated to use the IExternalSourceService from the ExternalSourceServiceFactory.

Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Thanks @jtschuster!

@filipw filipw merged commit 6aace68 into OmniSharp:master Mar 5, 2022
@filipw
Copy link
Member

filipw commented Mar 5, 2022

thanks!

jtschuster added a commit to jtschuster/omnisharp-vscode that referenced this pull request Mar 6, 2022
PR OmniSharp/omnisharp-roslyn#2315 add a gototypedefinition endpoint. This PR adds a provider that utilizes the endpoint.
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.

3 participants