Intellisense for cast, indexers and operators #47511

MaStr11 · 2020-09-07T11:18:50Z

A test class with indexers, conversions, and operators can be found here:
https://gist.github.com/MaStr11/b87367ac2cad5d7554d89a8c8379081b

MaStr11 · 2020-09-11T11:17:02Z

@CyrusNajmabadi I started a draft to see what building blocks are needed to achieve this. I started with enhancing SymbolCompletionProvider.cs but I'm not sure whether that is the right place to implement this. The symbols for the completion list are calculated by the CSharpRecommendationServiceRunner.cs and I can't tell whether we should add symbols there (see 495ce89. I find the service quite complicated and I don't know if symbols like indexers and operators should be added there or in a separate place). The other problem is, that SymbolCompletionProvider has some simple rules on how to apply the completion (basically only adding text). But in the indexer/operator case we need to replace previous tokens (see screencast above), so I needed to override GetChangeAsync.

I think it would be better to implement this feature in its own CompletionProvider without relying on the CSharpRecommendationServiceRunner. I think the best candidate would be AbstractSymbolCompletionProvider or even LSPCompletionProvider.

What do you think is the best way forward?

PS: No need to review the PR. The code is only prototyped to get the screencast running.

MaStr11

@CyrusNajmabadi Please see my last comment from 3 days ago #47511 (comment).

I gave a dedicated CompletionProvider a try in 31f1090. This means, that the explicit cast completion is now duplicated. I did this, so you can compare both approaches.

Implementing a dedicated provider seems preferable to me, because of:

Simpler fine control over the circumstances, when to offer operator/indexer symbols. syntaxTree.FindTokenOnLeftOfPosition(position).IsKind(SyntaxKind.DotToken) seems to be sufficient and simple. The CSharpRecommendationServiceRunner, on the other hand, does a lot of work before actually calculating the symbols to provide and I think it will be complicated to decide there if the new symbols should be added or not.
SymbolCompletionItem contains a lot of functionality needed (GetDescriptionAsync, GetSymbolsAsync, etc.). This limits the need to access functionality provided by SymbolCompletionProvider and its base class AbstractRecommendationServiceBasedCompletionProvider
SymbolCompletionProvider adds "SymbolInfo" as a property to the CompletionItem, but at least for the conversion, it seems preferable to add "SymbolEncoding" instead.
The TextChanges to apply are more complicated than what SymbolCompletionProvider provides and GetChangeAsync needs to be overridden anyway.

I need some feedback now, which of the both are more appropriate. Once I have directions on that, I can start writing tests (and fix the failing tests).

...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs

CyrusNajmabadi · 2020-09-14T17:33:56Z

Implementing a dedicated provider seems preferable to me,

this makes a ton of sense to me. I will try to get to reviewing this soon :)

CyrusNajmabadi · 2020-09-14T17:34:25Z

(i was on a vacation, so i didn't see this stuff until now).

...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs

MaStr11 · 2020-09-16T13:42:38Z

@CyrusNajmabadi Can I get a first review, please? If you want to play around with it, I created a gist with a class with indexers, operators, and conversions here:

https://gist.github.com/MaStr11/b87367ac2cad5d7554d89a8c8379081b

These parts are implemented:

explicit user-defined conversions
indexer

The explicit conversion is fixed like so:

// Before
expr.$$
// After
((float)expr).$$

But the spec in #46266 defined it, like so:

// Before
expr.$$
// After
(float)expr$$

I think both approaches are sensible. Do you want me to change it according to the spec (I missed the spec until I was finished)?

If there is more than one indexer defined, I currently suggest both overloads:

I think it should only show one overload and the and one more overload hint. What do you think?

There is a bug with the completion currently:
Steps to reproduce

Open completion after the dot
Type some text
Commit the conversion

Expected

After typing starts, the conversion suggestion is no longer shown.

Actual

The conversion suggestion stays on the list and can be selected. The first input (f) is highlighted (bold), but the second input (l) is not.

This is something that needs to be fixed along with the sorting (indexer/operator/conversion should go to the end of the list).

CyrusNajmabadi · 2020-09-16T17:30:30Z

The explicit conversion is fixed like so:

The way you're doing this is the right way :) we should end with ((foo)bar).$$

CyrusNajmabadi · 2020-09-16T17:31:19Z

I think it should only show one overload and the and one more overload hint. What do you think?

I woudl just show this as:

this[]

indicating there's an indexer, using the normal indexer syntax. for methods we don't include parameter types, so i would not include them here. The description for that item will give you that info.

CyrusNajmabadi · 2020-09-16T17:33:12Z

The conversion suggestion stays on the list and can be selected. The first input (f) is highlighted (bold), but the second input (l) is not.

that's bizarre. woudl want this fixed. this list should filter and highlight normally :)

CyrusNajmabadi · 2020-09-16T17:33:53Z

Also, once committed, we should not end up with:

We'd want to end with ((float)w).$$

...Features/CSharpTest/Completion/CompletionProviders/OperatorIndexerCompletionProviderTests.cs

...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs

CyrusNajmabadi

Looking really good so far! :)

