diff --git a/Optilizer/Optilizer.CodeFixes/GeometryUnionInLoopCodeFixProvider.cs b/Optilizer/Optilizer.CodeFixes/GeometryUnionInLoopCodeFixProvider.cs new file mode 100644 index 0000000..401d1e2 --- /dev/null +++ b/Optilizer/Optilizer.CodeFixes/GeometryUnionInLoopCodeFixProvider.cs @@ -0,0 +1,158 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using System.Collections.Immutable; +using System.Composition; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; + +namespace Optilizer +{ + [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(GeometryUnionInLoopCodeFixProvider)), Shared] + public class GeometryUnionInLoopCodeFixProvider : CodeFixProvider + { + public sealed override ImmutableArray FixableDiagnosticIds => + ImmutableArray.Create(GeometryUnionInLoopAnalyzer.DiagnosticId); + + public sealed override FixAllProvider GetFixAllProvider() => + WellKnownFixAllProviders.BatchFixer; + + public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken) + .ConfigureAwait(false); + + var diagnostic = context.Diagnostics.First(); + var diagnosticSpan = diagnostic.Location.SourceSpan; + + // Find the Union invocation + var unionInvocation = root.FindNode(diagnosticSpan) as InvocationExpressionSyntax; + if (unionInvocation == null) return; + + // Find the containing loop statement + var loopStatement = unionInvocation.FirstAncestorOrSelf(node => + node is ForStatementSyntax || + node is ForEachStatementSyntax || + node is WhileStatementSyntax); + + if (loopStatement == null) return; + + context.RegisterCodeFix( + CodeAction.Create( + title: "Replace with CascadedPolygonUnion for better performance", + createChangedDocument: c => ReplaceWithCascadedUnion(context.Document, loopStatement, unionInvocation, c), + equivalenceKey: "ReplaceBatchUnion"), + diagnostic); + } + + private async Task ReplaceWithCascadedUnion( + Document document, + StatementSyntax loopStatement, + InvocationExpressionSyntax unionInvocation, + CancellationToken cancellationToken) + { + var root = await document.GetSyntaxRootAsync(cancellationToken); + var semanticModel = await document.GetSemanticModelAsync(cancellationToken); + + // Analyze the loop pattern to understand how Union is being used + var (collectionVariable, accumulatorVariable) = AnalyzeLoopPattern(loopStatement, unionInvocation); + + if (collectionVariable == null) + return document; // Can't determine pattern, no fix available + + // Create the replacement code - simple assignment to the accumulator variable + var replacementCode = accumulatorVariable != null + ? $"{accumulatorVariable} = CascadedPolygonUnion.Union({collectionVariable});" + : $"var result = CascadedPolygonUnion.Union({collectionVariable});"; + + var replacementStatement = SyntaxFactory.ParseStatement(replacementCode) + .WithLeadingTrivia(loopStatement.GetLeadingTrivia()) + .WithTrailingTrivia(loopStatement.GetTrailingTrivia()); + + // Replace the loop with the new statement + var newRoot = root.ReplaceNode(loopStatement, replacementStatement); + + // Add using directive if needed + newRoot = AddUsingDirectiveIfNeeded(newRoot, "NetTopologySuite.Operation.Union"); + + return document.WithSyntaxRoot(newRoot); + } + + private (string collectionVariable, string accumulatorVariable) AnalyzeLoopPattern( + StatementSyntax loopStatement, + InvocationExpressionSyntax unionInvocation) + { + string collectionVariable = null; + string accumulatorVariable = null; + + switch (loopStatement) + { + case ForEachStatementSyntax forEachLoop: + collectionVariable = forEachLoop.Expression.ToString(); + + // Look for assignment pattern like: result = result.Union(item) + var assignment = unionInvocation.FirstAncestorOrSelf(); + if (assignment != null && assignment.IsKind(SyntaxKind.SimpleAssignmentExpression)) + { + accumulatorVariable = assignment.Left.ToString(); + } + break; + + case ForStatementSyntax forLoop: + // For loops are more complex, try to identify the collection being iterated + var loopBody = forLoop.Statement; + var elementAccess = loopBody.DescendantNodes() + .OfType() + .FirstOrDefault(); + + if (elementAccess != null) + { + collectionVariable = elementAccess.Expression.ToString(); + } + + // Look for assignment pattern + var forAssignment = unionInvocation.FirstAncestorOrSelf(); + if (forAssignment != null && forAssignment.IsKind(SyntaxKind.SimpleAssignmentExpression)) + { + accumulatorVariable = forAssignment.Left.ToString(); + } + break; + + case WhileStatementSyntax whileLoop: + // While loops are complex, try basic pattern detection + var whileAssignment = unionInvocation.FirstAncestorOrSelf(); + if (whileAssignment != null && whileAssignment.IsKind(SyntaxKind.SimpleAssignmentExpression)) + { + accumulatorVariable = whileAssignment.Left.ToString(); + } + break; + } + + return (collectionVariable, accumulatorVariable); + } + + + + private SyntaxNode AddUsingDirectiveIfNeeded(SyntaxNode root, string namespaceName) + { + if (!(root is CompilationUnitSyntax compilationUnit)) + return root; + + // Check if the using directive already exists + var existingUsing = compilationUnit.Usings + .FirstOrDefault(u => u.Name.ToString() == namespaceName); + + if (existingUsing != null) + return root; // Already exists + + // Add the using directive + var newUsing = SyntaxFactory.UsingDirective(SyntaxFactory.ParseName(namespaceName)); + var newUsings = compilationUnit.Usings.Add(newUsing); + + return compilationUnit.WithUsings(newUsings); + } + } +} \ No newline at end of file diff --git a/Optilizer/Optilizer.Test/GeometryUnionInLoopTests.cs b/Optilizer/Optilizer.Test/GeometryUnionInLoopTests.cs new file mode 100644 index 0000000..2eef359 --- /dev/null +++ b/Optilizer/Optilizer.Test/GeometryUnionInLoopTests.cs @@ -0,0 +1,246 @@ +using System.Threading.Tasks; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using VerifyCS = Optilizer.Test.CSharpCodeFixVerifier< + Optilizer.GeometryUnionInLoopAnalyzer, + Optilizer.GeometryUnionInLoopCodeFixProvider>; + +namespace Optilizer.Test +{ + [TestClass] + public class GeometryUnionInLoopTests + { + [TestMethod] + public async Task ForEachLoop_WithGeometryUnion_ReportsWarning() + { + var test = @" +using System.Collections.Generic; + +namespace NetTopologySuite.Geometries +{ + public abstract class Geometry + { + public abstract Geometry Union(Geometry other); + } +} + +namespace TestNamespace +{ + using NetTopologySuite.Geometries; + + class C + { + void M(List geometries) + { + Geometry result = null; + foreach (var geom in geometries) + { + result = {|#0:result.Union(geom)|}; + } + } + } +}"; + + var expected = VerifyCS.Diagnostic("GIS001").WithLocation(0); + await VerifyCS.VerifyAnalyzerAsync(test, expected); + } + + [TestMethod] + public async Task GeometryUnion_OutsideLoop_NoWarning() + { + var test = @" +namespace NetTopologySuite.Geometries +{ + public abstract class Geometry + { + public abstract Geometry Union(Geometry other); + } +} + +namespace TestNamespace +{ + using NetTopologySuite.Geometries; + + class C + { + void M(Geometry geom1, Geometry geom2) + { + var result = geom1.Union(geom2); + } + } +}"; + + await VerifyCS.VerifyAnalyzerAsync(test); + } + + [TestMethod] + public async Task NonGeometryUnion_InLoop_NoWarning() + { + var test = @" +using System.Collections.Generic; +using System.Linq; + +class C +{ + void M(List arrays) + { + int[] result = null; + foreach (var array in arrays) + { + result = result.Union(array).ToArray(); + } + } +}"; + + await VerifyCS.VerifyAnalyzerAsync(test); + } + + [TestMethod] + public async Task ForLoop_WithGeometryUnion_ReportsWarning() + { + var test = @" +using System.Collections.Generic; + +namespace NetTopologySuite.Geometries +{ + public abstract class Geometry + { + public abstract Geometry Union(Geometry other); + } +} + +namespace TestNamespace +{ + using NetTopologySuite.Geometries; + + class C + { + void M(List geometries) + { + Geometry result = null; + for (int i = 0; i < geometries.Count; i++) + { + result = {|#0:result.Union(geometries[i])|}; + } + } + } +}"; + + var expected = VerifyCS.Diagnostic("GIS001").WithLocation(0); + await VerifyCS.VerifyAnalyzerAsync(test, expected); + } + + [TestMethod] + public async Task WhileLoop_WithGeometryUnion_ReportsWarning() + { + var test = @" +using System.Collections.Generic; + +namespace NetTopologySuite.Geometries +{ + public abstract class Geometry + { + public abstract Geometry Union(Geometry other); + } +} + +namespace TestNamespace +{ + using NetTopologySuite.Geometries; + + class C + { + void M(List geometries) + { + Geometry result = null; + int i = 0; + while (i < geometries.Count) + { + result = {|#0:result.Union(geometries[i])|}; + i++; + } + } + } +}"; + + var expected = VerifyCS.Diagnostic("GIS001").WithLocation(0); + await VerifyCS.VerifyAnalyzerAsync(test, expected); + } + + [TestMethod] + public async Task CodeFix_ForEachLoop_ReplacesWithCascadedUnion() + { + var test = @" +using System.Collections.Generic; + +namespace NetTopologySuite.Geometries +{ + public abstract class Geometry + { + public abstract Geometry Union(Geometry other); + } +} + +namespace NetTopologySuite.Operation.Union +{ + public static class CascadedPolygonUnion + { + public static NetTopologySuite.Geometries.Geometry Union(System.Collections.Generic.IEnumerable geometries) => null; + } +} + +namespace TestNamespace +{ + using NetTopologySuite.Geometries; + + class C + { + void M(List geometries) + { + Geometry result = null; + foreach (var geom in geometries) + { + result = {|#0:result.Union(geom)|}; + } + } + } +}"; + + var fixedTest = @" +using System.Collections.Generic; +using NetTopologySuite.Operation.Union; + +namespace NetTopologySuite.Geometries +{ + public abstract class Geometry + { + public abstract Geometry Union(Geometry other); + } +} + +namespace NetTopologySuite.Operation.Union +{ + public static class CascadedPolygonUnion + { + public static NetTopologySuite.Geometries.Geometry Union(System.Collections.Generic.IEnumerable geometries) => null; + } +} + +namespace TestNamespace +{ + using NetTopologySuite.Geometries; + + class C + { + void M(List geometries) + { + Geometry result = null; + result = CascadedPolygonUnion.Union(geometries); + } + } +}"; + + var expected = VerifyCS.Diagnostic("GIS001").WithLocation(0); + await VerifyCS.VerifyCodeFixAsync(test, expected, fixedTest); + } + } +} \ No newline at end of file diff --git a/Optilizer/Optilizer.Test/Optilizer.Test.csproj b/Optilizer/Optilizer.Test/Optilizer.Test.csproj index 39d8712..d769ee5 100644 --- a/Optilizer/Optilizer.Test/Optilizer.Test.csproj +++ b/Optilizer/Optilizer.Test/Optilizer.Test.csproj @@ -1,7 +1,7 @@ - netcoreapp3.1 + net8.0 true true diff --git a/Optilizer/Optilizer/GeometryUnionInLoopAnalyzer.cs b/Optilizer/Optilizer/GeometryUnionInLoopAnalyzer.cs new file mode 100644 index 0000000..c91a132 --- /dev/null +++ b/Optilizer/Optilizer/GeometryUnionInLoopAnalyzer.cs @@ -0,0 +1,105 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using System.Collections.Immutable; +using System.Linq; + +namespace Optilizer +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public class GeometryUnionInLoopAnalyzer : DiagnosticAnalyzer + { + public const string DiagnosticId = "GIS001"; + + private static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor( + DiagnosticId, + "Avoid calling Geometry.Union inside loops", + "Calling Geometry.Union repeatedly in a loop has poor performance. Consider using CascadedPolygonUnion.Union for batch operations.", + "Performance", + DiagnosticSeverity.Warning, + isEnabledByDefault: true); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterSyntaxNodeAction(AnalyzeForStatement, SyntaxKind.ForStatement); + context.RegisterSyntaxNodeAction(AnalyzeForEachStatement, SyntaxKind.ForEachStatement); + context.RegisterSyntaxNodeAction(AnalyzeWhileStatement, SyntaxKind.WhileStatement); + } + + private void AnalyzeForStatement(SyntaxNodeAnalysisContext context) + { + var forStatement = (ForStatementSyntax)context.Node; + AnalyzeLoopBody(context, forStatement.Statement); + } + + private void AnalyzeForEachStatement(SyntaxNodeAnalysisContext context) + { + var forEachStatement = (ForEachStatementSyntax)context.Node; + AnalyzeLoopBody(context, forEachStatement.Statement); + } + + private void AnalyzeWhileStatement(SyntaxNodeAnalysisContext context) + { + var whileStatement = (WhileStatementSyntax)context.Node; + AnalyzeLoopBody(context, whileStatement.Statement); + } + + private void AnalyzeLoopBody(SyntaxNodeAnalysisContext context, StatementSyntax loopBody) + { + if (loopBody == null) return; + + // Find all invocation expressions in the loop body + var invocations = loopBody.DescendantNodes() + .OfType() + .Where(invocation => IsGeometryUnionCall(invocation, context.SemanticModel)); + + foreach (var invocation in invocations) + { + context.ReportDiagnostic(Diagnostic.Create(Rule, invocation.GetLocation())); + } + } + + private bool IsGeometryUnionCall(InvocationExpressionSyntax invocation, SemanticModel semanticModel) + { + var symbolInfo = semanticModel.GetSymbolInfo(invocation); + if (!(symbolInfo.Symbol is IMethodSymbol methodSymbol)) + return false; + + // Check if the method name is "Union" + if (methodSymbol.Name != "Union") + return false; + + // Check if the containing type is a Geometry type from NetTopologySuite + var containingType = methodSymbol.ContainingType; + if (containingType == null) + return false; + + // Check if it's from NetTopologySuite.Geometries namespace + return IsNetTopologySuiteGeometryType(containingType); + } + + private bool IsNetTopologySuiteGeometryType(INamedTypeSymbol typeSymbol) + { + // Check if the type is in NetTopologySuite.Geometries namespace + if (typeSymbol.ContainingNamespace?.ToDisplayString() == "NetTopologySuite.Geometries") + return true; + + // Check base types recursively + var baseType = typeSymbol.BaseType; + while (baseType != null) + { + if (baseType.ContainingNamespace?.ToDisplayString() == "NetTopologySuite.Geometries" && + baseType.Name == "Geometry") + return true; + baseType = baseType.BaseType; + } + + return false; + } + } +} \ No newline at end of file diff --git a/README.md b/README.md index d3b2349..6a65939 100644 --- a/README.md +++ b/README.md @@ -3,8 +3,28 @@ This is a Roslyn analyzer library focusing on finding and fixing code that has a Usually these syntaxes have a good performance with smaller datasets (like in development environments) and start under performing when deployed to production servers where used on large datasets. +## GIS001 - Avoid calling Geometry.Union inside loops + +**Problem:** Calling `Geometry.Union()` repeatedly in loops (for, foreach, while) has poor performance characteristics, especially with large geometry collections. Each union operation can be expensive and doing them individually creates O(n²) complexity in many cases. + +**Solution:** Use `NetTopologySuite.Operation.Union.CascadedPolygonUnion.Union()` for batch operations, which provides much better performance through optimized algorithms. + +**Example:** +```csharp +// Bad - Poor performance +Geometry result = null; +foreach (var geom in geometries) +{ + result = result.Union(geom); +} + +// Good - Much better performance +Geometry result = CascadedPolygonUnion.Union(geometries); +``` + ### List of analyzers | Code | Description | | -------- | ------- | | NC001 | Avoid ?? inside Contains within IQueryable.Where | | NC002 | Avoid ?? inside Contains in LINQ query where clause | +| GIS001 | Avoid calling Geometry.Union inside loops |