Implement FSharpQuickInfoProvider (by @OmarTawfik) #1849

vasily-kirichenko · 2016-11-26T13:35:39Z

No description provided.

dsyme · 2016-11-26T13:48:13Z

vsintegration/src/FSharp.Editor/QuickInfoProvider.fs

Replace with |> CommonRoslynHelpers.StartAsyncAsTask cancellationToken now please

dsyme · 2016-11-26T13:51:10Z

vsintegration/src/FSharp.Editor/QuickInfoProvider.fs

In the GotoDefinition implementation we now have

match classifiedSpan.ClassificationType with | ClassificationTypeNames.ClassName | ClassificationTypeNames.DelegateName | ClassificationTypeNames.EnumName | ClassificationTypeNames.InterfaceName | ClassificationTypeNames.ModuleName | ClassificationTypeNames.StructName | ClassificationTypeNames.TypeParameterName | ClassificationTypeNames.Identifier ->

So I'm assuming quickinfo won't work at operators with this PR.

I think you should abstract out the tryClassifyAtPosition implementation and use it in both places.

dsyme · 2016-11-26T13:52:03Z

vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj

Why are these needed? thx

PresentationFrameword is needed for Label type.
WindowsBase is for FrameworkElement.

dsyme · 2016-11-26T13:53:40Z

Could you add one or two unit tests that exercises ProvideQuickInfo please? There are many other unit tests already for the underlying logic of course, but having something in place to enable further future testing at the level of ProvideQuickInfo would be great

dsyme · 2016-11-26T13:53:57Z

@vasily-kirichenko Absolutely fantastic to see this in place.

dsyme · 2016-11-26T14:18:01Z

vsintegration/src/FSharp.Editor/QuickInfoProvider.fs

The C# and VB implementations have very different constructor signatures?

http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp.EditorFeatures/QuickInfo/SemanticQuickInfoProvider.cs,13

Perhaps that's the reason for the composition error?

Please note I understand nothing about MEF composition :/

Yes, C# provider imports other dependencies, but I don't think it's the problem. We successfully import SVsServiceProvider in several places. I don't know much about MEF either :(

vasily-kirichenko · 2016-11-26T16:07:04Z

Could you add one or two unit tests that exercises ProvideQuickInfo please? There are many other unit tests already for the underlying logic of course, but having something in place to enable further future testing at the level of ProvideQuickInfo would be great

I'd like to make it work first. I still throws exception.

dsyme · 2016-11-26T20:21:57Z

vsintegration/src/FSharp.Editor/QuickInfoProvider.fs

Did this work?

Yes, it works now.

vasily-kirichenko · 2016-11-26T20:30:15Z

vasily-kirichenko · 2016-11-26T20:34:35Z

It kind of works now. The tooltip is always positioned below all visible text, not close to the symbol.

dsyme · 2016-11-26T20:36:36Z

@vasily-kirichenko Yeah, getting the positioning right is really important.

dsyme · 2016-11-26T20:36:49Z

Great to see the progress!

vladima · 2016-11-26T22:57:08Z

vsintegration/src/FSharp.Editor/CommonHelpers.fs

I don't think this cache works given that CWT referential identity of an object as a key

Ah ok. That was my code. Needs to be fixed

@vasily-kirichenko Could you check the identity used by ConditionalWeakTable and see if it uses .Equals and .GetHashCode or just object identity? Probably the latter. Thanks. (Not sure if there's any good key available in that case, maybe just remove this CWT for now)

Looks like it's pure hash table https://referencesource.microsoft.com/#mscorlib/system/runtime/compilerservices/ConditionalWeakTable.cs,464
Is it ok or I should remove the cache?

dsyme · 2016-11-27T00:40:42Z

@vasily-kirichenko See #1855 for one methodology to add tests. I particularly like to write tests without using the TestCase attribute, and instead just a loop. And use modules not classes.

dsyme · 2016-11-27T00:42:37Z

src/fsharp/FSharp.Compiler/FSharp.Compiler.fsproj

Could you remove these changes from the PR please? Presumably the lines are needed. Just remove them locally

These lines prevents the project to be load in VS.

Yep, I know. But we shouldn't check in their removal until we find out what they are for. People working on master who want to load FSHarp.Compiler into VS can just remove them by hand until tomorrow, thanks

dsyme · 2016-11-27T00:45:12Z

vsintegration/src/FSharp.Editor/CompletionProvider.fs

Please just use single line formatting for this.

I'll do, but I don't see your point. Is this better?

I cannot even see how many parameters this method has and what's the result type.

dsyme · 2016-11-27T00:45:49Z

vsintegration/src/FSharp.Editor/CommonHelpers.fs

Please use single line formatting for this - multi-line is not useful - it's basically a record in any case

dsyme · 2016-11-27T00:46:02Z

vsintegration/src/FSharp.Editor/CommonHelpers.fs

Likewise - single line please

dsyme · 2016-11-27T00:46:57Z

vsintegration/src/FSharp.Editor/QuickInfoProvider.fs

Please use new Label(Content=content, Foreground=SolidColorBrush(Colors.Black))

dsyme · 2016-11-27T00:47:10Z

vsintegration/src/FSharp.Editor/QuickInfoProvider.fs

Single line signature please

dsyme · 2016-11-27T00:47:30Z

vsintegration/src/FSharp.Editor/QuickInfoProvider.fs

Please use ParseAndCheckFileInProject (see #1855 )

dsyme · 2016-11-27T00:49:41Z

vsintegration/src/FSharp.Editor/QuickInfoProvider.fs

What is 189? The tokenTag I guess. I think you can use tagOfToken on an identifier

dsyme · 2016-11-27T00:51:21Z

vsintegration/src/FSharp.Editor/QuickInfoProvider.fs

Please just use null given this is implementing a C# interface

(Or use None followed by Option.toObj at the end)

dsyme · 2016-11-27T00:52:13Z

vsintegration/src/FSharp.Editor/GoToDefinitionService.fs

Single line signature please

(or formatted like the original)

Maybe minimize the diff in this method? Many of the changes are formatting

dsyme · 2016-11-27T00:53:39Z

@vasily-kirichenko This looks great. I left a bunch of fairly minor comments.

dsyme

see requested changes (Relatively minor)

vasily-kirichenko · 2016-11-27T09:33:26Z

I've ported somewhat hacky code from VFPT to get current theme colors (https://github.com/fsprojects/VisualFSharpPowerTools/blob/03c613acd3aa67cb5e3a339d3dcf1c7c783f30c2/src/FSharp.Editing.VisualStudio/Common/VSColors.fs)

It works:

If anybody knows a better way to do it, please tell us.

forki · 2016-11-27T10:09:36Z

Awesome!

dsyme · 2016-11-27T10:43:42Z

This is fantastic. Looks ready to go to me, once green

dsyme · 2016-11-27T11:20:16Z

@vasily-kirichenko Thank you!

dsyme · 2016-11-27T11:29:35Z

Looks like it's pure hash table https://referencesource.microsoft.com/#mscorlib/system/runtime/compilerservices/ConditionalWeakTable.cs,464 Is it ok or I should remove the cache?

Yes, that's an object identity hash, not a call to obj.GetHashCode(). We should remove the cache for now

dsyme · 2016-11-27T13:06:02Z

Just to mention that after this change both the QUickInfo and errors show when hovering over a red squiggly

This is good, but disambiguating between these using colors will be necessary

vasily-kirichenko · 2016-11-27T14:30:34Z

@dsyme we don't have control over errors formatting.

cloudRoutine · 2016-11-27T22:48:04Z

both the QuickInfo and errors show when hovering over a red squiggly

A long needed and overdue feature 😄

JoshVarty · 2016-11-28T18:27:52Z

vsintegration/src/FSharp.Editor/CommonHelpers.fs

+open System.Reflection
+open Microsoft.VisualStudio.Shell
+open System.Windows.Controls
+// A port of https://github.com/tomasr/viasfora/blob/master/Viasfora/Util/VsColors.cs


If you guys are supporting VS 2010 in this release then you'll need to interact with colors this way. In which case I'd add a comment that says something like "We want to support Visual Studio 2010, but EnvironmentColors weren't added until Visual Studio 2012 so we have to load the VS2012 DLL dynamically and interact with themes using reflection." (The file you've linked to doesn't exist at that location anymore)

If this .vsix isn't going to be used in VS2010 then I believe you can take a dependency on Microsoft.VisualStudio.Shell.11.0 and refer to EnvironmentColors directly.

We don't support any VS version prior to 2017.

I've already removed that code.

We now use IClassificationFormatMapService https://github.com/Microsoft/visualfsharp/blob/master/vsintegration/src/FSharp.Editor/QuickInfoProvider.fs#L63

dsyme · 2016-11-29T19:48:10Z

@cartermp @brettfo @vasily-kirichenko I'm noticing a lot of issues where VIsual Studio seems to hang while getting QuickInfo during debugging of F# code.

I'm not sure if this is our problem, but it feels like it is. It may make sense to work out how to turn off semantic/error quick info while debugging F# code. At least we need to investigate further

Pilchie · 2016-11-29T21:15:24Z

Is it quick info, or determining data-tip spans? Perhaps something in that code path is sub-optimal?

dsyme · 2016-11-30T13:03:20Z

@Pilchie Hard to tell. It's an outright hang of the VS IDE. I got one stack trace where the main UI thread was on roslyn code calling through to a quick info provider in a blocking wait (the word "QuickInfo" was definitely on the Roslyn call stack on the UI thread). I couldn't see any other useful stacks. I presume it was the F# provider for quick info tips. I'll try to repro and get better stack traces.

Is there a way to disable the F# Quick Info service when debugging is active if we need it?

thanks

vasily-kirichenko · 2016-11-30T13:20:28Z

Maybe it's not related, but I think this code https://github.com/Microsoft/visualfsharp/blob/master/vsintegration/src/FSharp.Editor/QuickInfoProvider.fs#L81-L88 is too heavy weight to get qualified name island, start column of the last part of it and its TextSpan. Maybe it's possible to use something like QuickParse.GetCompleteIdentifierIsland and create a TextSpan from it somehow?

dsyme · 2016-11-30T14:01:49Z

@vasily-kirichenko Hmmm... it felt more async-lock-up-the-system than that

vasily-kirichenko · 2016-11-30T14:07:48Z

however, code is not changed during debugging, so FCS should cache everything and answer very fast. It's strange. I've not able to investigate it today, been trying to build behind the proxy.

vasily-kirichenko · 2016-11-30T17:49:16Z

@dsyme on a hello world console application debugger works OK for me: it does not show tooltips and it does show proper debug information (current variable values).

On VFT solution it does not work, hanging on this dialog:

Did you mean this behavior?

cartermp · 2016-11-30T17:49:59Z

@vasily-kirichenko I've noticed that too

Pilchie · 2016-11-30T17:56:06Z

If it hangs forever, a dump with heap would really make investigations go a lot faster.

vasily-kirichenko · 2016-11-30T18:04:53Z

@Pilchie it does not hang forever, sorry for misinformation. The dialog is shown until you move mouse somewhere else, after that VS is responsive until you hover the mouse over another symbol again.

vasily-kirichenko · 2016-11-30T18:05:42Z

I debugged it and I was wrong: IQuickInfoProvider.GetItemAsync is called in debug mode, however with no results.

vasily-kirichenko · 2016-11-30T20:03:53Z

I make IBreakpointResolutionService no-op and it has not helped:

interface IBreakpointResolutionService with
        member this.ResolveBreakpointAsync(_document: Document, _textSpan: TextSpan, _cancellationToken: CancellationToken): Task<BreakpointResolutionResult> =
            Task.FromResult null

(I mean "Getting datatip text" dialog still hangs)

vasily-kirichenko · 2016-11-30T20:09:25Z

I also make IQuickInfoProvider noop, too and it has not helped either:

interface IQuickInfoProvider with
        override this.GetItemAsync(_document: Document, _position: int, _cancellationToken: CancellationToken): Task<QuickInfoItem> =
            Task.FromResult null

vasily-kirichenko · 2016-11-30T20:19:10Z

Obviously it hangs in ILanguageDebugInfoService.GetDataTipInfoAsync. I added try...with block around it and it did not help.

dsyme · 2016-12-01T12:07:39Z

@vasily-kirichenko COuld you add a new bug with repro steps you are using? This seems important to fix (even though we're only seeing it in the VisualFSharp.sln right now, I presume it happens on other large solutions)

vasily-kirichenko · 2016-12-02T09:28:52Z

@dsyme done #1915

@OmarTawfik

* Implement FSharpQuickInfoProvider (by @OmarTawfik) * extract tryClassifyAtPosition and reuse it in QuickInfoProvider * address code review * fix QuickInfo text span * don't show empty tooltip * add a QuickInfoProvider test * QuickInfoProvider respects current theme colors * revert problem stuff into FSharp.Compiler.fsproj

msftclas added the cla-already-signed label Nov 26, 2016

vasily-kirichenko mentioned this pull request Nov 26, 2016

Roslyn Integration needs implementation of QuickInfoProvider service #1598

Closed

dsyme reviewed Nov 26, 2016

View reviewed changes

vasily-kirichenko force-pushed the quick-info branch from b8cc1b9 to ed912f4 Compare November 26, 2016 16:02

dsyme changed the title ~~Implement FSharpQuickInfoProvider (by @OmarTawfik)~~ [WIP] Implement FSharpQuickInfoProvider (by @OmarTawfik) Nov 26, 2016

dsyme reviewed Nov 26, 2016

View reviewed changes

vladima reviewed Nov 26, 2016

View reviewed changes

dsyme reviewed Nov 27, 2016

View reviewed changes

vsintegration/src/FSharp.Editor/CommonHelpers.fs Outdated

Copy link

Contributor

dsyme Nov 27, 2016

Choose a reason for hiding this comment

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

Likewise - single line please

dsyme reviewed Nov 27, 2016

View reviewed changes

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

Copy link

Contributor

dsyme Nov 27, 2016

Choose a reason for hiding this comment

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

Single line signature please

dsyme reviewed Nov 27, 2016

View reviewed changes

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

Copy link

Contributor

dsyme Nov 27, 2016

Choose a reason for hiding this comment

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

Please use ParseAndCheckFileInProject (see #1855 )

dsyme reviewed Nov 27, 2016

View reviewed changes

dsyme requested changes Nov 27, 2016

View reviewed changes

QuickInfoProvider respects current theme colors

6a32c40

dsyme changed the title ~~[WIP] Implement FSharpQuickInfoProvider (by @OmarTawfik)~~ Implement FSharpQuickInfoProvider (by @OmarTawfik) Nov 27, 2016

revert problem stuff into FSharp.Compiler.fsproj

d0f57ca

dsyme merged commit f67db1f into dotnet:master Nov 27, 2016

JoshVarty reviewed Nov 28, 2016

View reviewed changes

Implement FSharpQuickInfoProvider (by @OmarTawfik) #1849

Implement FSharpQuickInfoProvider (by @OmarTawfik) #1849

Uh oh!

Conversation

vasily-kirichenko commented Nov 26, 2016

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 commented Nov 26, 2016

Uh oh!

dsyme commented Nov 26, 2016

Uh oh!

dsyme Nov 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyme Nov 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasily-kirichenko commented Nov 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasily-kirichenko commented Nov 26, 2016

Uh oh!

vasily-kirichenko commented Nov 26, 2016

Uh oh!

dsyme commented Nov 26, 2016

Uh oh!

dsyme commented Nov 26, 2016

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 commented Nov 27, 2016 • 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!

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!

Choose a reason for hiding this comment

dsyme Nov 26, 2016 •

edited

Loading

dsyme Nov 26, 2016 •

edited

Loading

dsyme commented Nov 27, 2016 •

edited

Loading

JoshVarty Nov 28, 2016 •

edited

Loading