CyrusNajmabadi · 2020-09-16T22:07:50Z

LMK when you want a pass.

...rtable/Completion/CompletionProviders/OperatorsAndIndexer/UnnamedSymbolCompletionProvider.cs

...pletion/CompletionProviders/OperatorsAndIndexer/UnnamedSymbolCompletionProvider_Operators.cs

genlu · 2021-03-19T17:43:39Z

...pletion/CompletionProviders/OperatorsAndIndexer/UnnamedSymbolCompletionProvider_Operators.cs

+            if (opPosition.HasFlag(OperatorPosition.Infix))
+                return await ReplaceTextAfterOperatorAsync(document, item, text: $" {item.DisplayText} ", cancellationToken).ConfigureAwait(false);
+
+            if (opPosition.HasFlag(OperatorPosition.Postfix))


So increment/decrement operator will always complete as postfix (also as displayed in inline description). Not sure we make the enum a flag then, it might make the intention clearer w/o that.

genlu · 2021-03-19T18:10:59Z

src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests.vb

@@ -7688,6 +7688,65 @@ class C
            End Using
        End Function

+        <WorkItem(47511, "https://github.com/dotnet/roslyn/pull/47511")>
+        <WpfFact, Trait(Traits.Feature, Traits.Features.Completion)>
+        Public Sub ConversionsOperatorsAndIndexerAreShownBelowMethodsAndPropertiesAndBeforeUnimportedItems()


Please test the commit and filter behavior for the new provider, since these tests go through Editor layer and act more like integration tests for completion.

genlu · 2021-03-19T18:13:31Z

...pletion/CompletionProviders/OperatorsAndIndexer/UnnamedSymbolCompletionProvider_Operators.cs

+            }
+
+            var opCharacters = ImmutableArray.CreateRange(filterCharacters);
+            s_operatorRules = CompletionItemRules.Default


Are there tests for the commit/filter char modification?

genlu · 2021-03-19T18:15:49Z

...rtable/Completion/CompletionProviders/OperatorsAndIndexer/UnnamedSymbolCompletionProvider.cs

+            };
+        }
+
+        public override async Task<CompletionDescription?> GetDescriptionAsync(


Please add some tests for description.

genlu

LGTM besides some minor suggestions. Can't wait to try this out, thanks! :)

CyrusNajmabadi · 2021-03-19T21:51:35Z

Will add tests tahnks!

ghost

Auto-approval

CyrusNajmabadi · 2021-03-19T21:53:32Z

ack. i thought i'd already pushed a change, so auto-merge wouldn't merge in. but it was on a machien that hadn't pulled, so the push got rejected. i will make a followup PR for this (including hte ability to disable this feature if we want).

CyrusNajmabadi · 2021-03-19T23:03:02Z

thanks @MaStr11 for the great feature. @jinujoseph @mikadumont @kendrahavens for visibility on this! :)

@CyrusNajmabadi

Follow up to dotnet#47511 I missed placing the `[Fact]` attribute on one test method. @CyrusNajmabadi

The recent completion rewrite makes completion faster for a large number of cases, but it has a couple of issues: * Any completion provider that needs to make edits beyond the few we've special cased doesn't work correctly. Roslyn is looking to add more of these, such as dotnet/roslyn#47511. * Completion providers that we do special case can be _slow_. Override and partial method completion in big types is painful, taking over a minute to show up sometimes. To resolve this, I've added support for async edits by adding a new completion end point: /completion/afterInsert. It takes the item that was inserted, the file, and the new cursor position. It relies on the InsertText that was inserted still allowing for resolving the completion in the same way as before, so I had to force a few providers that don't behave well here to be eagerly resolved, regardless. I also kept import completion using the standard /completion/resolve step to resolve extra edits, as they don't need to modify the current cursor location at all.

MaStr11 commented Sep 14, 2020

View reviewed changes

CyrusNajmabadi self-requested a review September 14, 2020 17:34

CyrusNajmabadi self-assigned this Sep 14, 2020

CyrusNajmabadi reviewed Sep 14, 2020

View reviewed changes

...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs Outdated Show resolved Hide resolved