Implementation of new editor completion API #25770

dpoeschl · 2018-03-27T21:03:34Z

Previous review: #25537

TODOs: https://github.com/dotnet/roslyn/projects/38

Ask Mode template not completed

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes

(either VSO or GitHub links)

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

AmadeusW

Looks good :) I left a few comments but I haven't looked in depth at committing and handling non-roslyn items.

AmadeusW · 2018-03-30T18:03:29Z

...mplementation/IntelliSense/EditorCompletionImplementation/CompletionCommitManagerProvider.cs

+    [ContentType(ContentTypeNames.RoslynContentType)]
+    internal class CompletionItemSourceProvider : IAsyncCompletionCommitManagerProvider
+    {
+        IAsyncCompletionCommitManager _instance;


Consider cleaning up when the Roslyn workspace is unloaded

@AmadeusW Why? We have no sane unload operation.

AmadeusW · 2018-03-30T18:03:59Z

...ures/Core/Implementation/IntelliSense/EditorCompletionImplementation/CompletionItemSource.cs

+        internal const string TriggerBuffer = nameof(TriggerBuffer);
+        internal const string MatchPriority = nameof(MatchPriority);
+        internal const string SelectionBehavior = nameof(SelectionBehavior);
+        internal const string InsertionText = "InsertionText";


AmadeusW · 2018-03-30T18:10:19Z

...ures/Core/Implementation/IntelliSense/EditorCompletionImplementation/CompletionItemSource.cs

+                return false;
+            }
+
+            // TODO: TypeChar of 0 means Invoke or InvokeAndCommitIfUnique. An API update will make this better.


How would you like the API to look like?
I'd make the comment explicitly state that we have to provide a span in Invoke or InvokeAndCommitIfUnique scenarios

AmadeusW · 2018-03-30T18:10:33Z

...e/Implementation/IntelliSense/EditorCompletionImplementation/CompletionItemSourceProvider.cs

+    [ContentType(ContentTypeNames.RoslynContentType)]
+    internal class CompletionSourceProvider : IAsyncCompletionSourceProvider
+    {
+        IAsyncCompletionSource _instance;


cleanup when workspace is unloaded

AmadeusW · 2018-03-30T18:13:44Z

...e/Implementation/IntelliSense/EditorCompletionImplementation/EditorAsyncCompletionService.cs

+
+        private static bool IsAllPunctuation(string filterText)
+        {
+            foreach (var ch in filterText)


for loop will reduce allocations

This is not correct. Neither form will result in allocations. The generated code is not quite identical in debug builds but for all practical purposes it's the same. With optimizations enabled the generated IL is equal.

That's good to know, thanks!

AmadeusW · 2018-03-30T18:14:56Z

src/EditorFeatures/TestUtilities/Completion/AbstractCompletionProviderTests.cs


            if (usePreviousCharAsTrigger)
            {
-                trigger = CompletionTrigger.CreateInsertionTrigger(insertedCharacter: code.ElementAt(position - 1));
+                trigger = RoslynCompletion.CompletionTrigger.CreateInsertionTrigger(insertedCharacter: code.ElementAt(position - 1));


is there a potential error when document is empty or position is otherwise 0?

AmadeusW · 2018-03-30T18:18:44Z

src/VisualStudio/CSharp/Impl/Snippets/SnippetCommandHandler.cs

@@ -21,6 +21,7 @@ namespace Microsoft.VisualStudio.LanguageServices.CSharp.Snippets
    [Name("CSharp Snippets")]
    [Order(After = CodeAnalysis.Editor.PredefinedCommandHandlerNames.Completion)]
    [Order(After = CodeAnalysis.Editor.PredefinedCommandHandlerNames.IntelliSense)]
+    [Order(After = "CompletionCommandHandlers")]


PredefinedCompletionNames.CompletionCommandHandlers

AmadeusW · 2018-03-30T18:19:29Z

src/VisualStudio/IntegrationTest/TestUtilities/OutOfProcess/Editor_OutOfProc.cs

-        public bool IsCaretOnScreen()
-            => _editorInProc.IsCaretOnScreen();
+        //public bool IsCaretOnScreen()
+        //    => _editorInProc.IsCaretOnScreen();


did completion affect this, or is this unrelated?

This fixes a signing regression caused be #25751. That changed us to directly call the SWIX build props / targets vs. getting them implicitly from the microbuild props / targets. One of the behaviors that the microbuild props / targets had that wasn't accounted for was the signing of the VSIX after they are constructed. Hence this lead to signing errors on insertion. This PR fixes that by making the following changes: 1. Adds SWIX VSIX to SignToolData.json so they are accounted for during normal batch signing. 1. Moves the SWIX build to the pre-sign portion of our build. 1. Removes the references from VSMAN -> SWIX. These references are there only to enforce ordering which is unnecessary. Having them remain risks that building the VSMAN will cause the SWIX to be re-built which could possibly invalidate signing.

ivanbasov · 2018-06-07T18:26:11Z

@dotnet-bot retest

dpoeschl · 2018-06-04T18:01:52Z

...s/Core/Implementation/IntelliSense/EditorCompletionImplementation/CompletionCommitManager.cs

+                    commitCharacter,
+                    token).WaitAndGetResult(token);
+
+                edit.Replace(change.TextChange.Span.ToSpan(), change.TextChange.NewText);


dpoeschl · 2018-06-04T18:01:56Z

...s/Core/Implementation/IntelliSense/EditorCompletionImplementation/CompletionCommitManager.cs

+                    token).WaitAndGetResult(token);
+
+                edit.Replace(change.TextChange.Span.ToSpan(), change.TextChange.NewText);
+                edit.Apply();


dpoeschl · 2018-06-04T18:04:46Z

...s/Core/Implementation/IntelliSense/EditorCompletionImplementation/CompletionCommitManager.cs

+            char commitCharacter,
+            CancellationToken token)
+        {
+            var document = buffer.CurrentSnapshot.GetOpenDocumentInCurrentContextWithChanges();


dpoeschl · 2018-06-04T18:05:23Z

...s/Core/Implementation/IntelliSense/EditorCompletionImplementation/CompletionCommitManager.cs

+            }
+
+            var completionService = document.GetLanguageService<CompletionService>();
+            if (!item.Properties.TryGetProperty<RoslynCompletionItem>(CompletionItemSource.RoslynItem, out var roslynItem))


