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.

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

Improve editorconfig options caching on ProjectState #61131

Merged
merged 4 commits into from
May 10, 2022

Conversation

tmat
Copy link
Member

@tmat tmat commented May 4, 2022

Fixes #41840 by adding a new compiler API:

public class AnalyzerConfigOptions
{
    ...
    /// <summary>
    /// Enumerates keys of all available options in no specific order.
    /// </summary>
    /// <exception cref="NotImplementedException">Not implemented by the derived type.</exception>
    public virtual IEnumerable<string> Keys
         => throw new NotImplementedException();
    ...
}

Use this API in Code Style and Workspace layers to parse NamingStylePreferences from the options.

ProjectState maintains a cache of editorconfig options for given source file path. The cache holds on raw options in form of ImmutableDictionary<string, string> as returned by the compiler editorconfig parser. We currently wrap that dictionary in a new instance of DictionaryAnalyzerConfigOptions every time options are queried for the path. Elsewhere in the code we also maintain conditional weak table of the raw options dictionary to parsed NameStylePreferences for caching purposes. As a TODO in the code says this caching logic is rather fragile. In yet another place in the code we have reflection code that relies on the fact that DictionaryAnalyzerConfigOptions, and not any other type, is used to represent the options. This is also fragile since there is no direct relationship between the helper that uses reflection and DictionaryAnalyzerConfigOptions type.

This change introduces StructuredAnalyzerConfigOptions type that is used to cache both raw and parsed editorconfig options on ProjectState. The type itself provides facilities to cache NameStylePreferences (or possibly other expensive to parse options in future) as well as translation between Workspace, Compiler and Code Style layer representations of the options.

@tmat tmat changed the title StructuredAnalyzerConfigOptions Improve editoroptions caching on ProjectState May 5, 2022
@tmat tmat force-pushed the StructuredACO branch from d42205c to 7719856 Compare May 5, 2022 04:30
@tmat tmat force-pushed the StructuredACO branch from b009f73 to b4e152f Compare May 5, 2022 15:54
@tmat tmat changed the title Improve editoroptions caching on ProjectState Improve editorconfig options caching on ProjectState May 5, 2022
@tmat tmat marked this pull request as ready for review May 5, 2022 18:48
@tmat tmat requested review from a team as code owners May 5, 2022 18:48
@tmat
Copy link
Member Author

tmat commented May 5, 2022

@333fred PTAL, compiler public API change.

@tmat
Copy link
Member Author

tmat commented May 5, 2022

@JoeRobich @sharwell @jmarolf @mavasani PTAL

@chsienki
Copy link
Contributor

chsienki commented May 5, 2022

Compiler side changes LGTM, assuming we ok the overall API.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Compiler changes LGTM. The API needs to be reviewed by API design, but as long as this doesn't ship publicly before then it's ok to proceed.

@jmarolf jmarolf self-assigned this May 6, 2022
Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

Review started but giving early feedback to address the comparer validation

@@ -513,7 +513,7 @@ private void InitializeCachedOptions(OptionSet solutionOptions)
Interlocked.CompareExchange(ref _cachedOptions, newAsyncLazy, comparand: null);
}

internal Task<ImmutableDictionary<string, string>> GetAnalyzerOptionsAsync(CancellationToken cancellationToken)
internal async Task<AnalyzerConfigData?> GetAnalyzerOptionsAsync(CancellationToken cancellationToken)
Copy link
Member

Choose a reason for hiding this comment

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

💭 This seems like a good candidate for ValueTask<>

Copy link
Member Author

Choose a reason for hiding this comment

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

Will follow up.

@tmat tmat merged commit 7be9fe7 into dotnet:main May 10, 2022
@tmat tmat deleted the StructuredACO branch May 10, 2022 00:44
@ghost ghost added this to the Next milestone May 10, 2022
333fred added a commit that referenced this pull request May 10, 2022
333fred added a commit that referenced this pull request May 11, 2022
tmat added a commit that referenced this pull request May 12, 2022
tmat added a commit that referenced this pull request May 13, 2022
333fred added a commit to 333fred/roslyn that referenced this pull request May 16, 2022
…o poison

* upstream/features/required-members: (413 commits)
  [EE] Implement IDkmClrFullNameProvider2 in Roslyn's ResultProvider Formatter. (dotnet#60522)
  Remove parameter null-checking from the Language Feature Status list (dotnet#61302)
  Add pointer for `AnalysisLevel` to warning waves doc (dotnet#61196)
  Add an UWP OptProf test for IDE
  Add test
  Fix issue where we were getting a raw-string in a skipped token, causing a crash
  Fix several LSP completion kind mappings (dotnet#61243)
  Relax assertion in SyntheticBoundNodeFactory.Convert (dotnet#61287)
  Enable add usings on paste by default (dotnet#61299)
  Fix focus on rename UI opening (dotnet#60846)
  Update PublishData.json with new package
  Remove Razor and editor inference document option providers (dotnet#61091)
  Fix nested in generic type binding issues in enabled nullability context (dotnet#61182)
  Revert "Revert "Improve editorconfig options caching on ProjectState (dotnet#61131)" (dotnet#61216)" (dotnet#61283)
  Move MSBuild back to 16.5.0.
  Fix a few build and package issues (dotnet#61273)
  lint
  Revert workspaces msbuild changes.
  Use SegmentedHashSet<T> to eliminate LOH allocations in AsyncBatchingWorkQueue
  NRT
  ...
@Cosifne Cosifne modified the milestones: Next, 17.3 P2 May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need API for enumerating AnalyzerConfigOptions
6 participants