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

Remove potential partial snapshot in codefixservice #59586

Merged
merged 5 commits into from
Feb 17, 2022
Merged

Remove potential partial snapshot in codefixservice #59586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
02603da
Remove potential partial snapshot in codefixservice
CyrusNajmabadi Feb 16, 2022
7be7d7d
Merge remote-tracking branch 'upstream/main' into partialSnapshot
CyrusNajmabadi Feb 16, 2022
de39539
Docs
CyrusNajmabadi Feb 16, 2022
3e68dd2
Simplify code
CyrusNajmabadi Feb 16, 2022
731c9ac
Simplify code
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
61 changes: 36 additions & 25 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 @@ -40,12 +40,11 @@ internal partial class CodeFixService : ICodeFixService

private readonly IDiagnosticAnalyzerService _diagnosticService;
private readonly ImmutableArray<Lazy<CodeFixProvider, CodeChangeProviderMetadata>> _fixers;
private readonly Dictionary<string, List<Lazy<CodeFixProvider, CodeChangeProviderMetadata>>> _fixersPerLanguageMap;
private readonly ImmutableDictionary<string, ImmutableArray<Lazy<CodeFixProvider, CodeChangeProviderMetadata>>> _fixersPerLanguageMap;

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

// Shared by project fixers and workspace fixers.
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;
Expand All @@ -56,6 +55,7 @@ internal partial class CodeFixService : ICodeFixService

private ImmutableDictionary<CodeFixProvider, ImmutableArray<DiagnosticId>> _fixerToFixableIdsMap = ImmutableDictionary<CodeFixProvider, ImmutableArray<DiagnosticId>>.Empty;
private ImmutableDictionary<object, FixAllProviderInfo?> _fixAllProviderMap = ImmutableDictionary<object, FixAllProviderInfo?>.Empty;
private ImmutableDictionary<CodeFixProvider, CodeChangeProviderMetadata?> _fixerToMetadataMap = ImmutableDictionary<CodeFixProvider, CodeChangeProviderMetadata?>.Empty;

