Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: trivial parameterization anti-patterns analysis support #61

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ public static readonly DiagnosticDescriptor
DivideByZero = SqlWarning("DAP237", "Divide by zero", "Division by zero detected"),
TrivialOperand = SqlWarning("DAP238", "Trivial operand", "Operand makes this calculation trivial; it can be simplified"),
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");
VariableParameterConflict = SqlError("DAP240", "Parameter/variable conflict", "The declaration of variable '{0}' conflicts with a parameter"),
DeagleGross marked this conversation as resolved.
Show resolved Hide resolved
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");
}
}
21 changes: 19 additions & 2 deletions src/Dapper.AOT.Analyzers/CodeAnalysis/DapperAnalyzer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using Dapper.Internal.Roslyn;
using Dapper.SqlAnalysis;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using System;
Expand Down Expand Up @@ -371,7 +372,23 @@ private void ValidateSql(in OperationAnalysisContext ctx, IOperation sqlSource,
if (caseSensitive) flags |= SqlParseInputFlags.CaseSensitive;

// can we get the SQL itself?
if (!TryGetConstantValueWithSyntax(sqlSource, out string? sql, out var sqlSyntax)) return;
if (!TryGetConstantValueWithSyntax(sqlSource, out string? sql, out var sqlSyntax, out var syntaxKind))
{
DiagnosticDescriptor? descriptor = syntaxKind switch
{
SyntaxKind.InterpolatedStringExpression or SyntaxKind.InterpolatedStringText
=> Diagnostics.InterpolatedStringSqlExpression,
SyntaxKind.AddExpression
=> Diagnostics.ConcatenatedStringSqlExpression,
_ => null
};

if (descriptor is not null)
{
ctx.ReportDiagnostic(Diagnostic.Create(descriptor, sqlSource.Syntax.GetLocation()));
}
return;
}
if (string.IsNullOrWhiteSpace(sql) || !HasWhitespace.IsMatch(sql)) return; // need non-trivial content to validate

location ??= ctx.Operation.Syntax.GetLocation();
Expand Down Expand Up @@ -461,7 +478,7 @@ internal static Location SharedParseArgsAndFlags(in ParseState ctx, IInvocationO
switch (arg.Parameter?.Name)
{
case "sql":
if (TryGetConstantValueWithSyntax(arg, out string? s, out _))
if (TryGetConstantValueWithSyntax(arg, out string? s, out _, out _))
{
sql = s;
}
Expand Down
81 changes: 75 additions & 6 deletions src/Dapper.AOT.Analyzers/Internal/Inspection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
using Dapper.Internal.Roslyn;
using Dapper.SqlAnalysis;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
DeagleGross marked this conversation as resolved.
Show resolved Hide resolved
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Operations;
using System;
using System.Collections.Generic;
Expand Down Expand Up @@ -888,7 +890,7 @@ public static bool IsDapperMethod(this IInvocationOperation operation, out Opera
public static bool HasAll(this OperationFlags value, OperationFlags testFor) => (value & testFor) == testFor;

public static bool TryGetConstantValue<T>(IOperation op, out T? value)
=> TryGetConstantValueWithSyntax(op, out value, out _);
=> TryGetConstantValueWithSyntax(op, out value, out _, out _);

public static ITypeSymbol? GetResultType(this IInvocationOperation invocation, OperationFlags flags)
{
Expand All @@ -907,14 +909,15 @@ internal static bool CouldBeNullable(ITypeSymbol symbol) => symbol.IsValueType
? symbol.NullableAnnotation == NullableAnnotation.Annotated
: symbol.NullableAnnotation != NullableAnnotation.NotAnnotated;

public static bool TryGetConstantValueWithSyntax<T>(IOperation val, out T? value, out SyntaxNode? syntax)
public static bool TryGetConstantValueWithSyntax<T>(IOperation val, out T? value, out SyntaxNode? syntax, out SyntaxKind syntaxKind)
{
try
{
if (val.ConstantValue.HasValue)
{
value = (T?)val.ConstantValue.Value;
syntax = val.Syntax;
syntaxKind = syntax.GetKind();
return true;
}
if (val is IArgumentOperation arg)
Expand All @@ -932,22 +935,86 @@ public static bool TryGetConstantValueWithSyntax<T>(IOperation val, out T? value
{
value = (T?)field.Field.ConstantValue;
syntax = field.Field.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax();
syntaxKind = syntax!.GetKind();
return true;
}

// local constants
if (val is ILocalReferenceOperation local && local.Local.HasConstantValue)
if (val is ILocalReferenceOperation local)
{
value = (T?)local.Local.ConstantValue;
syntax = local.Local.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax();
return true;
if (local.Local.HasConstantValue)
{
value = (T?)local.Local.ConstantValue;
syntax = local.Local.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax();
syntaxKind = syntax!.GetKind();
return true;
}

var referenceSyntax = local.Local?.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax();
if (referenceSyntax is VariableDeclaratorSyntax varDeclSyntax)
{
var initializer = varDeclSyntax.Initializer?.Value;
if (initializer is not null)
{
if (initializer is InterpolatedStringExpressionSyntax)
{
value = default!;
syntax = null;
syntaxKind = SyntaxKind.InterpolatedStringExpression;
return false;
}

if (initializer is BinaryExpressionSyntax)
{
// since we are parsing `string` here, `BinaryExpression` stands for string concatenation
value = default!;
syntax = null;
syntaxKind = SyntaxKind.AddExpression;
return false;
}
}
}
}

if (val is IInterpolatedStringOperation)
{
value = default!;
syntax = null;
syntaxKind = SyntaxKind.InterpolatedStringExpression;
return false;
}

if (val is IBinaryOperation op)
{
// since we are parsing `string` here, `BinaryOperation` stands for string concatenation
value = default!;
syntax = null;
syntaxKind = SyntaxKind.AddExpression;
return false;
}

if (val is IInvocationOperation invocation)
{
// `string.Format()` special case
if (invocation.TargetMethod is {
Name: "Format",
ContainingType: { SpecialType: SpecialType.System_String },
ContainingNamespace: { Name: "System" }
})
{
value = default!;
syntax = null;
syntaxKind = SyntaxKind.AddExpression;
return false;
}
}

// other non-trivial default constant values
if (val.ConstantValue.HasValue)
{
value = (T?)val.ConstantValue.Value;
syntax = val.Syntax;
syntaxKind = syntax!.GetKind();
return true;
}

Expand All @@ -957,12 +1024,14 @@ public static bool TryGetConstantValueWithSyntax<T>(IOperation val, out T? value
// we already ruled out explicit constant above, so: must be default
value = default;
syntax = val.Syntax;
syntaxKind = syntax!.GetKind();
return true;
}
}
catch { }
value = default!;
syntax = null;
syntaxKind = SyntaxKind.None;
return false;
}

Expand Down
14 changes: 14 additions & 0 deletions src/Dapper.AOT.Analyzers/Internal/Roslyn/SyntaxNodeExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;

namespace Dapper.Internal.Roslyn
{
internal static class SyntaxNodeExtensions
{
public static SyntaxKind GetKind(this SyntaxNode syntaxNode)
{
if (syntaxNode is null) return SyntaxKind.None;
return syntaxNode.Kind();
}
}
}
53 changes: 53 additions & 0 deletions test/Dapper.AOT.Test/Verifiers/DAP241.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
using Dapper.CodeAnalysis;
using System.Threading.Tasks;
using Xunit;
using Diagnostics = Dapper.CodeAnalysis.DapperAnalyzer.Diagnostics;
namespace Dapper.AOT.Test.Verifiers;

public class DAP241 : Verifier<DapperAnalyzer>
DeagleGross marked this conversation as resolved.
Show resolved Hide resolved
{
[Fact]
public Task InterpolatedStringDetection_DirectUsage() => CSVerifyAsync(""""
using Dapper;
using System.Data;
using System.Data.Common;

[DapperAot]
class HasUnusedParameter
{
void SomeMethod(DbConnection connection)
{
int id = 1;
_ = connection.Query<int>({|#0:$"select Id from Customers where Id = {id}"|});
}
}
"""",
DefaultConfig,
[
Diagnostic(Diagnostics.InterpolatedStringSqlExpression).WithLocation(0),
]
);

[Fact]
public Task InterpolatedStringDetection_LocalVariableUsage() => CSVerifyAsync(""""
using Dapper;
using System.Data;
using System.Data.Common;

[DapperAot]
class HasUnusedParameter
{
void SomeMethod(DbConnection connection)
{
int id = 1;
var sqlQuery = $"select Id from Customers where Id = {id}";
_ = connection.Query<int>({|#0:sqlQuery|});
}
}
"""",
DefaultConfig,
[
Diagnostic(Diagnostics.InterpolatedStringSqlExpression).WithLocation(0),
]
);
}
56 changes: 56 additions & 0 deletions test/Dapper.AOT.Test/Verifiers/DAP242.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
using Dapper.CodeAnalysis;
using Microsoft.CodeAnalysis.Testing;
using System.Threading.Tasks;
using Xunit;
using Diagnostics = Dapper.CodeAnalysis.DapperAnalyzer.Diagnostics;
namespace Dapper.AOT.Test.Verifiers;

public class DAP242 : Verifier<DapperAnalyzer>
{
[Fact]
public Task ConcatenatedStringDetection_DirectUsage_PlusInt() => Test("""
int id = 1;
_ = connection.Query<int>({|#0:"select Id from Customers where Id = " + id|});
""", Diagnostic(Diagnostics.ConcatenatedStringSqlExpression).WithLocation(0));

[Fact]
public Task ConcatenatedStringDetection_DirectUsage_PlusString() => Test("""
string id = "1";
_ = connection.Query<int>({|#0:"select Id from Customers where Id = " + id|});
""", Diagnostic(Diagnostics.ConcatenatedStringSqlExpression).WithLocation(0));

[Fact]
public Task ConcatenatedStringDetection_DirectUsage_MultiVariablesConcat() => Test("""
int id = 1;
string name = "dima";
_ = connection.Query<int>({|#0:"select Id from Customers where Id = " + id + " and Name = " + name|});
""", Diagnostic(Diagnostics.ConcatenatedStringSqlExpression).WithLocation(0));

[Fact]
public Task ConcatenatedStringDetection_LocalVariableUsage() => Test("""
int id = 1;
var sqlQuery = "select Id from Customers where Id = " + id;
_ = connection.Query<int>({|#0:sqlQuery|});
""", Diagnostic(Diagnostics.ConcatenatedStringSqlExpression).WithLocation(0));

[Fact]
public Task ConcatenatedStringDetection_StringFormat() => Test("""
int id = 1;
_ = connection.Query<int>({|#0:string.Format("select Id from Customers where Id = {0}", id)|});
""", Diagnostic(Diagnostics.ConcatenatedStringSqlExpression).WithLocation(0));

private Task Test(string methodCode, params DiagnosticResult[] expected) => CSVerifyAsync($$"""
using Dapper;
using System.Data;
using System.Data.Common;

[DapperAot]
class HasUnusedParameter
{
void SomeMethod(DbConnection connection)
{
{{methodCode}}
}
}
""", DefaultConfig, expected);
}
Loading