Simplify code in analyzer executor #76943

CyrusNajmabadi · 2025-01-27T23:16:21Z

No description provided.

CyrusNajmabadi · 2025-01-27T23:16:42Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

@@ -204,7 +204,7 @@ public void ExecuteInitializeMethod(HostSessionStartAnalysisScope sessionScope,
            // The Initialize method should be run asynchronously in case it is not well behaved, e.g. does not terminate.
            ExecuteAndCatchIfThrows(
                sessionScope.Analyzer,
-                data => data.analyzer.Initialize(data.context),
+                static data => data.analyzer.Initialize(data.context),


made all lambdas explicitly static to ensure we don't accidentally capture in them.

CyrusNajmabadi · 2025-01-27T23:17:02Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

+            // This context doesn't build up any state as we pass it to the Action method of the analyzer. As such, we
+            // can use the same instance across all actions.
+            var context = new AnalyzerCompilationStartAnalysisContext(compilationScope,
+                Compilation, AnalyzerOptions, _compilationAnalysisValueProviderFactory, cancellationToken);


lifted contexts out of loops. this avoids allocations for the class-based ones, and removes unnecessary cpu for the struct ones.

CyrusNajmabadi · 2025-01-27T23:19:16Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

-                        {
-                            data.action(data.context);
-                            data.blockEndActions?.AddAll(data.scope.CodeBlockEndActions);
-                            data.syntaxNodeActions?.AddRange(data.scope.SyntaxNodeActions);


these two lines don't need to be in the lambda . we are protecting the call out to data.action as that is user code which may throw. after taht, all we're doing is taking the data we collected and adding to the builders we own. this is all code we own and cannot throw. removing from teh lambda greatly simplifies the lambda (making it like all teh rest we have) and greatly simplifes the data we pass to it.

CyrusNajmabadi · 2025-01-27T23:19:38Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

                        new AnalysisContextInfo(Compilation, declaredSymbol, declaredNode),
                        cancellationToken);
+
+                    codeBlockEndActions!.AddAll(codeBlockScope.CodeBlockEndActions);
+                    syntaxNodeActions!.AddRange(codeBlockScope.SyntaxNodeActions);


the ?. was unnecessary. we always have these values when going into these particular codepaths.

note: the ! will go away in the strong-typign PR as we will have a strong tie that if we get here, we must have made these temp collections to store the results in.

CyrusNajmabadi · 2025-01-27T23:20:33Z

@RikkiGibson @jcouv this pulls out a bunch of hte machanical changes from the the other prs. If this codes in first, it will make the others easier to review.

CyrusNajmabadi · 2025-01-27T23:39:31Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

            foreach (var startAction in actions)
            {
-                cancellationToken.ThrowIfCancellationRequested();


cancellation check moved into ExecuteAndCatchIfThrows for consistency and simplicity for all callers.

CyrusNajmabadi · 2025-01-27T23:39:39Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

+            // can use the same instance across all actions.
+            var context = new AnalyzerCompilationStartAnalysisContext(compilationScope,
+                Compilation, AnalyzerOptions, _compilationAnalysisValueProviderFactory, cancellationToken);
+            var contextInfo = new AnalysisContextInfo(Compilation);


contexxt info also lifted out if we have loops.

CyrusNajmabadi · 2025-01-27T23:40:01Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

-                    (action: startAction.Action, context),
-                    new AnalysisContextInfo(Compilation),
+                    static data => data.startAction.Action(data.context),
+                    (startAction, context),


made t he lambda and arguments pattern consistent across all usages in the file.

CyrusNajmabadi · 2025-01-27T23:40:15Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

-                data => data.action(data.context),
-                (action, context),
+                static data => data.suppressor.ReportSuppressions(data.context),
+                (suppressor, context),
                new AnalysisContextInfo(Compilation),


not in a loop, so didn't bother making a local.

CyrusNajmabadi · 2025-01-27T23:40:59Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

@@ -201,11 +201,10 @@ public void ExecuteInitializeMethod(HostSessionStartAnalysisScope sessionScope,
        {
            var context = new AnalyzerAnalysisContext(sessionScope, severityFilter);

-            // The Initialize method should be run asynchronously in case it is not well behaved, e.g. does not terminate.


comment is non-sensical. we don't run things asynchronously, and we have no intent to ever do that. this may reflect an idea early on at a point in time when analyzers were being initially developed.

CyrusNajmabadi · 2025-01-27T23:42:16Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

-                var context = new SemanticModelAnalysisContext(semanticModel, AnalyzerOptions, diagReporter.AddDiagnosticAction,
-                    isSupportedDiagnostic, filterSpan, isGeneratedCode, cancellationToken);
-
-                // Catch Exception from action.


pointless comment that was used inconsistently. The method is ExecuteAndCatchIfThrows. We don't need to comment that that's what it is doing (especially only on a subset of the calls to it).

CyrusNajmabadi · 2025-01-27T23:42:47Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

-                            },
-                            (action: operationBlockStartAction.Action, context: operationStartContext, scope: operationBlockScope, blockEndActions: operationBlockEndActions, operationActions: operationActions),
+                            static data => data.operationBlockStartAction.Action(data.operationStartContext),
+                            (operationBlockStartAction, operationStartContext),
                            new AnalysisContextInfo(Compilation, declaredSymbol),


note: this context info will be lifted out of the loop in another pr (when this all gets strongly typed).

CyrusNajmabadi · 2025-01-27T23:43:49Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

-                        data => data.action(data.context),
-                        (action: codeBlockAction.Action, context: context),
+                        static data => data.codeBlockAction.Action(data.context),
+                        (codeBlockAction, context),
                        new AnalysisContextInfo(Compilation, declaredSymbol, declaredNode),


same with this. the moving to strongly typed code will allow us to live this out of the loop.

CyrusNajmabadi · 2025-01-27T23:45:02Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

@@ -964,7 +995,10 @@ public void ExecuteSyntaxNodeActions<TLanguageKindEnum>(

            var diagReporter = GetAddSemanticDiagnostic(model.SyntaxTree, spanForContainingTopmostNodeForAnalysis, analyzer, cancellationToken);

-            using var _ = PooledDelegates.GetPooledFunction((d, ct, arg) => arg.self.IsSupportedDiagnostic(arg.analyzer, d, ct), (self: this, analyzer), out Func<Diagnostic, CancellationToken, bool> isSupportedDiagnostic);
+            using var _ = PooledDelegates.GetPooledFunction(
+                static (d, ct, arg) => arg.self.IsSupportedDiagnostic(arg.analyzer, d, ct),


note: i don't like the use of 'ct' as a name here, as it means code can accidentally capture an outer variable called 'cancellationToken'. However, as these are all static lambdas now, we don't have to worry about this. So i kept the name the same.

CyrusNajmabadi · 2025-01-27T23:50:31Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

+            using var _ = PooledDelegates.GetPooledFunction(
+                static (d, supportedSuppressions) => supportedSuppressions.Contains(d),
+                supportedSuppressions,
+                out Func<SuppressionDescriptor, bool> isSupportedSuppression);


moved to pooling here to avoid delegate allocation, and for consistency with our other methods.

CyrusNajmabadi · 2025-01-27T23:51:04Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

@@ -292,15 +294,19 @@ public void ExecuteSuppressionAction(DiagnosticSuppressor suppressor, ImmutableA
            cancellationToken.ThrowIfCancellationRequested();

            var supportedSuppressions = _analyzerManager.GetSupportedSuppressionDescriptors(suppressor, this, cancellationToken);
-            Func<SuppressionDescriptor, bool> isSupportedSuppression = supportedSuppressions.Contains;
-            Action<SuppressionAnalysisContext> action = suppressor.ReportSuppressions;


this delegate allocation was unnecessary. we can just pass the suppressor to ExecuteAndCatchIfThrows which can then call .ReportSuppressions on htat without needing a delegate indirectino.

CyrusNajmabadi · 2025-01-28T00:05:22Z

@RikkiGibson @jcouv this is ready for review (this was the correct PR to ping. ignore the other one :)).

CyrusNajmabadi · 2025-01-30T00:47:37Z

@RikkiGibson @jcouv ptal :)

CyrusNajmabadi · 2025-01-31T16:51:35Z

@RikkiGibson @jcouv ptal. Thanks!

RikkiGibson · 2025-01-31T17:35:33Z

Reviewing now

RikkiGibson · 2025-01-31T17:23:28Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

-            Action<SuppressionAnalysisContext> action = suppressor.ReportSuppressions;
+
+            using var _ = PooledDelegates.GetPooledFunction(
+                static (d, supportedSuppressions) => supportedSuppressions.Contains(d),


nit: this would have helped me understand what this code was doing a little faster.

Suggested change

static (d, supportedSuppressions) => supportedSuppressions.Contains(d),

static (descriptor, supportedSuppressions) => supportedSuppressions.Contains(descriptor),

I think the fact that the resulting function signature is only hinted at in the out argument, which comes last, made the first few of these I read a bit slow to grasp.

If you decide to make a change here, feel free to do so in follow-up PR. Thanks

sure. let me do that in followup. i agree that the single letters are not always great.

RikkiGibson · 2025-01-31T17:27:22Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs


            foreach (var symbolAction in symbolActions)
            {
-                var action = symbolAction.Action;
-                var kinds = symbolAction.Kinds;
+                cancellationToken.ThrowIfCancellationRequested();


It looks like this call was removed from some loops, for example for CompilationStart actions, but left in others. Was there a particular reason for that?

i removed when the loop was simple and called directly into a method taht checked the CT> i kept when the loop was non-trivial and might not call something that checked.

RikkiGibson · 2025-01-31T17:37:28Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

@@ -890,13 +923,14 @@ private void ExecuteBlockActions<TBlockAction>(
                var codeBlockAction = blockAction as CodeBlockAnalyzerAction;
                if (codeBlockAction != null)
                {
+                    // This context is a struct, so it's fine to create a new one for each action.


It looks like we're not lifting this out because we don't know which one we will need on each iteration? Is that right?

correct. but in a followup this will be lifted out as these iterations will be separated in a strongly typed fashion for the node vs iop cases.

sharwell · 2025-01-31T21:29:30Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

                            new AnalysisContextInfo(Compilation, declaredSymbol),
                            cancellationToken);
+
+                        operationBlockEndActions!.AddAll(operationBlockScope.OperationBlockEndActions);


If this really is always non-null, it would be better to change its initial assignment from try-cast to direct cast.

This code gets rewritten in a follow-up where there is no cast at all. This pr is just about pulling out mechanical, simple, changes from the more complex pr.

Simplify code in analyzer executor

e89381e

CyrusNajmabadi requested a review from a team as a code owner January 27, 2025 23:16

dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 27, 2025

CyrusNajmabadi commented Jan 27, 2025

View reviewed changes

Revert

0117775

jcouv self-assigned this Jan 27, 2025

jcouv added the Area-Compilers label Jan 27, 2025

CyrusNajmabadi added 2 commits January 27, 2025 15:31

Lift contexts outwards

09abeb1

Simplify further

3b5bc8c

CyrusNajmabadi commented Jan 27, 2025

View reviewed changes

Pool function for consistency

cb750e7

CyrusNajmabadi commented Jan 27, 2025

View reviewed changes

jjonescz approved these changes Jan 30, 2025

View reviewed changes

RikkiGibson self-assigned this Jan 30, 2025

RikkiGibson approved these changes Jan 31, 2025

View reviewed changes

sharwell reviewed Jan 31, 2025

View reviewed changes

CyrusNajmabadi merged commit cb465c9 into dotnet:main Jan 31, 2025
24 checks passed

CyrusNajmabadi deleted the executerCleanup branch January 31, 2025 23:53

dotnet-policy-service bot added this to the Next milestone Jan 31, 2025

This was referenced Feb 4, 2025

[Automated] PRs inserted in VS build feature.debugger.main-35803.76 #77009

Closed

[Automated] PRs inserted in VS build main-35803.71 #77010

Closed

[Automated] PRs inserted in VS build feature.d18initial-10304.00 #77043

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify code in analyzer executor #76943

Simplify code in analyzer executor #76943

CyrusNajmabadi commented Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi commented Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi commented Jan 28, 2025

CyrusNajmabadi commented Jan 30, 2025

CyrusNajmabadi commented Jan 31, 2025

RikkiGibson commented Jan 31, 2025

RikkiGibson Jan 31, 2025

RikkiGibson Jan 31, 2025

CyrusNajmabadi Jan 31, 2025

RikkiGibson Jan 31, 2025

CyrusNajmabadi Jan 31, 2025

RikkiGibson Jan 31, 2025

CyrusNajmabadi Jan 31, 2025

sharwell Jan 31, 2025

CyrusNajmabadi Jan 31, 2025

	static (d, supportedSuppressions) => supportedSuppressions.Contains(d),
	static (descriptor, supportedSuppressions) => supportedSuppressions.Contains(descriptor),

Simplify code in analyzer executor #76943

Simplify code in analyzer executor #76943

Conversation

CyrusNajmabadi commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 28, 2025

CyrusNajmabadi commented Jan 30, 2025

CyrusNajmabadi commented Jan 31, 2025

RikkiGibson commented Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment