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 C# symbols #3357

Merged
merged 28 commits into from Aug 21, 2017

Conversation

Projects
None yet
9 participants
@vasily-kirichenko
Contributor

vasily-kirichenko commented Jul 21, 2017

This is saul@dad4934

1

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

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jul 21, 2017

Contributor

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!? :)

Contributor

dsyme commented Jul 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 21, 2017

Contributor

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?

Contributor

forki commented Jul 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Jul 21, 2017

Contributor

Is this something [anyone] is working on?

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

Contributor

dsyme commented Jul 21, 2017

Is this something [anyone] is working on?

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

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 21, 2017

Contributor

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!

Contributor

forki commented Jul 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@saul

saul Jul 21, 2017

Contributor

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

Contributor

saul commented Jul 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@saul

saul Jul 21, 2017

Contributor

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.

Contributor

saul commented Jul 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Jul 21, 2017

Contributor

@saul any chance you remember that interface name?

Contributor

vasily-kirichenko commented Jul 21, 2017

@saul any chance you remember that interface name?

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Jul 21, 2017

Contributor

@Pilchie can you help?

Contributor

vasily-kirichenko commented Jul 21, 2017

@Pilchie can you help?

@saul

This comment has been minimized.

Show comment
Hide comment
@saul

saul Jul 21, 2017

Contributor

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

Contributor

saul commented Jul 21, 2017

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

vasily-kirichenko added some commits Jul 21, 2017

@saul

This comment has been minimized.

Show comment
Hide comment
@saul

saul Jul 22, 2017

Contributor

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

Contributor

saul commented Jul 22, 2017

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

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Jul 22, 2017

Contributor

@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.

Contributor

vasily-kirichenko commented Jul 22, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Jul 22, 2017

Contributor

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...

Contributor

vasily-kirichenko commented Jul 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Jul 22, 2017

Contributor

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

Contributor

vasily-kirichenko commented Jul 22, 2017

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

@saul

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Jul 22, 2017

Contributor

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

Contributor

vasily-kirichenko commented Jul 22, 2017

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

@saul

This comment has been minimized.

Show comment
Hide comment
@saul

saul Jul 22, 2017

Contributor

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.

Contributor

saul commented Jul 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Jul 22, 2017

Contributor

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

Contributor

vasily-kirichenko commented Jul 22, 2017

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

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko
Contributor

vasily-kirichenko commented Jul 22, 2017

image

@saul

This comment has been minimized.

Show comment
Hide comment
@saul

saul Jul 22, 2017

Contributor

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

Contributor

saul commented Jul 22, 2017

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

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Jul 22, 2017

Contributor

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

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

Contributor

vasily-kirichenko commented Jul 22, 2017

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

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

Merge pull request #11 from saul/go-to-csharp-declaration
Jump to property definition instead of getter/setter
@saul

This comment has been minimized.

Show comment
Hide comment
@saul

saul Aug 20, 2017

Contributor

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?

Contributor

saul commented Aug 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Aug 20, 2017

Contributor

Go to props works great 👍

Contributor

vasily-kirichenko commented Aug 20, 2017

Go to props works great 👍

@realvictorprm

This comment has been minimized.

Show comment
Hide comment
@realvictorprm

realvictorprm Aug 20, 2017

Contributor
Contributor

realvictorprm commented Aug 20, 2017

saul and others added some commits Aug 20, 2017

Merge pull request #12 from saul/go-to-csharp-declaration
Add ExternalSymbol to FSharp.Compiler.Service.fsproj
@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Aug 21, 2017

Contributor

@dotnet-bot test Release_ci_part3 please

Contributor

vasily-kirichenko commented Aug 21, 2017

@dotnet-bot test Release_ci_part3 please

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Aug 21, 2017

Contributor

@dotnet-bot test this please

Contributor

vasily-kirichenko commented Aug 21, 2017

@dotnet-bot test this please

saul and others added some commits Aug 21, 2017

Merge pull request #13 from saul/patch-1
Fix FSharp.Compiler.Private.BuildFromSource.fsproj
let fail defaultReason =
match item with
| Item.CtorGroup (_, (ILMeth (_,ilinfo,_)) :: _) ->

This comment has been minimized.

@dsyme

dsyme Aug 21, 2017

Contributor

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

dsyme Aug 21, 2017

Contributor

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

dsyme approved these changes Aug 21, 2017

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

@cartermp

This comment has been minimized.

Show comment
Hide comment
@cartermp

cartermp Aug 21, 2017

Collaborator

@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.

Collaborator

cartermp commented Aug 21, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko
Contributor

vasily-kirichenko commented Aug 21, 2017

@dsyme done.

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Aug 21, 2017

Contributor

@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

Contributor

dsyme commented Aug 21, 2017

@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

@KevinRansom KevinRansom merged commit 9606209 into Microsoft:master Aug 21, 2017

6 checks passed

Ubuntu14.04 Release Build Build finished.
Details
Windows_NT Debug Build Build finished.
Details
Windows_NT Release_ci_part1 Build Build finished.
Details
Windows_NT Release_ci_part2 Build Build finished.
Details
Windows_NT Release_ci_part3 Build Build finished.
Details
Windows_NT Release_net40_no_vs Build Build finished.
Details
@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko
Contributor

vasily-kirichenko commented Aug 21, 2017

:)

@realvictorprm

This comment has been minimized.

Show comment
Hide comment
@realvictorprm

realvictorprm Aug 21, 2017

Contributor
Contributor

realvictorprm commented Aug 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment