Skip to content

Commit

Permalink
Fix S2857 FP: Rule is not checking SQL keywords in const interepolate…
Browse files Browse the repository at this point in the history 
…d string (#8966)
  • Loading branch information
@gregory-paidis-sonarsource
gregory-paidis-sonarsource committed Mar 21, 2024
1 parent 8bf481c commit 0400e52
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 59 deletions.
69 changes: 36 additions & 33 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/SqlKeywordsDelimitedBySpace.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,11 @@ public StringConcatenationWalker(SonarSyntaxNodeReportingContext context) =>

public override void VisitInterpolatedStringExpression(InterpolatedStringExpressionSyntax node)
{
if (TryGetConstantValuesOfInterpolatedStringExpression(node, out var stringParts))
if (TryGetConstantValues(node, out var stringParts)
&& stringParts.Count > 0
&& StartsWithSqlKeyword(stringParts[0].Text.Trim()))
{
RaiseIssueIfSpaceDoesNotExistBetweenStrings(stringParts);
RaiseIssueIfNotDelimited(stringParts);
}
base.VisitInterpolatedStringExpression(node);
}
Expand All @@ -140,25 +142,17 @@ public override void VisitBinaryExpression(BinaryExpressionSyntax node)
&& TryGetStringWrapper(node.Right, out var rightSide)
&& StartsWithSqlKeyword(leftSide.Text.Trim()))
{
var strings = new List<StringWrapper>();
strings.Add(leftSide);
strings.Add(rightSide);
var onlyStringsInConcatenation = AddStringsToList(node, strings);
if (!onlyStringsInConcatenation)
var strings = new List<StringWrapper> { leftSide, rightSide };
if (TryExtractNestedStrings(node, strings))
{
return;
RaiseIssueIfNotDelimited(strings);
}

RaiseIssueIfSpaceDoesNotExistBetweenStrings(strings);
}
else
{
Visit(node.Left);
Visit(node.Right);
}
Visit(node.Left);
Visit(node.Right);
}

private void RaiseIssueIfSpaceDoesNotExistBetweenStrings(List<StringWrapper> stringWrappers)
private void RaiseIssueIfNotDelimited(List<StringWrapper> stringWrappers)
{
for (var i = 0; i < stringWrappers.Count - 1; i++)
{
Expand All @@ -175,31 +169,40 @@ private void RaiseIssueIfSpaceDoesNotExistBetweenStrings(List<StringWrapper> str
}
}

private static bool TryGetStringWrapper(ExpressionSyntax expression, out StringWrapper stringWrapper)
private bool TryGetStringWrapper(ExpressionSyntax expression, out StringWrapper stringWrapper)
{
if (expression is LiteralExpressionSyntax literal && literal.IsKind(SyntaxKind.StringLiteralExpression))
{
stringWrapper = new StringWrapper(literal, literal.Token.ValueText);
return true;
}

if (expression is InterpolatedStringExpressionSyntax interpolatedString)
else if (expression is InterpolatedStringExpressionSyntax interpolatedString)
{
stringWrapper = new StringWrapper(interpolatedString, interpolatedString.GetContentsText());
return true;
}

stringWrapper = null;
return false;
// if this is a nested binary, we skip it so that we can raise when we visit it.
// Otherwise, FindConstantValue will merge it into one value.
else if (expression.RemoveParentheses() is not BinaryExpressionSyntax
&& expression.FindConstantValue(context.SemanticModel) is string constantValue)
{
stringWrapper = new StringWrapper(expression, constantValue);
return true;
}
else
{
stringWrapper = null;
return false;
}
}

/**
* Returns
* - true if all the found elements are string literals.
* - false if, inside the chain of binary expressions, some elements are not string literals or
* - true if all the found elements have constant string value.
* - false if, inside the chain of binary expressions, some element's value cannot be computed or
* some binary expressions are not additions.
*/
private static bool AddStringsToList(BinaryExpressionSyntax node, List<StringWrapper> strings)
private bool TryExtractNestedStrings(BinaryExpressionSyntax node, List<StringWrapper> strings)
{
// this is the left-most node of a concatenation chain
// collect all string literals
Expand All @@ -213,25 +216,25 @@ private static bool AddStringsToList(BinaryExpressionSyntax node, List<StringWra
}
else
{
// we are in a binary expression, but it's not only of strings or not only concatenations
// we are in a binary expression, but it's not only of constants or concatenations
return false;
}
parent = parent.Parent;
}
return true;
}

