I'm getting unwanted navigation when holding `Ctrl` and hovering over operators like `&&` or `>`. Not sure if caused by this PR or a bug in VS.

[vzarytovskii]

I'm getting unwanted navigation when holding Ctrl and hovering over operators like && or >. Not sure if caused by this PR or a bug in VS.

Originally posted by @kerams in https://github.com/dotnet/fsharp/pull/17047#issuecomment-2068051467

[majocha]

Just hovering over external symbol, (not just operators) while holding Ctrl causes VS (Roslyn?) to call IFSharpGoToDefinitionService.FindDefinitionsAsync.

Which in turn calls NavigateToExternalDeclarationAsync:

https://github.com/dotnet/fsharp/blob/6420e36b89d05c54f49d8fa81f9b4922dd0a8fa0/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs#L775-L789

Somehow this doesn't happen that often with vsix built from main, but I can repro this in debug mode each time.

[vzarytovskii]

Yeah, that's definitely either VS or Roslyn. @CyrusNajmabadi is it something you can control?

[CyrusNajmabadi]

Are they clicking while holding Ctrl? If so, this is by design. Ctrl-click navigates you to the thing under the caret.

I believe there is a setting for this in vs if the user doesn't like that behavior.

[vzarytovskii]

Are they clicking while holding Ctrl? If so, this is by design. Ctrl-click navigates you to the thing under the caret.

I believe there is a setting for this in vs if the user doesn't like that behavior.

I believe it's just hovering, @majocha may confirm/deny it.

[majocha]

Yes, it's just hover.

More interestingly, if you enable "View > Code Definition Window", the unwanted navigation can happen just with typing.

For example:

Type Console.Out. Move the cursor back with left-arrow, and bam, navigation to metadata.

I suspect the problem is that our code path for external symbols ends up calling NavigateToExternalDeclarationAsync instead of just returning a FSharpNavigableItem array.

VS calls IFSharpGoToDefinitionService.FindDefinitionsAsync -> the symbol is external -> we end up here:

https://github.com/dotnet/fsharp/blob/6420e36b89d05c54f49d8fa81f9b4922dd0a8fa0/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs#L784

[hyazinthh]

This happens to me since 17.11 a lot. I had to disable the Ctrl + click shortcut because VS became unusable otherwise. Not sure if this is the same issue, it doesn't seem to be related to just operators in my case.

[vzarytovskii]

@CyrusNajmabadi @tmat FYI - I also am seeing it more on VS main, it seems Roslyn calls us when no explicit user action is performed.

[majocha]

I went through the code and I think it's because we do open a text window during FindDefinitionsAsync.

Repro:

• Put a breakpoint here:

https://github.com/dotnet/fsharp/blob/6cd4d598f6ab714e7598c886ccb44cbfae187de6/vsintegration/src/FSharp.Editor/LanguageService/MetadataAsSource.fs#L102

or

https://github.com/dotnet/fsharp/blob/6cd4d598f6ab714e7598c886ccb44cbfae187de6/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs#L604

and here

https://github.com/dotnet/fsharp/blob/6cd4d598f6ab714e7598c886ccb44cbfae187de6/vsintegration/src/FSharp.Editor/Navigation/GoToDefinitionService.fs#L21-L22

• Start debug instance.

• Type System.Console in some F# source window.

• Ctrl + hover over it.

FindDefinitionsAsync is being called on hover, and a window shows up. It appears we're expected to just return NavigableItem and let VS / Roslyn do the navigation?

[vzarytovskii]

I went through the code and I think it's because we do open a text window during FindDefinitionsAsync.

Repro:

• Put a breakpoint here:

https://github.com/dotnet/fsharp/blob/6cd4d598f6ab714e7598c886ccb44cbfae187de6/vsintegration/src/FSharp.Editor/LanguageService/MetadataAsSource.fs#L102

or

https://github.com/dotnet/fsharp/blob/6cd4d598f6ab714e7598c886ccb44cbfae187de6/vsintegration/src/FSharp.Editor/Navigation/GoToDefinition.fs#L604

and here

https://github.com/dotnet/fsharp/blob/6cd4d598f6ab714e7598c886ccb44cbfae187de6/vsintegration/src/FSharp.Editor/Navigation/GoToDefinitionService.fs#L21-L22

• Start debug instance.

• Type System.Console in some F# source window.

• Ctrl + hover over it.

FindDefinitionsAsync is being called on hover, and a window shows up.

It appears we're expected to just return NavigableItem and let VS / Roslyn do the navigation?

Sigh...this was a fix because Roslyn started calling our new interface and not old one, since we use our own implementation for decompiled F# code, that was the only way of opening it.

[vzarytovskii]

This was the change - https://github.com/dotnet/fsharp/pull/17047

[majocha]

This is a bit confusing, Roslyn has two interfaces that both declare FindDefinitionsAsync method with the same signature:

IFSharpFindDefinitionService

https://github.com/dotnet/roslyn/blob/08a167c19e5e04742b0922bdb1ea8046e9364f4b/src/VisualStudio/ExternalAccess/FSharp/GoToDefinition/IFSharpFindDefinitionService.cs#L12-L17

and

IFSharpGoToDefinitionService

https://github.com/dotnet/roslyn/blob/08a167c19e5e04742b0922bdb1ea8046e9364f4b/src/VisualStudio/ExternalAccess/FSharp/Editor/IFSharpGoToDefinitionService.cs#L14-L19

We implement both.

I just wonder if the correct one is getting called. Clearly Roslyn (or VS) calls IFSharpGoToDefinitionService on hover, but we don't want to GTD on hover. Maybe it meant IFSharpFindDefinitionService.FindDefinitionsAsync instead? A mistake?

[majocha]

OK, now I get it. IFSharpGoToDefinitionService is the old service being replaced. Related Roslyn issue: Transition F# and TS to IFindDefinitionService

[vzarytovskii]

OK, now I get it. IFSharpGoToDefinitionService is the old service being replaced.

Related Roslyn issue: Transition F# and TS to IFindDefinitionService

Yeah, I had to make it work for the new one, and I think one difference was that I couldn't pass down a callback to Roslyn which would be called to open metadata document.

[majocha]

It seems Roslyn's implementation doesn't deal with showing windows or any navigation at all: https://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/GoToDefinition/AbstractFindDefinitionService.cs,17 It just returns INavigableItems,

I suspect we already have everything in place to fix this with some refactoirng, we have a Document from metadata and we generate navItems for it in NavigateToExternalDeclarationAsync.