Yes, such as Razor items.

dpoeschl · 2018-06-04T18:06:32Z

...ures/Core/Implementation/IntelliSense/EditorCompletionImplementation/CompletionItemSource.cs

+            var completionList = await completionService.GetCompletionsAsync(
+                document,
+                triggerLocation,
+                GetRoslynTrigger(trigger)).ConfigureAwait(false);


dpoeschl · 2018-06-04T19:46:53Z

src/EditorFeatures/TestUtilities/AbstractCommandHandlerTestState.cs

@@ -26,7 +26,7 @@ internal abstract class AbstractCommandHandlerTestState : IDisposable
        public readonly TestWorkspace Workspace;
        public readonly IEditorOperations EditorOperations;
        public readonly ITextUndoHistoryRegistry UndoHistoryRegistry;
-        private readonly ITextView _textView;
+        protected readonly ITextView _textView;


dpoeschl · 2018-06-04T19:48:31Z

src/EditorFeatures/TestUtilities/ExperimentationService/TestExperimentationServiceInternal.cs

+namespace Microsoft.CodeAnalysis.Test.Utilities.ExperimentationService
+{
+    [Export(typeof(IExperimentationServiceInternal)), Shared]
+    internal class TestExperimentationServiceInternal : IExperimentationServiceInternal


@ivanbasov Do we need to be able to set this up differently for old/new unit tests?

dpoeschl · 2018-06-04T19:49:10Z

src/EditorFeatures/TestUtilities/ExperimentationService/TestExperimentationServiceInternal.cs

+namespace Microsoft.CodeAnalysis.Test.Utilities.ExperimentationService
+{
+    [Export(typeof(IExperimentationServiceInternal)), Shared]
+    internal class TestExperimentationServiceInternal : IExperimentationServiceInternal


dpoeschl · 2018-06-04T19:58:51Z

...ures/Core/Implementation/IntelliSense/EditorCompletionImplementation/CompletionItemSource.cs

+                attributeImages = ImmutableArray.Create(
+                    new ImageElement(
+                        warningImage,
+                        "Temporary Automation Name")); // TODO: Get automation names, here and below


dpoeschl · 2018-06-04T20:01:21Z

...plementation/IntelliSense/EditorCompletionImplementation/EditorAsyncCompletionItemManager.cs

+            ImmutableArray<EditorCompletion.CompletionFilter> activeFilters,
+            bool mustSetSelection)
+        {
+            // TODO: DismissIfEmpty?


ivanbasov · 2018-06-07T23:40:52Z

retest this please

ivanbasov · 2018-08-08T00:19:04Z

now it is covered by #29016. I'll remove this branch when I'll migrate #28219 to a new base.

dpoeschl requested review from a team as code owners March 27, 2018 21:03

dpoeschl mentioned this pull request Mar 27, 2018

Implementation of new editor completion API #25537

Closed

dpoeschl force-pushed the features/AsyncCompletion branch from 8f88f2f to 0edd0b8 Compare March 30, 2018 17:27

AmadeusW reviewed Mar 30, 2018

View reviewed changes

jcouv added the Area-IDE label Mar 30, 2018

AmadeusW mentioned this pull request Apr 13, 2018

NullReferenceException when displaying tooltip for the suggestion mode item #26145

Closed

jinujoseph added this to the 15.8 milestone Apr 15, 2018

dpoeschl and others added 17 commits May 10, 2018 16:10

Implementation and cleanup of completion

c2ae761

NoWarn for some reason

97657fa

Update Editor NuGet references

35196e0

Actually update editor nugets

5d4652a

TextInternal version

983d67b

Use a usable textinternal

555d529

New NuGets

abbb9ea

Newer NuGets

3c66d37

Disable completion during Inline Rename

cadd494

Update NuGets

16919a4

New initial selection handling WIP

b5d045b

Newer NuGets

17b5144

Race condition avoided...

33bff91

Fixup Packages

943cced

It builds again

d0095d8

Update NuGets and build

8b9ac6b

dpoeschl removed request for a team May 14, 2018 21:38

dpoeschl force-pushed the features/AsyncCompletion branch from 7136c48 to 8b9ac6b Compare May 14, 2018 21:40

dpoeschl and others added 12 commits June 4, 2018 12:46

Remove another trace of TestWaitIndicator

ffdd4fe

Begin to explain IFeaturesCustomCommitCompletionProvider

33facbb

More TODO links

a729eb2

Need to export the TestWaitIndicator

626e95d

Skip failing tests

c7c2318

VB Test Updates

02be360

fixing broken oldCompletion unit tests

89802a1

Skip the rest of the failing VB tests

9d37ced

move intellisense tests

17aaa26

expect to fix 41 of 54 failures

e924113

correct fix for generatetype tests

8ec341e

fixing one more test

85db98a

ivanbasov and others added 3 commits June 7, 2018 12:03

more tests fixed

ffc0763

Update NuGets

935eced

Fix a couple snippet tests

4c734f1

dpoeschl commented Jun 7, 2018

View reviewed changes

ivanbasov added 2 commits June 7, 2018 16:06

fixed flaky tests

ca903bf

Merge branch 'master-vs-deps' into features/AsyncCompletion

ea29b6d

resolving split string literal tests

b2d4d95

ivanbasov approved these changes Jun 8, 2018

View reviewed changes

dpoeschl and others added 2 commits June 10, 2018 14:24

Disable VB tests for documented reasons

b56c3d8

fix the KnownImageIds.LocalVariable which was missed in the previous PR

4599877

jinujoseph modified the milestones: 15.8, 16.0 Jun 15, 2018

jinujoseph mentioned this pull request Jun 15, 2018

Switch to a more appropriate model for supporting prefix/suffix text in completoin items. #26764

Merged

tmat mentioned this pull request Aug 4, 2018

Implementation of new editor completion API #29016

Merged

ivanbasov closed this Aug 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of new editor completion API #25770

Implementation of new editor completion API #25770

dpoeschl commented Mar 27, 2018 •

edited

Loading

AmadeusW left a comment

AmadeusW Mar 30, 2018

jasonmalinowski May 22, 2018

AmadeusW Mar 30, 2018

AmadeusW Mar 30, 2018

AmadeusW Mar 30, 2018

AmadeusW Mar 30, 2018

sharwell May 8, 2018 •

edited

Loading

AmadeusW Jun 1, 2018

AmadeusW Mar 30, 2018

AmadeusW Mar 30, 2018

AmadeusW Mar 30, 2018

ivanbasov commented Jun 7, 2018

dpoeschl Jun 4, 2018

dpoeschl Jun 4, 2018

dpoeschl Jun 4, 2018

dpoeschl Jun 4, 2018

dpoeschl Jun 4, 2018

dpoeschl Jun 4, 2018

dpoeschl Jun 4, 2018

dpoeschl Jun 4, 2018

dpoeschl Jun 4, 2018

dpoeschl Jun 4, 2018

ivanbasov commented Jun 7, 2018

ivanbasov commented Aug 8, 2018

Implementation of new editor completion API #25770

Implementation of new editor completion API #25770

Conversation

dpoeschl commented Mar 27, 2018 • edited Loading

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?

Test documentation updated?

AmadeusW left a 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 May 8, 2018 • edited Loading

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

ivanbasov commented Jun 7, 2018

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

Choose a reason for hiding this comment

ivanbasov commented Jun 7, 2018

ivanbasov commented Aug 8, 2018

dpoeschl commented Mar 27, 2018 •

edited

Loading

sharwell May 8, 2018 •

edited

Loading