Assorted SQL analyzer fixes (#82)

* fix #79

* fix #80; add new rule 243 for invalid datepart expressions

* groundwork for aggregate

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

Author: mgravell

docs/rules/DAP243.md

@@ -0,0 +1,12 @@
+# DAP243
+
+Date functions like `DATEADD`, `DATEPART` expect a *datepart* token as the first argument; for example in
+
+``` sql
+DATEPART(year, GETDATE())
+```
+
+the `year` is the *datepart* token. These values cannot be parameterized or take a value
+from a column - they are a special kind of literal value.
+
+For the list of valid tokens, see [here](https://learn.microsoft.com/sql/t-sql/functions/dateadd-transact-sql).

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

@@ -93,6 +93,8 @@ public static readonly DiagnosticDescriptor
         InvalidNullExpression = SqlWarning("DAP239", "Invalid null expression", "Operation requires a non-null operand"),
         VariableParameterConflict = SqlError("DAP240", "Parameter/variable conflict", "The declaration of variable '{0}' conflicts with a parameter"),
         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");
+        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"),
+        SelectAggregateMismatch = SqlWarning("DAP244", "SELECT aggregate mismatch", "SELECT has mixture of aggregate and non-aggregate expressions");
     }
 }

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

@@ -4,10 +4,7 @@
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.SqlServer.TransactSql.ScriptDom;
-using System;
-using System.Data;
 using System.Diagnostics;
-using static Dapper.SqlAnalysis.TSqlProcessor;
 
 namespace Dapper.Internal;
 
@@ -126,6 +123,10 @@ protected override void OnFromMultiTableMissingAlias(Location location)
         => OnDiagnostic(DapperAnalyzer.Diagnostics.FromMultiTableMissingAlias, location);
     protected override void OnFromMultiTableUnqualifiedColumn(Location location, string name)
         => OnDiagnostic(DapperAnalyzer.Diagnostics.FromMultiTableUnqualifiedColumn, location, name);
+
+    protected override void OnInvalidDatepartToken(Location location)
+        => OnDiagnostic(DapperAnalyzer.Diagnostics.InvalidDatepartToken, location);
+
     protected override void OnNonIntegerTop(Location location)
         => OnDiagnostic(DapperAnalyzer.Diagnostics.NonIntegerTop, location);
     protected override void OnNonPositiveTop(Location location)
@@ -138,6 +139,8 @@ protected override void OnSelectFirstTopError(Location location)
         => OnDiagnostic(DapperAnalyzer.Diagnostics.SelectFirstTopError, location);
     protected override void OnSelectSingleRowWithoutWhere(Location location)
         => OnDiagnostic(DapperAnalyzer.Diagnostics.SelectSingleRowWithoutWhere, location);
+    protected override void OnSelectAggregateAndNonAggregate(Location location)
+        => OnDiagnostic(DapperAnalyzer.Diagnostics.SelectAggregateMismatch, location);
     protected override void OnSelectSingleTopError(Location location)
         => OnDiagnostic(DapperAnalyzer.Diagnostics.SelectSingleTopError, location);
 

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

@@ -238,6 +238,8 @@ protected virtual void OnSelectFirstTopError(Location location)
         => OnError($"SELECT for First* should use TOP 1", location);
     protected virtual void OnSelectSingleRowWithoutWhere(Location location)
         => OnError($"SELECT for single row without WHERE or (TOP and ORDER BY)", location);
+    protected virtual void OnSelectAggregateAndNonAggregate(Location location)
+        => OnError($"SELECT has mixture of aggregate and non-aggregate expressions", location);
     protected virtual void OnNonPositiveTop(Location location)
         => OnError($"TOP literals should be positive", location);
     protected virtual void OnNonPositiveFetch(Location location)
@@ -250,6 +252,9 @@ protected virtual void OnFromMultiTableMissingAlias(Location location)
         => OnError($"FROM expressions with multiple elements should use aliases", location);
     protected virtual void OnFromMultiTableUnqualifiedColumn(Location location, string name)
         => OnError($"FROM expressions with multiple elements should qualify all columns; it is unclear where '{name}' is located", location);
+
+    protected virtual void OnInvalidDatepartToken(Location location)
+        => OnError($"Valid datepart token expected", location);
     protected virtual void OnTopWithOffset(Location location)
         => OnError($"TOP cannot be used when OFFSET is specified", location);
 
@@ -702,6 +707,66 @@ public override void Visit(QuerySpecification node)
             base.Visit(node);
         }
 