private bool TryGetConstantValuesOfInterpolatedStringExpression(InterpolatedStringExpressionSyntax interpolatedStringExpression, out List<StringWrapper> parts)
private bool TryGetConstantValues(InterpolatedStringExpressionSyntax interpolatedStringExpression, out List<StringWrapper> parts)
{
parts = new List<StringWrapper>();
foreach (var interpolatedStringContent in interpolatedStringExpression.Contents)
parts = [];
foreach (var content in interpolatedStringExpression.Contents)
{
if (interpolatedStringContent is InterpolationSyntax interpolation
if (content is InterpolationSyntax interpolation
&& interpolation.Expression.FindConstantValue(context.SemanticModel) is string constantValue)
{
parts.Add(new StringWrapper(interpolatedStringContent, constantValue));
parts.Add(new StringWrapper(content, constantValue));
}
else if (interpolatedStringContent is InterpolatedStringTextSyntax interpolatedText)
else if (content is InterpolatedStringTextSyntax interpolatedText)
{
parts.Add(new StringWrapper(interpolatedText, interpolatedText.TextToken.Text));
}
Expand All @@ -258,7 +261,7 @@ private static bool IsInvalidCombination(char first, char second)

return IsAlphaNumericOrInvalidCharacters(first) && IsAlphaNumericOrInvalidCharacters(second);

bool IsAlphaNumericOrInvalidCharacters(char c) =>
static bool IsAlphaNumericOrInvalidCharacters(char c) =>
char.IsLetterOrDigit(c) || InvalidCharacters.Contains(c);
}
}
Expand Down
23 changes: 12 additions & 11 deletions analyzers/tests/SonarAnalyzer.Test/Rules/SqlKeywordsDelimitedBySpaceTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,57 +25,58 @@ namespace SonarAnalyzer.Test.Rules
[TestClass]
public class SqlKeywordsDelimitedBySpaceTest
{
private readonly VerifierBuilder builder = new VerifierBuilder<SqlKeywordsDelimitedBySpace>().AddReferences(NuGetMetadataReference.SystemDataSqlClient());
private static readonly VerifierBuilder Builder = new VerifierBuilder<SqlKeywordsDelimitedBySpace>()
.AddReferences(NuGetMetadataReference.SystemDataSqlClient());

[TestMethod]
public void SqlKeywordsDelimitedBySpace_Csharp8() =>
builder.AddPaths("SqlKeywordsDelimitedBySpace.cs")
Builder.AddPaths("SqlKeywordsDelimitedBySpace.cs")
.WithOptions(ParseOptionsHelper.FromCSharp8)
.Verify();

[TestMethod]
public void SqlKeywordsDelimitedBySpace_UsingInsideNamespace() =>
builder.AddPaths("SqlKeywordsDelimitedBySpace_InsideNamespace.cs")
Builder.AddPaths("SqlKeywordsDelimitedBySpace_InsideNamespace.cs")
.WithConcurrentAnalysis(false)
.Verify();

[TestMethod]
public void SqlKeywordsDelimitedBySpace_DefaultNamespace() =>
builder.AddPaths("SqlKeywordsDelimitedBySpace_DefaultNamespace.cs")
Builder.AddPaths("SqlKeywordsDelimitedBySpace_DefaultNamespace.cs")
.AddTestReference()
.VerifyNoIssueReported();

#if NET

[TestMethod]
public void SqlKeywordsDelimitedBySpace_CSharp10_GlobalUsings() =>
builder.AddPaths("SqlKeywordsDelimitedBySpace.CSharp10.GlobalUsing.cs", "SqlKeywordsDelimitedBySpace.CSharp10.GlobalUsingConsumer.cs")
Builder.AddPaths("SqlKeywordsDelimitedBySpace.CSharp10.GlobalUsing.cs", "SqlKeywordsDelimitedBySpace.CSharp10.GlobalUsingConsumer.cs")
.WithOptions(ParseOptionsHelper.FromCSharp10)
.WithConcurrentAnalysis(false)
.Verify();

[TestMethod]
public void SqlKeywordsDelimitedBySpace_CSharp10_FileScopesNamespace() =>
builder.AddPaths("SqlKeywordsDelimitedBySpace.CSharp10.FileScopedNamespaceDeclaration.cs")
Builder.AddPaths("SqlKeywordsDelimitedBySpace.CSharp10.FileScopedNamespaceDeclaration.cs")
.WithOptions(ParseOptionsHelper.FromCSharp10)
.WithConcurrentAnalysis(false)
.Verify();

[TestMethod]
public void SqlKeywordsDelimitedBySpace_CSharp10() =>
builder.AddPaths("SqlKeywordsDelimitedBySpace.CSharp10.cs")
Builder.AddPaths("SqlKeywordsDelimitedBySpace.CSharp10.cs")
.WithOptions(ParseOptionsHelper.FromCSharp10)
.Verify();

[TestMethod]
public void SqlKeywordsDelimitedBySpace_CSharp11() =>
builder.AddPaths("SqlKeywordsDelimitedBySpace.CSharp11.cs")
Builder.AddPaths("SqlKeywordsDelimitedBySpace.CSharp11.cs")
.WithOptions(ParseOptionsHelper.FromCSharp11)
.Verify();

[TestMethod]
public void SqlKeywordsDelimitedBySpace_CSharp12() =>
builder.AddPaths("SqlKeywordsDelimitedBySpace.CSharp12.cs")
Builder.AddPaths("SqlKeywordsDelimitedBySpace.CSharp12.cs")
.WithOptions(ParseOptionsHelper.FromCSharp12)
.WithConcurrentAnalysis(false)
.Verify();
Expand All @@ -95,7 +96,7 @@ public void SqlKeywordsDelimitedBySpace_CSharp12() =>
[DataRow("PetaPoco")]
[DataTestMethod]
public void SqlKeywordsDelimitedBySpace_DotnetFramework(string sqlNamespace) =>
builder
Builder
.AddReferences(MetadataReferenceFacade.SystemData)
.AddReferences(NuGetMetadataReference.Dapper())
.AddReferences(NuGetMetadataReference.EntityFramework())
Expand Down Expand Up @@ -124,7 +125,7 @@ public class Test
[DataRow("ServiceStack.OrmLite")]
[DataTestMethod]
public void SqlKeywordsDelimitedBySpace_DotnetCore(string sqlNamespace) =>
builder
Builder
.AddReferences(MetadataReferenceFacade.SystemData)
.AddReferences(NuGetMetadataReference.MicrosoftEntityFrameworkCore("2.2.0"))
.AddReferences(NuGetMetadataReference.ServiceStackOrmLite())
Expand Down
52 changes: 38 additions & 14 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/SqlKeywordsDelimitedBySpace.CSharp10.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,32 @@ public void Method()
}
}

