Fix EC69 (#80)

.github/workflows/build-and-test.yml

@@ -27,7 +27,7 @@ jobs:
 
     - run: dotnet test TestsOnly.slnf -c Release --no-build --logger "trx;LogFileName=test_results.trx" -p:CollectCoverage=true -p:Threshold=80 -p:CoverletOutputFormat='cobertura'
 
-    - uses: danielpalme/ReportGenerator-GitHub-Action@5
+    - uses: danielpalme/ReportGenerator-GitHub-Action@5.3.8
       with:
         reports: '/home/runner/work/ecoCode-csharp/ecoCode-csharp/src/EcoCode.Tests/coverage.cobertura.xml'
         targetdir: 'CoverageReports'

Directory.Packages.props

@@ -21,8 +21,8 @@
     <PackageVersion Include="Microsoft.Extensions.Logging" Version="8.0.0" />
     <PackageVersion Include="Microsoft.NET.Test.Sdk" Version="17.10.0" />
     <PackageVersion Include="Microsoft.SourceLink.GitHub" Version="8.0.0" />
-    <PackageVersion Include="MSTest.TestAdapter" Version="3.4.3" />
-    <PackageVersion Include="MSTest.TestFramework" Version="3.4.3" />
+    <PackageVersion Include="MSTest.TestAdapter" Version="3.5.0" />
+    <PackageVersion Include="MSTest.TestFramework" Version="3.5.0" />
     <PackageVersion Include="Roslynator.Analyzers" Version="4.12.4" />
     <PackageVersion Include="Roslynator.CodeAnalysis.Analyzers" Version="4.12.4" />
     <PackageVersion Include="Spectre.Console" Version="0.49.1" />

ecoCode-csharp.sln

@@ -36,7 +36,6 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution
 		Directory.Packages.props = Directory.Packages.props
 		EcoCode.globalconfig = EcoCode.globalconfig
 		global.json = global.json
-		NuGet.Config = NuGet.Config
 		Rules to work on.md = Rules to work on.md
 		SharedAssemblyInfo.cs = SharedAssemblyInfo.cs
 	EndProjectSection
@@ -84,6 +83,9 @@ Global
 		{375CBD0D-5754-352F-9D9D-ED72E2F1B0ED} = {089100B1-113F-4E66-888A-E83F3999EAFD}
 	EndGlobalSection
 EndGlobal
+
+	EndGlobalSection
+EndGlobal
 }
 	EndGlobalSection
 EndGlobal

src/EcoCode.Core/Analyzers/EC69.DontCallFunctionsInLoopConditions.cs

@@ -47,40 +47,53 @@ private static void AnalyzeLoopNode(SyntaxNodeAnalysisContext context)
         {
             if (node is not InvocationExpressionSyntax invocation) continue;
 
+            // Analyze the object on which the method is called, if any
+            var callee = invocation.Expression switch
+            {
+                MemberAccessExpressionSyntax m => m.Expression,
+                MemberBindingExpressionSyntax => (node.Parent as ConditionalAccessExpressionSyntax)?.Expression, // ?. operator
+                _ => null,
+            };
+            if (callee is not null && context.SemanticModel.GetSymbolInfo(callee).Symbol is ISymbol c && c.IsVariable())
+                _ = loopInvariantSymbols.Add(c);
+
+            // Analyze the arguments of the method call
             foreach (var arg in invocation.ArgumentList.Arguments)
             {
-                if (context.SemanticModel.GetSymbolInfo(arg.Expression).Symbol is ISymbol symbol && symbol.IsVariable())
-                    _ = loopInvariantSymbols.Add(symbol);
+                if (context.SemanticModel.GetSymbolInfo(arg.Expression).Symbol is ISymbol s && s.IsVariable())
+                    _ = loopInvariantSymbols.Add(s);
             }
         }
-        if (loopInvariantSymbols.Count == 0) return;
-
-        // Step 2: Remove the variables that are mutated in the loop body or the for loop incrementors
-        RemoveMutatedSymbols(expression.DescendantNodes(), loopInvariantSymbols, context.SemanticModel);
-        if (loopInvariantSymbols.Count == 0) return;
 
