[17.14] Move Copilot context provider to EA.Copilot and handler to LanguageServer #78322

genlu · 2025-04-25T18:29:27Z

backport #77973

…rver

CyrusNajmabadi · 2025-04-25T18:32:49Z

src/Features/ExternalAccess/Copilot/Completion/CodeSnippetItem.cs

+
+namespace Microsoft.CodeAnalysis.ExternalAccess.Copilot.Completion;
+
+internal record CodeSnippetItem : IContextItem


Suggested change

internal record CodeSnippetItem : IContextItem

internal selaed record CodeSnippetItem : IContextItem

CyrusNajmabadi · 2025-04-25T18:32:57Z

src/Features/ExternalAccess/Copilot/Completion/CodeSnippetItem.cs

+
+internal record CodeSnippetItem : IContextItem
+{
+    public CodeSnippetItem(string uri, string value, string[]? additionalUris = null, int importance = Completion.Importance.Default)


primary constructor?

CyrusNajmabadi · 2025-04-25T18:33:13Z

src/Features/ExternalAccess/Copilot/Completion/CodeSnippetItem.cs

+    public string[]? AdditionalUris { get; init; }
+
+    [JsonPropertyName("importance")]
+    public int Importance { get; init; }


can just be [property: attributes on primary constructor parameters.

consider doc'ing these.

CyrusNajmabadi · 2025-04-25T18:33:46Z

src/Features/ExternalAccess/Copilot/Completion/ICSharpCopilotContextProviderService.cs

+
+internal interface ICSharpCopilotContextProviderService
+{
+    IAsyncEnumerable<IContextItem> GetContextItemsAsync(Document document, int position, IReadOnlyDictionary<string, object> activeExperiments, CancellationToken cancellationToken);


what's teh object value for activeExperiments?

CyrusNajmabadi · 2025-04-25T18:34:30Z

src/Features/ExternalAccess/Copilot/Completion/IContextItem.cs

+namespace Microsoft.CodeAnalysis.ExternalAccess.Copilot.Completion;
+
+[JsonDerivedType(typeof(CodeSnippetItem))]
+[JsonDerivedType(typeof(TraitItem))]


ni: call these CodeSnippetContextItem and TraitContextItem to make it clearer what they are.

CyrusNajmabadi · 2025-04-25T18:34:53Z

src/Features/ExternalAccess/Copilot/Completion/IContextProvider.cs

+        Document document,
+        int position,
+        IReadOnlyDictionary<string, object> activeExperiments,
+        Func<ImmutableArray<IContextItem>, CancellationToken, ValueTask> callback,


why pass an array, instead of just an itemt at a time, if it's a callback?

CyrusNajmabadi · 2025-04-25T18:35:11Z

src/Features/ExternalAccess/Copilot/Completion/TraitItem.cs

+
+namespace Microsoft.CodeAnalysis.ExternalAccess.Copilot.Completion;
+
+internal record TraitItem : IContextItem


Suggested change

internal record TraitItem : IContextItem

internal sealed record TraitItem : IContextItem

CyrusNajmabadi · 2025-04-25T18:35:22Z

src/Features/ExternalAccess/Copilot/Completion/TraitItem.cs

+
+internal record TraitItem : IContextItem
+{
+    public TraitItem(string name, string value, int importance = Completion.Importance.Default)


primary constructor, with attribute on params.

CyrusNajmabadi · 2025-04-25T18:35:32Z

src/Features/ExternalAccess/Copilot/Completion/TraitItem.cs

+
+namespace Microsoft.CodeAnalysis.ExternalAccess.Copilot.Completion;
+
+internal record TraitItem : IContextItem


what does 'trait' mean here?

CyrusNajmabadi · 2025-04-25T18:35:45Z

src/Features/ExternalAccess/Copilot/Internal/Completion/CSharpContextProviderService.cs

+[Shared]
+[Export(typeof(ICSharpCopilotContextProviderService))]


Suggested change

[Shared]

[Export(typeof(ICSharpCopilotContextProviderService))]

[Export(typeof(ICSharpCopilotContextProviderService)), Shared]

nit.

CyrusNajmabadi · 2025-04-25T18:35:53Z

src/Features/ExternalAccess/Copilot/Internal/Completion/CSharpContextProviderService.cs

+    public CSharpContextProviderService([ImportMany] IEnumerable<IContextProvider> providers)
+    {
+        Providers = providers.ToImmutableArray();
+    }


primary constructor.

CyrusNajmabadi · 2025-04-25T18:45:25Z

src/Features/ExternalAccess/Copilot/Internal/Completion/CSharpContextProviderService.cs

+            {
+            }
+        },
+        cancellationToken));


we have a better api for this. ProducerConsumer. You can write this as:

return ProducerConsumer<IContextItem>.RunAsync( static (callback, args, cancellationToken) => { await RoslynParallel.ForEachAsync( [email protected], cancellationToken, (provider, cancellationToken) => provider.ProviderContextItemsAsync( args.document, args.position, args.activeExperiments, callback, cancellationToken).ConfigureAwait(False); } args: (@this: this, document, position, activeExperiments), cancellationToken);

CyrusNajmabadi · 2025-04-25T18:46:03Z

src/Features/ExternalAccess/Copilot/Microsoft.CodeAnalysis.ExternalAccess.Copilot.csproj

@@ -29,6 +30,10 @@
    <ProjectReference Include="..\..\Core\Portable\Microsoft.CodeAnalysis.Features.csproj" />
  </ItemGroup>

+  <ItemGroup>
+    <PackageReference Include="Microsoft.VisualStudio.Threading" />


this seems undesirable. this will also make it so we can't use this code in vscode. i dont' see anything that would make this necessary.

CyrusNajmabadi · 2025-04-25T18:46:36Z

...deAnalysis.LanguageServer/LanguageServer/Handler/CopilotCompletion/CodeSnippetContextItem.cs

+
+namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Copilot;
+
+internal sealed class CodeSnippetContextItem


called 'ContextItem', but doesn't implement `IContextItem'?

CyrusNajmabadi · 2025-04-25T18:47:08Z

....CodeAnalysis.LanguageServer/LanguageServer/Handler/CopilotCompletion/ContextResolveParam.cs

+
+namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Copilot;
+
+internal sealed class ContextResolveParam


this is a fairly vague name. consider calling CopilotContextResultParam

CyrusNajmabadi · 2025-04-25T18:47:47Z

...ageServer/LanguageServer/Handler/CopilotCompletion/CopilotCompletionResolveContextHandler.cs

+[Shared]
+[Method(MethodName)]
+[ExportCSharpVisualBasicStatelessLspService(typeof(CopilotCompletionResolveContextHandler), WellKnownLspServerKinds.Any)]


Suggested change

[Shared]

[Method(MethodName)]

[ExportCSharpVisualBasicStatelessLspService(typeof(CopilotCompletionResolveContextHandler), WellKnownLspServerKinds.Any)]

[Method(MethodName)]

[ExportCSharpVisualBasicStatelessLspService(typeof(CopilotCompletionResolveContextHandler), WellKnownLspServerKinds.Any), Shared]

CyrusNajmabadi · 2025-04-25T18:47:56Z

...ageServer/LanguageServer/Handler/CopilotCompletion/CopilotCompletionResolveContextHandler.cs

+
+    [ImportingConstructor]
+    [Obsolete("This exported object must be obtained through the MEF export provider.", error: true)]
+    public CopilotCompletionResolveContextHandler(ICSharpCopilotContextProviderService contextProviderService)


primary constructor

CyrusNajmabadi · 2025-04-25T18:48:52Z

...ageServer/LanguageServer/Handler/CopilotCompletion/CopilotCompletionResolveContextHandler.cs

+        var builder = ImmutableArray.CreateBuilder<IContextItem>();
+        var activeExperiments = param.GetUnpackedActiveExperiments();
+
+        await foreach (var item in ContextProviderService.GetContextItemsAsync(document, position, activeExperiments, cancellationToken).ConfigureAwait(false))


since your'e not streaming anything, you can just make GetContextItemsAsync return an immtuable array. in that case, the impl of it gets a lot simpler too.

i would recomend that htis stream though. that way the host can call into this, and we do the best we can. and it cancels us once enough time has passed.

CyrusNajmabadi · 2025-04-25T18:49:00Z

...oft.CodeAnalysis.LanguageServer/LanguageServer/Handler/CopilotCompletion/TraitContextItem.cs

+
+namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Copilot;
+
+internal sealed class TraitContextItem


doesn't implement IContextItem?

CyrusNajmabadi · 2025-04-25T18:52:41Z

Since this is a backport, i'm ok with it being mechanical. but i do think we should clean up some code here, and potentially rethink how some things are wokring.

Move Copilot context provider to EA.Copilot and handler to LanguageSe…

d037a59

…rver

ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 25, 2025

dotnet-policy-service bot added the VSCode label Apr 25, 2025

CyrusNajmabadi reviewed Apr 25, 2025

View reviewed changes


		namespace Microsoft.CodeAnalysis.ExternalAccess.Copilot.Completion;

		internal record CodeSnippetItem : IContextItem

	internal record CodeSnippetItem : IContextItem
	internal selaed record CodeSnippetItem : IContextItem


		namespace Microsoft.CodeAnalysis.ExternalAccess.Copilot.Completion;

		internal record TraitItem : IContextItem

	internal record TraitItem : IContextItem
	internal sealed record TraitItem : IContextItem

		[Shared]
		[Export(typeof(ICSharpCopilotContextProviderService))]


		namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Copilot;

		internal sealed class CodeSnippetContextItem


		namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Copilot;

		internal sealed class ContextResolveParam


		namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Copilot;

		internal sealed class TraitContextItem

[17.14] Move Copilot context provider to EA.Copilot and handler to LanguageServer #78322

Are you sure you want to change the base?

[17.14] Move Copilot context provider to EA.Copilot and handler to LanguageServer #78322

Uh oh!

Conversation

genlu commented Apr 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Apr 25, 2025

Uh oh!

Uh oh!