analyzer: Warn about potentially incorrect pseudo-positional argument…

…s processing (#118)

* verify that it does not throw error

* split test

* add diagnostic

* docs

* expand test & address PR comments

* fix unwanted indent

* change phrasing in analyzer diagnostics as well

* address PR comments

* add processing of errors and other errors

* only 1 regex execution per sql analysis

* final touch

* review

docs/rules/DAP245.md

@@ -0,0 +1,19 @@
+# DAP245
+
+There is a non-zero chance of Dapper treating character sequence in SQL query like `?something?` as a pseudo-positional parameter **inside of a literal**,
+even if that is not an actual pseudo-positional parameter like in this example:
+```sql
+select '?this_is_my_string_data?' 
+```
+
+_Note: it will not be considered a pseudo-positional parameter in case there are spaces:_
+```sql
+select '?this is not pseudo-positional?' 
+```
+
+See [github issue](https://github.com/DapperLib/DapperAOT/issues/60) for more details.
+
+To mitigate, you can split the string via concatenation:
+``` sql
+select '?this_is' + 'my_string_data?' 
+```

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

@@ -101,6 +101,8 @@ 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"),
-        SelectAggregateMismatch = 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"),
+        PseudoPositionalParameter = SqlError("DAP245", "Avoid SQL pseudo-positional parameter", "It is more like Dapper will incorrectly treat this literal as a pseudo-positional parameter")
+        ;
     }
 }

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

@@ -76,6 +76,9 @@ protected override void OnNullLiteralComparison(Location location)
     protected override void OnParseError(ParseError error, Location location)
         => OnDiagnostic(DapperAnalyzer.Diagnostics.ParseError, location, error.Number, error.Message);
 
+    protected override void OnPseudoPositionalParameter(Location location)
+        => OnDiagnostic(DapperAnalyzer.Diagnostics.PseudoPositionalParameter, location);
+
     protected override void OnScalarVariableUsedAsTable(Variable variable)
         => OnDiagnostic(DapperAnalyzer.Diagnostics.ScalarVariableUsedAsTable, variable.Location, variable.Name);
 

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

@@ -0,0 +1,69 @@
+using System.Collections.Generic;
+using System.Linq;
+using System.Text.RegularExpressions;
+using Dapper.Internal;
+using Microsoft.SqlServer.TransactSql.ScriptDom;
+
+namespace Dapper.SqlAnalysis;
+
+internal readonly struct SqlProcessingContext
+{
+    private readonly TSqlProcessor.VariableTrackingVisitor _variableVisitor;
+    private readonly IReadOnlyDictionary<int, string> _pseudoPositionalArgumentsOffsetNames;
+
+    public SqlProcessingContext(TSqlProcessor.VariableTrackingVisitor variableVisitor, string sql)
+    {
+        _variableVisitor = variableVisitor;
+        _pseudoPositionalArgumentsOffsetNames = BuildPseudoPositionalArgsOffsetNamesMap();
+
+        IReadOnlyDictionary<int, string> BuildPseudoPositionalArgsOffsetNamesMap()
+        {
+            var offsetNamesMap = new Dictionary<int, string>();
+            var regexMatch = CompiledRegex.PseudoPositional.Match(sql);
+            while (regexMatch.Success)
+            {
+                foreach (var match in regexMatch.Groups.OfType<Match>())
+                {
+                    offsetNamesMap.Add(match.Index, match.Value.Trim('?'));
+                }
+
+                regexMatch = regexMatch.NextMatch();
+            }
+
+            return offsetNamesMap;
+        }
+    }
+
+    public IEnumerable<ParseError> GetErrorsToReport(IList<ParseError>? parseErrors)
+    {
+        if (parseErrors is null || parseErrors.Count == 0) yield break;
+        foreach (var parseError in parseErrors)
+        {
+            if (_pseudoPositionalArgumentsOffsetNames.Count != 0 && IsPseudoPositionalParameterGenuineUsage(parseError))
+            {
+                continue;
+            }
+
+            yield return parseError;
+        }
+    }
+
+    public void MarkPseudoPositionalVariablesUsed(in ISet<string> parameters)
+    {
+        foreach (var name in _pseudoPositionalArgumentsOffsetNames.Values)
+        {
+            if (_variableVisitor.TryGetByName(name, out var variable) && parameters.Contains(name))
+            {
+                var usedVar = variable.WithUsed();
+                usedVar = usedVar.WithConsumed();
+                _variableVisitor.SetVariable(usedVar);
+            }
+        }
+    }
+
+    bool IsPseudoPositionalParameterGenuineUsage(ParseError error)
+    {
+        if (error.Number != 46010) return false; // `46010` is `Incorrect syntax around ...`
+        return _pseudoPositionalArgumentsOffsetNames.ContainsKey(error.Offset);
+    }
+}

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