[ImportingConstructor]
[SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")]
Expand All @@ -71,13 +71,9 @@ public CodeFixService(
_fixers = fixers.ToImmutableArray();
_fixersPerLanguageMap = _fixers.ToPerLanguageMapWithMultipleLanguages();

_lazyFixerToMetadataMap = new(() => fixers.Where(service => service.IsValueCreated).ToImmutableDictionary(service => service.Value, service => service.Metadata));

_configurationProvidersMap = GetConfigurationProvidersPerLanguageMap(configurationProviders);
}

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

public async Task<FirstDiagnosticResult> GetMostSevereFixableDiagnosticAsync(
Document document, TextSpan range, CancellationToken cancellationToken)
{
Expand Down Expand Up @@ -433,7 +429,7 @@ await AppendFixesOrConfigurationsAsync(
getFixes: dxs =>
{
var fixerName = fixer.GetType().Name;
FixerToMetadataMap.TryGetValue(fixer, out var fixerMetadata);
var fixerMetadata = TryGetMetadata(fixer);

using (addOperationScope(fixerName))
using (RoslynEventSource.LogInformationalBlock(FunctionId.CodeFixes_GetCodeFixesAsync, fixerName, cancellationToken))
Expand Down Expand Up @@ -468,6 +464,27 @@ await AppendFixesOrConfigurationsAsync(
}
}

private CodeChangeProviderMetadata? TryGetMetadata(CodeFixProvider fixer)
{
return ImmutableInterlocked.GetOrAdd(
ref _fixerToMetadataMap,
fixer,
static (fixer, fixers) =>
{
foreach (var lazy in fixers)
{
if (lazy.IsValueCreated && lazy.Value == fixer)
return lazy.Metadata;
}

// Note: it feels very strange that we could ever not find a fixer in our list. However, this
// occurs in testing scenarios. I'm not sure if the tests represent a bogus potential input, or if
// this is something that can actually occur in practice and we want to keep working.
return null;
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 for thoughts on this. Specifically, if i throw here, tests in CodeFixService tests fail. but i think they're likely constructing this service wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

for example it does:

            var fixService = new CodeFixService(
                diagnosticService, logger, fixers, SpecializedCollections.EmptyEnumerable<Lazy<IConfigurationFixProvider, CodeChangeProviderMetadata>>());

So we have a list of fixers, but no metadata about htem. i can't tell if htis is a reasonable or rational think that coudl actually happen in practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you recall which tests were failing? From what I vaguely recall, we had some scenario where if a code fixer throws some exception during creation due to some missing types or something, we wouldn't be able to create an instance of the fixer, and had to handle this error case with nulls. I am wondering if the failures you saw were coming from tests explicitly validating exception during fixer creation.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, it was tests in CodeFixServiceTets. THere we manually instnatiated some test-only CodeFixProviders, then instantiated the CodeFixService. But we passed along an empty metadata-array, presumably because we didn't care in those tests and the code was resilient to not having it. But i can't tell if that's a real scenario taht occurs in practice.

},
_fixers);
}

private static async Task<ImmutableArray<CodeFix>> GetCodeFixesAsync(
Document document, TextSpan span, CodeFixProvider fixer, CodeChangeProviderMetadata? fixerMetadata, CodeActionOptions options,
ImmutableArray<Diagnostic> diagnostics,
Expand Down Expand Up @@ -611,7 +628,7 @@ await diagnosticsWithSameSpan.OrderByDescending(d => d.Severity)
}

// If the fix provider supports fix all occurrences, then get the corresponding FixAllProviderInfo and fix all context.
var fixAllProviderInfo = extensionManager.PerformFunction<FixAllProviderInfo?>(fixer, () => ImmutableInterlocked.GetOrAdd(ref _fixAllProviderMap, fixer, FixAllProviderInfo.Create), defaultValue: null);
var fixAllProviderInfo = extensionManager.PerformFunction(fixer, () => ImmutableInterlocked.GetOrAdd(ref _fixAllProviderMap, fixer, FixAllProviderInfo.Create), defaultValue: null);

FixAllState? fixAllState = null;
var supportedScopes = ImmutableArray<FixAllScope>.Empty;
Expand Down Expand Up @@ -840,7 +857,7 @@ private ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<DiagnosticId,
{
var lazyMap = new Lazy<ImmutableDictionary<DiagnosticId, ImmutableArray<CodeFixProvider>>>(() =>
{
var mutableMap = new Dictionary<DiagnosticId, List<CodeFixProvider>>();
using var _ = PooledDictionary<DiagnosticId, ArrayBuilder<CodeFixProvider>>.GetInstance(out var mutableMap);

foreach (var lazyFixer in lazyFixers)
{
Expand All @@ -856,18 +873,12 @@ private ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<DiagnosticId,
continue;
}

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

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

return immutableMap.ToImmutable();
return mutableMap.ToImmutableDictionary(kvp => kvp.Key, kvp => kvp.Value.ToImmutableAndFree());
}, isThreadSafe: true);

fixerMap = fixerMap.Add(diagnosticId, lazyMap);
Expand All @@ -890,7 +901,7 @@ private static ImmutableDictionary<LanguageKind, Lazy<ImmutableArray<IConfigurat

return configurationFixerMap.ToImmutable();

static ImmutableArray<IConfigurationFixProvider> GetConfigurationFixProviders(List<Lazy<IConfigurationFixProvider, CodeChangeProviderMetadata>> languageKindAndFixers)
static ImmutableArray<IConfigurationFixProvider> GetConfigurationFixProviders(ImmutableArray<Lazy<IConfigurationFixProvider, CodeChangeProviderMetadata>> languageKindAndFixers)
{
using var builderDisposer = ArrayBuilder<IConfigurationFixProvider>.GetInstance(out var builder);
var orderedLanguageKindAndFixers = ExtensionOrderer.Order(languageKindAndFixers);
Expand Down Expand Up @@ -932,19 +943,19 @@ private ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<CodeFixProvid
return languageMap.ToImmutable();
}

private ImmutableDictionary<DiagnosticId, List<CodeFixProvider>> GetProjectFixers(Project project)
private ImmutableDictionary<DiagnosticId, ImmutableArray<CodeFixProvider>> GetProjectFixers(Project project)
{
// 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
? ImmutableDictionary<DiagnosticId, ImmutableArray<CodeFixProvider>>.Empty
: _projectFixersMap.GetValue(project.AnalyzerReferences, _ => ComputeProjectFixers(project));
}

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

var builder = ImmutableDictionary.CreateBuilder<DiagnosticId, List<CodeFixProvider>>();
using var _ = PooledDictionary<DiagnosticId, ArrayBuilder<CodeFixProvider>>.GetInstance(out var builder);
foreach (var reference in project.AnalyzerReferences)
{
var projectCodeFixerProvider = _analyzerReferenceToFixersMap.GetValue(reference, _createProjectCodeFixProvider);
Expand All @@ -956,13 +967,13 @@ private ImmutableDictionary<DiagnosticId, List<CodeFixProvider>> ComputeProjectF
if (string.IsNullOrWhiteSpace(id))
continue;

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

return builder.ToImmutable();
return builder.ToImmutableDictionary(kvp => kvp.Key, kvp => kvp.Value.ToImmutableAndFree());
}
}
}
8 changes: 4 additions & 4 deletions src/Workspaces/Core/Portable/Shared/Extensions/ILanguageMetadataExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,24 +41,24 @@ public static ImmutableDictionary<string, ImmutableArray<Lazy<TInterface, TMetad
return builder.Select(kvp => new KeyValuePair<string, ImmutableArray<Lazy<TInterface, TMetadata>>>(kvp.Key, kvp.Value.ToImmutableAndFree())).ToImmutableDictionary();
}

public static Dictionary<string, List<Lazy<TInterface, TMetadata>>> ToPerLanguageMapWithMultipleLanguages<TInterface, TMetadata>(this IEnumerable<Lazy<TInterface, TMetadata>> services)
public static ImmutableDictionary<string, ImmutableArray<Lazy<TInterface, TMetadata>>> ToPerLanguageMapWithMultipleLanguages<TInterface, TMetadata>(this IEnumerable<Lazy<TInterface, TMetadata>> services)
where TMetadata : ILanguagesMetadata
{
var map = new Dictionary<string, List<Lazy<TInterface, TMetadata>>>();
using var _ = PooledDictionary<string, ArrayBuilder<Lazy<TInterface, TMetadata>>>.GetInstance(out var map);

foreach (var service in services)
{
foreach (var language in service.Metadata.Languages.Distinct())
{
if (!string.IsNullOrEmpty(language))
{
var list = map.GetOrAdd(language, _ => new List<Lazy<TInterface, TMetadata>>());
var list = map.GetOrAdd(language, _ => ArrayBuilder<Lazy<TInterface, TMetadata>>.GetInstance());
list.Add(service);
}
}
}

return map;
return map.ToImmutableDictionary(kvp => kvp.Key, kvp => kvp.Value.ToImmutableAndFree());
}
}
}