Fix Intellisense in debugger tool windows by olegtk · Pull Request #24484 · dotnet/roslyn

olegtk · 2018-01-26T17:33:34Z

Ask Mode template

Customer scenario

When debugging a VB application, a customer opens Immediate window and types "?" to dump a value of a variable. Instead of showing a completion, "??" is written to the Immediate Window.

Bugs this fixes

https://devdiv.visualstudio.com/DevDiv/_workitems/edit/557166

Workarounds, if any

N/A

Risk

Low, the fix only affects DebuggerIntellisenseFilter class and follows the approach already used by VenusCommandFilter.

Performance impact

No perf impact is expected as the fix just restores execution of existing command handlers in this scenario.

Is this a regression from a previous update?

It's a regression from 15.6 Preview2 caused by migrating all Roslyn command handlers to the new editor commanding.

Root cause analysis

We missed this issue in partner manual testing stage because we didn't include debugger team into the list of affected partners. Also this scenario is only covered by CQ tests, which took long to run and analyze.
This PR adds 2 basic C# and VB integration tests to cover this scenario in earlier development stage.

How was the bug found?

CQ test failure.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/557166

DebuggerIntellisenseFilter, just like VenusCommandFilter routes commands to the legacy command handler service, which now is no-op for VB/C#. And so now (again, just like VenusCommandFilter) in order to execute command handlers migrated to the editor commanding it needs to hookup the new editor command handler service, configured to the correct text view and subject buffer.

sharwell · 2018-01-26T19:10:38Z

