Merge pull request #80 from ATrefzer/review-improvements

Review improvements

Author: ATrefzer

CLAUDE.md

@@ -47,7 +47,7 @@ Debug shortcut: passing a single arg `-load:<project.json>` auto-loads a saved p
 Five projects wired together in `CSharpCodeAnalyst.sln`:
 
 - **`CodeGraph/`** — pure, UI-free domain model. Contains the `CodeElement` / `Relationship` / `CodeGraph` graph types (`Graph/`), graph algorithms (`Algorithms/Cycles`, `Algorithms/Metrics`, `Algorithms/Partitioning`), exporters (`Export/` — DGML, DSI, PlantUML, JSON), and `Exploration/CodeGraphExplorer` (traversal queries used from the UI context menus). No WPF dependencies — reference this project from tests and tools.
-- **`CodeParser/`** — Roslyn front-end that turns an `.sln` or `.csproj` into a `CodeGraph`. Entry point: `Parser.ParseAsync(path)`. Works in **two passes**: `HierarchyAnalyzer` finds code elements and parent/child links, then `RelationshipAnalyzer.AnalyzeRelationshipsMultiThreaded` walks method and lambda bodies to build relationships. `Initializer.InitializeMsBuildLocator()` **must** be called once before any parse (both `App.StartUi` and the test fixture `Init` do this).
+- **`CodeParser/`** — Roslyn front-end that turns an `.sln` or `.csproj` into a `CodeGraph`. Entry point: `Parser.ParseAsync(path)`. Works in **two passes**: `HierarchyAnalyzer` finds code elements and parent/child links, then `RelationshipAnalyzer.AnalyzeRelationships` walks method and lambda bodies to build relationships (parallel by default; pass `maxDegreeOfParallelism: 1` for a single-threaded debug run). `Initializer.InitializeMsBuildLocator()` **must** be called once before any parse (both `App.StartUi` and the test fixture `Init` do this).
 - **`CSharpCodeAnalyst/`** — WPF front-end. Organized by feature under `Features/` (`CycleGroups`, `Graph`, `Tree`, `AdvancedSearch`, `Analyzers/ArchitecturalRules`, `Analyzers/EventRegistration`, `Ai`, `Import`, `Export`, `Metrics`, `Partitions`, `Refactoring`, `Gallery`, `Help`, `Info`). Cross-cutting infrastructure lives in `Shared/` (messaging, notifications, data grid, search, filter, WPF helpers). `Configuration/` holds `AppSettings` (from `appsettings.json`), `UserPreferences` (persisted to `userSettings.json`), and `AiCredentialStorage`. Persistence of saved projects is in `Persistence/` (JSON, with DTOs under `Dto/`).
 - **`Tests/`** (project name `CodeParserTests`) — NUnit suite. `ApprovalTests/` parses the `TestSuite/` C# solution once per fixture and asserts on the resulting graph; `UnitTests/` covers cycles, exploration, export, search, architectural rules, etc.
 - **`ApprovalTestTool/`** — standalone console app that clones external repos listed in `Repositories.txt`, parses each at a pinned commit, hashes the graph dump, and diffs against references. Used to catch parser regressions on real codebases; not part of the CI test run.

CodeParser/Parser/Artifacts.cs

@@ -13,10 +13,19 @@ public class Artifacts(
     ReadOnlyCollection<INamedTypeSymbol> allNamedTypesInSolution,
     ReadOnlyDictionary<string, ISymbol> elementIdToSymbolMap,
     ReadOnlyDictionary<IAssemblySymbol, List<GlobalStatementSyntax>> globalStatementsByAssembly,
-    ReadOnlyDictionary<string, CodeElement> symbolKeyToElementMap)
+    ReadOnlyDictionary<string, CodeElement> symbolKeyToElementMap,
+    ReadOnlyDictionary<string, List<INamedTypeSymbol>> interfaceImplementations)
 {
     public ReadOnlyCollection<INamedTypeSymbol> AllNamedTypesInSolution { get; } = allNamedTypesInSolution;
     public ReadOnlyDictionary<string, ISymbol> ElementIdToSymbolMap { get; } = elementIdToSymbolMap;
     public ReadOnlyDictionary<IAssemblySymbol, List<GlobalStatementSyntax>> GlobalStatementsByAssembly { get; } = globalStatementsByAssembly;
     public ReadOnlyDictionary<string, CodeElement> SymbolKeyToElementMap { get; } = symbolKeyToElementMap;
+
+    /// <summary>
+    ///     Maps an interface's <see cref="SymbolExtensions.Key" /> to all named types that have that interface
+    ///     in their <see cref="INamedTypeSymbol.AllInterfaces" /> (directly or via a base type). Precomputed
+    ///     once in phase 1 so phase 2 can resolve "who implements this interface" in O(1) instead of scanning
+    ///     every type for every interface member.
+    /// </summary>
+    public ReadOnlyDictionary<string, List<INamedTypeSymbol>> InterfaceImplementations { get; } = interfaceImplementations;
 }

