Go to C# symbols - #3357

vasily-kirichenko · 2017-07-21T08:07:19Z

This is saul@dad4934

@majocha any ideas how to make the tooltip links work as well for C# symbols?

dsyme · 2017-07-21T08:48:05Z

Oh wow. A while back I promised a bounty (ok, a beer, a book, whatever) to whoever did this :) But do you get it or @saul!? :)

forki · 2017-07-21T08:50:26Z

this is seriously nice, but the other direction is even more important since many projects want to write domain logic in F# and UI in C#. AFAIK this would require mapping the F# TAST to roslyn TAST. Is this something the F# team or the roslyn team is working on?

dsyme · 2017-07-21T08:54:10Z

Is this something [anyone] is working on?

I don't think I've seen anyone on any team prototype this

forki · 2017-07-21T08:56:53Z

sad since this was one of the promised things when the roslyn topic came up in VF# ;-) "we could benefit from the infrastructure and the roslyn team would help ..." ;-)

Anyway nice to see one direction is at least possible! Kudos!

saul · 2017-07-21T08:58:14Z

I don't think this is much different to what I committed months ago - you can see by the number of 'todos' this isn't nearly complete! Thanks to @vasily-kirichenko for bringing it up to date

saul · 2017-07-21T09:31:09Z

It's actually not too difficult to implement it "the other way" without us having to move everything over to the Roslyn symbol API. There's a legacy interface that is used as a fallback if Roslyn can't find a symbol - I can't find the exact name but it should be simple for us to implement.

vasily-kirichenko · 2017-07-21T10:06:04Z

@saul any chance you remember that interface name?

vasily-kirichenko · 2017-07-21T10:06:46Z

@Pilchie can you help?

saul · 2017-07-21T10:21:29Z

I can't - it was @CyrusNajmabadi who told me.

saul · 2017-07-22T08:36:54Z

@vasily-kirichenko I've found Cyrus's comment detailing how to go the other way: dotnet/roslyn#16511 (comment)

vasily-kirichenko · 2017-07-22T09:30:46Z

@saul I added a IVsSymbolicNavigationNotify impl, but it's not called on F12.

@CyrusNajmabadi any ideas? I've failed to find an example of how to expose this interface anywhere.

vasily-kirichenko · 2017-07-22T09:49:59Z

Ah, looks like Roslyn query IVsSymbolicNavigationNotify casting IVsHierarchy to it, so it seems we should implement the interface in one of node classes in ProjectSystem...

vasily-kirichenko · 2017-07-22T13:31:15Z

I tried to implement it in our ProjectNode class, the methods are not called either.

saul · 2017-07-22T13:38:44Z

Google is your friend :) You have to register it: https://msdn.microsoft.com/en-us/library/microsoft.visualstudio.shell.interop.vssymbolicnavigationmanagerclass.registersymbolicnavigationnotify.aspx

vasily-kirichenko · 2017-07-22T14:24:29Z

serviceProvider.GetService<VsSymbolicNavigationManagerClass, IVsSymbolicNavigationManager>() returns null being called in FSharpGoToDefinitionService ctor :(

saul · 2017-07-22T18:08:43Z

According to docs.microsoft.com, "This interface is available via QueryService(Type, Object)." It looks like there's a difference between GetService and QueryService - try QueryService instead.

vasily-kirichenko · 2017-07-22T18:39:14Z

@saul I've tried QueryService, it returns non-zero code.

vasily-kirichenko · 2017-07-22T18:48:06Z

saul · 2017-07-22T19:01:43Z

Looks like the manager isn't used in Roslyn - it's pulled off the IVsHierarchy of the open document? Or something similar:

https://github.com/dotnet/roslyn/blob/6847f1e5a909395aae9456e8f366cbf4deb86b69/src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioSymbolNavigationService.cs#L172

vasily-kirichenko · 2017-07-22T19:02:12Z

However, according to Roslyn sources, it does not use the manager, it gets Project from Document:

https://github.com/dotnet/roslyn/blob/master/src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioSymbolNavigationService.cs#L300

then tries to cast it to IVsSymbolicNavigationNotify:

https://github.com/dotnet/roslyn/blob/master/src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioSymbolNavigationService.cs#L284

vasily-kirichenko · 2017-07-22T19:03:51Z

:) Yes. See my comment #3357 (comment)

I implemented IVsSymbolicNavigationNotify in ProjectNode, but it was not called.

saul · 2017-08-20T19:11:56Z

To whoever reviews this (@dsyme/@KevinRansom), it's worth pointing out that I want to add a test suite to this. I want to use the code that I've been demoing in the gifs, as it's a complete real-world use of this functionality. How best can I integrate it into our test suite?

vasily-kirichenko · 2017-08-20T19:13:06Z

Go to props works great 👍

realvictorprm · 2017-08-20T19:45:33Z

Awesome! Am 20.08.2017 9:13 PM schrieb "Vasily Kirichenko" <notifications@github.com

…

: Go to props works great 👍 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3357 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYPM8Hs64hn-77rFScEWBc7fRVRnIIVHks5saIVGgaJpZM4OfFig> .

Add ExternalSymbol to FSharp.Compiler.Service.fsproj

vasily-kirichenko · 2017-08-21T07:22:43Z

@dotnet-bot test Release_ci_part3 please

vasily-kirichenko · 2017-08-21T07:23:09Z

@dotnet-bot test this please

Fix FSharp.Compiler.Private.BuildFromSource.fsproj

dsyme · 2017-08-21T13:02:00Z

+    | Array of inner: ExternalType
+    | Pointer of inner: ExternalType
+    | TypeVar of typeName: string
+with


Remove with please

dsyme · 2017-08-21T13:02:22Z

+            None
+
+[<RequireQualifiedAccess>]
+type ExternalType =


Add a /// comment please, also to each union case

dsyme · 2017-08-21T13:02:51Z

+type ParamTypeSymbol =
+    | Param of ExternalType
+    | Byref of ExternalType
+with


Remove with please

dsyme · 2017-08-21T13:03:08Z

+    | Field of typeName: string * name: string
+    | Event of typeName: string * name: string
+    | Property of typeName: string * name: string
+with


Remove with please

dsyme · 2017-08-21T13:03:34Z

+    | Array of inner: ExternalType
+    | Pointer of inner: ExternalType
+    | TypeVar of typeName: string
+with


Remove withs please

dsyme · 2017-08-21T13:06:03Z

-
-              let fail defaultReason = 
-                  match item with            
+                  | Item.CtorGroup (_, (ILMeth (_,ilinfo,_)) :: _) ->


Typically, we should use property accessors on ILMeth rather than pattern matching on its contents. It's not curcial though - we can clean up the code later

dsyme · 2017-08-21T13:06:29Z

        let task = tcs.Task
        let disposeReg() = barrier.Stop(); if not task.IsCanceled then reg.Dispose()
        Async.StartWithContinuations(
-                  async { do! Async.SwitchToThreadPool()


Is this a cosmetic whitespace change?

dsyme

I've left minor review comments - it looks great!

More unit testing would be appreciated but I'm OK with merging as @cartermp has tested many cases manually

…ation

cartermp · 2017-08-21T15:33:59Z

@dsyme Do you have a recommended place where unit tests could go here? It feels like a good opportunity to create a new test project specifically for the language service, rather than adding on to the older tests which happen to test the language service alongside other changes.

vasily-kirichenko · 2017-08-21T16:00:51Z

@dsyme done.

dsyme · 2017-08-21T16:03:38Z

@dsyme Do you have a recommended place where unit tests could go here?

I believe adding tests to tests\service\... would make sense

Alternatively vsintegration\tests\unittests\GoToDefinitionServiceTests.fs

vasily-kirichenko · 2017-08-21T17:32:37Z

:)

realvictorprm · 2017-08-21T17:56:24Z

Nice Am 21.08.2017 7:32 PM schrieb "Vasily Kirichenko" <notifications@github.com

…

: :) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3357 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYPM8GoxGcoUIkV34gDSZXkkS6SoJCzXks5sab86gaJpZM4OfFig> .

saul · 2017-08-21T18:06:07Z

+                      | :? OperationCanceledException -> 
+                          tcs.TrySetCanceled(cancellationToken)  |> ignore
+                      | exn ->
+                          System.Diagnostics.Trace.WriteLine("Visual F# Tools: exception swallowed and not passed to Roslyn: {0}", exn.Message)


Nooooo 😱 This wasn't a whitespace change - the old code (that you've reverted to) didn't assert on failure, and Trace.WriteLine doesn't take a format string! This bit me for a while. I'll open another PR fixing this.

Oh, sorry, man :(

* go to C# symbols * fix tests * cleanup * fix tests * support field, events and constructors * fix tests * implement IVsSymbolicNavigationNotify * cleanup * fix * cleanup * Improve package definition specs fro F# VS Package * Fixed some bugs with nested types with fields/events * fix compilation * use AppTy AP * Add support for overloaded methods, generic methods, generic (nested) classes, constructors * Jump to property definition instead of getter/setter * Add ExternalSymbol to FSharp.Compiler.Service.fsproj * Fix FSharp.Compiler.Private.BuildFromSource.fsproj * address code review

go to C# symbols

a6841cd

msftclas added the cla-already-signed label Jul 21, 2017

vasily-kirichenko added 5 commits July 21, 2017 15:40

fix tests

1873524

cleanup

1492a70

fix tests

44fc79e

support field, events and constructors

c95ca11

fix tests

12cb8ae

implement IVsSymbolicNavigationNotify

d10c5d6

cleanup

bfdd846

saul and others added 2 commits August 20, 2017 22:09

Add ExternalSymbol to FSharp.Compiler.Service.fsproj

d486c58

Merge pull request #12 from saul/go-to-csharp-declaration

9f159f0

Add ExternalSymbol to FSharp.Compiler.Service.fsproj

saul and others added 2 commits August 21, 2017 09:18

Fix FSharp.Compiler.Private.BuildFromSource.fsproj

67dd9a2

Merge pull request #13 from saul/patch-1

3202db2

Fix FSharp.Compiler.Private.BuildFromSource.fsproj

dsyme reviewed Aug 21, 2017

View reviewed changes

dsyme approved these changes Aug 21, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into go-to-csharp-declar…

94747c7

…ation

address code review

8f22343

KevinRansom approved these changes Aug 21, 2017

View reviewed changes

KevinRansom merged commit 9606209 into dotnet:master Aug 21, 2017

saul reviewed Aug 21, 2017

View reviewed changes

iainnicol mentioned this pull request Aug 24, 2017

Go To Definition: C# to F# #3497

Closed

jannesiera mentioned this pull request Feb 7, 2019

Goto Definition F# to C# and Vice Versa dotnet/roslyn#8105

Closed

Uh oh!

Conversation

vasily-kirichenko commented Jul 21, 2017

Uh oh!

dsyme commented Jul 21, 2017

Uh oh!

forki commented Jul 21, 2017

Uh oh!

dsyme commented Jul 21, 2017

Uh oh!

forki commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saul commented Jul 21, 2017

Uh oh!

saul commented Jul 21, 2017

Uh oh!

vasily-kirichenko commented Jul 21, 2017

Uh oh!

vasily-kirichenko commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saul commented Jul 21, 2017

Uh oh!

saul commented Jul 22, 2017

Uh oh!

vasily-kirichenko commented Jul 22, 2017

Uh oh!

vasily-kirichenko commented Jul 22, 2017

Uh oh!

vasily-kirichenko commented Jul 22, 2017

Uh oh!

saul commented Jul 22, 2017

Uh oh!

vasily-kirichenko commented Jul 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saul commented Jul 22, 2017

Uh oh!

vasily-kirichenko commented Jul 22, 2017

Uh oh!

vasily-kirichenko commented Jul 22, 2017

Uh oh!

saul commented Jul 22, 2017

Uh oh!

vasily-kirichenko commented Jul 22, 2017

Uh oh!

vasily-kirichenko commented Jul 22, 2017

Uh oh!

saul commented Aug 20, 2017

Uh oh!

vasily-kirichenko commented Aug 20, 2017

Uh oh!

realvictorprm commented Aug 20, 2017 via email

Uh oh!

vasily-kirichenko commented Aug 21, 2017

Uh oh!

vasily-kirichenko commented Aug 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyme left a comment

Choose a reason for hiding this comment

Uh oh!

cartermp commented Aug 21, 2017

Uh oh!

vasily-kirichenko commented Aug 21, 2017

Uh oh!

forki commented Jul 21, 2017 •

edited

Loading

vasily-kirichenko commented Jul 21, 2017 •

edited

Loading

vasily-kirichenko commented Jul 22, 2017 •

edited

Loading

dsyme commented Aug 21, 2017 •

edited

Loading