Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CodeFixService simplifications #59582

Merged
merged 3 commits into from
Feb 16, 2022
Merged

CodeFixService simplifications #59582

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e96fb91
Simplify fields and lambdas
CyrusNajmabadi Feb 16, 2022
2017f92
Simplify further
CyrusNajmabadi Feb 16, 2022
25a439f
Simplify further
CyrusNajmabadi Feb 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 44 additions & 66 deletions src/Features/Core/Portable/CodeFixes/CodeFixService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,25 +39,23 @@ internal partial class CodeFixService : ICodeFixService
new((d1, d2) => DiagnosticId.CompareOrdinal(d1.Id, d2.Id));

private readonly IDiagnosticAnalyzerService _diagnosticService;
private readonly ImmutableArray<Lazy<CodeFixProvider, CodeChangeProviderMetadata>> _fixers;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't store as an IEnumerable.

private readonly Dictionary<string, List<Lazy<CodeFixProvider, CodeChangeProviderMetadata>>> _fixersPerLanguageMap;

private readonly Func<Workspace, ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<DiagnosticId, ImmutableArray<CodeFixProvider>>>>> _getWorkspaceFixersMap;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove unnecessary callbacks. These can just be normal methods in this type.

private ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<DiagnosticId, ImmutableArray<CodeFixProvider>>>>? _lazyWorkspaceFixersMap;
private readonly ConditionalWeakTable<IReadOnlyList<AnalyzerReference>, ImmutableDictionary<DiagnosticId, List<CodeFixProvider>>> _projectFixersMap;
private readonly ConditionalWeakTable<IReadOnlyList<AnalyzerReference>, ImmutableDictionary<DiagnosticId, List<CodeFixProvider>>> _projectFixersMap = new();

// Shared by project fixers and workspace fixers.
private ImmutableDictionary<CodeFixProvider, ImmutableArray<DiagnosticId>> _fixerToFixableIdsMap = ImmutableDictionary<CodeFixProvider, ImmutableArray<DiagnosticId>>.Empty;
private readonly Lazy<ImmutableDictionary<CodeFixProvider, CodeChangeProviderMetadata>> _lazyFixerToMetadataMap;
private readonly ConditionalWeakTable<AnalyzerReference, ProjectCodeFixProvider> _analyzerReferenceToFixersMap = new();
private readonly ConditionalWeakTable<AnalyzerReference, ProjectCodeFixProvider>.CreateValueCallback _createProjectCodeFixProvider = r => new ProjectCodeFixProvider(r);
private readonly ImmutableDictionary<LanguageKind, Lazy<ImmutableArray<IConfigurationFixProvider>>> _configurationProvidersMap;
private readonly ImmutableArray<Lazy<IErrorLoggerService>> _errorLoggers;

private readonly Func<Workspace, ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<CodeFixProvider, int>>>> _getFixerPriorityMap;
private ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<DiagnosticId, ImmutableArray<CodeFixProvider>>>>? _lazyWorkspaceFixersMap;
private ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<CodeFixProvider, int>>>? _lazyFixerPriorityMap;

private readonly ConditionalWeakTable<AnalyzerReference, ProjectCodeFixProvider> _analyzerReferenceToFixersMap;
private readonly ConditionalWeakTable<AnalyzerReference, ProjectCodeFixProvider>.CreateValueCallback _createProjectCodeFixProvider;

private readonly ImmutableDictionary<LanguageKind, Lazy<ImmutableArray<IConfigurationFixProvider>>> _configurationProvidersMap;
private readonly IEnumerable<Lazy<IErrorLoggerService>> _errorLoggers;

private ImmutableDictionary<object, FixAllProviderInfo?> _fixAllProviderMap;
private ImmutableDictionary<CodeFixProvider, ImmutableArray<DiagnosticId>> _fixerToFixableIdsMap = ImmutableDictionary<CodeFixProvider, ImmutableArray<DiagnosticId>>.Empty;
private ImmutableDictionary<object, FixAllProviderInfo?> _fixAllProviderMap = ImmutableDictionary<object, FixAllProviderInfo?>.Empty;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move mutable fields to bottom.