@@ -86,16 +86,22 @@ public virtual bool Execute(string sql, ImmutableArray<SqlParameter> members = d
         {
             Flags |= SqlParseOutputFlags.SqlAdjustedForDapperSyntax;
         }
+
+        var sqlProcessingContext = new SqlProcessingContext(_visitor, fixedSql);
+
+        var memberNamesSet = new HashSet<string>(members.Select(x => x.Name), CaseSensitive ? StringComparer.InvariantCulture : StringComparer.InvariantCultureIgnoreCase);
+        sqlProcessingContext.MarkPseudoPositionalVariablesUsed(memberNamesSet);
+
         var parser = new TSql160Parser(true, SqlEngineType.All);
         TSqlFragment tree;
         using (var reader = new StringReader(fixedSql))
         {
             tree = parser.Parse(reader, out var errors);
             if (errors is not null && errors.Count != 0)
             {
-                Flags |= SqlParseOutputFlags.SyntaxError;
-                foreach (var error in errors)
+                foreach (var error in sqlProcessingContext.GetErrorsToReport(errors))
                 {
+                    Flags |= SqlParseOutputFlags.SyntaxError;
                     OnParseError(error, new Location(error.Line, error.Column, error.Offset, 0));
                 }
             }
@@ -118,7 +124,7 @@ public virtual bool Execute(string sql, ImmutableArray<SqlParameter> members = d
             {
                 if (_visitor.KnownParameters) OnVariableNotDeclared(variable);
             }
-            else if (variable.IsUnconsumed && !variable.IsTable && !variable.IsOutputParameter)
+            else if (variable is { IsUnconsumed: true, IsTable: false, IsOutputParameter: false })
             {
                 if (_visitor.AssignmentTracking) OnVariableValueNotConsumed(variable);
             }
@@ -173,7 +179,10 @@ protected virtual void OnTableVariableUsedAsScalar(Variable variable)
         => OnError($"Table variable {variable.Name} is used like a scalar", variable.Location);
 
     protected virtual void OnNullLiteralComparison(Location location)
-        => OnError($"Null literals should not be used in binary comparisons; prefer 'is null' and 'is not null'", location);
+        => OnError("Null literals should not be used in binary comparisons; prefer 'is null' and 'is not null'", location);
+
+    protected virtual void OnPseudoPositionalParameter(Location location)
+        => OnError("It is more like Dapper will incorrectly treat this literal as a pseudo-positional parameter", location);
 
     private void OnSimplifyExpression(Location location, int? value)
         => OnSimplifyExpression(location, value is null ? "null" : value.Value.ToString(CultureInfo.InvariantCulture));
@@ -186,77 +195,77 @@ private void OnSimplifyExpression(Location location, decimal? value)
         });
 
     protected virtual void OnDivideByZero(Location location)
-        => OnError($"Division by zero detected", location);
+        => OnError("Division by zero detected", location);
     protected virtual void OnTrivialOperand(Location location)
-        => OnError($"Operand makes this calculation trivial; it can be simplified", location);
+        => OnError("Operand makes this calculation trivial; it can be simplified", location);
     protected virtual void OnInvalidNullExpression(Location location)
-        => OnError($"Operation requires non-null operand", location);
+        => OnError("Operation requires non-null operand", location);
 
     protected virtual void OnSimplifyExpression(Location location, string value)
         => OnError($"Expression can be simplified to '{value}'", location);
 
     protected virtual void OnAdditionalBatch(Location location)
-        => OnError($"Multiple batches are not permitted", location);
+        => OnError("Multiple batches are not permitted", location);
 
     protected virtual void OnGlobalIdentity(Location location)
-        => OnError($"@@identity should not be used; use SCOPE_IDENTITY() instead", location);
+        => OnError("@@identity should not be used; use SCOPE_IDENTITY() instead", location);
 
     protected virtual void OnSelectScopeIdentity(Location location)
-        => OnError($"Consider OUTPUT INSERTED.yourid on the INSERT instead of SELECT SCOPE_IDENTITY()", location);
+        => OnError("Consider OUTPUT INSERTED.yourid on the INSERT instead of SELECT SCOPE_IDENTITY()", location);
 
     protected virtual void OnExecComposedSql(Location location)
-        => OnError($"EXEC with composed SQL may be susceptible to SQL injection; consider EXEC sp_executesql with parameters", location);
+        => OnError("EXEC with composed SQL may be susceptible to SQL injection; consider EXEC sp_executesql with parameters", location);
 
     protected virtual void OnTableVariableOutputParameter(Variable variable)
         => OnError($"Table variable {variable.Name} cannot be used as an output parameter", variable.Location);
 
     protected virtual void OnInsertColumnsNotSpecified(Location location)
-        => OnError($"Target columns for INSERT should be explicitly specified", location);
+        => OnError("Target columns for INSERT should be explicitly specified", location);
 
     protected virtual void OnInsertColumnMismatch(Location location)
-        => OnError($"The columns specified in the INSERT do not match the source columns provided", location);
+        => OnError("The columns specified in the INSERT do not match the source columns provided", location);
     protected virtual void OnInsertColumnsUnbalanced(Location location)
-        => OnError($"The rows specified in the INSERT have differing widths", location);
+        => OnError("The rows specified in the INSERT have differing widths", location);
 
     protected virtual void OnSelectStar(Location location)
-        => OnError($"SELECT columns should be explicitly specified", location);
+        => OnError("SELECT columns should be explicitly specified", location);
     protected virtual void OnSelectEmptyColumnName(Location location, int column)
         => OnError($"SELECT statement with missing column name: {column}", location);
     protected virtual void OnSelectDuplicateColumnName(Location location, string name)
         => OnError($"SELECT statement with duplicate column name: {name}", location);
     protected virtual void OnSelectAssignAndRead(Location location)
-        => OnError($"SELECT statement has both assignment and results", location);
+        => OnError("SELECT statement has both assignment and results", location);
 
     protected virtual void OnDeleteWithoutWhere(Location location)
-        => OnError($"DELETE statement without WHERE clause", location);
+        => OnError("DELETE statement without WHERE clause", location);
     protected virtual void OnUpdateWithoutWhere(Location location)
-        => OnError($"UPDATE statement without WHERE clause", location);
+        => OnError("UPDATE statement without WHERE clause", location);
 
     protected virtual void OnSelectSingleTopError(Location location)
-        => OnError($"SELECT for Single* should use TOP 2; if you do not need to test over-read, use First*", location);
+        => OnError("SELECT for Single* should use TOP 2; if you do not need to test over-read, use First*", location);
     protected virtual void OnSelectFirstTopError(Location location)
-        => OnError($"SELECT for First* should use TOP 1", 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);
+        => 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);
+        => OnError("SELECT has mixture of aggregate and non-aggregate expressions", location);
     protected virtual void OnNonPositiveTop(Location location)
