diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/SqlKeywordsDelimitedBySpace.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/SqlKeywordsDelimitedBySpace.cs index 34f07f06994..5c5a1ffaa93 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/SqlKeywordsDelimitedBySpace.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/SqlKeywordsDelimitedBySpace.cs @@ -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); } @@ -140,25 +142,17 @@ public override void VisitBinaryExpression(BinaryExpressionSyntax node) && TryGetStringWrapper(node.Right, out var rightSide) && StartsWithSqlKeyword(leftSide.Text.Trim())) { - var strings = new List(); - strings.Add(leftSide); - strings.Add(rightSide); - var onlyStringsInConcatenation = AddStringsToList(node, strings); - if (!onlyStringsInConcatenation) + var strings = new List { 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 stringWrappers) + private void RaiseIssueIfNotDelimited(List stringWrappers) { for (var i = 0; i < stringWrappers.Count - 1; i++) { @@ -175,31 +169,40 @@ private void RaiseIssueIfSpaceDoesNotExistBetweenStrings(List 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 strings) + private bool TryExtractNestedStrings(BinaryExpressionSyntax node, List strings) { // this is the left-most node of a concatenation chain // collect all string literals @@ -213,7 +216,7 @@ private static bool AddStringsToList(BinaryExpressionSyntax node, List parts) + private bool TryGetConstantValues(InterpolatedStringExpressionSyntax interpolatedStringExpression, out List parts) { - parts = new List(); - 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)); } @@ -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); } } diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/SqlKeywordsDelimitedBySpaceTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/SqlKeywordsDelimitedBySpaceTest.cs index b432438fce3..d821d95ab82 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Rules/SqlKeywordsDelimitedBySpaceTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/SqlKeywordsDelimitedBySpaceTest.cs @@ -25,23 +25,24 @@ namespace SonarAnalyzer.Test.Rules [TestClass] public class SqlKeywordsDelimitedBySpaceTest { - private readonly VerifierBuilder builder = new VerifierBuilder().AddReferences(NuGetMetadataReference.SystemDataSqlClient()); + private static readonly VerifierBuilder Builder = new VerifierBuilder() + .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(); @@ -49,33 +50,33 @@ public void SqlKeywordsDelimitedBySpace_DefaultNamespace() => [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(); @@ -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()) @@ -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()) diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/SqlKeywordsDelimitedBySpace.CSharp10.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/SqlKeywordsDelimitedBySpace.CSharp10.cs index 6a8619af276..14ab56b1f4d 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/SqlKeywordsDelimitedBySpace.CSharp10.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/SqlKeywordsDelimitedBySpace.CSharp10.cs @@ -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 { @@ -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 } } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/SqlKeywordsDelimitedBySpace.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/SqlKeywordsDelimitedBySpace.cs index 71ea838dcc3..0979dd1e9ba 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/SqlKeywordsDelimitedBySpace.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/SqlKeywordsDelimitedBySpace.cs @@ -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)