-        foreach (var inc in incrementors)
-            RemoveMutatedSymbols(inc.DescendantNodesAndSelf(), loopInvariantSymbols, context.SemanticModel);
-        if (loopInvariantSymbols.Count == 0) return;
+        // Step 2: Remove the variables that are mutated in the loop body and/or the for loop incrementors
+        if (loopInvariantSymbols.Count != 0)
+        {
+            RemoveMutatedSymbols(expression.DescendantNodes(), loopInvariantSymbols, context.SemanticModel);
+            foreach (var inc in incrementors)
+                RemoveMutatedSymbols(inc.DescendantNodesAndSelf(), loopInvariantSymbols, context.SemanticModel);
+        }
 
         // Step 3: Identify conditions that are loop invariant
         foreach (var node in condition.DescendantNodes())
         {
             if (node is not InvocationExpressionSyntax invocation) continue;
 
-            bool loopInvariant = true;
-
-            foreach (var arg in invocation.ArgumentList.Arguments)
+            if (invocation.Expression is MemberBindingExpressionSyntax memberBinding)
             {
-                if (context.SemanticModel.GetSymbolInfo(arg.Expression).Symbol is ISymbol symbol && !loopInvariantSymbols.Contains(symbol))
+                if (node.Parent is ConditionalAccessExpressionSyntax conditionalAccess &&
+                    IsLoopInvariant(conditionalAccess.Expression) &&
+                    invocation.ArgumentList.Arguments.All(arg => IsLoopInvariant(arg.Expression)))
                 {
-                    loopInvariant = false;
-                    break;
+                    context.ReportDiagnostic(Diagnostic.Create(Descriptor, conditionalAccess.GetLocation()));
                 }
+                continue;
             }
 
-            if (loopInvariant)
+            if ((invocation.Expression is not MemberAccessExpressionSyntax memberAccess || IsLoopInvariant(memberAccess.Expression)) &&
+                invocation.ArgumentList.Arguments.All(arg => IsLoopInvariant(arg.Expression)))
+            {
                 context.ReportDiagnostic(Diagnostic.Create(Descriptor, invocation.GetLocation()));
+            }
         }
 
         static void RemoveMutatedSymbols(IEnumerable<SyntaxNode> nodes, HashSet<ISymbol> loopInvariantSymbols, SemanticModel semanticModel)
@@ -100,5 +113,10 @@ static void RemoveMutatedSymbols(IEnumerable<SyntaxNode> nodes, HashSet<ISymbol>
                     return;
             }
         }
+
+        bool IsLoopInvariant(ExpressionSyntax expr) =>
+            context.SemanticModel.GetSymbolInfo(expr).Symbol is not ISymbol s ||
+            !s.IsVariable() ||
+            loopInvariantSymbols.Contains(s);
     }
 }

src/EcoCode.Core/Extensions/SymbolExtensions.cs

@@ -1,26 +1,32 @@
-namespace EcoCode.Extensions;
+using Microsoft.CodeAnalysis;
+
+namespace EcoCode.Extensions;
 
 /// <summary>Extensions methods for <see cref="ISymbol"/>.</summary>
 public static class SymbolExtensions
 {
     /// <summary>Returns whether the symbol is a variable.</summary>
     /// <param name="symbol">The symbol.</param>
     /// <returns>True if the symbol is a variable, false otherwise.</returns>
-    public static bool IsVariable(this ISymbol symbol) =>
-        symbol is ILocalSymbol or IFieldSymbol or IPropertySymbol or IParameterSymbol;
+    public static bool IsVariable(this ISymbol symbol) => symbol switch
+    {
+        IFieldSymbol fieldSymbol => !fieldSymbol.HasConstantValue,
+        ILocalSymbol localSymbol => !localSymbol.HasConstantValue,
+        _ => symbol is IPropertySymbol or IParameterSymbol,
+    };
 
     /// <summary>Returns whether the symbol is a variable of the specified type.</summary>
     /// <param name="symbol">The symbol.</param>
     /// <param name="type">The variable type.</param>
     /// <returns>True if the symbol is a variable of the given type, false otherwise.</returns>
     public static bool IsVariableOfType(this ISymbol symbol, SpecialType type) => symbol switch
     {
-        ILocalSymbol s => s.Type.SpecialType,
-        IFieldSymbol s => s.Type.SpecialType,
-        IPropertySymbol s => s.Type.SpecialType,
-        IParameterSymbol s => s.Type.SpecialType,
-        _ => SpecialType.None,
-    } == type;
+        IFieldSymbol s => !s.HasConstantValue && s.Type.SpecialType == type,
+        ILocalSymbol s => !s.HasConstantValue && s.Type.SpecialType == type,
+        IPropertySymbol s => s.Type.SpecialType == type,
+        IParameterSymbol s => s.Type.SpecialType == type,
+        _ => false,
+    };
 
     /// <summary>Returns whether the symbol implements a given interface.</summary>
     /// <param name="symbol">The symbol.</param>

src/EcoCode.Tests/Tests/EC69.DontCallFunctionsInLoopConditions.Tests.cs

@@ -6,10 +6,10 @@ public sealed class DontCallFunctionsInLoopConditionsTests
     private static readonly AnalyzerDlg VerifyAsync = TestRunner.VerifyAsync<DontCallFunctionsInLoopConditions>;
 
     [TestMethod]