-        => OnError($"TOP literals should be positive", location);
+        => OnError("TOP literals should be positive", location);
     protected virtual void OnNonPositiveFetch(Location location)
-        => OnError($"FETCH literals should be positive", location);
+        => OnError("FETCH literals should be positive", location);
     protected virtual void OnNegativeOffset(Location location)
-        => OnError($"OFFSET literals should be non-negative", location);
+        => OnError("OFFSET literals should be non-negative", location);
     protected virtual void OnNonIntegerTop(Location location)
-        => OnError($"TOP literals should be integers", location);
+        => OnError("TOP literals should be integers", location);
     protected virtual void OnFromMultiTableMissingAlias(Location location)
-        => OnError($"FROM expressions with multiple elements should use aliases", 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);
+        => OnError("Valid datepart token expected", location);
     protected virtual void OnTopWithOffset(Location location)
-        => OnError($"TOP cannot be used when OFFSET is specified", location);
+        => OnError("TOP cannot be used when OFFSET is specified", location);
 
 
     internal readonly struct Location
@@ -364,7 +373,8 @@ private Variable(scoped in Variable source, Location location)
         public Variable WithLocation(TSqlFragment node) => new(in this, node);
         public Variable WithName(string name) => new(in this, name);
     }
-    class LoggingVariableTrackingVisitor : VariableTrackingVisitor
+
+    internal class LoggingVariableTrackingVisitor : VariableTrackingVisitor
     {
         private readonly Action<string> log;
         public LoggingVariableTrackingVisitor(SqlParseInputFlags flags, TSqlProcessor parser, Action<string> log) : base(flags, parser)
@@ -406,7 +416,8 @@ public override void Visit(TSqlFragment node)
             base.Visit(node);
         }
     }