src/VisualStudio/Core/Def/Implementation/AbstractVsTextViewFilter`2.cs

        where TLanguageService : AbstractLanguageService<TPackage, TLanguageService>
    {
-        private readonly AbstractLanguageService<TPackage, TLanguageService> _languageService;
+        protected readonly AbstractLanguageService<TPackage, TLanguageService> _languageService;


💡 If you need to make this protected, use a property instead:

protected AbstractLanguageService<TPackage, TLanguageService> LanguageService { get; }

sharwell · 2018-01-26T19:16:52Z

src/VisualStudio/Core/Def/Implementation/DebuggerIntelliSense/DebuggerIntellisenseFilter.cs

+            var vsCommandHandlerServiceAdapter = vsCommandHandlerServiceAdapterFactory.Create(ConvertTextView(), 
+                GetSubjectBufferContainingCaret(), // our override doesn't actually check the caret and always returns _context.Buffer
+                NextCommandTarget);
+            NextCommandTarget = vsCommandHandlerServiceAdapter;


❓ Why still have the previous SetNextFilter method writing to the same field?

Good catch. There is a weird dependency between SetNextFilter() and SetContext()/SetCommandHandlers() here, but I found a way to unify them so that NextCommadTarget is set in only one place

sharwell · 2018-01-26T21:12:05Z

src/VisualStudio/Core/Def/Implementation/DebuggerIntelliSense/DebuggerIntellisenseFilter.cs

                // types escape, so treat SCROLLUP like CANCEL. It's actually a CANCEL.
                case VSConstants.VSStd2KCmdID.SCROLLUP:
-                    ExecuteCancel(subjectBuffer, contentType, executeNextCommandTarget);
+                    ExecuteCancel(subjectBuffer, contentType, () =>


💭 It's not clear to me why only two of the four uses of executeNextCommandTarget changed.

these 2 handle one command but execute another (SCROLLUP -> Cancel and TYPECHAR -> ShowMemberList). executeNextCommandTarget captures the original handled command so we cannot use it.

Why did it work before?

Previously it didn't matter because both Cancel and ShowMemberList commands were pretty much guaranteed to be handled by Roslyn command handlers inside ICommandHandlerService (poorly named CurrentHandlers property) - executeNextCommandTarget was never called.

Now we have a chain of (ICommandHandlerService: legacy command handlers if any) -> (IEditorCommandHandlerService: migrated command handlers) and all Roslyn handlers are actually in the latter (former is kept for TS/F#). So properly passing executed command matters now.

jasonmalinowski

@sharwell asks many wise questions.

jasonmalinowski · 2018-01-26T21:36:49Z

src/VisualStudio/Core/Def/Implementation/DebuggerIntelliSense/DebuggerIntellisenseFilter.cs

        internal void SetCommandHandlers(ITextBuffer buffer)
        {
            this.CurrentHandlers = _commandFactory.GetService(buffer);
+            // Chain in editor command handler service. It will execute all our command handlers migrated to the modern editor commanding


Newline before this.

jasonmalinowski · 2018-01-26T21:38:55Z

src/VisualStudio/IntegrationTest/IntegrationTests/CSharp/CSharpImmediate.cs

+            VisualStudio.ImmediateWindow.ShowImmediateWindow(clearAll: true);
+            VisualStudio.SendKeys.Send("?n");
+            VisualStudio.Workspace.WaitForAsyncOperations(FeatureAttribute.CompletionSet);
+            VisualStudio.SendKeys.Send("1", VirtualKey.Enter);


Given this is typing out the 1, it means a lack of completion would still pass. Should it send a tab instead?

good point, let me try it. Is there a way to validate the completion is up in your integration tests without going deep into editor bits?
Btw, Apex can do that, it also fully supports debugger toolwindows :)

Found a way to make these tests rely on completion by sending Tab as you suggested.

olegtk · 2018-01-26T23:27:15Z

src/VisualStudio/IntegrationTest/IntegrationTests/VisualBasic/BasicImmediate.cs

@@ -39,8 +39,8 @@ End Module
            VisualStudio.ImmediateWindow.ShowImmediateWindow(clearAll: true);
            VisualStudio.SendKeys.Send("?");


VB unlike C# shows completion on ?

@olegtk ? is a special thing, since VB makes a distinction between expressions and statements in the immediate window.

olegtk · 2018-01-27T00:24:47Z

@dotnet-bot retest windows_release_unit32_prtest please

jasonmalinowski · 2018-01-30T01:28:03Z

src/VisualStudio/Core/Def/Implementation/DebuggerIntelliSense/DebuggerIntellisenseFilter.cs

+        private void SetNextFilterWorker()
+        {
+            // We have a new _originalNextCommandFilter or new _context, reset NextCommandTarget chain based on their values.
+            // The chain is formed like this: 


I appreciate this comment greatly, thank you.

jasonmalinowski · 2018-01-30T01:31:08Z

src/VisualStudio/Core/Def/Implementation/DebuggerIntelliSense/DebuggerIntellisenseFilter.cs

+            //     -> IVsCommandHandlerServiceAdapter (our command handlers migrated to the modern editor commanding)
+            //            -> original next command filter
+            var nextCommandFilter = _originalNextCommandFilter;
+            if (_context != null)


I admit I don't quite get this check here....can we add a comment to explain it? Is this some thing where we create one of these prior to having the buffer, and that's what the later hookup is for?

jasonmalinowski

Other than a request for another comment, this looks good. Thank you for adding integration tests too!

jasonmalinowski · 2018-01-30T01:33:58Z

@olegtk Just to check: do we even need this filter anymore? If it is doing some special stuff, does it need to be chaining to the legacy Roslyn ones now? I'm not sure if we have F# or TypeScript stuff sitting atop this?

(more asking just to know what code might be dead or nearly dead.)

…into dev/olegtk/FixIntellisenseInImmediate

jinujoseph · 2018-01-31T03:59:07Z

@olegtk Feel free to merge once test validation from partners sign off.
Approved to merge via Link .

olegtk · 2018-01-31T23:40:07Z

We still need this filter because it's doing some unusual command routing (Scrollup->Cancel, TypeChar->TypeChar+ShowMemberList) etc. It can probably be done via regular command handling though.
Chaining legacy ICommandHandlerService can be removed once TS/F# migrate (no idea if they use this, I kept it just because we are in Escrow).

…into dev/olegtk/FixIntellisenseInImmediate

olegtk added 4 commits January 25, 2018 19:45

basic fix for intellisense in Immediate window

18e0f7a

better comments and cleanup

f476f8d

Add basic integration tests

4602e9a

cleanup inproc Immediate window integration test helper

a87ec7b

olegtk requested a review from a team as a code owner January 26, 2018 17:33

olegtk self-assigned this Jan 26, 2018

olegtk added this to the 15.6 milestone Jan 26, 2018

olegtk added the Area-IDE label Jan 26, 2018

fix incorrect comment

38182c3

sharwell reviewed Jan 26, 2018

View reviewed changes

jasonmalinowski reviewed Jan 26, 2018

View reviewed changes

olegtk added 2 commits January 26, 2018 15:14

address PR feedback

f040655

create Immediate window on ImmediateWindow_InProc.GetText()

eaefc42

olegtk commented Jan 26, 2018

View reviewed changes

jasonmalinowski reviewed Jan 30, 2018

View reviewed changes

jasonmalinowski approved these changes Jan 30, 2018

View reviewed changes

Merge branch 'dev15.6.x-vs-deps' of https://github.com/dotnet/roslyn …

229bd79

…into dev/olegtk/FixIntellisenseInImmediate

sharwell approved these changes Jan 30, 2018

View reviewed changes

jinujoseph added the Needs Shiproom Approval label Jan 31, 2018

jinujoseph added Approved to merge and removed Needs Shiproom Approval labels Jan 31, 2018

olegtk added 2 commits January 31, 2018 15:49

Merge branch 'dev15.6.x-vs-deps' of https://github.com/dotnet/roslyn …

a6de872

…into dev/olegtk/FixIntellisenseInImmediate

add a comment to address PR feedback

f18155f

olegtk merged commit a5a1dd8 into dev15.6.x-vs-deps Feb 1, 2018

davkean deleted the dev/olegtk/FixIntellisenseInImmediate branch June 21, 2018 06:39

		@@ -39,8 +39,8 @@ End Module
		VisualStudio.ImmediateWindow.ShowImmediateWindow(clearAll: true);
		VisualStudio.SendKeys.Send("?");

Conversation

olegtk commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

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!

jasonmalinowski left a comment

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!

olegtk commented Jan 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski commented Jan 30, 2018

Uh oh!

jinujoseph commented Jan 31, 2018

Uh oh!

olegtk commented Jan 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

olegtk commented Jan 26, 2018 •

edited

Loading