Clickable QuickInfo with "go to type" #2574

majocha · 2017-03-10T12:45:36Z

Reading unfamiliar code with scarce type annotations I often wish I could click the types in quickinfo to go directly to some definition.

I put together a proof of concept. The code is hacky, usability is not that good, but it's a start and shows that a lot is possible :)

EDIT
Changed links to show when mouse enters the QuickInfo.
Linked(shared among many projects) files also are handled.
Added small tooltip showing file path on hover over link.

msftclas · 2017-03-10T12:45:40Z

@majocha,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

smoothdeveloper · 2017-03-10T12:54:20Z

That is totally awesome :)

vasily-kirichenko · 2017-03-10T12:56:38Z

OMG it's awesome! I've seen such functionality only once, in Nemerle VS integration.

vasily-kirichenko · 2017-03-10T12:58:21Z

Is it possible to make Quick Info hierarchical? I mean if you hover over a symbol on a tooltip, another tooltip appears and so on.

smoothdeveloper · 2017-03-10T12:58:47Z

In future we could have right click / find usages as well?

vasily-kirichenko · 2017-03-10T13:01:00Z

src/fsharp/TastOps.fs

    | Some pathText -> pathText +.+ nmF xref

 let tagEntityRefName (xref: EntityRef) name =
+    let rng = Some ( box xref.DefinitionRange )


Why we need to box here?

ah, because it's a hack. extra info passed around is just obj option because I initially wasn't sure what's needed to navigate.

majocha · 2017-03-10T13:01:01Z

@vasily-kirichenko I guess it is. It's all just a WPF FrameworkElement. With some work put in sky is the limit.

vasily-kirichenko · 2017-03-10T13:01:23Z

src/utils/sformat.fs

-        | Delegate of string
-        | Enum of string
-        | Event of string
+        | Alias of obj option * string


Why not put range here?

I'm not sure if it is defined and accessible yet here?

Yeah, it's defined later. Maybe introduce a new type just for this? I feel myself (very) uncomfortable looking at type erasure.

Yes. Even if you need the hack you can use type BoxRange = BoxRange of obj and then it's very easy to check the casts in/out of the type are valid

vasily-kirichenko · 2017-03-10T13:05:10Z

You committed ValudTuple.dll again :)

majocha · 2017-03-10T13:06:10Z

@vasily-kirichenko dang it!

vasily-kirichenko · 2017-03-10T13:10:07Z

src/utils/sformat.fs

    module TaggedTextOps =
 #endif
-        let tagAlias = TaggedText.Alias
+        let private blank name = None, name


no need for private here because there it has signature file.

vasily-kirichenko · 2017-03-10T13:11:30Z

src/utils/sformat.fsi

+        val tagUnionCase : (string -> TaggedText)
+        val tagDelegate : (string -> TaggedText)
+        val tagEnum : (string -> TaggedText)
+        val tagEvent : (string -> TaggedText)


Why these parens? Whether add them to all the functions or remove everywhere.

They were needed for some reason. Something about function values...

vasily-kirichenko · 2017-03-10T13:14:19Z

vsintegration/src/FSharp.Editor/QuickInfo/FSharpDeferredContent.fs

+open System.Windows
+open Microsoft.CodeAnalysis.Editor
+open Microsoft.CodeAnalysis.Editor.Shared.Utilities
+open Microsoft.CodeAnalysis


the last is unused.

vasily-kirichenko · 2017-03-10T13:15:02Z

vsintegration/src/FSharp.Editor/QuickInfo/FSharpDeferredContent.fs

+        for NavigableRoslynText(tag, text, xopt) in content do
+            let rangeOpt =
+                match xopt with
+                | Some xref ->


xopt and xref are very puzzling names.

majocha · 2017-03-10T13:27:07Z

@vasily-kirichenko thanks for the review. I'll clean it up and work some more soon.

And I need to find a way to get rid of ValueTuple :)

vasily-kirichenko · 2017-03-10T13:51:46Z

vsintegration/src/FSharp.Editor/QuickInfo/FSharpDeferredContent.fs

+
+                    let h = Documents.Hyperlink(Documents.Run(text))
+                    h.Click.Add <| fun _ ->
+                        Logging.Logging.logInfof "click! %A" range


Remove logging, please. It will appear on VS output pane even if vsix is built in release configuration.

vasily-kirichenko · 2017-03-10T13:56:21Z

vsintegration/src/FSharp.Editor/QuickInfo/QuickInfoProvider.fs

+        let documentNavigationService =workspace.Services.GetService<IDocumentNavigationService>()
+        let filePath = System.IO.Path.GetFullPathSafe range.FileName
+        match workspace.CurrentSolution.GetDocumentIdsWithFilePath(filePath) |> List.ofSeq with
+        | id :: _ ->


If file is linked to several projects, it's possible to jump to the wrong document. We need to find a document for which FSharpSymbol.Assembly.SimpleName = document.Project.AssemblyName.

This means that we have to pass assembly name alongside with range.

See in my PR here https://github.com/Microsoft/visualfsharp/pull/2552/files#diff-ddd3e07525bd2cd12359212293648353R74

