Skip to content

Fix S2857 FP: Rule is not checking SQL keywords in const interepolated string #8966

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.

Sign up for GitHub

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

Merged
costin-zaharia-sonarsource merged 2 commits into from
Mar 21, 2024
Merged

Fix S2857 FP: Rule is not checking SQL keywords in const interepolated string #8966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a1a2f3
Fix S2857 FP: Rule is not checking SQL keywords in const interepolate…
gregory-paidis-sonarsource Mar 21, 2024
fe1937c
Some naming changes
gregory-paidis-sonarsource Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 36 additions & 33 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/SqlKeywordsDelimitedBySpace.cs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Copy link
Contributor Author

@gregory-paidis-sonarsource gregory-paidis-sonarsource Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these do not contain SQL keywords, so now they are correctly compliant.

}
}
}
3 changes: 2 additions & 1 deletion analyzers/tests/SonarAnalyzer.Test/TestCases/SqlKeywordsDelimitedBySpace.cs
View file
Open in desktop
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