Remove diagnostic scaffolding for sending diagnostics events from HostDiagnosticUpdateSource. #72731
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
|
|
||
| public abstract Workspace Workspace { get; } | ||
|
|
||
| public event EventHandler<ImmutableArray<DiagnosticsUpdatedArgs>>? DiagnosticsUpdated; |
There was a problem hiding this comment.
this was the member i was removing first. it was only referenced from tests, so it culd be safely removed. this then became a therad that removed more and more.
|
|
||
| public event EventHandler<ImmutableArray<DiagnosticsUpdatedArgs>>? DiagnosticsUpdated; | ||
|
|
||
| public void RaiseDiagnosticsUpdated(ImmutableArray<DiagnosticsUpdatedArgs> args) |
There was a problem hiding this comment.
this was hte method that invoked hte event. no more event, no more method.
| using var argsBuilder = TemporaryArray<DiagnosticsUpdatedArgs>.Empty; | ||
| var analyzers = analyzerReference.GetAnalyzers(language); | ||
| AddArgsToClearAnalyzerDiagnostics(ref argsBuilder.AsRef(), analyzers, projectId); | ||
| RaiseDiagnosticsUpdated(argsBuilder.ToImmutableAndClear()); |
There was a problem hiding this comment.
this was a call to raise hte event. no more event, then no more method. once tehre's no more method, there's no need to do thingsl ike add arguments to builders anymore.
| { | ||
| var newDiags = existing.Where(d => d.ProjectId != projectId).ToImmutableHashSet(); | ||
| if (newDiags.Count < existing.Count && | ||
| ImmutableInterlocked.TryUpdate(ref _analyzerHostDiagnosticsMap, analyzer, newDiags, existing)) |
There was a problem hiding this comment.
similarly. nothing was ever reading this _analyzerHostDiagnosticsMap. removnig that pulled on more of these therads.
| private static readonly object s_analyzerChangedErrorId = new(); | ||
|
|
||
| private readonly VisualStudioWorkspaceImpl _workspace; | ||
| private readonly HostDiagnosticUpdateSource _updateSource; |
There was a problem hiding this comment.
since nothing was listening to the _updateSource anymore, there's no need for the AnalyerFileWatchService to send it any events.
Note: we may need to do work in LSP pull diags to pull on this type in case it has created an diags. If that's the case, then we're already in a broken state. We can see how important that scenario is, and trivially make that work again with LSP pull diags if needed.
|
|
||
| lock (_gate) | ||
| { | ||
| _diagnosticMap.GetOrAdd(projectId, id => new HashSet<object>()).Add(key); |
There was a problem hiding this comment.
similarly, nothing ever read this map. removed and kept pulling the thread.
| Dim hostDiagnosticUpdateSource = New HostDiagnosticUpdateSource(lazyWorkspace) | ||
|
|
||
| Dim eventHandler = New EventHandlers(file) | ||
| AddHandler hostDiagnosticUpdateSource.DiagnosticsUpdated, AddressOf eventHandler.DiagnosticAddedTest |
There was a problem hiding this comment.
this was the only location that .DiagnosticsUpdated was referenced. removing this meant all the above code could be removed.
CyrusNajmabadi
changed the title
| private readonly VisualStudioWorkspaceImpl _workspace; | ||
| private readonly HostDiagnosticUpdateSource _updateSource; | ||
| private readonly IVsFileChangeEx _fileChangeService; | ||
| private readonly IVsFileChangeEx _fileChangeService = (IVsFileChangeEx)serviceProvider.GetService(typeof(SVsFileChangeEx)); |
There was a problem hiding this comment.
Ouch. This is not valid in a MEF part. Will this be removed soon or should I fix it after this change merges?
There was a problem hiding this comment.
feel free to fix.
| private readonly DiagnosticDescriptor _analyzerChangedRule = new( | ||
| id: IDEDiagnosticIds.AnalyzerChangedId, | ||
| title: ServicesVSResources.AnalyzerChangedOnDisk, | ||
| messageFormat: ServicesVSResources.The_analyzer_assembly_0_has_changed_Diagnostics_may_be_incorrect_until_Visual_Studio_is_restarted, |
There was a problem hiding this comment.
❓ Do we still get this message displayed in the IDE?
There was a problem hiding this comment.
Currently (even without my PR) it will not. That said, there are two paths (beyond this pr) to move forward on:
- make it so that these diagnostics can proffered up so the user can see them.
- change analyzers so that reloading is fine. This is something we can support now, and @jasonmalinowski was going to make this work very soon anyways.
|
@mavasani ptal :) |
| using Microsoft.CodeAnalysis.Diagnostics; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.Workspaces.ProjectSystem; | ||
|
|
||
| // TODO: see if we can get rid of this interface by appropriately rewriting HostDiagnosticUpdateSource to live at the workspaces layer. | ||
| internal interface IProjectSystemDiagnosticSource | ||
| { | ||
| void ClearAllDiagnosticsForProject(ProjectId projectId); | ||
| void ClearAnalyzerReferenceDiagnostics(AnalyzerFileReference fileReference, string language, ProjectId projectId); | ||
| void ClearDiagnosticsForProject(ProjectId projectId, object key); | ||
| DiagnosticData CreateAnalyzerLoadFailureDiagnostic(AnalyzerLoadFailureEventArgs e, string fullPath, ProjectId projectId, string language); |
There was a problem hiding this comment.
Seems like this can be a utility helper method now and IProjectSystemDiagnosticSource interface can be completely removed.
There was a problem hiding this comment.
Yup. I can work on that next
No description provided.