CodeParser/Parser/HierarchyAnalyzer.cs

@@ -59,10 +59,39 @@ internal HierarchyAnalyzer(Progress progress, ParserConfig config, ParserDiagnos
             _allNamedTypesInSolution.AsReadOnly(),
             _elementIdToSymbolMap.AsReadOnly(),
             _globalStatementsByAssembly.AsReadOnly(),
-            _symbolKeyToElementMap.AsReadOnly());
+            _symbolKeyToElementMap.AsReadOnly(),
+            BuildInterfaceImplementations(_allNamedTypesInSolution).AsReadOnly());
         return (_codeGraph, result);
     }
 
+    /// <summary>
+    ///     Builds the interface-key -> implementing-types lookup once, so phase 2 does not rebuild a Key()
+    ///     string per type per interface for every interface member. Each (type, interface) pair contributes
+    ///     once, mirroring the previous per-lookup scan over AllInterfaces.
+    /// </summary>
+    private static Dictionary<string, List<INamedTypeSymbol>> BuildInterfaceImplementations(
+        IEnumerable<INamedTypeSymbol> allNamedTypes)
+    {
+        var map = new Dictionary<string, List<INamedTypeSymbol>>();
+        foreach (var type in allNamedTypes)
+        {
+            // AllInterfaces also includes interfaces implemented in a base type - same as the old scan.
+            foreach (var interfaceSymbol in type.AllInterfaces)
+            {
+                var interfaceKey = interfaceSymbol.Key();
+                if (!map.TryGetValue(interfaceKey, out var implementingTypes))
+                {
+                    implementingTypes = [];
+                    map[interfaceKey] = implementingTypes;
+                }
+
+                implementingTypes.Add(type);
+            }
+        }
+
+        return map;
+    }
+
     /// <summary>
     ///     Remove all projects that do not pass our include filter or cannot be parsed.
     /// </summary>

CodeParser/Parser/LambdaBodyWalker.cs

@@ -85,15 +85,6 @@ public override void VisitInvocationExpression(InvocationExpressionSyntax node)
         base.VisitInvocationExpression(node);
     }
 
-    public override void VisitAssignmentExpression(AssignmentExpressionSyntax node)
-    {
-        // Track event registration/unregistration (handled by AnalyzeAssignment)
-        // Property/field access on both sides is handled by the walker's normal traversal,
-        // which correctly uses "Uses" relationships for lambdas (see VisitIdentifierName and VisitMemberAccessExpression)
-        Analyzer.AnalyzeEventRegistrationAssignment(SourceElement, node, SemanticModel);
-        base.VisitAssignmentExpression(node);
-    }
-
     /// <summary>
     ///     Visit standalone identifiers (properties, fields, etc.).
     ///     Uses "Uses" relationship for lambda bodies (we don't know when/if lambda executes).
@@ -107,13 +98,13 @@ public override void VisitIdentifierName(IdentifierNameSyntax node)
 
     public override void VisitMemberAccessExpression(MemberAccessExpressionSyntax node)
     {
-        // Delegate to AnalyzeMemberAccess with "Uses" relationship type for lambdas
-        // Same rationale as VisitAssignmentExpression - we don't know when/if the lambda executes
+        // Delegate to AnalyzeMemberAccess with "Uses" relationship type for lambdas.
         Analyzer.AnalyzeMemberAccess(SourceElement, node, SemanticModel, RelationshipType.Uses);
 
-        // Now safe to call base traversal since VisitIdentifierName is overridden in this class
-        // and will use RelationshipType.Uses (not the default Calls)
-        base.VisitMemberAccessExpression(node);
+        // Visit only the Expression (left side: obj in obj.Member). AnalyzeMemberAccess already owns the
+        // .Name, so - like MethodBodyWalker - we must not descend into it via base (that would re-run
+        // VisitIdentifierName on the member name and only AddRelationship dedup would hide the double work).
+        Visit(node.Expression);
     }
 
     public override void VisitSimpleLambdaExpression(SimpleLambdaExpressionSyntax node)

