-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Intellisense for cast, indexers and operators #47511
Conversation
@CyrusNajmabadi I started a draft to see what building blocks are needed to achieve this. I started with enhancing 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 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 bySymbolCompletionProvider
and its base classAbstractRecommendationServiceBasedCompletionProvider
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
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
this makes a ton of sense to me. I will try to get to reviewing this soon :) |
(i was on a vacation, so i didn't see this stuff until now). |
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
@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:
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: There is a bug with the completion currently:
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). |
The way you're doing this is the right way :) we should end with |
I woudl just show this as:
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. |
that's bizarre. woudl want this fixed. this list should filter and highlight normally :) |
...Features/CSharpTest/Completion/CompletionProviders/OperatorIndexerCompletionProviderTests.cs
Outdated
Show resolved
Hide resolved
...Features/CSharpTest/Completion/CompletionProviders/OperatorIndexerCompletionProviderTests.cs
Outdated
Show resolved
Hide resolved
...Features/CSharpTest/Completion/CompletionProviders/OperatorIndexerCompletionProviderTests.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
...Features/CSharp/Portable/Completion/CompletionProviders/OperatorIndexerCompletionProvider.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really good so far! :)
LMK when you want a pass. |
...rtable/Completion/CompletionProviders/OperatorsAndIndexer/UnnamedSymbolCompletionProvider.cs
Show resolved
Hide resolved
...rtable/Completion/CompletionProviders/OperatorsAndIndexer/UnnamedSymbolCompletionProvider.cs
Show resolved
Hide resolved
...pletion/CompletionProviders/OperatorsAndIndexer/UnnamedSymbolCompletionProvider_Operators.cs
Show resolved
Hide resolved
if (opPosition.HasFlag(OperatorPosition.Infix)) | ||
return await ReplaceTextAfterOperatorAsync(document, item, text: $" {item.DisplayText} ", cancellationToken).ConfigureAwait(false); | ||
|
||
if (opPosition.HasFlag(OperatorPosition.Postfix)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
|
||
var opCharacters = ImmutableArray.CreateRange(filterCharacters); | ||
s_operatorRules = CompletionItemRules.Default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there tests for the commit/filter char modification?
}; | ||
} | ||
|
||
public override async Task<CompletionDescription?> GetDescriptionAsync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some tests for description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides some minor suggestions. Can't wait to try this out, thanks! :)
Will add tests tahnks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto-approval
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). |
thanks @MaStr11 for the great feature. @jinujoseph @mikadumont @kendrahavens for visibility on this! :) |
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.
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.
Fixes #46266
A test class with indexers, conversions, and operators can be found here:
https://gist.github.com/MaStr11/b87367ac2cad5d7554d89a8c8379081b