-    class VariableTrackingVisitor : TSqlFragmentVisitor
+
+    internal class VariableTrackingVisitor : TSqlFragmentVisitor
     {
         // important note for anyone maintaining this;
         //
@@ -474,7 +485,19 @@ private bool SlowTryGetVariable(string name, out Variable variable)
             return false;
         }
 
-        private Variable SetVariable(Variable variable) => _variables[variable.Name] = variable;
+        internal Variable SetVariable(Variable variable) => _variables[variable.Name] = variable;
+
+        internal bool TryGetByName(string name, out Variable variable)
+        {
+            if (_variables.TryGetValue(name, out var value))
+            {
+                variable = value;
+                return true;
+            }
+
+            variable = default;
+            return false;
+        }
 
         private readonly Dictionary<string, Variable> _variables;
         private int batchCount;
@@ -623,6 +646,32 @@ public override void Visit(BooleanComparisonExpression node)
             base.Visit(node);
         }
 
+        public override void Visit(WhereClause whereClause)
+        {
+            base.Visit(whereClause);
+        }
+
+        public override void Visit(StringLiteral node)
+        {
+            foreach (var token in node.ScriptTokenStream)
+            {
+                if (token.TokenType is not TSqlTokenType.AsciiStringLiteral)
+                {
+                    continue;
+                }
+
+                int firstAppearance;
+                if ((firstAppearance = token.Text.IndexOf('?')) >= 0
+                    && token.Text.IndexOf('?', firstAppearance + 1) >= 0 // if there are 2 appearances of `?`
+                    && CompiledRegex.PseudoPositional.IsMatch(node.Value))
+                {
+                    parser.OnPseudoPositionalParameter(node);
+                }
+            }
+
+            base.Visit(node);
+        }
+
         public override void ExplicitVisit(QuerySpecification node)
         {
             var oldDemandAlias = _demandAliases;
@@ -647,6 +696,11 @@ public override void ExplicitVisit(QuerySpecification node)
             }
         }
 
+        public override void Visit(QueryExpression expression)
+        {
+            base.Visit(expression);
+        }
+
         public override void ExplicitVisit(SelectSetVariable node)
         {
             Visit(node);