CodeParser/Parser/MethodBodyWalker.cs

@@ -34,12 +34,6 @@ public override void VisitInvocationExpression(InvocationExpressionSyntax node)
         base.VisitInvocationExpression(node);
     }
 
-    public override void VisitAssignmentExpression(AssignmentExpressionSyntax node)
-    {
-        Analyzer.AnalyzeEventRegistrationAssignment(SourceElement, node, SemanticModel);
-        base.VisitAssignmentExpression(node);
-    }
-
     /// <summary>
     ///     Constructor chaining: ": base(...)" and ": this(...)".
     /// </summary>

CodeParser/Parser/Parser.cs

@@ -109,7 +109,7 @@ private void WorkspaceFailedHandler(WorkspaceDiagnosticEventArgs e)
 
         // Second Pass: Build Relationships
         var phase2 = new RelationshipAnalyzer(_progress, config);
-        await phase2.AnalyzeRelationshipsMultiThreaded(solution, codeGraph, artifacts);
+        await phase2.AnalyzeRelationships(solution, codeGraph, artifacts);
 
         sw.Stop();
         Trace.TraceInformation("Analyzing relationships: " + sw.Elapsed);

CodeParser/Parser/RelationshipAnalyzer.cs

@@ -369,10 +369,12 @@ public void AnalyzeObjectCreation(CodeElement sourceElement, SemanticModel seman
     }
 
     /// <summary>
-    ///     Entry for relationship analysis.
-    ///     The code graph is updated in place.
+    ///     Builds all relationships (phase 2). The code graph is updated, the artifacts are read only.
+    ///     Pass <paramref name="maxDegreeOfParallelism" /> = 1 for a deterministic single-threaded run
+    ///     (useful when debugging); the default (-1) lets the scheduler use all available cores.
     /// </summary>
-    public Task AnalyzeRelationshipsMultiThreaded(Solution solution, CodeGraph.Graph.CodeGraph codeGraph, Artifacts artifacts)
+    public Task AnalyzeRelationships(Solution solution, CodeGraph.Graph.CodeGraph codeGraph, Artifacts artifacts,
+        int maxDegreeOfParallelism = -1)
     {
         ArgumentNullException.ThrowIfNull(solution, nameof(solution));
         ArgumentNullException.ThrowIfNull(codeGraph, nameof(codeGraph));
@@ -385,10 +387,11 @@ public Task AnalyzeRelationshipsMultiThreaded(Solution solution, CodeGraph.Graph
         var numberOfCodeElements = _codeGraph.Nodes.Count;
         _processedCodeElements = 0;
 
-        // Take a snapshot of internal elements to avoid collection modification during parallel iteration
+        // Take a snapshot of internal elements to avoid collection modification during iteration
         var internalElements = _codeGraph.Nodes.Values.ToList();
 
-        Parallel.ForEach(internalElements, AnalyzeRelationshipsLocal);
+        var options = new ParallelOptions { MaxDegreeOfParallelism = maxDegreeOfParallelism };
+        Parallel.ForEach(internalElements, options, AnalyzeRelationshipsLocal);
 
         // After parallel processing, add all external elements to the graph
         AddExternalElementsToGraph();
@@ -433,43 +436,6 @@ private void AddExternalElementsToGraph()
         }
     }
 