-    public async Task EmptyCodeAsync() => await VerifyAsync("").ConfigureAwait(false);
+    public Task EmptyCodeAsync() => VerifyAsync("");
 
     [TestMethod]
-    public async Task ForLoopFunctionCallShouldNotBeCalledAsync() => await VerifyAsync("""
+    public Task ForLoopFunctionCallShouldNotBeCalledAsync() => VerifyAsync("""
         public class Test
         {
             private const int C = 10;
@@ -30,10 +30,10 @@ public void Run(int p)
                 }
             }
         }
-        """).ConfigureAwait(false);
+        """);
 
     [TestMethod]
-    public async Task WhileLoopFunctionCallShouldNotBeCalledAsync() => await VerifyAsync("""
+    public Task WhileLoopFunctionCallShouldNotBeCalledAsync() => VerifyAsync("""
         public class Test
         {
             private const int C = 10;
@@ -54,10 +54,10 @@ public void Run(int p)
                 }
             }
         }
-        """).ConfigureAwait(false);
+        """);
 
     [TestMethod]
-    public async Task DoWhileLoopFunctionCallShouldNotBeCalledAsync() => await VerifyAsync("""
+    public Task DoWhileLoopFunctionCallShouldNotBeCalledAsync() => VerifyAsync("""
         public class Test
         {
             private const int C = 10;
@@ -78,5 +78,45 @@ public void Run(int p)
                 } while (i < V1 && i < [|V2()|] && i < V3(i) && i < V3(j) && i < [|V3(k)|] && i < [|V3(p)|] && i < [|V3(C)|]);
             }
         }
-        """).ConfigureAwait(false);
+        """);
+
+    [TestMethod]
+    public Task WithLoopVariantNullablesAsync() => VerifyAsync("""
+        using System;
+        using System.IO;
+        public class Test
+        {
+            public static void Run1(string? path)
+            {
+                for (path = Path.GetDirectoryName(path); !path.Equals(@"S:\", StringComparison.OrdinalIgnoreCase); path = Path.GetDirectoryName(path)) { }
+                for (path = Path.GetDirectoryName(path); path?.Equals(@"S:\", StringComparison.OrdinalIgnoreCase) != true; path = Path.GetDirectoryName(path)) { }
+
+                while (!path.Equals(@"S:\", StringComparison.OrdinalIgnoreCase)) path = Path.GetDirectoryName(path);
+                while (path?.Equals(@"S:\", StringComparison.OrdinalIgnoreCase) != true) path = Path.GetDirectoryName(path);
+
+                do path = Path.GetDirectoryName(path); while (!path.Equals(@"S:\", StringComparison.OrdinalIgnoreCase));
+                do path = Path.GetDirectoryName(path); while (path?.Equals(@"S:\", StringComparison.OrdinalIgnoreCase) != true);
+            }
+        }
+        """);
+
+    [TestMethod]
+    public Task WithLoopInvariantNullablesAsync() => VerifyAsync("""
+        using System;
+        using System.IO;
+        public class Test
+        {
+            public static void Run(string? path)
+            {
+                for (path = Path.GetDirectoryName(path); ![|path.Equals(@"S:\", StringComparison.OrdinalIgnoreCase)|]; ) { }
+                for (path = Path.GetDirectoryName(path); [|path?.Equals(@"S:\", StringComparison.OrdinalIgnoreCase)|] != true; ) { }
+        
+                while (![|path.Equals(@"S:\", StringComparison.OrdinalIgnoreCase)|]) ;
+                while ([|path?.Equals(@"S:\", StringComparison.OrdinalIgnoreCase)|] != true) ;
+        
+                do ; while (![|path.Equals(@"S:\", StringComparison.OrdinalIgnoreCase)|]);
+                do ; while ([|path?.Equals(@"S:\", StringComparison.OrdinalIgnoreCase)|] != true);
+            }
+        }
+        """);
 }

src/EcoCode.Tests/Tests/EC72.DontExecuteSqlCommandsInLoops.Tests.cs

@@ -6,10 +6,10 @@ public sealed class DontExecuteSqlCommandsInLoopsTests
     private static readonly AnalyzerDlg VerifyAsync = TestRunner.VerifyAsync<DontExecuteSqlCommandsInLoops>;
 
     [TestMethod]
-    public async Task EmptyCodeAsync() => await VerifyAsync("").ConfigureAwait(false);
+    public Task EmptyCodeAsync() => VerifyAsync("");
 
     [TestMethod]
-    public async Task DontExecuteSqlCommandsInLoopsAsync() => await VerifyAsync("""
+    public Task DontExecuteSqlCommandsInLoopsAsync() => VerifyAsync("""
         using System.Data;
         public class Test
         {
@@ -30,5 +30,5 @@ public void Run(int p)
                 }
             }
         }
-        """).ConfigureAwait(false);
+        """);
 }