This one is tricky. It seems workspace will not open another editor if we already have a file opened in IDE from the wrong project. But you're right, main concern is to not open from wrong project during navigation.
Range doesn't carry assembly info but we have the Symbol because we're in the context of quickinfo, so it's doable.

dsyme · 2017-03-10T14:53:38Z

src/utils/sformat.fs

-        let tagEvent = TaggedText.Event
+
+        let tagClass name = if Set.contains name keywordTypes then TaggedText.Keyword name else TaggedText.Class(None, name)
+        let tagUnionCase = blank >> TaggedText.UnionCase


If you want to avoid the parentheses in the signature file then just use an explicit argument rather than >>

dsyme · 2017-03-10T15:30:42Z

vsintegration/src/FSharp.Editor/QuickInfo/FSharpDeferredContent.fs

+                | Some xref ->
+                    match xref with
+                    | :? Microsoft.FSharp.Compiler.Range.range as range 
+                      when range.FileName <> "startup" ->


Please make "startup" a named constant in the compiler and replace all its occurrences in the compiler codebase with a reference to that constant?

There already is one in defined in TcGlogals.fs :) but I use a value Range.rangeStartup instead of the string now.

vasily-kirichenko · 2017-03-15T05:25:08Z

@majocha I think this PR is ready? Remove WIP from the title, please.

cloudRoutine · 2017-03-15T05:29:04Z

@vasily-kirichenko I don't think it's worthwhile to revert it as the default behavior, adding an option to disable it is plenty.

majocha · 2017-03-15T06:54:47Z

@Pilchie this PR uses Roslyn implementation of QuickInfo controller, only rendering is augmented. Tooltip should show up and disappear exactly as it normally does.

majocha · 2017-03-15T08:27:05Z

@Pilchie to address your first question, about ctrl+click extension:
"Go to definition" is not always helpful in F#, where return type and parameter types of a function are often inferred.
When there is a type annotation it would be a matter of two clicks: go to definition, go to definition on a type. But I was often getting stuck browsing F# code when there was no type annotation at the definition.

olegtk · 2017-03-15T17:08:37Z

The editor also supports advanced interactive QuickInfo popup if you want more control over its behavior. This is how "Show potential fixes" link and floating LightBulb in QuickInfo over an error squiggle are implemented.
https://msdn.microsoft.com/en-us/library/microsoft.visualstudio.language.intellisense.iinteractivequickinfocontent.aspx

saul · 2017-03-15T22:34:55Z

vsintegration/src/FSharp.Editor/QuickInfo/FSharpDeferredContent.fs

+                | Some(range) when canGoto range ->
+                    let h = Documents.Hyperlink(Documents.Run(text), ToolTip = range.FileName)
+                    h.Click.Add <| fun _ ->
+                        Forms.SendKeys.Send("{ESC}") //TODO: Dismiss QuickInfo properly, maybe using IQuickInfoSession.Dismiss()


Can we fix this hack before merging?

saul · 2017-03-15T22:35:47Z

src/utils/sformat.fs

    open ReflectionAdapters
 #endif

+    [<NoEquality; NoComparison>]


This still makes me really uneasy. I don't know what @KevinRansom and @dsyme think of this - surely there's a way we can sort this out without having to resort to 'obj'. We're just completely subverting the type system here and saying 'trust me I know what I'm doing'.

I guess this is fixable, by moving range file on it's own (if it doesn't contain too much stuff as well), maybe try a PR on this PR which just moves the files in the relevant fsprojs if that's doable.

I think the hack is fine (looking at unboxRange and its usage) but it needs an explicit comment where the type is defined as well as in unboxRange, and having a statement (beside the picture from @vasily-kirichenko) that this move was attempted but seemed disruptive would be good too.

Sure, I understand the rationale, but it's easy to move the file to be on its own. There's no need for us to hack around like this. Just move the file up to the top of the project - like you said it's only a small type.

@saul I'm ok with moving the file unless it proves very difficult, it's totally fair to push back on this kind of hack.

This file is a bit odd because it also forms part of FSHarp.Core - we should probably just duplicate it out so that the compiler and FSHarp.Core are independent code bases, and we don't have all the whacky #if COMPILER