-    /// <summary>
-    ///     The code graph is updated, the artifacts are read only.
-    /// </summary>
-    public Task AnalyzeRelationshipsSingleThreaded(Solution solution, CodeGraph.Graph.CodeGraph codeGraph, Artifacts artifacts)
-    {
-        _codeGraph = codeGraph;
-        _artifacts = artifacts;
-
-        var numberOfCodeElements = _codeGraph.Nodes.Count;
-        var loop = 0;
-
-        // Take a snapshot to avoid collection modification during iteration
-        var internalElements = _codeGraph.Nodes.Values.ToList();
-
-        foreach (var element in internalElements)
-        {
-            loop++;
-
-            if (!_artifacts.ElementIdToSymbolMap.TryGetValue(element.Id, out var symbol))
-            {
-                // INamespaceSymbol
-                continue;
-            }
-
-            AnalyzeRelationships(solution, element, symbol);
-            SendParserPhase2Progress(loop, numberOfCodeElements);
-        }
-
-        // Add external elements to the graph
-        AddExternalElementsToGraph();
-
-        // Analyze global statements for each assembly
-        AnalyzeGlobalStatementsForAssembly(solution);
-
-        return Task.CompletedTask;
-    }
-
     private void SendParserPhase2Progress(int processed, int total)
     {
         var currentProgress = (long)Math.Floor(processed / (double)total * 100);
@@ -763,10 +729,10 @@ private void FindImplementationsForInterfaceMember(CodeElement element, ISymbol
     /// </summary>
     private IEnumerable<INamedTypeSymbol> FindTypesImplementingInterface(INamedTypeSymbol interfaceSymbol)
     {
-        // Note: AllInterfaces returns all interfaces found at this type, regardless if it is implemented in a base class or not.
+        // The interface-key -> implementing-types map is precomputed in phase 1 (see Artifacts). It already
+        // accounts for interfaces implemented in a base type, since it is built from AllInterfaces.
         var interfaceKey = interfaceSymbol.Key();
-        return _artifacts!.AllNamedTypesInSolution
-            .Where(type => type.AllInterfaces.Any(i => i.Key() == interfaceKey));
+        return _artifacts!.InterfaceImplementations.GetValueOrDefault(interfaceKey) ?? [];
     }
 
     /// <summary>

CodeParser/Parser/SyntaxWalkerBase.cs

@@ -6,7 +6,16 @@
 namespace CodeParser.Parser;
 
 /// <summary>
+///     <code>
 ///     Handling here does not distinguish between method or lambda bodies.
+///     | Visit                    | Method                                         | Lambda                                  |
+///     |--------------------------|------------------------------------------------|-----------------------------------------|
+///     | `IdentifierName`         | `AnalyzeIdentifier` (Calls)                    | `AnalyzeIdentifier` (**Uses**)          |
+///     | `Invocation`             | `AnalyzeInvocation` (Calls **+ Event-Invoke**) | inline Uses, ** kein** Event-Invoke     |
+///     | `ObjectCreation`         | `AnalyzeObjectCreation` (** Creates**)         | `TrackObjectCreationAsUses` (** Uses**) |
+///     | nested Lambdas           | spawns `LambdaBodyWalker`                      | skipped(!)                              |
+///     | `ConstructorInitializer` | yes                                            | no (Lambdas have none)                  |
+/// </code>
 /// </summary>
 internal class SyntaxWalkerBase : CSharpSyntaxWalker
 {
@@ -27,6 +36,18 @@ protected SyntaxWalkerBase(ISyntaxNodeHandler analyzer, CodeElement sourceElemen
     // their relationship type (Calls for MethodBodyWalker, Uses for LambdaBodyWalker).
     // Each walker overrides VisitIdentifierName with the appropriate RelationshipType parameter.
 
+    /// <summary>
+    ///     Event registration/unregistration (event += / -= handler). Identical for method and lambda
+    ///     bodies: the registration itself is the same edge
+    ///     Property/field access on either side is covered by the normal identifier/member-access traversal
+    ///     ("Uses" relationships for lambdas (see VisitIdentifierName and VisitMemberAccessExpression))
+    /// </summary>
+    public override void VisitAssignmentExpression(AssignmentExpressionSyntax node)
+    {
+        Analyzer.AnalyzeEventRegistrationAssignment(SourceElement, node, SemanticModel);
+        base.VisitAssignmentExpression(node);
+    }
+
     public override void VisitLocalDeclarationStatement(LocalDeclarationStatementSyntax node)
     {
         Analyzer.AnalyzeLocalDeclaration(SourceElement, node, SemanticModel);