dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.1k stars 4.04k forks source link

Add a public API for getting expanded completions #46432

Open 333fred opened 4 years ago

333fred commented 4 years ago

Today, we don't have a public API for getting expanded (unimported) completions from CompletionService.GetCompletionsAsync. This means that consumers like Omnisharp are unable to use the provider without resorting to private reflection, which isn't great. I'd like to propose that we add a new overload:

        /// <summary>
        /// Gets the completions available at the caret position.
        /// </summary>
        /// <param name="document">The document that completion is occurring within.</param>
        /// <param name="caretPosition">The position of the caret after the triggering action.</param>
        /// <param name="trigger">The triggering action.</param>
        /// <param name="roles">Optional set of roles associated with the editor state.</param>
        /// <param name="options">Optional options that override the default options.</param>
        /// <param name="includeExpanded">Include expanded items, such as methods or types from unimported namespaces.</param>
        /// <param name="cancellationToken"></param>
        public abstract Task<CompletionList> GetCompletionsAsync(
            Document document,
            int caretPosition,
            CompletionTrigger trigger = default,
            ImmutableHashSet<string> roles = null,
            OptionSet options = null,
            bool includeExpanded = false,
            CancellationToken cancellationToken = default);

This could then be used by Omnisharp to implement unimported types completion, which they currently do not have.

Additionally, I'd like us to consider exposing the results of these changes as a series of individual line changes, not an entire text document. The reason for this is to work better with LSP: a CompletionItem in LSP is defined as this (I've stripped out irrelevant bits to this request, the full type definition can be found here):

interface CompletionItem {
    /**
     * An edit which is applied to a document when selecting this completion. When an edit is provided the value of
     * `insertText` is ignored.
     *
     * *Note:* The range of the edit must be a single line range and it must contain the position at which completion
     * has been requested.
     */
    textEdit?: TextEdit;

    /**
     * An optional array of additional text edits that are applied when
     * selecting this completion. Edits must not overlap (including the same insert position)
     * with the main edit nor with themselves.
     *
     * Additional text edits should be used to change text unrelated to the current cursor position
     * (for example adding an import statement at the top of the file if the completion item will
     * insert an unqualified type).
     */
    additionalTextEdits?: TextEdit[];
}

There is no space for a full document here. Omnisharp would have likely have to do some very hacky workarounds to get this into a good format. Additionally, this will be more friendly to network connections: for example, NullableReferenceTypeTests.cs is 4.5 MB. GitHub refuses to display it in the browser. It would be expensive to sync the full document over an LSP connection, particularly if that connection is to a codespaces instance.

CyrusNajmabadi commented 4 years ago

I don't think we need the overload. This can/shoudl jsut be controlled by the 'Options' object already passed in.

CyrusNajmabadi commented 4 years ago

Additionally, I'd like us to consider exposing the results of these changes as a series of individual line changes, not an entire text document.

This can be done on teh consumption side by doign a diff and determining what individual edits to make.

333fred commented 4 years ago

This can be done on teh consumption side by doign a diff and determining what individual edits to make.

Are there existing APIs in Roslyn for this? If not, we should definitely consider adding them on this side, as we're going to need them for our own completion provider.

333fred commented 4 years ago

I don't think we need the overload. This can/shoudl jsut be controlled by the 'Options' object already passed in.

I'd be fine with this, personally. The option for this today is internal, so that would need to be publicly exposed.

CyrusNajmabadi commented 4 years ago

Are there existing APIs in Roslyn for this?

I think there's IDocumentDifferenceService. However, i wouldn't even use that. I'd just use an off-the-shelf diff-merge-patch library for this basic string diffing task.

genlu commented 4 years ago

Proposal: either expose CompletionOptions.ShowItemsFromUnimportedNamespaces directly, or change it to a more generic CompletionOptions.ShowExpandedItems and expose that.

jinujoseph commented 4 years ago

Design meeting notes

For this request counter proposal is to let's create an external access layer for omnisharp and get them to move towards it. That will give us a clear understanding of API they are using which will also help us to understand if the LSP route will work for them in the future.

CyrusNajmabadi commented 1 week ago

Please work with Fred to determien if we need something here. If so, we shoudl schedule it. If not, we shoudl close this out.