-
Notifications
You must be signed in to change notification settings - Fork 4.3k
refactor diagnostic analyzer service v1 state management code #930
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
a174a29
95b41fa
2fe10f2
f9d09d9
12a8352
fa8f20c
eb2e6f4
93435e0
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 |
|---|---|---|
| @@ -1,9 +1,11 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using System.IO; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.CodeAnalysis.ErrorReporting; | ||
| using Roslyn.Utilities; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.Diagnostics | ||
|
Contributor
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. My latest review removes a lot of methods that I recently added from this helper type. It will now just have 2 static methods used by IDE driver and HostAnalyzerManager for supported diagnostics.
Contributor
Author
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. cool! but still merge conflict :( |
||
| { | ||
|
|
@@ -12,12 +14,12 @@ internal static class AnalyzerHelper | |
| private const string CSharpCompilerAnalyzerTypeName = "Microsoft.CodeAnalysis.Diagnostics.CSharp.CSharpCompilerDiagnosticAnalyzer"; | ||
| private const string VisualBasicCompilerAnalyzerTypeName = "Microsoft.CodeAnalysis.Diagnostics.VisualBasic.VisualBasicCompilerDiagnosticAnalyzer"; | ||
|
|
||
| public static bool IsBuiltInAnalyzer(DiagnosticAnalyzer analyzer) | ||
| public static bool IsBuiltInAnalyzer(this DiagnosticAnalyzer analyzer) | ||
| { | ||
| return analyzer is IBuiltInAnalyzer || analyzer is DocumentDiagnosticAnalyzer || analyzer is ProjectDiagnosticAnalyzer || IsCompilerAnalyzer(analyzer); | ||
| return analyzer is IBuiltInAnalyzer || analyzer is DocumentDiagnosticAnalyzer || analyzer is ProjectDiagnosticAnalyzer || analyzer.IsCompilerAnalyzer(); | ||
| } | ||
|
|
||
| public static bool IsCompilerAnalyzer(DiagnosticAnalyzer analyzer) | ||
| public static bool IsCompilerAnalyzer(this DiagnosticAnalyzer analyzer) | ||
| { | ||
| // TODO: find better way. | ||
| var typeString = analyzer.GetType().ToString(); | ||
|
|
@@ -34,43 +36,61 @@ public static bool IsCompilerAnalyzer(DiagnosticAnalyzer analyzer) | |
| return false; | ||
| } | ||
|
|
||
| public static Action<Diagnostic> GetAddExceptionDiagnosticDelegate(DiagnosticAnalyzer analyzer, AbstractHostDiagnosticUpdateSource hostDiagnosticUpdateSource, Project project) | ||
| public static ValueTuple<string, VersionStamp> GetUniqueId(this DiagnosticAnalyzer analyzer) | ||
| { | ||
| // Get the unique ID for given diagnostic analyzer. | ||
| // note that we also put version stamp so that we can detect changed analyzer. | ||
| var type = analyzer.GetType(); | ||
| return ValueTuple.Create(type.AssemblyQualifiedName, GetAnalyzerVersion(type.Assembly.Location)); | ||
| } | ||
|
|
||
| public static Action<Diagnostic> GetAddExceptionDiagnosticDelegate(this DiagnosticAnalyzer analyzer, AbstractHostDiagnosticUpdateSource hostDiagnosticUpdateSource, Project project) | ||
| { | ||
| return diagnostic => | ||
| hostDiagnosticUpdateSource?.ReportAnalyzerDiagnostic(analyzer, diagnostic, project.Solution.Workspace, project); | ||
| } | ||
|
|
||
| public static Action<Diagnostic> GetAddExceptionDiagnosticDelegate(DiagnosticAnalyzer analyzer, AbstractHostDiagnosticUpdateSource hostDiagnosticUpdateSource, Workspace workspace) | ||
| public static Action<Diagnostic> GetAddExceptionDiagnosticDelegate(this DiagnosticAnalyzer analyzer, AbstractHostDiagnosticUpdateSource hostDiagnosticUpdateSource, Workspace workspace) | ||
| { | ||
| return diagnostic => | ||
| hostDiagnosticUpdateSource?.ReportAnalyzerDiagnostic(analyzer, diagnostic, workspace, null); | ||
| } | ||
|
|
||
| public static AnalyzerExecutor GetAnalyzerExecutorForSupportedDiagnostics( | ||
| DiagnosticAnalyzer analyzer, | ||
| this DiagnosticAnalyzer analyzer, | ||
| AbstractHostDiagnosticUpdateSource hostDiagnosticUpdateSource, | ||
| Func<Exception, DiagnosticAnalyzer, bool> continueOnAnalyzerException, | ||
| Func<Exception, DiagnosticAnalyzer, bool> continueOnAnalyzerException, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| var addExceptionDiagnostic = GetAddExceptionDiagnosticDelegate(analyzer, hostDiagnosticUpdateSource, hostDiagnosticUpdateSource?.Workspace); | ||
| var addExceptionDiagnostic = analyzer.GetAddExceptionDiagnosticDelegate(hostDiagnosticUpdateSource, hostDiagnosticUpdateSource?.Workspace); | ||
|
|
||
| // Skip telemetry logging if the exception is thrown as we are computing supported diagnostics and | ||
| // we can't determine if any descriptors support getting telemetry without having the descriptors. | ||
| return AnalyzerExecutor.CreateForSupportedDiagnostics(addExceptionDiagnostic, continueOnAnalyzerException, cancellationToken); | ||
| } | ||
|
|
||
| public static AnalyzerExecutor GetAnalyzerExecutor( | ||
| DiagnosticAnalyzer analyzer, | ||
| AbstractHostDiagnosticUpdateSource hostDiagnosticUpdateSource, | ||
| Project project, | ||
| Compilation compilation, | ||
| this DiagnosticAnalyzer analyzer, | ||
| AbstractHostDiagnosticUpdateSource hostDiagnosticUpdateSource, | ||
| Project project, | ||
| Compilation compilation, | ||
| Action<Diagnostic> addDiagnostic, | ||
| AnalyzerOptions analyzerOptions, | ||
| Func<Exception, DiagnosticAnalyzer, bool> continueOnAnalyzerException, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| var addExceptionDiagnostic = GetAddExceptionDiagnosticDelegate(analyzer, hostDiagnosticUpdateSource, project); | ||
| var addExceptionDiagnostic = analyzer.GetAddExceptionDiagnosticDelegate(hostDiagnosticUpdateSource, project); | ||
| return AnalyzerExecutor.Create(compilation, analyzerOptions, addDiagnostic, addExceptionDiagnostic, continueOnAnalyzerException, cancellationToken); | ||
| } | ||
|
|
||
| private static VersionStamp GetAnalyzerVersion(string path) | ||
| { | ||
| if (path == null || !File.Exists(path)) | ||
| { | ||
| return VersionStamp.Default; | ||
| } | ||
|
|
||
| return VersionStamp.Create(File.GetLastWriteTimeUtc(path)); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,12 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Collections.Immutable; | ||
| using System.Composition; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.CodeAnalysis.Shared.TestHooks; | ||
| using Microsoft.CodeAnalysis.SolutionCrawler; | ||
| using Microsoft.CodeAnalysis.Text; | ||
| using Roslyn.Utilities; | ||
|
|
@@ -15,45 +17,51 @@ namespace Microsoft.CodeAnalysis.Diagnostics | |
| [Shared] | ||
| internal partial class DiagnosticAnalyzerService : IDiagnosticAnalyzerService | ||
| { | ||
| private readonly WorkspaceAnalyzerManager _workspaceAnalyzerManager; | ||
| private readonly HostAnalyzerManager _hostAnalyzerManager; | ||
|
Contributor
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. Thanks for the rename, I have always felt WorkspaceAnalyzerManager was the wrong term I chose, given that it is independent of workspace. |
||
| private readonly AbstractHostDiagnosticUpdateSource _hostDiagnosticUpdateSource; | ||
| private readonly IAsynchronousOperationListener _listener; | ||
|
|
||
| [ImportingConstructor] | ||
| public DiagnosticAnalyzerService([Import(AllowDefault = true)]IWorkspaceDiagnosticAnalyzerProviderService diagnosticAnalyzerProviderService = null, | ||
| public DiagnosticAnalyzerService( | ||
| [ImportMany] IEnumerable<Lazy<IAsynchronousOperationListener, FeatureMetadata>> asyncListeners, | ||
| [Import(AllowDefault = true)]IWorkspaceDiagnosticAnalyzerProviderService diagnosticAnalyzerProviderService = null, | ||
| [Import(AllowDefault = true)]AbstractHostDiagnosticUpdateSource hostDiagnosticUpdateSource = null) | ||
| : this(workspaceAnalyzerAssemblies: diagnosticAnalyzerProviderService != null ? | ||
| diagnosticAnalyzerProviderService.GetWorkspaceAnalyzerAssemblies() : | ||
| SpecializedCollections.EmptyEnumerable<string>(), | ||
| hostDiagnosticUpdateSource: hostDiagnosticUpdateSource) | ||
| { | ||
| _listener = new AggregateAsynchronousOperationListener(asyncListeners, FeatureAttribute.DiagnosticService); | ||
| } | ||
|
|
||
| public IAsynchronousOperationListener Listener => _listener; | ||
|
|
||
| private DiagnosticAnalyzerService(IEnumerable<string> workspaceAnalyzerAssemblies, AbstractHostDiagnosticUpdateSource hostDiagnosticUpdateSource) : this() | ||
| { | ||
| _workspaceAnalyzerManager = new WorkspaceAnalyzerManager(workspaceAnalyzerAssemblies, hostDiagnosticUpdateSource); | ||
| _hostAnalyzerManager = new HostAnalyzerManager(workspaceAnalyzerAssemblies, hostDiagnosticUpdateSource); | ||
| _hostDiagnosticUpdateSource = hostDiagnosticUpdateSource; | ||
| } | ||
|
|
||
| // internal for testing purposes. | ||
| internal DiagnosticAnalyzerService(ImmutableArray<AnalyzerReference> workspaceAnalyzers, AbstractHostDiagnosticUpdateSource hostDiagnosticUpdateSource = null) : this() | ||
| { | ||
| _workspaceAnalyzerManager = new WorkspaceAnalyzerManager(workspaceAnalyzers, hostDiagnosticUpdateSource); | ||
| _hostAnalyzerManager = new HostAnalyzerManager(workspaceAnalyzers, hostDiagnosticUpdateSource); | ||
| _hostDiagnosticUpdateSource = hostDiagnosticUpdateSource; | ||
| } | ||
|
|
||
| public ImmutableDictionary<string, ImmutableArray<DiagnosticDescriptor>> GetDiagnosticDescriptors(Project projectOpt) | ||
| { | ||
| if (projectOpt == null) | ||
| { | ||
| return _workspaceAnalyzerManager.GetHostDiagnosticDescriptorsPerReference(); | ||
| return _hostAnalyzerManager.GetHostDiagnosticDescriptorsPerReference(); | ||
| } | ||
|
|
||
| return _workspaceAnalyzerManager.CreateDiagnosticDescriptorsPerReference(projectOpt); | ||
| return _hostAnalyzerManager.CreateDiagnosticDescriptorsPerReference(projectOpt); | ||
| } | ||
|
|
||
| public ImmutableArray<DiagnosticDescriptor> GetDiagnosticDescriptors(DiagnosticAnalyzer analyzer) | ||
| { | ||
| return _workspaceAnalyzerManager.GetDiagnosticDescriptors(analyzer); | ||
| return _hostAnalyzerManager.GetDiagnosticDescriptors(analyzer); | ||
| } | ||
|
|
||
| public void Reanalyze(Workspace workspace, IEnumerable<ProjectId> projectIds = null, IEnumerable<DocumentId> documentIds = 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.
Why? I'd thought we explicitly want to de-duplicate. Is the suggestion here that situation is too contrived to be assumed realistic?
I could accidentally come up in a situation with duplicate analyzer references. A project nuget reference might be pulling in analyzer and it might also be installed in the box on some machine which wants specific analyzer enabled for all development. I don't want to remove project nuget reference if not all of machines on which project is developed on has VSIX or host analyzer.
Is it non-trivial to de-duplicate here? Else, we should just do 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.
@mavasani that de-duplication will work. so, having same analyzer reference (basically same file) in multiple places regardless of layer (host, project) works.
the one I removed is same instance of diagnostic analyzer in two different analyzer reference. for this to happen, people has to do what the test did. create custom analyzer references and put same instance of diagnostic analyzer in multiple references.
I don't think that is something we need to care.
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.
Agreed, thanks for the explanation.