-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 all 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 |
---|---|---|
|
@@ -43,28 +43,46 @@ public sealed class Project : RunApiOutput | |
public required ImmutableArray<SimpleDiagnostic> Diagnostics { get; init; } | ||
} | ||
} | ||
internal sealed class SimpleDiagnostic | ||
|
||
internal sealed record SimpleDiagnostic | ||
{ | ||
public required Position Location { get; init; } | ||
public required string Message { get; init; } | ||
|
||
/// <summary> | ||
/// An adapter of <see cref="FileLinePositionSpan"/> that ensures we JSON-serialize only the necessary fields. | ||
/// </summary> | ||
public readonly struct Position | ||
public readonly record struct Position | ||
{ | ||
public string Path { get; init; } | ||
public LinePositionSpan Span { get; init; } | ||
public LinePositionSpanInternal Span { get; init; } | ||
} | ||
} | ||
|
||
public static implicit operator Position(FileLinePositionSpan fileLinePositionSpan) => new() | ||
{ | ||
Path = fileLinePositionSpan.Path, | ||
Span = fileLinePositionSpan.Span, | ||
}; | ||
internal record struct LinePositionInternal | ||
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. Why "internal" as a name here? |
||
{ | ||
public int Line { get; init; } | ||
public int Character { get; init; } | ||
} | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/// <summary> | ||
/// Workaround for inability to deserialize directly to <see cref="LinePositionSpan"/>. | ||
/// </summary> | ||
internal record struct LinePositionSpanInternal | ||
{ | ||
public LinePositionInternal Start { get; init; } | ||
public LinePositionInternal End { get; init; } | ||
|
||
public LinePositionSpan ToLinePositionSpan() | ||
{ | ||
return new LinePositionSpan( | ||
start: new LinePosition(Start.Line, Start.Character), | ||
end: new LinePosition(End.Line, End.Character)); | ||
} | ||
} | ||
|
||
[JsonSerializable(typeof(RunApiInput))] | ||
[JsonSerializable(typeof(RunApiOutput))] | ||
[JsonSerializable(typeof(LinePositionSpanInternal))] | ||
internal partial class RunFileApiJsonSerializerContext : JsonSerializerContext; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
// 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 sealed class VirtualProjectXmlDiagnosticSourceProvider(VirtualProjectXmlProvider virtualProjectXmlProvider) : IDiagnosticSourceProvider | ||
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. It looks like we have an AbstractDocumentDiagnosticSource that implements some of this for us? |
||
{ | ||
public const string FileBasedPrograms = nameof(FileBasedPrograms); | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public bool IsDocument => true; | ||
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 VirtualProjectXmlDiagnosticSource(context.Document, virtualProjectXmlProvider)]; | ||
|
||
return ValueTask.FromResult(sources); | ||
} | ||
|
||
private sealed class VirtualProjectXmlDiagnosticSource(Document document, VirtualProjectXmlProvider virtualProjectXmlProvider) : IDiagnosticSource | ||
{ | ||
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 = new FixedSizeArrayBuilder<DiagnosticData>(simpleDiagnostics.Length); | ||
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: FileBasedPrograms, | ||
category: FileBasedPrograms, | ||
Comment on lines
+53
to
+54
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. Are we ever expecting to have some sort of ID for these? |
||
message: simpleDiagnostic.Message, | ||
severity: DiagnosticSeverity.Error, | ||
defaultSeverity: DiagnosticSeverity.Error, | ||
isEnabledByDefault: true, | ||
// Warning level 0 is used as a placeholder when the diagnostic has error severity | ||
warningLevel: 0, | ||
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; | ||
} | ||
|
||
/// <summary> | ||
/// These diagnostics are from the last time 'dotnet run-api' was invoked, which only occurs when a design time build is performed. | ||
/// <seealso cref="IDiagnosticSource.IsLiveSource"/>. | ||
/// </summary> | ||
/// <returns></returns> | ||
public bool IsLiveSource() => false; | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
public string ToDisplayString() => nameof(VirtualProjectXmlDiagnosticSource); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
using System.Text.Json; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.CSharp; | ||
using Microsoft.CodeAnalysis.Diagnostics; | ||
using Microsoft.CodeAnalysis.Host.Mef; | ||
using Microsoft.CodeAnalysis.Text; | ||
using Microsoft.Extensions.Logging; | ||
|
@@ -21,9 +22,63 @@ 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 ValueTask UnloadCachedDiagnosticsAsync(string path) | ||
{ | ||
using (await _gate.DisposableWaitAsync(CancellationToken.None)) | ||
{ | ||
_diagnosticsByFilePath.Remove(path); | ||
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. Does this need to be doing a refresh here? |
||
} | ||
} | ||
|
||
internal async Task<(string VirtualProjectXml, ImmutableArray<SimpleDiagnostic> Diagnostics)?> GetVirtualProjectContentAsync(string documentFilePath, ILogger logger, CancellationToken cancellationToken) | ||
{ | ||
var result = await GetVirtualProjectContentImplAsync(documentFilePath, logger, cancellationToken); | ||
if (result is { } 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)) | ||
diagnosticRefresher.RequestWorkspaceRefresh(); | ||
} | ||
} | ||
else | ||
{ | ||
using (await _gate.DisposableWaitAsync(CancellationToken.None)) | ||
{ | ||
if (_diagnosticsByFilePath.TryGetValue(documentFilePath, out var diagnostics)) | ||
{ | ||
_diagnosticsByFilePath.Remove(documentFilePath); | ||
if (!diagnostics.IsDefaultOrEmpty) | ||
{ | ||
// diagnostics have changed from "non-empty" to "unloaded". refresh. | ||
diagnosticRefresher.RequestWorkspaceRefresh(); | ||
} | ||
} | ||
} | ||
} | ||
|
||
return result; | ||
} | ||
|
||
private async Task<(string VirtualProjectXml, ImmutableArray<SimpleDiagnostic> Diagnostics)?> GetVirtualProjectContentImplAsync(string documentFilePath, ILogger logger, CancellationToken cancellationToken) | ||
{ | ||
var workingDirectory = Path.GetDirectoryName(documentFilePath); | ||
var process = dotnetCliHelper.Run(["run-api"], workingDirectory, shouldLocalizeOutput: true, keepStandardInputOpen: true); | ||
|
@@ -70,7 +125,6 @@ internal class VirtualProjectXmlProvider(DotnetCliHelper dotnetCliHelper) | |
|
||
if (response is RunApiOutput.Project project) | ||
{ | ||
|
||
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. |
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,6 +181,14 @@ protected sealed record RemoteProjectLoadResult(RemoteProjectFile ProjectFile, P | |
protected abstract Task<RemoteProjectLoadResult?> TryLoadProjectInMSBuildHostAsync( | ||
BuildHostProcessManager buildHostProcessManager, string projectPath, CancellationToken cancellationToken); | ||
|
||
/// <summary>Called after a project is unloaded to allow the subtype to clean up any resources associated with the project.</summary> | ||
/// <remarks> | ||
/// Note that this refers to unloading of the project on the project-system level. | ||
/// So, for example, changing the target frameworks of a project, or transitioning between | ||
/// "file-based program" and "true miscellaneous file", will not result in this being called. | ||
/// </remarks> | ||
protected abstract ValueTask OnProjectUnloadedAsync(string projectFilePath); | ||
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. Why is this needed? Isn't it the derived type already the one calling Unload, so why can't it just call it's logic directly? |
||
|
||
/// <returns>True if the project needs a NuGet restore, false otherwise.</returns> | ||
private async Task<bool> ReloadProjectAsync(ProjectToLoad projectToLoad, ToastErrorReporter toastErrorReporter, BuildHostProcessManager buildHostProcessManager, CancellationToken cancellationToken) | ||
{ | ||
|
@@ -445,5 +453,7 @@ protected async ValueTask UnloadProjectAsync(string projectPath) | |
throw ExceptionUtilities.UnexpectedValue(loadState); | ||
} | ||
} | ||
|
||
await OnProjectUnloadedAsync(projectPath); | ||
} | ||
} |
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.
Trying to figure out the semantics of OnProjectUnloadedAsync
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.
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.
Is there any way we can tell when that transition happens? So that we can clear out diagnostics if its not a file based program any longer?
Maybe it would be possible to verify that the workspace kind is still the language server workspace? IIRC there would be a workspace changed event triggering a diagnostics refresh, so there should be a diagnostics requests to the server in that scenario already.
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.
Hypothetically, upon reload, we could see if some of the targets we unloaded were associated with a different ProjectFactory than before, and react to that somehow.
roslyn/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs
Line 266 in 8fefbab
Note also the assert that tracked targets for a given project, are all associated with only one projectFactory at a given time:
roslyn/src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs
Line 287 in 8fefbab
In practice I do not think we are going to need a special behavior here. Each design time build, either of FBP or "rich misc file", is going to result in run-api reporting some set of diagnostics, and we will always surface the latest set for the project. And, we expect run-api is not going to report anything for the "non-FBP" case. It would be hard to observe it reporting in the non-FBP case, since it only reports on bad "app directives" currently, and presence of those will mark the file as an FBP.
Long and short is I do not expect an unwanted/buggy behavior here from failing to take an explicit step to clear the diagnostics out when transitioning between FBP and non-FBP. (I'll verify this manually but I keep having to change between different branches and would like to fix in a follow up in the case we really do have a problem here.)
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.
wfm