+        public override void ExplicitVisit(FunctionCall node)
+        {
+            ScalarExpression? ignore = null;
+            if (node.Parameters.Count != 0 && IsSpecialDateFunction(node.FunctionName.Value))
+            {
+                ValidateDateArg(ignore = node.Parameters[0]);
+            }
+            var oldIgnore = _demandAliases.IgnoreNode; // stash
+            _demandAliases.IgnoreNode = ignore;
+            base.ExplicitVisit(node); // dive
+            _demandAliases.IgnoreNode = oldIgnore; // restore
+
+            static bool IsSpecialDateFunction(string name)
+                => name.StartsWith("DATE", StringComparison.OrdinalIgnoreCase)
+                && IsAnyCaseInsensitive(name, DateTokenFunctions);
+        }
+
+        static bool IsAnyCaseInsensitive(string value, string[] options)
+        {
+            if (value is not null)
+            {
+                foreach (var option in options)
+                {
+                    if (string.Equals(option, value, StringComparison.OrdinalIgnoreCase))
+                    {
+                        return true;
+                    }
+                }
+            }
+            return false;
+        }
+        // arg0 has special meaning - not a column/etc
+        static readonly string[] DateTokenFunctions = ["DATE_BUCKET", "DATEADD", "DATEDIFF", "DATEDIFF_BIG", "DATENAME", "DATEPART", "DATETRUNC"];
+        static readonly string[] DateTokens = [
+            "year", "yy", "yyyy",
+            "quarter", "qq", "q",
+            "month", "mm", "m",
+            "dayofyear", "dy", "y",
+            "day", "dd", "d",
+            "week", "wk", "ww",
+            "weekday", "dw", "w",
+            "hour", "hh",
+            "minute", "mi", "n",
+            "second", "ss", "s",
+            "millisecond", "ms",
+            "microsecond", "mcs",
+            "nanosecond", "ns"
+            ];
+
+        private void ValidateDateArg(ScalarExpression value)
+        {
+            if (!(value is ColumnReferenceExpression col
+                && col.MultiPartIdentifier.Count == 1 && IsAnyCaseInsensitive(
+                    col.MultiPartIdentifier[0].Value, DateTokens)))
+            {
+                parser.OnInvalidDatepartToken(value);
+            }
+
+        }
+
         public override void Visit(SelectStatement node)
         {
             if (node.QueryExpression is QuerySpecification spec)
@@ -717,13 +782,15 @@ public override void Visit(SelectStatement node)
                 var checkNames = ValidateSelectNames;
                 HashSet<string> names = checkNames ? new HashSet<string>(StringComparer.InvariantCultureIgnoreCase) : null!;
 
+                var aggregate = AggregateFlags.None;
                 int index = 0;
                 foreach (var el in spec.SelectElements)
                 {
                     switch (el)
                     {
                         case SelectStarExpression:
                             parser.OnSelectStar(el);
+                            aggregate |= AggregateFlags.HaveNonAggregate;
                             reads++;
                             break;
                         case SelectScalarExpression scalar:
@@ -747,6 +814,7 @@ public override void Visit(SelectStatement node)
                                     parser.OnSelectDuplicateColumnName(scalar, name!);
                                 }
                             }
+                            aggregate |= IsAggregate(scalar.Expression);
                             reads++;
                             break;
                         case SelectSetVariable:
@@ -755,33 +823,43 @@ public override void Visit(SelectStatement node)
                     }
                     index++;
                 }
+
+                if (aggregate == (AggregateFlags.HaveAggregate | AggregateFlags.HaveNonAggregate))
+                {
+                    parser.OnSelectAggregateAndNonAggregate(spec);
+                }
+
                 if (reads != 0)
                 {
                     if (sets != 0)
                     {
                         parser.OnSelectAssignAndRead(spec);
                     }
-                    bool firstQuery = AddQuery();
-                    if (firstQuery && SingleRow // optionally enforce single-row validation
-                        && spec.FromClause is not null) // no "from" is things like 'select @id, @name' - always one row
+                    if (node.Into is null) // otherwise not actually a query
                     {
-                        bool haveTopOrFetch = false;
-                        if (spec.TopRowFilter is { Percent: false, Expression: ScalarExpression top })
-                        {
-                            haveTopOrFetch = EnforceTop(top);
-                        }
-                        else if (spec.OffsetClause is { FetchExpression: ScalarExpression fetch })
+                        bool firstQuery = AddQuery();
+                        if (firstQuery && SingleRow // optionally enforce single-row validation
+                            && spec.FromClause is not null) // no "from" is things like 'select @id, @name' - always one row
                         {
-                            haveTopOrFetch = EnforceTop(fetch);
-                        }
+                            bool haveTopOrFetch = false;
+                            if (spec.TopRowFilter is { Percent: false, Expression: ScalarExpression top })
+                            {
+                                haveTopOrFetch = EnforceTop(top);
+                            }
+                            else if (spec.OffsetClause is { FetchExpression: ScalarExpression fetch })
+                            {
+                                haveTopOrFetch = EnforceTop(fetch);
+                            }
 
-                        // we want *either* a WHERE (which we will allow with/without a TOP),
-                        // or a TOP + ORDER BY
-                        if (!IsUnfiltered(spec.FromClause, spec.WhereClause)) { } // fine
-                        else if (haveTopOrFetch && spec.OrderByClause is not null) { } // fine
-                        else
-                        {
-                            parser.OnSelectSingleRowWithoutWhere(node);
+                            // we want *either* a WHERE (which we will allow with/without a TOP),
+                            // or a TOP + ORDER BY
+                            if (!IsUnfiltered(spec.FromClause, spec.WhereClause)) { } // fine
+                            else if (haveTopOrFetch && spec.OrderByClause is not null) { } // fine
+                            else if ((aggregate & (AggregateFlags.HaveAggregate | AggregateFlags.Uncertain)) != 0) { } // fine
+                            else
+                            {
+                                parser.OnSelectSingleRowWithoutWhere(node);
+                            }
                         }
                     }
                 }
@@ -790,6 +868,50 @@ public override void Visit(SelectStatement node)
             base.Visit(node);
         }
 
+        enum AggregateFlags
+        {
+            None = 0,
+            HaveAggregate = 1 << 0,
+            HaveNonAggregate = 1 << 1,
+            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 etc inside an aggregate expression,
+            //   otherwise they contribute HaveNonAggregate
+            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)
@@ -1291,7 +1413,7 @@ bool TrySimplifyExpression(ScalarExpression node)
         {
             if (_expressionAlreadyEvaluated) return true;
 
-            if (node is UnaryExpression {  UnaryExpressionType: UnaryExpressionType.Negative, Expression: IntegerLiteral or NumericLiteral })
+            if (node is UnaryExpression { UnaryExpressionType: UnaryExpressionType.Negative, Expression: IntegerLiteral or NumericLiteral })
             {
                 // just "-4" or similar; don't warn the user to simplify that!
                 _expressionAlreadyEvaluated = true;
@@ -1340,6 +1462,9 @@ public override void ExplicitVisit(BinaryExpression node)
 
         public override void Visit(OutputClause node)
         {
+            // note that this doesn't handle OUTPUT INTO,
+            // which is via OutputIntoClause
+
             AddQuery(); // works like a query
             base.Visit(node);
         }
@@ -1409,6 +1534,8 @@ public DemandAliasesState(bool active, TableReference? amnesty)
             public readonly bool Active;
             public readonly TableReference? Amnesty; // we can't validate the target until too late
             public bool AmnestyNodeIsAlias;
+
+            public ScalarExpression? IgnoreNode { get; set; }
         }
 
         private DemandAliasesState _demandAliases;
@@ -1435,7 +1562,8 @@ public override void Visit(TableReferenceWithAlias node)
         }
         public override void Visit(ColumnReferenceExpression node)
         {
-            if (_demandAliases.Active && node.MultiPartIdentifier.Count == 1)
+            if (_demandAliases.Active && node.MultiPartIdentifier.Count == 1
+                && !ReferenceEquals(node, _demandAliases.IgnoreNode))
             {
                 parser.OnFromMultiTableUnqualifiedColumn(node, node.MultiPartIdentifier[0].Value);
             }

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

@@ -46,4 +46,17 @@ exec SomeLookupFetch
             Diagnostic(Diagnostics.ExecuteCommandWithQuery).WithLocation(0),
         ]);
 
+    [Fact]
+    public Task ReportWhenNoQueryExpected() => SqlVerifyAsync("""
+        SELECT [Test]
+        FROM MyTable
+        """, SqlAnalysis.SqlParseInputFlags.ExpectNoQuery,
+    Diagnostic(Diagnostics.ExecuteCommandWithQuery).WithLocation(Execute));
+
+    [Fact] // false positive scenario, https://github.com/DapperLib/DapperAOT/issues/79
+    public Task DoNotReportSelectInto() => SqlVerifyAsync("""
+        SELECT [Test]
+        INTO #MyTemporaryTable FROM MyTable
+        """, SqlAnalysis.SqlParseInputFlags.ExpectNoQuery);
+
 }

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

@@ -47,4 +47,17 @@ exec SomeLookupInsert 'North'
             Diagnostic(Diagnostics.QueryCommandMissingQuery).WithLocation(0),
         ]);
 
+    [Fact]
+    public Task DoNotReportWhenQueryExpected() => SqlVerifyAsync("""
+        SELECT [Test]
+        FROM MyTable
+        """, SqlAnalysis.SqlParseInputFlags.ExpectQuery);
+
+    [Fact]
+    public Task ReportSelectInto() => SqlVerifyAsync("""
+        SELECT [Test]
+        INTO #MyTemporaryTable FROM MyTable
+        """, SqlAnalysis.SqlParseInputFlags.ExpectQuery,
+            Diagnostic(Diagnostics.QueryCommandMissingQuery).WithLocation(Execute));
+
 }

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

@@ -16,5 +16,12 @@ from A a
         from A a
         inner join B b on b.X = a.Id
         """, Diagnostic(Diagnostics.FromMultiTableUnqualifiedColumn).WithLocation(0).WithArguments("Name"));
-    
+
+    [Fact] // false positive: https://github.com/DapperLib/DapperAOT/issues/80
+    public Task DoNotReportDateAdd() => SqlVerifyAsync("""
+        SELECT DATEADD(YEAR, 1, t.Year) AS NextYear
+        FROM MyTable t
+        JOIN MyOtherTable o ON o.Id = t.Id
+        """);
+
 }