Skip to content

Conversation

RikkiGibson
Copy link
Member

@RikkiGibson RikkiGibson commented Jul 7, 2025

Related to #78755.

Adjusts the scripting file path completion providers to work with #:project.

Remaining issues:

  • It feels like a new completion list should appear when a / or \ character is typed, so that you are guided through referencing a project in a nested folder. I'm not sure how to do that yet. Hitting ctrl+space works at least.
  • There are no recommenders for the directive names themselves project/property/package and so on.
  • When auto-save is on, design-time build errors tend to pop up for a lot of intermediate states of the completions. And when the errors are finally resolved, the error box is still there.. it's quite coarse grained. Possibly the box should go away if the errors go away.
fbp-project-completion.mp4

@@ -25,6 +27,6 @@ public LoadDirectiveCompletionProvider()

protected override string DirectiveName => "load";

protected override bool TryGetStringLiteralToken(SyntaxTree tree, int position, out SyntaxToken stringLiteral, CancellationToken cancellationToken)
=> DirectiveCompletionProviderUtilities.TryGetStringLiteralToken(tree, position, SyntaxKind.LoadDirectiveTrivia, out stringLiteral, cancellationToken);
protected override bool TryGetCompletionPrefix(SyntaxTree tree, int position, [NotNullWhen(true)] out string? literalValue, out TextSpan textSpan, CancellationToken cancellationToken)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
protected override bool TryGetCompletionPrefix(SyntaxTree tree, int position, [NotNullWhen(true)] out string? literalValue, out TextSpan textSpan, CancellationToken cancellationToken)
protected override bool TryGetCompletionPrefix(SyntaxTree tree, int position, [NotNullWhen(returnValue: true)] out string? literalValue, out TextSpan textSpan, CancellationToken cancellationToken)

Copy link
Member

Choose a reason for hiding this comment

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

After doing a search, I realize we are pretty inconsistent about this.

@RikkiGibson
Copy link
Member Author

It looks like the other completion providers have tests, this one should too.

@@ -70,6 +70,7 @@ public void TestCompletionProviderOrder()
// Built-in interactive providers
typeof(LoadDirectiveCompletionProvider),
typeof(ReferenceDirectiveCompletionProvider),
typeof(ProjectDirectiveCompletionProvider),
Copy link
Member

Choose a reason for hiding this comment

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

As per the prior meeting, i don't think this should be built into roslyn. It should just use hte public API surface.

Copy link
Member Author

@RikkiGibson RikkiGibson Jul 7, 2025

Choose a reason for hiding this comment

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

So should I move this over to dotnet/sdk where the file-based apps CLI work is being done? Or elsewhere? Are there any samples of how to package/import third party completion providers that I can work from?

Copy link
Member Author

@RikkiGibson RikkiGibson Jul 7, 2025

Choose a reason for hiding this comment

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

It looks like just shipping an analyzer dll which has the completion provider types in it should cause it to get picked up, a la the following completion provider in AspNetCoreAnalyzers https://github.com/dotnet/aspnetcore/blob/8883b98afedee35e8360db3f24ce8cd2e17217ad/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedLanguage/RoutePatternCompletionProvider.cs#L28

@CyrusNajmabadi
Copy link
Member

Adjusts the scripting file path completion providers to work with #:project.

I'm very wary about this. Why are project references happening now in a world where we're doing single file apps? What does this mean for all sort of our scenarios. For example, will we have the single files showing up in the normal workspace? With what sort of project, etc. etc.

@RikkiGibson
Copy link
Member Author

RikkiGibson commented Jul 7, 2025

Adjusts the scripting file path completion providers to work with #:project.

I'm very wary about this. Why are project references happening now in a world where we're doing single file apps? What does this mean for all sort of our scenarios. For example, will we have the single files showing up in the normal workspace? With what sort of project, etc. etc.

We discussed this in #78945 and offline, and arrived at a design that "file-based programs" would simply be loaded in the host workspace along with ordinary projects, which was implemented in #78975.

@RikkiGibson
Copy link
Member Author

I think I need guidance here on where we want to put this stuff. @jasonmalinowski @CyrusNajmabadi.

It sounds like we agree that #:package etc diagnostics, need to be reported via IDiagnosticProvider in the language server. So definitely some stuff will be in a "Roslyn internal" location, or, at least in the Roslyn repo.

The question is where do we put "other stuff", like the #:project completions in this PR, which could theoretically live outside of the Roslyn repo.

  1. We could put it in dotnet/sdk repo, or some other external repo, but, it creates a complication for how to ship the DLL(s). SDK itself doesn't really ship Roslyn extensions currently (analyzers, editor features, etc), from what I can see. The analyzers we sometimes refer to as "SDK analyzers" ship from dotnet/roslyn-analyzers repo.
  2. We could ship new dll(s) out of Roslyn repo named e.g. Microsoft.CodeAnalysis.FileBasedProgramsFeatures, which would ship and load automatically only with the language server (for now, at least).
  3. We could implement the editor features "in-box" in Microsoft.CodeAnalysis.Features, as this PR currently does, and as the current scripting completion providers are implemented.

My preference would be to do (3).

@jasonmalinowski
Copy link
Member

I don't imagine adding additional DLLs to the SDK would be much of an issue (at least compared to the other problems!) The bigger problem I see is how the actual discovery would work. We've discussed having NuGet or DLL-provided completion providers, and we might have some support in VS for Windows, but we'd probably have to do some work to even get VS Code to do that.

I also have specific concerns about the wisdom of putting a completion provider in the SDK too -- if that's having to touch a network, that means we're putting a very performance and security sensitive component into something that is by nature supposed to stay around for awhile. We've learned the hard way that putting generators into the SDK means a bad generator can break performance and it's challenging to get people to upgrade without servicing an SDK or NuGet package.

I see a lot of downsides of 1; I don't see a meaningful upside, but maybe these issues are just one we have to work through.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants