fix #81; add new rule DAP244 for mixing aggregate and non-aggregate data

Author: mgravell

docs/rules/DAP244.md

@@ -0,0 +1,28 @@
+# DAP244
+
+A `SELECT` expression cannot mix aggregate and non-aggregate results;
+
+Fine:
+
+``` sql
+select Name
+from SomeTable
+```
+
+or
+
+``` sql
+select COUNT(1)
+from SomeTable
+```
+
+Invalid:
+
+or
+
+``` sql
+select Name, COUNT(1)
+from SomeTable
+```
+
+Aggregate functions are [listed here](https://learn.microsoft.com/sql/t-sql/functions/aggregate-functions-transact-sql).

src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.Diagnostics.cs

@@ -95,6 +95,6 @@ public static readonly DiagnosticDescriptor
         InterpolatedStringSqlExpression = SqlWarning("DAP241", "Interpolated string usage", "Data values should not be interpolated into SQL string - use parameters instead"),
         ConcatenatedStringSqlExpression = SqlWarning("DAP242", "Concatenated string usage", "Data values should not be concatenated into SQL string - use parameters instead"),
         InvalidDatepartToken = SqlWarning("DAP243", "Valid datepart token expected", "Date functions require a recognized datepart argument"),
-            OnSelectAggregateAndNonAggregate = SqlWarning("DAP244", "SELECT aggregate mismatch", "SELECT has mixture of aggregate and non-aggregate expressions");
+        SelectAggregateMismatch = SqlWarning("DAP244", "SELECT aggregate mismatch", "SELECT has mixture of aggregate and non-aggregate expressions");
     }
 }

src/Dapper.AOT.Analyzers/Internal/DiagnosticTSqlProcessor.cs

@@ -140,7 +140,7 @@ protected override void OnSelectFirstTopError(Location location)
     protected override void OnSelectSingleRowWithoutWhere(Location location)
         => OnDiagnostic(DapperAnalyzer.Diagnostics.SelectSingleRowWithoutWhere, location);
     protected override void OnSelectAggregateAndNonAggregate(Location location)
-        => OnDiagnostic(DapperAnalyzer.Diagnostics.OnSelectAggregateAndNonAggregate, location);
+        => OnDiagnostic(DapperAnalyzer.Diagnostics.SelectAggregateMismatch, location);
     protected override void OnSelectSingleTopError(Location location)
         => OnDiagnostic(DapperAnalyzer.Diagnostics.SelectSingleTopError, location);
 

src/Dapper.AOT.Analyzers/SqlAnalysis/TSqlProcessor.cs

@@ -760,7 +760,7 @@ private void ValidateDateArg(ScalarExpression value)
         {
             if (!(value is ColumnReferenceExpression col
                 && col.MultiPartIdentifier.Count == 1 && IsAnyCaseInsensitive(
-                    col.MultiPartIdentifier[0].Value, DateTokens )))
+                    col.MultiPartIdentifier[0].Value, DateTokens)))
             {
                 parser.OnInvalidDatepartToken(value);
             }
@@ -873,17 +873,45 @@ enum AggregateFlags
             None = 0,
             HaveAggregate = 1 << 0,
             HaveNonAggregate = 1 << 1,
-            Uncertain = 1 <<  2,
+            Uncertain = 1 << 2,
         }
+
         private AggregateFlags IsAggregate(ScalarExpression expression)
         {
             // any use of an aggregate function contributes HaveAggregate
             // - there could be unary/binary operations on that aggregate function
-            // - column references inside the aggregate expression or a sub-query are fine,
+            // - column references etc inside an aggregate expression,
             //   otherwise they contribute HaveNonAggregate
-            return AggregateFlags.Uncertain;
+            switch (expression)
+            {
+                case Literal:
+                    return AggregateFlags.None;
+                case UnaryExpression ue:
+                    return IsAggregate(ue.Expression);
+                case BinaryExpression be:
+                    return IsAggregate(be.FirstExpression)
+                        | IsAggregate(be.SecondExpression);
+                case FunctionCall func when IsAggregateFunction(func.FunctionName.Value):
+                    return AggregateFlags.HaveAggregate; // don't need to look inside
+                case ScalarSubquery sq:
+                    throw new NotSupportedException();
+                case IdentityFunctionCall:
+                case PrimaryExpression:
+                    return AggregateFlags.HaveNonAggregate;
+                default:
+                    return AggregateFlags.Uncertain;
+            }
+
+            static bool IsAggregateFunction(string name)
+                => IsAnyCaseInsensitive(name, AggregateFunctions);
         }
 
+        static readonly string[] AggregateFunctions = [
+            "APPROX_COUNT_DISTINCT", "AVG","CHECKSUM_AGG", "COUNT", "COUNT_BIG",
+            "GROUPING", "GROUPING_ID", "MAX", "MIN", "STDEV",
+            "STDEVP", "STRING_AGG", "SUM", "VAR", "VARP",
+            ];
+
         private bool EnforceTop(ScalarExpression expr)
         {
             if (IsInt32(expr, out var i) == TryEvaluateResult.SuccessConstant && i.HasValue)

test/Dapper.AOT.Test/Verifiers/DAP231.cs

@@ -37,4 +37,14 @@ public Task SelectSingleRowWithoutFrom() => SqlVerifyAsync("""
         select HasRecords = case when exists (select top 1 1 from MyTable) then 1 else 0 end
         """, SqlParseInputFlags.SingleRow);
 
+    [Fact] // false positive https://github.com/DapperLib/DapperAOT/issues/81
+    public Task DoNotReportForAggregate() => SqlVerifyAsync("""
+        SELECT MAX(SomeColumn) FROM MyTable
+        """, SqlParseInputFlags.SingleRow);
+
+    [Fact]
+    public Task DoNotReportForMultipleAggregatesAndExpressions() => SqlVerifyAsync("""
+        SELECT -MAX(SomeColumn), COUNT(1) + 32 FROM MyTable
+        """, SqlParseInputFlags.SingleRow);
+
 }

test/Dapper.AOT.Test/Verifiers/DAP244.cs

@@ -0,0 +1,27 @@
+using Dapper.CodeAnalysis;
+using System.Threading.Tasks;
+using Xunit;
+using Diagnostics = Dapper.CodeAnalysis.DapperAnalyzer.Diagnostics;
+namespace Dapper.AOT.Test.Verifiers;
+
+public class DAP244 : Verifier<DapperAnalyzer>
+{
+    [Fact]
+    public Task ValidNonAggregate() => SqlVerifyAsync("""
+        select Name
+        from SomeTable
+        """);
+
+    [Fact]
+    public Task ValidAggregate() => SqlVerifyAsync("""
+        select COUNT(1), COUNT(DISTINCT Name)
+        from SomeTable
+        """);
+
+    [Fact]
+    public Task InvalidMix() => SqlVerifyAsync("""
+        {|#0:select COUNT(1), COUNT(DISTINCT Name), Name
+        from SomeTable|}
+        """, Diagnostic(Diagnostics.SelectAggregateMismatch).WithLocation(0));
+
+}