Cleanup in SettingsHelper and add caching of StyleCopSettings objects

[bjornhellander]

Partial fix for #3634.
Handles some review comments from PR #3642.

[codecov]

Codecov Report

Merging #3648 (8c7c516) into master (48ae833) will decrease coverage by 0.01%.
The diff coverage is 92.44%.

❗ Current head 8c7c516 differs from pull request most recent head a738da8. Consider uploading reports for the commit a738da8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3648      +/-   ##
==========================================
- Coverage   93.26%   93.26%   -0.01%     
==========================================
  Files        1080     1082       +2     
  Lines      113902   113999      +97     
  Branches     4035     4041       +6     
==========================================
+ Hits       106229   106319      +90     
- Misses       6636     6641       +5     
- Partials     1037     1039       +2     

[bjornhellander]

I had a look at #3642 (comment) and started trying out some ways to optimize the case where more than one action is registered in the same analyzer. I thought I had a pretty trivial change, but a lot of tests for SA1516 fail and I haven't been abe to find the root cause yet. The actions are simply not called. And these are the only tests failing, at least for c# 6 which I have been running. I see that the register calls that are relevant in the failing cases is to a new method that I added, but it is used in 27 other places as well and the tests for those analyzers all pass.

My idea was to create a custom version of RegisterCompilationStartAction and have that one lookup the settings from the cache. That way, each analyzer wouldn't need to do it themselves as I interpreted your comment to suggest, but instead keep it hidden inside for example the RegisterSyntaxNodeAction extension method.

StyleCopAnalyzers/StyleCop.Analyzers/StyleCop.Analyzers/AnalyzerExtensions.cs

Lines 20 to 28 in 77555fa

	public static void RegisterCompilationStartActionWithSettings(this AnalysisContext context, Action<CompilationStartAnalysisContextWithSettings> action)
	{
	context.RegisterCompilationStartAction(context =>
	{
	var settingsFile = context.GetStyleCopSettingsFile(context.CancellationToken);
	var contextWithSettings = new CompilationStartAnalysisContextWithSettings(context, settingsFile);
	action(contextWithSettings);
	});
	}

The new register method:

StyleCopAnalyzers/StyleCop.Analyzers/StyleCop.Analyzers/AnalyzerExtensions.cs

Lines 100 to 104 in 77555fa

	public static void RegisterSyntaxNodeAction<TLanguageKindEnum>(this CompilationStartAnalysisContextWithSettings context, Action<SyntaxNodeAnalysisContext> action, params TLanguageKindEnum[] syntaxKinds)
	where TLanguageKindEnum : struct
	{
	context.InnerContext.RegisterSyntaxNodeAction(action, syntaxKinds);
	}

The first and last calls to RegisterSyntaxNodeAction in SA1516 tries to register the actions (without settings object as parameter) that never get called:

StyleCopAnalyzers/StyleCop.Analyzers/StyleCop.Analyzers/LayoutRules/SA1516ElementsMustBeSeparatedByBlankLine.cs

Lines 122 to 134 in 77555fa

	public override void Initialize(AnalysisContext context)
	{
	context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
	context.EnableConcurrentExecution();
	context.RegisterCompilationStartActionWithSettings(context =>
	{
	context.RegisterSyntaxNodeAction(TypeDeclarationAction, SyntaxKinds.TypeDeclaration);
	context.RegisterSyntaxNodeAction(CompilationUnitAction, SyntaxKind.CompilationUnit);
	context.RegisterSyntaxNodeAction(NamespaceDeclarationAction, SyntaxKind.NamespaceDeclaration);
	context.RegisterSyntaxNodeAction(BasePropertyDeclarationAction, SyntaxKinds.BasePropertyDeclaration);
	});
	}

@sharwell I pushed anyway in case you or anyone else can spot any mistakes in the last commit which could explain the failing SA1516 tests. Or just stop me if you think this is a bad idea.

[bjornhellander]

Never mind. I found the mistake. Updates are coming.

[bjornhellander]

@sharwell I have tried to handle most of the comments you had on my previous pull request. My hope is that RegisterCompilationStartActionWithSettings will also make it easy to get a cache of StyleCopSettings objects without individual analyzers being aware of the cache, perhaps per compilation similarly to how the using alias cache for S1121 is implemented.

[bjornhellander]

Ok, so I continued with the actual StyleCopSettings caching as well. With these changes, the time spent in these analyzers are down another 30% in my benchmarks, down a total of 50% since I started looking at this. SA1121 is no longer in the top 10. Ready for review.

[bjornhellander]

Do you have time to review this, @sharwell?