-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Support live diagnostics in source generated files #58363
Conversation
1095369
to
1fa1970
Compare
public static readonly SourceGeneratedDocumentOperationService Instance = new(); | ||
|
||
public bool CanApplyChange => false; | ||
public bool SupportDiagnostics => true; |
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.
📝 The addition of documentServiceProvider
for SourceGeneratedDocumentState
is entirely to specify SupportDiagnostics
, which otherwise defaults to false
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.
The previous comment was not correct. Here is the corrected comment:
📝 The addition of documentServiceProvider
for SourceGeneratedDocumentState
is entirely to specify CanApplyChange
, which otherwise defaults to true
/// <summary> | ||
/// An event that is fired when a documents is opened in the editor. | ||
/// </summary> | ||
internal event EventHandler<DocumentEventArgs> SourceGeneratedDocumentOpened |
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.
📝 This is the trigger for solution crawler to run. I could use DocumentOpened
, but then we'd need to audit all consumers of this public API to make sure they are capable of handling events where a subsequent GetDocument
call returns null.
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.
My preference/gut feeling is to use DocumentOpened rather than introduce a new API, rather than keep making parallel APIs if possible. (If GetDocument was async from the start, I wouldn't have introduced parallel APIs in the first place.)
To your specific concern about GetDocument calls return null, that's already something any event handler needs to do. Since the event handler goes through an async queue, it's always possible the document has been closed and again removed in Workspace.CurrentSolution by the time this event is fired.
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.
⏱️ Am combining these events
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.
➡️ These events are now combined
16004f2
to
b119113
Compare
b119113
to
5434894
Compare
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.
I'm only halfway through the review; I'm hitting "request changes" since there's some comments so far that do need changes/tweaks but for now feel free to do nothing with my comments until I'm through the rest of the PR.
/// <summary> | ||
/// An event that is fired when a documents is opened in the editor. | ||
/// </summary> | ||
internal event EventHandler<DocumentEventArgs> SourceGeneratedDocumentOpened |
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.
My preference/gut feeling is to use DocumentOpened rather than introduce a new API, rather than keep making parallel APIs if possible. (If GetDocument was async from the start, I wouldn't have introduced parallel APIs in the first place.)
To your specific concern about GetDocument calls return null, that's already something any event handler needs to do. Since the event handler goes through an async queue, it's always possible the document has been closed and again removed in Workspace.CurrentSolution by the time this event is fired.
Document document; | ||
if (testDocument.IsSourceGenerated) | ||
document = await solution.GetRequiredProject(testDocument.Id.ProjectId).GetRequiredSourceGeneratedDocumentAsync(testDocument.Id, CancellationToken.None).ConfigureAwait(false); | ||
else | ||
document = solution.GetRequiredDocument(testDocument.Id); |
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.
Didn't we create a GetRequiredDocumentAsync() or were you wanting to be very explicit to make sure we get the right "kind" of document in each case?
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.
⏱️ This is being rewritten to use the simpler method
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.
➡️ Amended previous commit to simplify
document = await solution.GetRequiredProject(testDocument.Id.ProjectId).GetRequiredSourceGeneratedDocumentAsync(testDocument.Id, CancellationToken.None).ConfigureAwait(false); | ||
else | ||
document = solution.GetRequiredDocument(testDocument.Id); | ||
|
||
var text = document.GetTextSynchronously(CancellationToken.None); |
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.
You could make this async now if this method stays async.
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.
⏱️ Will update with #58363 (comment)
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.
➡️ Amended previous commit to simplify
src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs
Show resolved
Hide resolved
src/EditorFeatures/TestUtilities/Workspaces/TestHostDocument.cs
Outdated
Show resolved
Hide resolved
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.
Went through the rest of the PR, apologies @sharwell for not getting to this earlier in the week. Although some other concerns were added, nothing significant compared to the first part of the review; the "request changes" is mostly passing that through than anything else. The only general concern I had was for a lot of the uses of TryGetSourceGeneratedDocumentForAlreadyGeneratedId when calling the async method should be fine. That API only works if you can ensure that the generators have ran on the same snapshot more or less and in practice it's easier to just call the async method than convince yourself that it's correct. There's some code in the crawler that prior to your changes was calling GetDocument and then doing null checks, which makes me think there's some really funky existing behavior here as far as what snapshots have and haven't been analyzed; the existing code terrifies me so the Try* makes me even more worried.
Overall though this is really a fantastic start to getting this working; I don't see any insurmountable issues that I've raised. I admit a few of them have a few solutions and I don't have more than gut feel for which one I think is better, but I can absolutely be wrong on those gut feels.
this.OnDocumentClosed(documentId, testDocument.Loader); | ||
} | ||
|
||
public void OpenSourceGeneratedDocument(DocumentId documentId) |
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 two sets of methods if the implementations are the same?
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.
➡️ It wasn't obvious to me that consolidating this test API would maintain clarity at use sites
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.
The call sites did confuse me, so strong vote from me just to consolidate here. Put another way, there's no clarity to maintain in the first place. 😄
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.
❗ ➡️ VisualStudioWorkspace
does not support opening source generated documents through OpenDocument
. The separation in this class is required (OpenDocument overrides a base method, where OpenSourceGeneratedDocument is new).
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.
@sharwell: oh that should be fixed. If we did, does the split go away then? If so, file a bug and I'll mop this up separately. Consider adding a TODO here pointing to the bug.
...ures/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_GetDiagnostics.cs
Outdated
Show resolved
Hide resolved
// Open documents *should* always have their SourceText available, but we cannot guarantee | ||
// (i.e. assert) that they do. That's because we're not on the UI thread here, so there's |
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.
How is FindCorrespondingEditorTextSnapshot below working in the source generator case -- there's an icky problem here where we may have new diagnostics but the buffer might not be in sync yet?
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.
➡️ I can't find any purpose for this code.
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.
So this was all introduced in #24721; I guess the whole point is to fill in this snapshot map. I would expect this doesn't even work in the source generator case, so I'd assume that either [a] we don't need to touch this method at all, or [b] we need some further fixing below...
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.
@CyrusNajmabadi You remember more here? I see I reviewed that PR but not sure I remember the full details 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.
looking.
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.
yeah, ican see why we do this... but i'm also not sure what the best solution forward is. for one thing, i'm not sure why this is a mapping from diag to snapshot, instead of diag to Document (since that's waht we have now). then, later, when actually producing tags for another snapshot (which is async) we can then figure out what snapshot this doc actually applied to, and then map the diags forward.
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.
Offline discussion concluded here that we're doing this because when we produce diagnostics, that's running on a potentially older snapshot of an editor. We do this to capture the snapshot that maps to those old diagnostic, so we can then map them forward a-la tracking spans to the current snapshot. In the case of generated files, we have the reverse problem: we may have generated diagnostics from the future that need to be brought back. Simply not updating this code will do that (along with some clamping to the file).
src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.ProjectState.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Test/Diagnostics/DiagnosticAnalyzerServiceTests.cs
Outdated
Show resolved
Hide resolved
70b2510
to
d7c4073
Compare
27dad8e
to
b47ceb1
Compare
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.
This is getting really close; mostly looking for the comments from @mavasani and @CyrusNajmabadi if they can help me understand a few bits.
src/EditorFeatures/Core/Implementation/Diagnostics/AbstractDiagnosticsTaggerProvider.cs
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Handler/Diagnostics/WorkspacePullDiagnosticHandler.cs
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Telemetry/SyntaxTreeConfigurationOptions.cs
Outdated
Show resolved
Hide resolved
if (_workspaceThreadingService is not null) | ||
{ | ||
document = _workspaceThreadingService.Run(() => project.GetSourceGeneratedDocumentAsync(documentId, CancellationToken.None).AsTask()); | ||
} | ||
else | ||
{ | ||
document = project.GetSourceGeneratedDocumentAsync(documentId, CancellationToken.None).AsTask().WaitAndGetResult_CanCallOnBackground(CancellationToken.None); | ||
} |
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.
Just had a different idea that might alleviate my earlier concerns but avoid some of the ugliness here: we could absolutely make a TryGetSourceGeneratedDocument variant that just throws if we haven't ran generators. Or here you could call the Async form, and just immediately Contract.Throw() if the task that came back wasn't completed synchronously.
What would it look like to just make this method async and spread the async around?
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.
What would it look like to just make this method async and spread the async around?
⏱️ Much easier than expected...
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.
➡️ Now implemented
...ures/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs
Outdated
Show resolved
Hide resolved
var document = await project.Solution.GetDocumentAsync( | ||
documentId, | ||
includeSourceGenerated: project.Solution.Workspace.Services.GetService<ISyntaxTreeConfigurationService>() is { EnableOpeningSourceGeneratedFilesInWorkspace: true }, | ||
cancellationToken).ConfigureAwait(false); |
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.
Minor: This code pattern seems to be used at multiple places and is quite verbose. Probably deserves an extension method.
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.
It'll go away once we get rid of that feature flag that's controlling all of this. Not sure whether we'll get all the bugs fixed for 17.2, but hopefully by 17.3.
if (_syntaxTreeConfigurationService is { EnableOpeningSourceGeneratedFilesInWorkspace: true }) | ||
{ | ||
// TODO: if this becomes a hot spot, we should be able to expose/access the dictionary | ||
// underneath GetSourceGeneratedDocumentsAsync rather than create a new one 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.
I am not sure if all this code to detect added/removed/changed source generated documents belongs here. Shouldn't this already be retreivable from APIs on ProjectChanges
?
roslyn/src/Workspaces/Core/Portable/Workspace/Solution/Project.cs
Lines 470 to 473 in 0098764
/// <summary> | |
/// Gets an object that lists the added, changed and removed documents between this project and the specified project. | |
/// </summary> | |
public ProjectChanges GetChanges(Project oldProject) |
If not, then should be fix/update ProjectChanges
to provide this data?
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.
➡️ ProjectChanges occurs prior to source generators running (and we don't want to delay it by an hour waiting for a slow generator).
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.
This might be a good request to at least add the diff APIs to ProjectChanges, like some sort of "GetAddedSourceGeneratedDocumentsAsync", but not sure we've had another need for that yet.
@@ -334,7 +334,7 @@ private async Task ProcessDocumentAsync(ImmutableArray<IIncrementalAnalyzer> ana | |||
{ | |||
using (Logger.LogBlock(FunctionId.WorkCoordinator_ProcessDocumentAsync, w => w.ToString(), workItem, cancellationToken)) | |||
{ | |||
var textDocument = solution.GetTextDocument(documentId); | |||
var textDocument = solution.GetTextDocument(documentId) ?? await solution.GetSourceGeneratedDocumentAsync(documentId, cancellationToken).ConfigureAwait(false); |
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.
I would ideally expect GetTextDocument
to itself handle the source generated document case instead of having each caller require the code pattern being added here. That is the core purpose of this extension method, so the callers don't have to deal with each document kind. Is the primary thing blocking that the fact that GetTextDocument
is sync while GetSourceGeneratedDocumentAsync
is async, and changing GetTextDocument
to GetTextDocumentAsync
would be a much broader change? If so, can we open a tracking issue to clean this up with a follow-up change?
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.
➡️ GetTextDocument is fast, while GetSourceGeneratedDocumentAsync is unbounded slow. It is desirable for the callers of the slow path to be explicit.
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 the primary thing blocking that the fact that GetTextDocument is sync while GetSourceGeneratedDocumentAsync is async, and changing GetTextDocument to GetTextDocumentAsync would be a much broader change?
Unfortunately, yes. If Project.GetDocument() was async in the first place, we probably wouldn't have needed the other methods.
// We couldn't find a document matching a known ID when the item was created, so it may be a | ||
// source generator output. | ||
var documents = threadingContext.JoinableTaskFactory.Run(() => project.GetSourceGeneratedDocumentsAsync(cancellationToken).AsTask()); | ||
if (documentId is not null) |
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.
So do we have both cases here (document is null and not null) for DiagnosticTableItem reported on a source generated document? If so, do we know which diagnostic source is leading to each of these cases? IMO, this code feels like a hack and we should always have a non-null documentId for DiagnosticTableItem on a source generated document. This would be the case if we move all the code that is mapping diagnostic's reported file path to document ID to the diagnostic source that is creating the original DiagnosticData.
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.
➡️ The build runs in a different process. Not only is it possible that source generators have not run in this process at the point an error is reported by the build, but it's possible the design-time functionality of the source generator has been disabled and will never produce a document ID in process which maps to this error.
src/VisualStudio/Core/Def/Telemetry/VisualStudioSyntaxTreeConfigurationService.cs
Show resolved
Hide resolved
@@ -407,6 +408,11 @@ private static string GetAssemblyQualifiedName(Type type) | |||
tasks.Add(AnalyzeDocumentAsync(suppressionAnalyzer, document, span: null, bag.Add)); | |||
} | |||
|
|||
foreach (var document in await project.GetSourceGeneratedDocumentsAsync(cancellationToken).ConfigureAwait(false)) | |||
{ | |||
tasks.Add(AnalyzeDocumentAsync(suppressionAnalyzer, document, span: null, bag.Add)); |
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 do we want to show unnecessary suppressions on a source generated document? Is it possible to apply the remove unnecessary suppression code fix or would the fading just be a hint to update the source generator to stop generating the unnecessary pragma? Given that the source generator might be enabled on different projects with different set of analyzer references, I am not sure we should run this analyzer on source generated documents.
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.
➡️ Source generated documents are never linked files (even in multi-targeting projects, there is one unique document for each target). This provides fading functionality similar to how we fade unnecessary using directives even in generated code.
33f0774
to
47ee7c2
Compare
if (snapshot != null) | ||
{ | ||
_diagnosticIdToTextSnapshot.GetValue(updateGroupId, _ => snapshot); | ||
} |
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.
ok. baed on offline talk, i think we can remove all this.
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.
but updat ethis:
protected override ITaggerEventSource CreateEventSource(ITextView textViewOpt, ITextBuffer subjectBuffer)
{
return TaggerEventSources.Compose(
TaggerEventSources.OnDocumentActiveContextChanged(subjectBuffer),
TaggerEventSources.OnWorkspaceRegistrationChanged(subjectBuffer),
TaggerEventSources.OnDiagnosticsChanged(subjectBuffer, _diagnosticService));
}
Add TaggerEventSources.OnTextChanged
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.
and a comment that this is necessary because with SG documents we might produce diagnostics against a current doc, but an old ITextBuffer. and when the textbuffer becomes the present, we need to recompute so that tags are in the right place.
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.
Alright, I think we've sorted evrything out here. Other than the one change that @CyrusNajmabadi called out, this is good to go I think. @mavasani's comments I think are also good but can address separately.
363a243
to
d3490b9
Compare
d3490b9
to
ebd07cc
Compare
Fixes #49533
Fixes #50357
Fixes #50695
The following functionality does not yet work:
Double clicking an error in the Error List navigates to the source generated document (this is the live diagnostics equivalent to [need to find bug])(Now fixed for both build and live diagnostics)Errors do not appear inside the editor window for source generated documentsSquiggles should appear(Now fixed)Live errors should appear when enabled(Now fixed)Code fixes cannot be triggered from inside a source generated document(Now fixed)Diagnostics are not updated after source generated content changes unless the document is closed and reopened(Now fixed)Tests are needed for the following items revealed during local iterative testing:
Solution crawler runs after opening a source generated document(Covered byErrorTagGeneratedForErrorInSourceGeneratedDocument
)"Open File" diagnostics are correctly mapped before display in error list (otherwise it shows with no filename and line 1)(Covered byDiagnosticData_SourceGeneratedDocumentLocationIsPreserved
)Run Code Analysis produces and shows diagnostics in the error list for source generated documents(Covered byPullDiagnosticTests.TestWorkspaceDiagnosticsForSourceGeneratedFiles
)