Use completion item InsertionText in the debugger, even before any non-debugger completion sessions have run #26361

dpoeschl · 2018-04-24T17:37:27Z

This uncovered a weird issue in how completion services are used / shared between the normal editor and debugger scenarios. #26394 tracks a fix for that larger issue, while this PR addresses the immediate concern of uninitialized data.

Escrow Template

Customer scenario

The customer uses completion in the debugger (Watch window, etc.) before ever using completion in the regular editor. Items like List<> that are supposed to commit as just List will commit as List<>, and you then have to go back and clean it up.

Bugs this fixes

#24112

Workarounds, if any

Either

Run completion in the regular editor one time
Allow the incorrect code to be inserted by completion and then go clean it up before proceeding

Risk

The risk of this change is pretty low. We're making sure a cache is populated when it is first needed.

Performance impact

Basically none. No normal scenarios are impacted, and in the case of debugger-completion-before-editor-completion, we do a little bit of mef work when the completion list is shown where we didn't before, but that's necessary for correctness.

Is this a regression from a previous update?

No

Root cause analysis

The architecture of this partially lazily initialized type relies on an assumption that isn't being met currently.

How was the bug found?

Ad hoc testing

rchande · 2018-04-24T19:53:10Z

src/Features/Core/Portable/Completion/CompletionServiceWithProviders.cs

-                    _nameToProvider.TryGetValue(name, out provider);
+                    if (!_nameToProvider.TryGetValue(name, out provider))
+                    {
+                        CreateRoleProviders(ImmutableHashSet.Create<string>());


I guess this is normally initialized by the call to GetProviders? Why isn't that getting called?

CyrusNajmabadi · 2018-04-24T20:37:29Z

src/Features/Core/Portable/Completion/CompletionServiceWithProviders.cs

+                    if (!_nameToProvider.TryGetValue(name, out provider))
+                    {
+                        // Sometimes the _nameToProvider map hasn't been fully initialized in
+                        // debugger scenarios. If we couldn't find the provider, ensure everything


hrmm.. that makes me feel like that's an issue in that code path. How can the debugger sidestep the initialization of the providers?

CyrusNajmabadi · 2018-04-25T20:24:03Z

src/Features/Core/Portable/Completion/CompletionServiceWithProviders.cs

+        /// debugger before ever doing completion in a regular open document, then the completion
+        /// service being used to commit items hasn't been initialized (that is, 
+        /// ShouldTriggerCompletion has never been called). So, when it's time to lookup the
+        /// provider for the committed item, the cache has not been created, so we don't find


i'm trying to reconcile these bits:

In debugger scenarios, the completion service specific to that instance is used to trigger completion, but the one created for regular editing scenarios is used for showing the Description and for Committing.

Isn't that super wonky. We're working across two completion services instead of using the actual service that created the items. To me it feels like the bug is there. How/Why do we get into this situation in the first place?

CyrusNajmabadi · 2018-04-25T20:25:56Z

src/Features/Core/Portable/Completion/CompletionServiceWithProviders.cs

+        /// a problem for entries like "List&lt;&gt;", where the insertion text and DisplayText
+        /// differ. 
+        /// Refactoring such that the same completion service is used for triggering and 
+        /// descriptions/committing in debugger scenarios is risky, so we're taking a more


really? it seems super strange that we can even get into this situatoin in the first place, and it makes me very quesy about how these services are created and then used. i mean, it basically hopes/prays htat the one service will have no problem and have no expectations when getting data from another service. As htis bug indicates, that's clearly not a good assumption to make :)

can this be answered? It seems super weird to me that we even got into this situatoin in the first place. What codepath it working with multiple CompletionServices... adn why not just fix that codepath? That seems like teh real fix to have here...

CyrusNajmabadi · 2018-04-25T20:27:30Z

src/Features/Core/Portable/Completion/CompletionServiceWithProviders.cs

@@ -194,6 +218,12 @@ internal protected CompletionProvider GetProvider(CompletionItem item)
            {
                lock (_gate)
                {
+                    if (!_roleProvidersInitialized)
+                    {
+                        CreateRoleProviders(ImmutableHashSet.Create<string>());


Why is ImmutableHashSet.Create<string>() the right value to use here? Can you explain/comment why?

Also, still not quite understanding how this won't still have a problem elsewhere. for example, say that the item references a provider that thsi service doesn't know about. Won't this just return null, causing a crash elsewhere? or are we resilient to that?

Can this be answered?

sharwell · 2018-04-25T20:33:40Z

src/Features/Core/Portable/Completion/CompletionServiceWithProviders.cs

+                        CreateRoleProviders(ImmutableHashSet.Create<string>());
+                        _roleProvidersInitialized = true;
+                    }
+
                    _nameToProvider.TryGetValue(name, out provider);


❓ Why not change this to a GetOrAdd call that initializes the requested provider on demand, similar to how _rolesToProviders is used above? It seems this would be a clean (non-hacky) way to address the problem.

sharwell

Nice, I definitely like this more than the first revision. Perhaps update the escrow template now that the PR has been cleaned up?

CyrusNajmabadi · 2018-04-25T21:16:44Z

src/Features/Core/Portable/Completion/CompletionServiceWithProviders.cs

                }
            }

            return provider;
        }

+        private CompletionProvider GetProviderByName(string providerName)
+        {
+            var providers = GetProviders(roles: null);


can a comment be provided explaining why passing null for roles is correct here. I'm not quite understanding.

…rticular provider Fixes dotnet#24112

dpoeschl · 2018-04-26T19:28:07Z

retest windows_debug_spanish_unit32_prtest please
Reason: https://ci.dot.net/job/dotnet_roslyn/job/dev15.7.x/job/windows_debug_spanish_unit32_prtest/223/
Unrelated failure: Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Squiggles.ErrorSquiggleProducerTests.TestNoErrorsAfterProjectRemoved

CyrusNajmabadi · 2018-04-26T19:34:05Z

src/Features/Core/Portable/Completion/CompletionServiceWithProviders.cs

                }
            }

            return provider;
        }

+        private CompletionProvider GetProviderByName(string providerName)
+        {
+            var providers = GetAllProviders(roles: ImmutableHashSet<string>.Empty);


a comment here explaining why this is the right set of roles would be good. i actually do not know why this is correct.

dpoeschl · 2018-05-10T17:51:06Z

Tagging @jinujoseph for consideration.

jinujoseph · 2018-05-10T20:29:37Z

@dpoeschl can you retarget this to master (15.8.Preview2 )

jinujoseph · 2018-05-10T20:37:06Z

Approved to merge for 15.8.Preview2 (once we have green test )

CyrusNajmabadi · 2018-05-10T22:30:15Z

It concerns me that none of this has any documentation explaining waht is going on. This sort of fix invariably leads to problems that get compounded as more and more patches are piled on.

dpoeschl added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 24, 2018

dpoeschl requested review from a team as code owners April 24, 2018 17:37

dpoeschl changed the base branch from master to dev15.7.x April 24, 2018 17:37

dpoeschl requested review from rchande and removed request for a team April 24, 2018 17:37

dpoeschl changed the title ~~Use completion item InsertionText in the debugger, even before any non-debugger completion sessions have run~~ WIP: Use completion item InsertionText in the debugger, even before any non-debugger completion sessions have run Apr 24, 2018

rchande reviewed Apr 24, 2018

View reviewed changes

CyrusNajmabadi reviewed Apr 24, 2018

View reviewed changes

dpoeschl force-pushed the DebuggerIntellisenseInsertionText branch from cf970e9 to 0367baf Compare April 25, 2018 18:28

dpoeschl changed the title ~~WIP: Use completion item InsertionText in the debugger, even before any non-debugger completion sessions have run~~ Use completion item InsertionText in the debugger, even before any non-debugger completion sessions have run Apr 25, 2018

dpoeschl added Area-IDE and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Apr 25, 2018

dpoeschl mentioned this pull request Apr 25, 2018

Implement a better fix for when completion runs in the debugger before it runs in the normal editor #26394

Closed

rchande approved these changes Apr 25, 2018

View reviewed changes

dpoeschl force-pushed the DebuggerIntellisenseInsertionText branch from 0367baf to 28f9a82 Compare April 25, 2018 20:05

CyrusNajmabadi reviewed Apr 25, 2018

View reviewed changes

sharwell requested changes Apr 25, 2018

View reviewed changes

dpoeschl force-pushed the DebuggerIntellisenseInsertionText branch from 28f9a82 to e4db5ae Compare April 25, 2018 21:11

sharwell approved these changes Apr 25, 2018

View reviewed changes

CyrusNajmabadi reviewed Apr 25, 2018

View reviewed changes

Ensure completion providers have been indexed when searching for a pa…

7ede59e

…rticular provider Fixes dotnet#24112

dpoeschl force-pushed the DebuggerIntellisenseInsertionText branch from e4db5ae to 7ede59e Compare April 26, 2018 17:41

CyrusNajmabadi reviewed Apr 26, 2018

View reviewed changes

dpoeschl changed the base branch from dev15.7.x to dev15.8-preview1 April 27, 2018 22:20

dpoeschl changed the base branch from dev15.8-preview1 to master May 10, 2018 20:30

jinujoseph added the Bug label May 10, 2018

jinujoseph added this to the 15.8 milestone May 10, 2018

jinujoseph added the Approved to merge label May 10, 2018

dpoeschl merged commit a1c5beb into dotnet:master May 10, 2018

dpoeschl mentioned this pull request Jun 5, 2018

Incorrect text inserted from IntelliCode suggestions in Quick Watch screen #27395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use completion item InsertionText in the debugger, even before any non-debugger completion sessions have run #26361

Use completion item InsertionText in the debugger, even before any non-debugger completion sessions have run #26361

dpoeschl commented Apr 24, 2018 •

edited

Loading

rchande Apr 24, 2018

CyrusNajmabadi Apr 24, 2018

CyrusNajmabadi Apr 25, 2018

CyrusNajmabadi Apr 25, 2018

CyrusNajmabadi Apr 26, 2018

CyrusNajmabadi Apr 25, 2018

CyrusNajmabadi Apr 25, 2018

CyrusNajmabadi Apr 26, 2018

sharwell Apr 25, 2018

sharwell left a comment

CyrusNajmabadi Apr 25, 2018

dpoeschl commented Apr 26, 2018 •

edited

Loading

CyrusNajmabadi Apr 26, 2018

dpoeschl commented May 10, 2018

jinujoseph commented May 10, 2018

jinujoseph commented May 10, 2018

CyrusNajmabadi commented May 10, 2018

Use completion item InsertionText in the debugger, even before any non-debugger completion sessions have run #26361

Use completion item InsertionText in the debugger, even before any non-debugger completion sessions have run #26361

Conversation

dpoeschl commented Apr 24, 2018 • edited Loading

Escrow Template

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpoeschl commented Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

dpoeschl commented May 10, 2018

jinujoseph commented May 10, 2018

jinujoseph commented May 10, 2018

CyrusNajmabadi commented May 10, 2018

dpoeschl commented Apr 24, 2018 •

edited

Loading

dpoeschl commented Apr 26, 2018 •

edited

Loading