Skip to content

Conversation

@genlu
Copy link
Member

@genlu genlu commented Feb 5, 2025

No description provided.

@ghost ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 5, 2025
@dotnet-policy-service dotnet-policy-service bot added VSCode Needs API Review Needs to be reviewed by the API review council labels Feb 5, 2025
/// <inheritdoc cref="LspRequestContext.GetRequiredDocument()"/>
internal Document GetRequiredDocument() => context.GetRequiredDocument();

internal T GetRequiredService<T>() where T : class => context.GetRequiredService<T>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels very broad in exposing everything

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessarily opposed to this - they'd still be limited to accessing services that are directly made available by external access (since they have to pass the generic type to this call)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but they have access to the solution. So they can do solution.Services.GetService<T>. So i don't see why we need to broaden the surface area here. We should make EA as minimal as possible. only what is absolutley necessary to expose.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this we can MEF export services and use them in the language server extension itself, right (like this)? I think it provides more flexibility

@genlu genlu force-pushed the CopilotExternalAccess branch from 966bccf to 55cdd6f Compare February 5, 2025 04:50
/// <inheritdoc cref="LspRequestContext.GetRequiredDocument()"/>
internal Document GetRequiredDocument() => context.GetRequiredDocument();

internal T GetRequiredService<T>() where T : class => context.GetRequiredService<T>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessarily opposed to this - they'd still be limited to accessing services that are directly made available by external access (since they have to pass the generic type to this call)

@genlu genlu marked this pull request as ready for review February 7, 2025 01:51
@genlu genlu requested a review from a team as a code owner February 7, 2025 01:51
@genlu genlu force-pushed the CopilotExternalAccess branch from c3eec79 to c196fae Compare February 7, 2025 02:17
@genlu genlu requested a review from a team as a code owner February 7, 2025 20:04
internal readonly struct CopilotRequestContext(RequestContext context)
{
/// <inheritdoc cref="RequestContext.Solution"/>
public Solution? Solution => context.Solution;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we have requireslspsolution this should always be not null (would potentially throw here if context.Solution is null)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public Solution? Solution => context.Solution;
public Solution Solution => context.Solution ?? throw new InvalidOperationException();

You meant something like this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup


namespace Microsoft.CodeAnalysis.LanguageServer.ExternalAccess.Copilot;

internal abstract class AbstractCopilotLspServiceFactory : ILspServiceFactory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need the factory right now? If not I would avoid exposing it as at some point I may want to remove the factory entirely.

Copy link
Member Author

@genlu genlu Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I can remove it, included it since I saw compiler-dev-sdk uses it. Just to make sure, I could away use ExportCopilotStatelessLspServiceAttribute to export single instance services in the server, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup thats right

@genlu genlu enabled auto-merge February 12, 2025 20:03
@genlu genlu merged commit 36036f8 into dotnet:main Feb 12, 2025
28 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Feb 12, 2025
@genlu genlu deleted the CopilotExternalAccess branch February 12, 2025 23:49
@akhera99 akhera99 modified the milestones: Next, 17.14 P2 Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE Needs API Review Needs to be reviewed by the API review council untriaged Issues and PRs which have not yet been triaged by a lead VSCode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants