-
Notifications
You must be signed in to change notification settings - Fork 4.1k
File-based program directive diagnostics in editor #79421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
dd2cf21
b7bd8bc
d988e6d
64ad4a3
17e6589
409d966
7abc1c1
79d8a9d
a459e54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
using System.Collections.Immutable; | ||
using System.Composition; | ||
using Microsoft.CodeAnalysis.Diagnostics; | ||
using Microsoft.CodeAnalysis.Host.Mef; | ||
using Microsoft.CodeAnalysis.LanguageServer.Handler; | ||
using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; | ||
using Microsoft.CodeAnalysis.Text; | ||
using Roslyn.LanguageServer.Protocol; | ||
|
||
namespace Microsoft.CodeAnalysis.LanguageServer.FileBasedPrograms; | ||
|
||
[Export(typeof(IDiagnosticSourceProvider)), Shared] | ||
[method: ImportingConstructor] | ||
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
internal class VirtualProjectXmlDiagnosticSourceProvider(VirtualProjectXmlProvider virtualProjectXmlProvider) : IDiagnosticSourceProvider | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
public bool IsDocument => true; | ||
|
||
public const string FileBasedPrograms = nameof(FileBasedPrograms); | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public string Name => FileBasedPrograms; | ||
|
||
public bool IsEnabled(ClientCapabilities clientCapabilities) => true; | ||
|
||
public ValueTask<ImmutableArray<IDiagnosticSource>> CreateDiagnosticSourcesAsync(RequestContext context, CancellationToken cancellationToken) | ||
{ | ||
ImmutableArray<IDiagnosticSource> sources = context.Document is null | ||
? [] | ||
: [new DiagnosticSource(context.Document, virtualProjectXmlProvider)]; | ||
|
||
return ValueTask.FromResult(sources); | ||
} | ||
|
||
private class DiagnosticSource(Document document, VirtualProjectXmlProvider virtualProjectXmlProvider) : IDiagnosticSource | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
public async Task<ImmutableArray<DiagnosticData>> GetDiagnosticsAsync(RequestContext context, CancellationToken cancellationToken) | ||
{ | ||
if (string.IsNullOrEmpty(document.FilePath)) | ||
return []; | ||
|
||
var simpleDiagnostics = await virtualProjectXmlProvider.GetCachedDiagnosticsAsync(document.FilePath, cancellationToken); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of asking for cached diags, why not just ask for the diags appropriate for Document. ANd if they have been cached they are returned, otherwise they are computed. I'm very wary about this producing stale data that is then not updated properly as changed come in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems plausible, I am actually wondering what would happen if we simply changed this to "get the virtual project" again, using whatever content is in the current version of the doc, which will take a second or two usually, and forward the diagnostics. Then I guess this source could be marked as live. In the medium term I think we also want to try to forward certain msbuild diagnostics (e.g. if a bad id or version is used with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...except that run-api wants to take a file path for the file-based program, which it will read from disk. That means the diagnostics coming back would still be stale until user hits save. For the short term I think it is going to be better to start with the "non-live" diagnostics which are essentially driven by design time build, and in medium term make the necessary adjustments to make the subset of diagnostics live, which can be made live. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this fairly high traffic, i.e.:
Would mean we'd be continually launching processes in the background to re-ask for the content? |
||
if (simpleDiagnostics.IsDefaultOrEmpty) | ||
return []; | ||
|
||
var diagnosticDatas = ImmutableArray.CreateBuilder<DiagnosticData>(simpleDiagnostics.Length); | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
foreach (var simpleDiagnostic in simpleDiagnostics) | ||
{ | ||
var location = new FileLinePositionSpan(simpleDiagnostic.Location.Path, simpleDiagnostic.Location.Span.ToLinePositionSpan()); | ||
var diagnosticData = new DiagnosticData( | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
id: "FBP", | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
category: FileBasedPrograms, | ||
message: simpleDiagnostic.Message, | ||
severity: DiagnosticSeverity.Error, | ||
defaultSeverity: DiagnosticSeverity.Error, | ||
isEnabledByDefault: true, | ||
warningLevel: 1, | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
customTags: ImmutableArray<string>.Empty, | ||
properties: ImmutableDictionary<string, string?>.Empty, | ||
projectId: document.Project.Id, | ||
location: new DiagnosticDataLocation(location, document.Id) | ||
); | ||
diagnosticDatas.Add(diagnosticData); | ||
} | ||
return diagnosticDatas.MoveToImmutable(); | ||
} | ||
|
||
public TextDocumentIdentifier? GetDocumentIdentifier() | ||
{ | ||
return !string.IsNullOrEmpty(document.FilePath) | ||
? new VSTextDocumentIdentifier { ProjectContext = ProtocolConversions.ProjectToProjectContext(document.Project), DocumentUri = document.GetURI() } | ||
: null; | ||
Comment on lines
+74
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indenting is a bit strange here. |
||
} | ||
|
||
public ProjectOrDocumentId GetId() | ||
{ | ||
return new ProjectOrDocumentId(document.Id); | ||
} | ||
|
||
public Project GetProject() | ||
{ | ||
return document.Project; | ||
} | ||
|
||
public bool IsLiveSource() => false; | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public string ToDisplayString() => $"{nameof(VirtualProjectXmlProvider)}.{nameof(DiagnosticSource)}"; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,18 +11,34 @@ | |
using System.Text.Json; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.CSharp; | ||
using Microsoft.CodeAnalysis.Diagnostics; | ||
using Microsoft.CodeAnalysis.Host.Mef; | ||
using Microsoft.CodeAnalysis.LanguageServer.Handler; | ||
using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; | ||
using Microsoft.CodeAnalysis.Text; | ||
using Microsoft.Extensions.Logging; | ||
using Roslyn.LanguageServer.Protocol; | ||
using Roslyn.Utilities; | ||
|
||
namespace Microsoft.CodeAnalysis.LanguageServer.FileBasedPrograms; | ||
|
||
[Export(typeof(VirtualProjectXmlProvider)), Shared] | ||
[method: ImportingConstructor] | ||
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
internal class VirtualProjectXmlProvider(DotnetCliHelper dotnetCliHelper) | ||
internal class VirtualProjectXmlProvider(IDiagnosticsRefresher diagnosticRefresher, DotnetCliHelper dotnetCliHelper) | ||
{ | ||
private readonly SemaphoreSlim _gate = new(initialCount: 1); | ||
private readonly Dictionary<string, ImmutableArray<SimpleDiagnostic>> _diagnosticsByFilePath = []; | ||
Comment on lines
+27
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this state just be held in FileBasedProgramsProjectSystem rather than being held here? It seems a bit odd that this provider method (that was previously stateless) gets the state. |
||
|
||
internal async ValueTask<ImmutableArray<SimpleDiagnostic>> GetCachedDiagnosticsAsync(string path, CancellationToken cancellationToken) | ||
{ | ||
using (await _gate.DisposableWaitAsync(cancellationToken)) | ||
{ | ||
_diagnosticsByFilePath.TryGetValue(path, out var diagnostics); | ||
return diagnostics; | ||
} | ||
} | ||
|
||
internal async Task<(string VirtualProjectXml, ImmutableArray<SimpleDiagnostic> Diagnostics)?> GetVirtualProjectContentAsync(string documentFilePath, ILogger logger, CancellationToken cancellationToken) | ||
{ | ||
var workingDirectory = Path.GetDirectoryName(documentFilePath); | ||
|
@@ -70,7 +86,15 @@ internal class VirtualProjectXmlProvider(DotnetCliHelper dotnetCliHelper) | |
|
||
if (response is RunApiOutput.Project project) | ||
{ | ||
|
||
using (await _gate.DisposableWaitAsync(cancellationToken)) | ||
{ | ||
_diagnosticsByFilePath.TryGetValue(documentFilePath, out var previousCachedDiagnostics); | ||
_diagnosticsByFilePath[documentFilePath] = project.Diagnostics; | ||
|
||
// check for difference, and signal to host to update if so. | ||
if (previousCachedDiagnostics.IsDefault || !project.Diagnostics.SequenceEqual(previousCachedDiagnostics)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do we remove diagnostics? It looks really bad when we have stale diagnostics, so we definitely need to ensure we're clearing them when the file based project is deleted / renamed / otherwise changes contexts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We also probably need to remove them from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wrapping this method to try and ensure that the cache is always dealt with appropriately, whatever the response from run-api. |
||
diagnosticRefresher.RequestWorkspaceRefresh(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks correct. generally the guidance I have on using the refresher is to use it as sparingly as possible because it refreshes all diagnostics. It looks like you're already checking to make sure there are changes which is good. But the less that goes through this the better. Ideally we can get most diagnostics from the actual Roslyn snapshots, and then the normal on-type updates handle most changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What would it mean to do this? Does that require having a "live source" for diagnostics? e.g. getting the diagnostics through analyzer, or running run-api on the "snapshot" of the file-based program? |
||
} | ||
return (project.Content, project.Diagnostics); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the staleness issue semes real here. it looks like we could return incorrect diagnostics for a solution snapshot. and it's unclear what would snap the ide out of that state. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is updated each time a design time build occurs, which for the moment occurs whenever the file-based program is saved to disk. The cached diagnostics would probably need to be updated in the other code paths also. e.g. if run-api completely fails, the stale diagnostics we already had in the cache are likely not useful and should be cleared out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think I fully grasp this concern or what should be done to address it. |
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why "internal" as a name here?