[ImportingConstructor]
[SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")]
Expand All @@ -67,24 +65,15 @@ public CodeFixService(
[ImportMany] IEnumerable<Lazy<CodeFixProvider, CodeChangeProviderMetadata>> fixers,
[ImportMany] IEnumerable<Lazy<IConfigurationFixProvider, CodeChangeProviderMetadata>> configurationProviders)
{
_errorLoggers = loggers;
_diagnosticService = diagnosticAnalyzerService;
_errorLoggers = loggers.ToImmutableArray();

_lazyFixerToMetadataMap = new(() => fixers.Where(service => service.IsValueCreated).ToImmutableDictionary(service => service.Value, service => service.Metadata));
var fixersPerLanguageMap = fixers.ToPerLanguageMapWithMultipleLanguages();
var configurationProvidersPerLanguageMap = configurationProviders.ToPerLanguageMapWithMultipleLanguages();

_getWorkspaceFixersMap = workspace => GetFixerPerLanguageMap(fixersPerLanguageMap, workspace);
_configurationProvidersMap = GetConfigurationProvidersPerLanguageMap(configurationProvidersPerLanguageMap);
_fixers = fixers.ToImmutableArray();
_fixersPerLanguageMap = _fixers.ToPerLanguageMapWithMultipleLanguages();

// REVIEW: currently, fixer's priority is statically defined by the fixer itself. might considering making it more dynamic or configurable.
_getFixerPriorityMap = workspace => GetFixerPriorityPerLanguageMap(fixersPerLanguageMap, workspace);
_lazyFixerToMetadataMap = new(() => fixers.Where(service => service.IsValueCreated).ToImmutableDictionary(service => service.Value, service => service.Metadata));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, when this runs in practice are there any fixers already created or is this just initializing an empty map?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(why this is stuffed in a Lazy of its own is terrifying...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mavasani this is a bit scary. this lazy is exposed through:

private ImmutableDictionary<CodeFixProvider, CodeChangeProviderMetadata> FixerToMetadataMap => _lazyFixerToMetadataMap.Value;

This means that we capture whatever the state of hte world is when this is computed. Meaning if a service hasn't been created, then it's never in this map. This seems fishy, esp in a multi-language world. How do we know that we won't capture a snapshot of things in a bad state?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attempting to fix that here: #59586


// Per-project fixers
_projectFixersMap = new ConditionalWeakTable<IReadOnlyList<AnalyzerReference>, ImmutableDictionary<string, List<CodeFixProvider>>>();
_analyzerReferenceToFixersMap = new ConditionalWeakTable<AnalyzerReference, ProjectCodeFixProvider>();
_createProjectCodeFixProvider = new ConditionalWeakTable<AnalyzerReference, ProjectCodeFixProvider>.CreateValueCallback(r => new ProjectCodeFixProvider(r));
_fixAllProviderMap = ImmutableDictionary<object, FixAllProviderInfo?>.Empty;
_configurationProvidersMap = GetConfigurationProvidersPerLanguageMap(configurationProviders);
}

private ImmutableDictionary<CodeFixProvider, CodeChangeProviderMetadata> FixerToMetadataMap => _lazyFixerToMetadataMap.Value;
Expand Down Expand Up @@ -171,7 +160,7 @@ public async Task<ImmutableArray<CodeFixCollection>> GetFixesAsync(
// group diagnostics by their diagnostics span
// invariant: later code gathers & runs CodeFixProviders for diagnostics with one identical diagnostics span (that gets set later as CodeFixCollection's TextSpan)
// order diagnostics by span.
SortedDictionary<TextSpan, List<DiagnosticData>>? aggregatedDiagnostics = null;
var aggregatedDiagnostics = new SortedDictionary<TextSpan, List<DiagnosticData>>();

// For 'CodeActionPriorityRequest.Normal' or 'CodeActionPriorityRequest.Low', we do not compute suppression/configuration fixes,
// those fixes have a dedicated request priority 'CodeActionPriorityRequest.Lowest'.
Expand All @@ -196,11 +185,10 @@ public async Task<ImmutableArray<CodeFixCollection>> GetFixesAsync(

cancellationToken.ThrowIfCancellationRequested();

aggregatedDiagnostics ??= new SortedDictionary<TextSpan, List<DiagnosticData>>();
aggregatedDiagnostics.GetOrAdd(diagnostic.GetTextSpan(), _ => new List<DiagnosticData>()).Add(diagnostic);
}

if (aggregatedDiagnostics == null)
if (aggregatedDiagnostics.Count == 0)
return ImmutableArray<CodeFixCollection>.Empty;

// Order diagnostics by DiagnosticId so the fixes are in a deterministic order.
Expand Down Expand Up @@ -240,10 +228,10 @@ int GetValue(CodeFixCollection c)
{
// Ensure that we do not register duplicate configuration fixes.
using var _ = PooledHashSet<string>.GetInstance(out var registeredConfigurationFixTitles);
foreach (var spanAndDiagnostic in aggregatedDiagnostics)
foreach (var (span, diagnosticList) in aggregatedDiagnostics)
{
await AppendConfigurationsAsync(
document, spanAndDiagnostic.Key, spanAndDiagnostic.Value,
document, span, diagnosticList,
result, registeredConfigurationFixTitles, cancellationToken).ConfigureAwait(false);
}
}
Expand Down Expand Up @@ -306,7 +294,7 @@ private bool TryGetWorkspaceFixersMap(Document document, [NotNullWhen(true)] out
{
if (_lazyWorkspaceFixersMap == null)
{
var workspaceFixersMap = _getWorkspaceFixersMap(document.Project.Solution.Workspace);
var workspaceFixersMap = GetFixerPerLanguageMap(document.Project.Solution.Workspace);
Interlocked.CompareExchange(ref _lazyWorkspaceFixersMap, workspaceFixersMap, null);
}

Expand All @@ -317,7 +305,7 @@ private bool TryGetWorkspaceFixersPriorityMap(Document document, [NotNullWhen(tr
{
if (_lazyFixerPriorityMap == null)
{
var fixersPriorityByLanguageMap = _getFixerPriorityMap(document.Project.Solution.Workspace);
var fixersPriorityByLanguageMap = GetFixerPriorityPerLanguageMap(document.Project.Solution.Workspace);
Interlocked.CompareExchange(ref _lazyFixerPriorityMap, fixersPriorityByLanguageMap, null);
}

Expand Down Expand Up @@ -802,8 +790,6 @@ private bool IsInteractiveCodeFixProvider(CodeFixProvider provider)
AddImport.AbstractAddImportCodeFixProvider;
}

private static readonly Func<DiagnosticId, List<CodeFixProvider>> s_createList = _ => new List<CodeFixProvider>();

private ImmutableArray<DiagnosticId> GetFixableDiagnosticIds(CodeFixProvider fixer, IExtensionManager? extensionManager)
{
// If we are passed a null extension manager it means we do not have access to a document so there is nothing to
Expand Down Expand Up @@ -846,18 +832,17 @@ private static ImmutableArray<string> GetAndTestFixableDiagnosticIds(CodeFixProv
}

private ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<DiagnosticId, ImmutableArray<CodeFixProvider>>>> GetFixerPerLanguageMap(
Dictionary<LanguageKind, List<Lazy<CodeFixProvider, CodeChangeProviderMetadata>>> fixersPerLanguage,
Workspace workspace)
{
var fixerMap = ImmutableDictionary.Create<LanguageKind, Lazy<ImmutableDictionary<DiagnosticId, ImmutableArray<CodeFixProvider>>>>();
var extensionManager = workspace.Services.GetService<IExtensionManager>();
foreach (var languageKindAndFixers in fixersPerLanguage)
foreach (var (diagnosticId, lazyFixers) in _fixersPerLanguageMap)
{
var lazyMap = new Lazy<ImmutableDictionary<DiagnosticId, ImmutableArray<CodeFixProvider>>>(() =>
{
var mutableMap = new Dictionary<DiagnosticId, List<CodeFixProvider>>();

foreach (var lazyFixer in languageKindAndFixers.Value)
foreach (var lazyFixer in lazyFixers)
{
if (!TryGetWorkspaceFixer(lazyFixer, workspace, logExceptionWithInfoBar: true, out var fixer))
{
Expand All @@ -871,37 +856,39 @@ private ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<DiagnosticId,
continue;
}

var list = mutableMap.GetOrAdd(id, s_createList);
var list = mutableMap.GetOrAdd(id, static _ => new List<CodeFixProvider>());
list.Add(fixer);
}
}

var immutableMap = ImmutableDictionary.CreateBuilder<DiagnosticId, ImmutableArray<CodeFixProvider>>();
foreach (var diagnosticIdAndFixers in mutableMap)
foreach (var (diagnosticId, fixers) in mutableMap)
{
immutableMap.Add(diagnosticIdAndFixers.Key, diagnosticIdAndFixers.Value.AsImmutableOrEmpty());
immutableMap.Add(diagnosticId, fixers.AsImmutableOrEmpty());
}

return immutableMap.ToImmutable();
}, isThreadSafe: true);

fixerMap = fixerMap.Add(languageKindAndFixers.Key, lazyMap);
fixerMap = fixerMap.Add(diagnosticId, lazyMap);
}

return fixerMap;
}

private static ImmutableDictionary<LanguageKind, Lazy<ImmutableArray<IConfigurationFixProvider>>> GetConfigurationProvidersPerLanguageMap(
Dictionary<LanguageKind, List<Lazy<IConfigurationFixProvider, CodeChangeProviderMetadata>>> configurationProvidersPerLanguage)
IEnumerable<Lazy<IConfigurationFixProvider, CodeChangeProviderMetadata>> configurationProviders)
{
var configurationFixerMap = ImmutableDictionary.Create<LanguageKind, Lazy<ImmutableArray<IConfigurationFixProvider>>>();
foreach (var languageKindAndFixers in configurationProvidersPerLanguage)
var configurationProvidersPerLanguageMap = configurationProviders.ToPerLanguageMapWithMultipleLanguages();

var configurationFixerMap = ImmutableDictionary.CreateBuilder<LanguageKind, Lazy<ImmutableArray<IConfigurationFixProvider>>>();
foreach (var (diagnosticId, lazyFixers) in configurationProvidersPerLanguageMap)
{
var lazyConfigurationFixers = new Lazy<ImmutableArray<IConfigurationFixProvider>>(() => GetConfigurationFixProviders(languageKindAndFixers.Value));
configurationFixerMap = configurationFixerMap.Add(languageKindAndFixers.Key, lazyConfigurationFixers);
var lazyConfigurationFixers = new Lazy<ImmutableArray<IConfigurationFixProvider>>(() => GetConfigurationFixProviders(lazyFixers));
configurationFixerMap.Add(diagnosticId, lazyConfigurationFixers);
}

return configurationFixerMap;
return configurationFixerMap.ToImmutable();

static ImmutableArray<IConfigurationFixProvider> GetConfigurationFixProviders(List<Lazy<IConfigurationFixProvider, CodeChangeProviderMetadata>> languageKindAndFixers)
{
Expand All @@ -916,19 +903,17 @@ static ImmutableArray<IConfigurationFixProvider> GetConfigurationFixProviders(Li
}
}

private ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<CodeFixProvider, int>>> GetFixerPriorityPerLanguageMap(
Dictionary<LanguageKind, List<Lazy<CodeFixProvider, CodeChangeProviderMetadata>>> fixersPerLanguage,
Workspace workspace)
private ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<CodeFixProvider, int>>> GetFixerPriorityPerLanguageMap(Workspace workspace)
{
var languageMap = ImmutableDictionary.CreateBuilder<LanguageKind, Lazy<ImmutableDictionary<CodeFixProvider, int>>>();
foreach (var languageAndFixers in fixersPerLanguage)
foreach (var (diagnosticId, lazyFixers) in _fixersPerLanguageMap)
{
var lazyMap = new Lazy<ImmutableDictionary<CodeFixProvider, int>>(() =>
{
var priorityMap = ImmutableDictionary.CreateBuilder<CodeFixProvider, int>();

var lazyFixers = ExtensionOrderer.Order(languageAndFixers.Value);
for (var i = 0; i < lazyFixers.Count; i++)
var fixers = ExtensionOrderer.Order(lazyFixers);
for (var i = 0; i < fixers.Count; i++)
{
if (!TryGetWorkspaceFixer(lazyFixers[i], workspace, logExceptionWithInfoBar: false, out var fixer))
{
Expand All @@ -941,7 +926,7 @@ private ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<CodeFixProvid
return priorityMap.ToImmutable();
}, isThreadSafe: true);

languageMap.Add(languageAndFixers.Key, lazyMap);
languageMap.Add(diagnosticId, lazyMap);
}

return languageMap.ToImmutable();
Expand All @@ -952,13 +937,14 @@ private ImmutableDictionary<DiagnosticId, List<CodeFixProvider>> GetProjectFixer
// TODO (https://github.com/dotnet/roslyn/issues/4932): Don't restrict CodeFixes in Interactive
return project.Solution.Workspace.Kind == WorkspaceKind.Interactive
? ImmutableDictionary<DiagnosticId, List<CodeFixProvider>>.Empty
: _projectFixersMap.GetValue(project.AnalyzerReferences, pId => ComputeProjectFixers(project));
: _projectFixersMap.GetValue(project.AnalyzerReferences, _ => ComputeProjectFixers(project));
}

private ImmutableDictionary<DiagnosticId, List<CodeFixProvider>> ComputeProjectFixers(Project project)
{
var extensionManager = project.Solution.Workspace.Services.GetService<IExtensionManager>();
ImmutableDictionary<DiagnosticId, List<CodeFixProvider>>.Builder? builder = null;

var builder = ImmutableDictionary.CreateBuilder<DiagnosticId, List<CodeFixProvider>>();
foreach (var reference in project.AnalyzerReferences)
{
var projectCodeFixerProvider = _analyzerReferenceToFixersMap.GetValue(reference, _createProjectCodeFixProvider);
Expand All @@ -968,22 +954,14 @@ private ImmutableDictionary<DiagnosticId, List<CodeFixProvider>> ComputeProjectF
foreach (var id in fixableIds)
{
if (string.IsNullOrWhiteSpace(id))
{
continue;
}

builder ??= ImmutableDictionary.CreateBuilder<DiagnosticId, List<CodeFixProvider>>();
var list = builder.GetOrAdd(id, s_createList);
var list = builder.GetOrAdd(id, static _ => new List<CodeFixProvider>());
list.Add(fixer);
}
}
}

if (builder == null)
{
return ImmutableDictionary<DiagnosticId, List<CodeFixProvider>>.Empty;
}

return builder.ToImmutable();
}
}
Expand Down