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

VS Go to definition from C# to F# #14377

Merged
merged 21 commits into from Jan 9, 2023

Conversation

vzarytovskii
Copy link
Member

@vzarytovskii vzarytovskii commented Nov 23, 2022

Note

Please note, that this is a a simple and naive implementation for navigation to F# symbol from C# project (Fixes #13882).

It does not yet support parameters-based navigation for methods, it will select just first implementation in case if there are overloads. This to be fixed in the next version.

Not entirely sure how to properly test it just yet. If you have better idea, please do share.

It selects target project based on assembly name, and searches symbol based on its DocumentCommentId, which has the following formats:

T: prefix for types

  • T:N.X.Nested - type
  • T:N.X.D - delegate

M: prefix is for methods

  • M:N.X.#ctor - constructor
  • M:N.X.#ctor(System.Int32) - constructor with one parameter
  • M:N.X.f - method with unit parameter
  • M:N.X.bb(System.String,System.Int32@) - method with two parameters
  • M:N.X.gg(System.Int16[],System.Int32[0:,0:]) - method with two parameters, 1d and 2d array
  • M:N.X.op_Addition(N.X,N.X) - operator
  • M:N.X.op_Explicit(N.X)~System.Int32 - operator with return type
  • M:N.GenericMethod.WithNestedType``1(N.GenericType{``0}.NestedType) - generic type with one parameter
  • M:N.GenericMethod.WithIntOfNestedType``1(N.GenericType{System.Int32}.NestedType) - generic type with one parameter
  • M:N.X.N#IX{N#KVP{System#String,System#Int32}}#IXA(N.KVP{System.String,System.Int32}) - explicit interface implementation

E: prefix for events

  • E:N.X.d.

F: prefix for fields

  • F:N.X.q - field

P: prefix for properties

  • P:N.X.prop - property with getter and setter

Supported scenarios:

  • Simple functions
  • Events, fields and properties
  • Constructors
  • Types
  • Generic functions
  • Overloaded methods, based on type parameters.
  • Operators
  • Records
  • Discriminated unions

The following are out of scope for this 1st implementation:

  • Overloaded methods/ctors based on method parameters.
    • Right now it will navigate to the 1st it finds.

Next steps:

  • In next iteration, we utilize IStreamingFindUsagesPresenter when we can't distinguish which exact method/function to go to.
  • We should utilize/port Roslyn's DocumentCommentId parser to fetch exact types for parameters and use them for lookup.

@vzarytovskii vzarytovskii marked this pull request as ready for review November 28, 2022 15:39
@vzarytovskii vzarytovskii requested a review from a team as a code owner November 28, 2022 15:39
@vzarytovskii
Copy link
Member Author

This is ready for the first iteration, please see comments in the OP, regarding tests and what's supported.

let mutable locations = Seq.empty

for project in projects do
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably do this in parallel.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a fool-proof, we expect it always be only a single project, parallel then will give you a bunch of overhead

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, didn't notice where it's coming from. Is it even theoretically possible there will be multiple projects with the same assembly name? Because maybe you could replace filter with tryFind and it would be a bit simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it even theoretically possible there will be multiple projects with the same assembly name?

Very unlikely, I have never seen it before.

Because maybe you could replace filter with tryFind and it would be a bit simpler.

I'll see how it looks with option

Copy link
Contributor

Choose a reason for hiding this comment

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

With option it could look something like this (After also adding DocCommentId.EntityPath):

let project = workspace.CurrentSolution.Projects |> Seq.tryFind (fun p -> p.IsFSharp && p.AssemblyName = assemblyName)
let locations = 
    project
    |> Option.map (fun project -> 
        let! checker, _, _, options = project.GetFSharpCompilationOptionsAsync(cancellationToken)
        let! result = checker.ParseAndCheckProject(options)
        let entity = path.EntityPath |> result.AssemblySignature.FindEntityByPath
        match path with
        | DocCommentId.Member ({ MemberOrValName = memberOrVal; GenericParameters = genericParametersCount }, memberType) ->
            entity |> tryFindVal memberOrVal documentationCommentId memberType genericParametersCount
        | DocCommentId.Field { MemberOrValName = memberOrVal } ->
            entity |> tryFindFieldByName memberOrVal
        | DocCommentId.Type _ -> 
            Seq.singleton entity.DeclarationLocation
        | DocCommentId.None -> 
            Seq.empty    
        |> Seq.map (fun m -> (m, project)))
    |> Option.defaultValue Seq.empty

A bit easier to see what's going on with arrows only going in one direction :)

let entity = result.AssemblySignature.FindEntityByPath (entityPath)
match entity with
| Some e ->
Copy link
Member Author

Choose a reason for hiding this comment

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

probably should replace with Option.map/iter or something

@psfinaki
Copy link
Member

Annoying question - any idea/chance to test this?

@vzarytovskii
Copy link
Member Author

Annoying question - any idea/chance to test this?

No, I've asked the same question in the first paragraph

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Looks pretty good, no tests yet though. (That's rich coming from me :-))

@vzarytovskii
Copy link
Member Author

Looks pretty good, no tests yet though. (That's rich coming from me :-))

Yeah, that's the question - I'm not yet entirely sure how to do that.

@vzarytovskii
Copy link
Member Author

Ok, this is ready, I failed (gave up really) to add proper tests in headless VS, but added to the important part - parser for doccomment id.
I suggest we merge it as is and test it, otherwise PR will go stale for longer time.

@vzarytovskii vzarytovskii changed the title WIP VS Go to definition from C# to F# VS Go to definition from C# to F# Jan 5, 2023
let mutable locations = Seq.empty

for project in projects do
Copy link
Contributor

Choose a reason for hiding this comment

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

With option it could look something like this (After also adding DocCommentId.EntityPath):

let project = workspace.CurrentSolution.Projects |> Seq.tryFind (fun p -> p.IsFSharp && p.AssemblyName = assemblyName)
let locations = 
    project
    |> Option.map (fun project -> 
        let! checker, _, _, options = project.GetFSharpCompilationOptionsAsync(cancellationToken)
        let! result = checker.ParseAndCheckProject(options)
        let entity = path.EntityPath |> result.AssemblySignature.FindEntityByPath
        match path with
        | DocCommentId.Member ({ MemberOrValName = memberOrVal; GenericParameters = genericParametersCount }, memberType) ->
            entity |> tryFindVal memberOrVal documentationCommentId memberType genericParametersCount
        | DocCommentId.Field { MemberOrValName = memberOrVal } ->
            entity |> tryFindFieldByName memberOrVal
        | DocCommentId.Type _ -> 
            Seq.singleton entity.DeclarationLocation
        | DocCommentId.None -> 
            Seq.empty    
        |> Seq.map (fun m -> (m, project)))
    |> Option.defaultValue Seq.empty

A bit easier to see what's going on with arrows only going in one direction :)

@vzarytovskii vzarytovskii merged commit 006d7c2 into dotnet:main Jan 9, 2023
@vzarytovskii vzarytovskii deleted the vs-external-goto-definition branch January 9, 2023 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

C# to F# go to definition navigates to metadata, and not the source code
5 participants