-
Notifications
You must be signed in to change notification settings - Fork 4k
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
CodeFixService simplifications #59582
Conversation
|
||
private readonly Func<Workspace, ImmutableDictionary<LanguageKind, Lazy<ImmutableDictionary<DiagnosticId, ImmutableArray<CodeFixProvider>>>>> _getWorkspaceFixersMap; |
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.
remove unnecessary callbacks. These can just be normal methods in this type.
@@ -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; |
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.
don't store as an IEnumerable.
|
||
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; |
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.
move mutable fields to bottom.
a6f8838
to
25a439f
Compare
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.
My only real complaint is there's still code left after all this cleanup. 😛
|
||
// 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)); |
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.
Out of curiosity, when this runs in practice are there any fixers already created or is this just initializing an empty map?
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 this is stuffed in a Lazy of its own is terrifying...)
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 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?
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.
attempting to fix that here: #59586
I'm working in this area, and makign a bunch of a changes. This extracts out some general cleanup that comes with no behavior changes.