@saul (I've done this kind of hack in the past but should have been scolded at the time!)

I'm afraid I can't do this one without help. I admit what I did here with the #if directives and attributes is a copy paste job without fully understanding what's going on.

vasily-kirichenko · 2017-03-16T14:00:56Z

It works great. However, I find showing full file path in a link tooltip a bit useless. What about showing full name instead? Or full name + file name, like

My.Ns.M1.foo
c:\path\types.fs

?

majocha · 2017-03-16T14:46:08Z

@vasily-kirichenko Doable. I think it is a matter of passing more useful info from AST than just the definition range? Alternatively, a lot more info can be fetched and filled in the background, up to getting another quickinfo and making it recursive. Maybe ctrl-click on a link could enter such "drill-down" mode? I'll experiment as soon as I can.

vasily-kirichenko · 2017-03-19T09:49:57Z

@KevinRansom any reason why this awesome PR still has not been merged?

majocha · 2017-03-20T11:55:59Z

According to this
http://source.roslyn.io/#Microsoft.VisualStudio.LanguageServices/Implementation/Workspace/VisualStudioDocumentNavigationService.cs,125
Roslyn already handles linked files in TryNavigateToSpan, switching to correct DocumentId as needed.
~~That means we don't need to do it again. It looks like it still does work.~~ Edit: Nope.
(Navigation Bar often shows wrong project but this seems unrelated? Edit: There is different behavior than in C# projects. Trying to select already opened linked file from different project, selection in Solution Explorer will jump around, instead of just seamlessly switching project in the NavBar. This is for another issue, I guess.

KevinRansom · 2017-03-20T17:26:27Z

@vasily-kirichenko ... I hadn't got around to re-reviewing it yet

KevinRansom · 2017-03-20T17:35:45Z

@majocha

Thank you for this PR.

Kevin

cloudRoutine · 2017-03-21T12:57:32Z

@majocha I've noticed that the links in the tooltips seem to favor the .fsi files over the .fs files. I think it would be good if tooltips in .fs files had links to other .fs and tooltips in .fsi files linking to other .fsi

cartermp · 2017-03-21T13:32:57Z

@cloudRoutine Could you create an issue?

cloudRoutine · 2017-03-21T13:34:46Z

sure, I've got some other ideas for functionality around this feature so I'll put it all together there

smoothdeveloper · 2017-03-21T17:02:20Z

This feature rocks, @majocha should it also enable clicking on the symbol itself (when not at the definition)?

I know we might support ctrl+click later, but when using the mouse to get the tooltip, it also makes sense to go to the definition on the symbol itself.

majocha · 2017-03-21T17:09:24Z

@smoothdeveloper Makes sense! @cloudRoutine I think a discussion issue would be great, where we could talk ideas, design and also implementation and integration of stuff.

* custom tooltip test * tagClass augmented * kinda works * getting there * module, alias * not sure if works * fix build * go away! * parens out * halleluyah it builds * blank space on empty doc removed * more sensible * handle linked files * collapse empty TextBlocks * styling, simple tooltip, handle scripts * dismiss quickinfo on navigation * reliably dismiss quickinfo session * interface instead of boxing

majocha added 9 commits February 24, 2017 00:59

custom tooltip test

ef42e70

tagClass augmented

5599231

wired together

3e1e8f9

kinda works

511b5e8

getting there

d782d27

module, alias

026a17b

not sure if works

5fcc40c

Merge branch 'master' into navigable

867b357

fix build

f70c6ce

msftclas added the cla-already-signed label Mar 10, 2017

vasily-kirichenko reviewed Mar 10, 2017

View reviewed changes

go away!

9db1af4

vasily-kirichenko reviewed Mar 10, 2017

View reviewed changes

dsyme reviewed Mar 10, 2017

View reviewed changes

majocha changed the title ~~[WIP] Clickable quickinfo~~ Clickable QuickInfo with "go to type" Mar 15, 2017

vasily-kirichenko approved these changes Mar 15, 2017

View reviewed changes

forki approved these changes Mar 15, 2017

View reviewed changes

saul suggested changes Mar 15, 2017

View reviewed changes

reliably dismiss quickinfo session

3f5185c

cloudRoutine mentioned this pull request Mar 18, 2017

[Feature Enhancement] Show inherited interfaces in tooltip #2646

Closed

majocha added 2 commits March 18, 2017 21:18

Merge remote-tracking branch 'origin/master' into navigable

2176e45

interface instead of boxing

466e7c5

vasily-kirichenko approved these changes Mar 19, 2017

View reviewed changes

KevinRansom approved these changes Mar 20, 2017

View reviewed changes

KevinRansom merged commit 13abe6f into dotnet:master Mar 20, 2017

cloudRoutine mentioned this pull request Mar 21, 2017

[Feature Enhancement] Quickinfo with Navigation links #2673

Closed

majocha deleted the navigable branch March 27, 2017 19:08

Clickable QuickInfo with "go to type" #2574

Clickable QuickInfo with "go to type" #2574

Uh oh!

Conversation

majocha commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Mar 10, 2017

Uh oh!

smoothdeveloper commented Mar 10, 2017

Uh oh!

vasily-kirichenko commented Mar 10, 2017

Uh oh!

vasily-kirichenko commented Mar 10, 2017

Uh oh!

smoothdeveloper commented Mar 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majocha commented Mar 10, 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!

vasily-kirichenko commented Mar 10, 2017

Uh oh!

majocha commented Mar 10, 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!

majocha commented Mar 10, 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!

Choose a reason for hiding this comment

Uh oh!

vasily-kirichenko commented Mar 15, 2017

Uh oh!

cloudRoutine commented Mar 15, 2017

Uh oh!

majocha commented Mar 15, 2017

Uh oh!

majocha commented Mar 15, 2017

Uh oh!

olegtk commented Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

majocha commented Mar 10, 2017 •

edited

Loading

olegtk commented Mar 15, 2017 •

edited

Loading

majocha commented Mar 20, 2017 •

edited

Loading