Refactoring symbolic execution analysis to run once per mehod #1281
Conversation
valhristov
requested review from
duncanp-sonar,
Evangelink and
michalb-sonar
as code owners
ghost
assigned
| @@ -1848,32 +1848,6 @@ | |||
| }, | |||
| { | |||
| "id": "S3900", | |||
| "message": "Refactor this method to add validation of parameter 'modelType' before using it.", | |||
There was a problem hiding this comment.
Since all SE checks run simultaneously we could generate bigger/or-shorter exploded graphs, thus creating some FNs in large methods. We could investigate the further increase of the EG limits in the future.
| { | ||
| context.RegisterExplodedGraphBasedAnalysis((e, c) => CheckForRedundantConditions(e, c)); | ||
| } | ||
| ISymbolicExecutionAnalyzer ISymbolicExecutionAnalyzerFactory.Create(CSharpExplodedGraph explodedGraph) => |
There was a problem hiding this comment.
The interface is internal because the CSEG is internal. I did not wanted to make the whole class internal, hence the explicit implementation
| VerifyAnalyzer(new[] { path }, new[] { diagnosticAnalyzer }, null, options, additionalReferences); | ||
| } | ||
|
|
||
| public static void VerifyAnalyzer(string path, IEnumerable<SonarDiagnosticAnalyzer> diagnosticAnalyzers, |
There was a problem hiding this comment.
Adding overload that allows you to run two analyzers in one test. This was needed for the InvalidCast rule which was split into two.
| @@ -146,19 +146,16 @@ public void Walk() | |||
| OnExplorationEnded(); | |||
| } | |||
|
|
|||
| internal void AddExplodedGraphCheck<T>(T check) | |||
| public T GetOrAddCheck<T>(Func<T> factory) | |||
There was a problem hiding this comment.
Since the SE analyzer registers states that subscribe for check events, adding a check does not replace an existing one, but reuses it.
| { | ||
| explodedGraph.ExplorationEnded -= OnExplorationEnded; | ||
|
|
||
| analyzerInstances.ForEach(analyzer => analyzer.Dispose()); |
There was a problem hiding this comment.
Unsubscribe from events.
| private void Analyze(CSharpExplodedGraph explodedGraph, SyntaxNodeAnalysisContext context) | ||
| { | ||
| var analyzerInstances = analyzerFactories | ||
| .Where(factory => factory.IsEnabled(context)) |
There was a problem hiding this comment.
The analyzer factories could be disabled for some methods (e.g. for tests or private methods)
| @@ -33,8 +34,7 @@ | |||
| namespace SonarAnalyzer.Rules.CSharp | |||
| { | |||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | |||
| [Rule(DiagnosticId)] | |||
There was a problem hiding this comment.
Rule attribute is removed because we have unit tests that ensure each rule is registered in exactly one diagnostic analyzer. InvalidCast is reported from two analyzers.
| @@ -32,7 +32,7 @@ namespace SonarAnalyzer.SymbolicExecution | |||
| { | |||
| internal abstract class AbstractExplodedGraph | |||
| { | |||
| internal const int MaxStepCount = 1000; | |||
| internal const int MaxStepCount = 2000; | |||
There was a problem hiding this comment.
Increased the exploration limit to avoid FNs.
| namespace SonarAnalyzer.Rules.CSharp | ||
| { | ||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| [Rule(NullPointerDereference.DiagnosticId)] |
There was a problem hiding this comment.
I don't like that the Symbolic Execution Engine has to know about the rules using it. As I told you I prefer to have all analyzer referencing the SEE.
There was a problem hiding this comment.
+1
It is not difficult to revert the dependency, but it will require significant testing, as we do not have dependent rules now and there could be potentially many cases where it could break. The current solution is cheap and still provides significant performance improvement.
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => | ||
| analyzerFactories.SelectMany(factory => factory.SupportedDiagnostics).ToImmutableArray(); | ||
|
|
||
| public SymbolicExecutionAnalyzer() : this( |
There was a problem hiding this comment.
Same thing here. IMO SEE should not know about other classes.
| analyzerInstances.ForEach(analyzer => analyzer.Dispose()); | ||
| } | ||
|
|
||
| void OnExplorationEnded(object sender, EventArgs e) => |
There was a problem hiding this comment.
IMO this shouldn't be this class reporting issues but the analyzers themselves. This class should only be responsible of doing the symbolic execution (the walk).
|
I would consider this PR as POC, we could look for better implementation in the future. |
Each existing SE rule was converted to a factory and analyzer (state) classes. The existing SE checks are not changed. There is a new diagnostic analyzer - SymbolicExecutionAnalyzer, which uses the registered factories to create analyzers for the corresponding rule when a SE analysis is requested.
Currently the factories are registered (hardcoded) in the SymbolicExecutionAnalyzer class' constructor. This could be changed in the future to reverse the dependency, but it will require more extensive testing, because we will introduce dependencies between diagnostic analyzers, which are generally meant to be independent. The hypothesis seems feasible, but it is not known whether it will work or not.
In general this will reduce the execution time of the its from ~600s to <500s (on my machine).
Best viewed without whitespace:
https://github.com/SonarSource/sonar-csharp/pull/1281/files?w=1