public class Interpolation
{
public const string select = "select"; // Compliant
public const string truncate = $"{nameof(truncate)}"; // Compliant
public const string @from = "from"; // Compliant

public const string s1 = $"{select}"; // Compliant
public const string s2 = $"{select}Name"; // Noncompliant
public const string s3 = $"select Name {"from"}"; // Compliant
public const string s4 = $"select Name {"from"}"; // Compliant
public const string s5 = $"{select} col1{@from}"; // Noncompliant {{Add a space before 'from'.}}
// ^^^^^^^
public const string s6 = $"{select} col1{{{@from}}}";
// ^^ {{Add a space before '}}'.}}
// ^^^^^^^@-1 {{Add a space before 'from'.}}
public const string s7 = $"{select} col1{{{@from}}}";
// ^^^^^^^ {{Add a space before 'from'.}}
// ^^@-1 {{Add a space before '}}'.}}

public const string s8 = truncate + $"col1{@from}"; // here we have an FN on col1 + @from, because of the mixed binary/interpolation scenario
// ^^^^^^^^^^^^^^ {{Add a space before 'col1{@from}'.}}

public const string s9 = truncate + $"table"; // Noncompliant
// ^^^^^^^^
}

// https://github.com/SonarSource/sonar-dotnet/issues/6355
public class Repro_6355
{
Expand All @@ -82,34 +108,32 @@ void Example(string parameter)
string empty = string.Empty;

const string s0 = $"{constOne}"; // Compliant
const string s1 = $"{constOne}Two"; // Noncompliant FP
const string s2 = $"{nameof(constOne)}Two"; // Noncompliant FP
const string s1 = $"{constOne}Two"; // Compliant
const string s2 = $"{nameof(constOne)}Two"; // Compliant
const string s3 = $"{parameter}"; // Error [CS0133]
const string s4 = $"{parameter}Two"; // Error [CS0133]
const string s5 = $"{nameof(parameter)}Two"; // Noncompliant FP
const string s5 = $"{nameof(parameter)}Two"; // Compliant
const string s6 = $"{nonConstOne}"; // Error [CS0133]
const string s7 = $"{nonConstOne}Two"; // Error [CS0133]
// Noncompliant@-1 FP
const string s8 = $"{nameof(nonConstOne)}Two"; // Noncompliant FP
const string s8 = $"{nameof(nonConstOne)}Two"; // Compliant
const string s9 = $"{empty}"; // Error [CS0133]
const string s10 = $"{empty}Two"; // Error [CS0133]
const string s11 = $"{nameof(empty)}Two"; // Noncompliant FP
const string s11 = $"{nameof(empty)}Two"; // Compliant

string s12 = $"{constOne}"; // Compliant
string s13 = $"{constOne}Two"; // Noncompliant FP
string s14 = $"{nameof(constOne)}Two"; // Noncompliant FP
string s13 = $"{constOne}Two"; // Compliant
string s14 = $"{nameof(constOne)}Two"; // Compliant
string s15 = $"{parameter}"; // Compliant
string s16 = $"{parameter}Two"; // Compliant
string s17 = $"{nameof(parameter)}Two"; // Noncompliant FP
string s17 = $"{nameof(parameter)}Two"; // Compliant
string s18 = $"{nonConstOne}"; // Compliant
string s19 = $"{nonConstOne}Two"; // Noncompliant FP
string s20 = $"{nameof(nonConstOne)}Two"; // Noncompliant FP
string s19 = $"{nonConstOne}Two"; // Compliant
string s20 = $"{nameof(nonConstOne)}Two"; // Compliant
string s21 = $"{empty}"; // Compliant
string s22 = $"{empty}Two"; // Compliant
string s23 = $"{nameof(empty)}Two"; // Noncompliant FP
string s23 = $"{nameof(empty)}Two"; // Compliant

string s24 = $"{{{nonConstOne}}}"; // Noncompliant FP
// Noncompliant@-1 FP
string s24 = $"{{{nonConstOne}}}"; // Compliant
}
}
}
3 changes: 2 additions & 1 deletion analyzers/tests/SonarAnalyzer.Test/TestCases/SqlKeywordsDelimitedBySpace.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,8 @@ void ValidCases(string parameter)
var parantheses = "SELECT (" +
"x.y,z.z)" + // FN - we ignore parantheses as they can lead to FPs
"FROM table;";
var parantheses2 = "SELECT " + ("all") + "FROM table"; // FN
var parantheses2 = "SELECT " + ("all") + "FROM table"; // Noncompliant {{Add a space before 'FROM'.}}
// ^^^^^^^^^^^^
}

public void InterpolatedAndRawStringsAreIgnored(string col1, string col2, string innerQuery)
Expand Down

0 comments on commit 0400e52

Please sign in to comment.