Merge CompletionServiceWithProviders into CompletionService#61943
Merge CompletionServiceWithProviders into CompletionService#61943genlu merged 15 commits intodotnet:mainfrom
Conversation
Remove `CompletionService.GetProviders(ImmutableHashSet<string> roles, CompletionTrigger trigger)`
Remove CompletionSerivce.GetProviders(ImmutableHashSet<string> roles)
It can't be implemented externally, and internal override should use the internal one.
|
|
||
| internal abstract CompletionRules GetRulesImpl(); | ||
|
|
||
| internal sealed override void FilterItems( |
There was a problem hiding this comment.
This is added so we can move TS to use the internal overload instead, which will provide some perf benefit plus we can then make the public overload non-virtual.
|
This is fine (and i don't think needs reviews). We've established that adding/removing overrides isn't an ABI/API break (as long as the overrides don't use covariant returns, and have the same parameter names. |
|
I believe this is also safe. virt to non-virt is not a break. All the existing virtcalls will work properly even if hte methods become non-virt. |
|
|
||
| return false; | ||
| return completionService.ShouldTriggerCompletion( | ||
| document.Project, document.Project.LanguageServices, sourceText, triggerLocation.Position, roslynTrigger, options, document.Project.Solution.Options, _roles); |
There was a problem hiding this comment.
Unrelated to the merge, but we didn't pass in TextView roles to the method call, which is fixed here
| /// </summary> | ||
| [Obsolete("Built-in providers will be ignored in a future release, please make them MEF exports instead.")] | ||
| protected virtual ImmutableArray<CompletionProvider> GetBuiltInProviders() | ||
| => ImmutableArray<CompletionProvider>.Empty; |
There was a problem hiding this comment.
does anyone override this? can we remove now (or in a quick followup pr?)
There was a problem hiding this comment.
Both F# and TS are still using this, unfortunately
https://github.com/dotnet/fsharp/blob/main/vsintegration/src/FSharp.Editor/Completion/CompletionService.fs
https://devdiv.visualstudio.com/DevDiv/_git/TypeScript-VS?path=/VS/LanguageService/TypeScriptLanguageService/Features/Completion/TypeScriptCompletionService.cs&version=GBmain&line=103&lineEnd=104&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents
| /// <remarks> | ||
| /// Backward compatibility only. | ||
| /// </remarks> | ||
| public CompletionRules GetRules() |
There was a problem hiding this comment.
consider obsoleting in future PR.
| } | ||
|
|
||
| internal virtual bool SupportsTriggerOnDeletion(CompletionOptions options) | ||
| => options.TriggerOnDeletion == true; |
There was a problem hiding this comment.
is thsi overridden?
| internal virtual async Task<CompletionDescription?> GetDescriptionAsync(Document document, CompletionItem item, CompletionOptions options, SymbolDescriptionOptions displayOptions, CancellationToken cancellationToken = default) | ||
| { | ||
| var provider = GetProvider(item); | ||
| if (provider is null) |
There was a problem hiding this comment.
I don't believe it's possible if the item is created "properly" via our system, but theoretically someone could pass in an arbitrary item they created themselves. So we'd need this in place to guard that
| { | ||
| return Task.FromResult(CompletionChange.Create(new TextChange(item.Span, item.DisplayText))); | ||
| var provider = GetProvider(item); | ||
| if (provider != null) |
There was a problem hiding this comment.
can this now ever be null?
| } | ||
|
|
||
| public void SuppressPartialSemantics() | ||
| => _completionServiceWithProviders._suppressPartialSemantics = true; |
| { | ||
| return GetEnumerator(); | ||
| } | ||
| } |
There was a problem hiding this comment.
was this a basic port? did you change anything? this will be easier to review if you put this in the CompletionServiceWithProviders file. then once the review it over, break into separate file.
There was a problem hiding this comment.
no code change in the method bodies. I have code movement in separate commits, which should show you what changed clearer than review everything
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
awesome. we should still take to API review to validate the rules on ABI compat. But i think this is 100% safe.
|
Mark this as draft until approved by API board |
|
API change is approved, ready for review |
See #61977 for more details on the change.