-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Remove CompilationPair abstraction in diagnostic analyzer subsystem. #80086
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
ab2b2de
1114e46
2527492
c37ee9e
a46e753
b051d87
2c15ab0
8ab740e
522ba7f
750bb54
06838ce
cb533f9
28b21a8
06364bc
dabdd08
5273899
325cfbb
1583c82
ccae16c
8ab1449
8b5ace9
10d5103
ceb530c
7cf5a48
6398a7b
a114a53
48f563e
006d929
5ff9b15
bf367f6
22c861a
f4eab9a
2180608
b1d93a6
45c14ba
d244d54
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,73 @@ | ||
| // 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.Generic; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Linq; | ||
| using Microsoft.CodeAnalysis.Diagnostics.Analyzers.NamingStyles; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.Diagnostics; | ||
|
|
||
| internal static class AnalyzerOptionsUtilities | ||
| { | ||
| /// <summary> | ||
| /// Combines two <see cref="AnalyzerOptions"/> instances into one. The resulting instance will have the | ||
| /// options merged from both. Options defined in <paramref name="projectAnalyzerOptions"/> ("EditorConfig options") | ||
| /// will take precedence over those in <paramref name="hostAnalyzerOptions"/> (VS UI options). | ||
| /// </summary> | ||
| public static AnalyzerOptions Combine(AnalyzerOptions projectAnalyzerOptions, AnalyzerOptions hostAnalyzerOptions) | ||
| { | ||
| return new AnalyzerOptions( | ||
| projectAnalyzerOptions.AdditionalFiles.AddRange(hostAnalyzerOptions.AdditionalFiles).Distinct(), | ||
| new CombinedAnalyzerConfigOptionsProvider(projectAnalyzerOptions, hostAnalyzerOptions)); | ||
| } | ||
|
|
||
| private sealed class CombinedAnalyzerConfigOptionsProvider( | ||
| AnalyzerOptions projectAnalyzerOptions, | ||
| AnalyzerOptions hostAnalyzerOptions) : AnalyzerConfigOptionsProvider | ||
| { | ||
| private readonly AnalyzerOptions _analyzerOptions = projectAnalyzerOptions; | ||
| private readonly AnalyzerOptions _hostAnalyzerOptions = hostAnalyzerOptions; | ||
|
|
||
| public override AnalyzerConfigOptions GlobalOptions | ||
| => new CombinedAnalyzerConfigOptions( | ||
| _analyzerOptions.AnalyzerConfigOptionsProvider.GlobalOptions, | ||
| _hostAnalyzerOptions.AnalyzerConfigOptionsProvider.GlobalOptions); | ||
|
|
||
| public override AnalyzerConfigOptions GetOptions(SyntaxTree tree) | ||
| => new CombinedAnalyzerConfigOptions( | ||
| _analyzerOptions.AnalyzerConfigOptionsProvider.GetOptions(tree), | ||
| _hostAnalyzerOptions.AnalyzerConfigOptionsProvider.GetOptions(tree)); | ||
|
|
||
| public override AnalyzerConfigOptions GetOptions(AdditionalText textFile) | ||
| => new CombinedAnalyzerConfigOptions( | ||
| _analyzerOptions.AnalyzerConfigOptionsProvider.GetOptions(textFile), | ||
| _hostAnalyzerOptions.AnalyzerConfigOptionsProvider.GetOptions(textFile)); | ||
|
|
||
| private sealed class CombinedAnalyzerConfigOptions( | ||
| AnalyzerConfigOptions projectOptions, | ||
| AnalyzerConfigOptions hostOptions) : StructuredAnalyzerConfigOptions | ||
| { | ||
| public override bool TryGetValue(string key, [NotNullWhen(true)] out string? value) | ||
| // Lookup in project options first. Editor config should override the values from the host. | ||
| => projectOptions.TryGetValue(key, out value) || hostOptions.TryGetValue(key, out value); | ||
|
|
||
| public override IEnumerable<string> Keys | ||
| => projectOptions.Keys.Union(hostOptions.Keys); | ||
|
|
||
| public override NamingStylePreferences GetNamingStylePreferences() | ||
| { | ||
| var preferences = (projectOptions as StructuredAnalyzerConfigOptions)?.GetNamingStylePreferences(); | ||
| if (preferences is { IsEmpty: false }) | ||
| return preferences; | ||
|
|
||
| preferences = (hostOptions as StructuredAnalyzerConfigOptions)?.GetNamingStylePreferences(); | ||
| if (preferences is { IsEmpty: false }) | ||
| return preferences; | ||
|
|
||
| return NamingStylePreferences.Empty; | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ namespace Microsoft.CodeAnalysis.Diagnostics; | |
| internal sealed partial class DiagnosticAnalyzerService | ||
| { | ||
| /// <summary> | ||
| /// Cached data from a <see cref="ProjectState"/> to the <see cref="CompilationWithAnalyzersPair"/>s | ||
| /// Cached data from a <see cref="ProjectState"/> to the <see cref="CompilationWithAnalyzers"/>s | ||
|
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. 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. Can do in followup. |
||
| /// we've created for it. Note: the CompilationWithAnalyzersPair instance is dependent on the set of <see | ||
| /// cref="DiagnosticAnalyzer"/>s passed along with the project. | ||
| /// <para/> | ||
|
|
@@ -33,14 +33,14 @@ private static readonly ConditionalWeakTable< | |
| ProjectState, | ||
| SmallDictionary< | ||
| (Checksum checksum, ImmutableArray<DiagnosticAnalyzer> analyzers), | ||
| AsyncLazy<CompilationWithAnalyzersPair?>>> s_projectToCompilationWithAnalyzers = new(); | ||
| AsyncLazy<CompilationWithAnalyzers?>>> s_projectToCompilationWithAnalyzers = new(); | ||
|
|
||
| /// <summary> | ||
| /// <summary> | ||
| /// Protection around the SmallDictionary in <see cref="s_projectToCompilationWithAnalyzers"/>. | ||
| /// </summary> | ||
| private static readonly SemaphoreSlim s_gate = new(initialCount: 1); | ||
|
|
||
| private static async Task<CompilationWithAnalyzersPair?> GetOrCreateCompilationWithAnalyzersAsync( | ||
| private static async Task<CompilationWithAnalyzers?> GetOrCreateCompilationWithAnalyzersAsync( | ||
| Project project, | ||
| ImmutableArray<DiagnosticAnalyzer> analyzers, | ||
| HostAnalyzerInfo hostAnalyzerInfo, | ||
|
|
@@ -57,7 +57,7 @@ private static readonly ConditionalWeakTable< | |
| var map = s_projectToCompilationWithAnalyzers.GetValue( | ||
| project.State, static _ => new(ChecksumAndAnalyzersEqualityComparer.Instance)); | ||
|
|
||
| AsyncLazy<CompilationWithAnalyzersPair?>? lazy; | ||
| AsyncLazy<CompilationWithAnalyzers?>? lazy; | ||
| using (await s_gate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false)) | ||
| { | ||
| var checksumAndAnalyzers = (checksum, analyzers); | ||
|
|
@@ -75,7 +75,7 @@ private static readonly ConditionalWeakTable< | |
| // <summary> | ||
| // Should only be called on a <see cref="Project"/> that <see cref="Project.SupportsCompilation"/>. | ||
| // </summary> | ||
| static async Task<CompilationWithAnalyzersPair?> CreateCompilationWithAnalyzersAsync( | ||
| static async Task<CompilationWithAnalyzers?> CreateCompilationWithAnalyzersAsync( | ||
| (Project project, | ||
| ImmutableArray<DiagnosticAnalyzer> analyzers, | ||
| HostAnalyzerInfo hostAnalyzerInfo, | ||
|
|
@@ -117,9 +117,13 @@ private static readonly ConditionalWeakTable< | |
| }; | ||
|
|
||
| // Create driver that holds onto compilation and associated analyzers | ||
| return new( | ||
| CreateCompilationWithAnalyzers(compilation, filteredProjectAnalyzers, project.State.ProjectAnalyzerOptions, exceptionFilter), | ||
| CreateCompilationWithAnalyzers(compilation, filteredHostAnalyzers, project.HostAnalyzerOptions, exceptionFilter)); | ||
| return CreateCompilationWithAnalyzers( | ||
| compilation, | ||
| filteredHostAnalyzers.Concat(filteredProjectAnalyzers).Distinct(), | ||
|
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.
Trying to work my way through the WhereAsArray mental gymnastics above, and I'm probably wrong, but it seems like all that reduces to this parameter just being analyzers.Where(a => !a.IsWorkspaceDiagnosticAnalyzer()) and not needing any of those intermediate calculations 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. yup. that seems to be true. Are you ok with that happening in a followup. My plan was to go over all this code and reduce complexity now that we only have one list of analyzers. Thanks! :) 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. Definitely ok with any change related to this to be in a followup PR |
||
| AnalyzerOptionsUtilities.Combine( | ||
| project.State.ProjectAnalyzerOptions, | ||
| project.HostAnalyzerOptions), | ||
| exceptionFilter); | ||
| } | ||
|
|
||
| static CompilationWithAnalyzers? CreateCompilationWithAnalyzers( | ||
|
|
||
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 correct behavior. The test here is testing " 'usings' should be INSIDE the namespace, and it is an ERROR otherwise". So it was a bug this wasn't moving in in CodeCleanup.
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.
Note: this differs from teh tests taht validate that nothing change when the option suggestions the change, but the severity is set to 'hidden'. in that case we do not make any change.