Fix S3247 FP: Rule should not trigger when variable is used for data …

…binding on Razor/MVC views (#8973)
SonarSource · Mar 22, 2024 · 8a64975 · 8a64975
1 parent b993838
commit 8a64975
Show file tree

Hide file tree

Showing 5 changed files with 131 additions and 16 deletions.
diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/CastShouldNotBeDuplicated.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/CastShouldNotBeDuplicated.cs
@@ -98,21 +98,23 @@ private static void IsExpression(SonarSyntaxNodeReportingContext analysisContext
             ReportPatternAtMainVariable(analysisContext, isExpression.Left, isExpression.GetLocation(), parentIfStatement.Statement, castType, ReplaceWithAsAndNullCheckMessage);
         }
 
-        private static List<Location> GetDuplicatedCastLocations(SonarSyntaxNodeReportingContext analysisContext, SyntaxNode parentStatement, TypeSyntax castType, SyntaxNode typedVariable)
+        private static List<Location> GetDuplicatedCastLocations(SonarSyntaxNodeReportingContext context, SyntaxNode parentStatement, TypeSyntax castType, SyntaxNode typedVariable)
         {
-            var typeExpressionSymbol = analysisContext.SemanticModel.GetSymbolInfo(typedVariable).Symbol
-                                       ?? analysisContext.SemanticModel.GetDeclaredSymbol(typedVariable);
-            return typeExpressionSymbol == null
-                ? new List<Location>()
+            var typeExpressionSymbol = context.SemanticModel.GetSymbolInfo(typedVariable).Symbol
+                                       ?? context.SemanticModel.GetDeclaredSymbol(typedVariable);
+
+            return typeExpressionSymbol is null
+                ? []
                 : parentStatement
                     .DescendantNodes()
                     .OfType<CastExpressionSyntax>()
                     .Where(x => x.Type.WithoutTrivia().IsEquivalentTo(castType.WithoutTrivia())
-                                && IsCastOnSameSymbol(x))
+                                && IsCastOnSameSymbol(x)
+                                && !CSharpFacade.Instance.Syntax.IsInExpressionTree(context.SemanticModel, x)) // see https://github.com/SonarSource/sonar-dotnet/issues/8735#issuecomment-1943419398
                     .Select(x => x.GetLocation()).ToList();
 
             bool IsCastOnSameSymbol(CastExpressionSyntax castExpression) =>
-                Equals(analysisContext.SemanticModel.GetSymbolInfo(castExpression.Expression).Symbol, typeExpressionSymbol);
+                Equals(context.SemanticModel.GetSymbolInfo(castExpression.Expression).Symbol, typeExpressionSymbol);
         }
 
         private static void ProcessPatternExpression(SonarSyntaxNodeReportingContext analysisContext,

diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/CastShouldNotBeDuplicatedTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/CastShouldNotBeDuplicatedTest.cs
@@ -25,38 +25,53 @@ namespace SonarAnalyzer.Test.Rules
     [TestClass]
     public class CastShouldNotBeDuplicatedTest
     {
-        private readonly VerifierBuilder builder = new VerifierBuilder<CastShouldNotBeDuplicated>();
+        private static readonly VerifierBuilder Builder = new VerifierBuilder<CastShouldNotBeDuplicated>();
+        public TestContext TestContext { get; set; }
 
         [TestMethod]
         public void CastShouldNotBeDuplicated() =>
-            builder.AddPaths("CastShouldNotBeDuplicated.cs").Verify();
+            Builder.AddPaths("CastShouldNotBeDuplicated.cs").Verify();
 
 #if NET
 
         [TestMethod]
         public void CastShouldNotBeDuplicated_CSharp9() =>
-            builder.AddPaths("CastShouldNotBeDuplicated.CSharp9.cs")
+            Builder.AddPaths("CastShouldNotBeDuplicated.CSharp9.cs")
                 .WithOptions(ParseOptionsHelper.FromCSharp9)
                 .Verify();
 
         [TestMethod]
         public void CastShouldNotBeDuplicated_CSharp10() =>
-            builder.AddPaths("CastShouldNotBeDuplicated.CSharp10.cs")
+            Builder.AddPaths("CastShouldNotBeDuplicated.CSharp10.cs")
                 .WithOptions(ParseOptionsHelper.FromCSharp10)
                 .Verify();
 
         [TestMethod]
         public void CastShouldNotBeDuplicated_CSharp11() =>
-            builder.AddPaths("CastShouldNotBeDuplicated.CSharp11.cs")
+            Builder.AddPaths("CastShouldNotBeDuplicated.CSharp11.cs")
                 .WithOptions(ParseOptionsHelper.FromCSharp11)
                 .Verify();
 
         [TestMethod]
         public void CastShouldNotBeDuplicated_CSharp12() =>
-            builder.AddPaths("CastShouldNotBeDuplicated.CSharp12.cs")
+            Builder.AddPaths("CastShouldNotBeDuplicated.CSharp12.cs")
                 .WithOptions(ParseOptionsHelper.FromCSharp12)
                 .Verify();
 
+        [TestMethod]
+        public void CastShouldNotBeDuplicated_MvcView() =>
+            Builder
+            .AddSnippet("""
+                public class Base {}
+                public class Derived: Base
+                {
+                    public int Prop { get; set; }
+                }
+                """)
+            .AddPaths("CastShouldNotBeDuplicated.cshtml")
+            .WithAdditionalFilePath(AnalysisScaffolding.CreateSonarProjectConfig(TestContext, ProjectType.Product))
+            .Verify();
+
 #endif
 
     }

diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/CastShouldNotBeDuplicated.cshtml b/analyzers/tests/SonarAnalyzer.Test/TestCases/CastShouldNotBeDuplicated.cshtml
@@ -0,0 +1,11 @@
+@* the next line is necessary for the razor generator to properly interpret the asp-for attribute *@
+@addTagHelper *, Microsoft.AspNetCore.Mvc.TagHelpers
+@model Base
+
+@if (Model is Derived) // Compliant
+{
+    <div>
+        <input class="form-control" type="text" asp-for="@(((Derived)Model).Prop)" /> @* // Compliant *@
+        <span asp-validation-for="@(((Derived)Model).Prop)" class="text-danger"></span> @* // Compliant *@
+    </div>
+}
diff --git a/analyzers/tests/SonarAnalyzer.TestFramework.Test/Verification/VerifierTest.cs b/analyzers/tests/SonarAnalyzer.TestFramework.Test/Verification/VerifierTest.cs
@@ -24,6 +24,7 @@
 using SonarAnalyzer.Rules.CSharp;
 using SonarAnalyzer.SymbolicExecution.Sonar.Analyzers;
 using SonarAnalyzer.TestFramework.Verification;
+using static SonarAnalyzer.TestFramework.Verification.Verifier;
 
 namespace SonarAnalyzer.Test.TestFramework.Tests.Verification;
 
@@ -261,6 +262,82 @@ public void Compile_Razor_NoReferences()
         references.Should().NotContain(x => x.Display.Contains("Microsoft.Azure.DocumentDB"));
     }
 
+    [TestMethod]
+    public void Compile_Razor_Snippet_NoName()
+    {
+        var compilation = DummyWithLocation
+            .AddPaths("Dummy.cshtml")
+            .AddSnippet("class Sample {}")
+            .WithLanguageVersion(LanguageVersion.Latest)
+            .Build()
+            .Compile(false)
+            .Single();
+
+        compilation.AdditionalSourceFiles.Should().ContainSingle();
+        ContainsAdditionalSourceFileWithName(compilation, "Dummy.cshtml");
+        ContainsSyntaxTreeWithName(compilation, "snippet.0.cs");
+    }
+
+    [TestMethod]
+    public void Compile_Razor_Snippet_Name_CSharp()
+    {
+        var compilation = DummyWithLocation
+            .AddPaths("Dummy.cshtml")
+            .AddSnippet("class Sample {}", "snippet.cs")
+            .WithLanguageVersion(LanguageVersion.Latest)
+            .Build()
+            .Compile(false)
+            .Single();
+
+        compilation.AdditionalSourceFiles.Should().ContainSingle();
+        ContainsAdditionalSourceFileWithName(compilation, "Dummy.cshtml");
+        ContainsSyntaxTreeWithName(compilation, "snippet.cs");
+    }
+
+    [TestMethod]
+    public void Compile_Razor_Snippet_Name_Cshtml()
+    {
+        var compilation = DummyWithLocation
+            .AddPaths("Dummy.cshtml")
+            .AddSnippet("class Sample {}", "snippet.cshtml")
+            .WithLanguageVersion(LanguageVersion.Latest)
+            .Build()
+            .Compile(false)
+            .Single();
+
+        compilation.AdditionalSourceFiles.Should().HaveCount(2);
+
+        ContainsAdditionalSourceFileWithName(compilation, "Dummy.cshtml");
+        ContainsAdditionalSourceFileWithName(compilation, "snippet.cshtml");
+
+        ContainsSyntaxTreeWithName(compilation, "snippet_cshtml.g.cs");
+    }
+
+    [TestMethod]
+    public void Compile_Razor_Snippet_MixedNames()
+    {
+        var compilation = DummyWithLocation
+            .AddPaths("Dummy.cshtml")
+            .AddSnippet("class Sample {}")                     // No name, .0.cs
+            .AddSnippet("class Sample {}")                     // No name, .1.cs
+            .AddSnippet("class Sample {}", "snippet.cs")       // Named, c#
+            .AddSnippet("class Sample {}", "snippet.cshtml")   // Named, c#
+            .WithLanguageVersion(LanguageVersion.Latest)
+            .Build()
+            .Compile(false)
+            .Single();
+
+        compilation.AdditionalSourceFiles.Should().HaveCount(2);
+
+        ContainsAdditionalSourceFileWithName(compilation, "Dummy.cshtml");
+        ContainsAdditionalSourceFileWithName(compilation, "snippet.cshtml");
+
+        ContainsSyntaxTreeWithName(compilation, "snippet.0.cs");
+        ContainsSyntaxTreeWithName(compilation, "snippet.1.cs");
+        ContainsSyntaxTreeWithName(compilation, "snippet.cs");
+        ContainsSyntaxTreeWithName(compilation, "snippet_cshtml.g.cs");
+    }
+
 #endif
 
     [TestMethod]
@@ -757,4 +834,10 @@ public void VerifyUtilityAnalyzer_VerifyProtobuf_PropagateFailedAssertion_VB()
 
     private string WriteFile(string name, string content) =>
         TestHelper.WriteFile(TestContext, $@"TestCases\{name}", content);
+
+    private static void ContainsAdditionalSourceFileWithName(CompilationData compilation, string suffix) =>
+        compilation.AdditionalSourceFiles.Should().ContainSingle(x => x.EndsWith(suffix));
+
+    private static void ContainsSyntaxTreeWithName(CompilationData compilation, string suffix) =>
+        compilation.Compilation.SyntaxTrees.Select(x => x.FilePath).Should().Contain(x => x.EndsWith(suffix));
 }
diff --git a/analyzers/tests/SonarAnalyzer.TestFramework/Verification/Verifier.cs b/analyzers/tests/SonarAnalyzer.TestFramework/Verification/Verifier.cs
@@ -244,6 +244,7 @@ private string PrepareRazorProject(string projectRoot, string langVersion)
     private List<string> PrepareRazorFiles(string projectRoot)
     {
         var razorFiles = new List<string>();
+        var snippetCount = 0;
         // To improve: Paths are currently relative to entry assembly => needs to be duplicated in different projects for now.
         foreach (var file in builder.Paths.Select(TestCasePath))
         {
@@ -254,11 +255,14 @@ private List<string> PrepareRazorFiles(string projectRoot)
                 razorFiles.Add(filePath);
             }
         }
-        foreach (var snippet in builder.Snippets.Where(x => IsRazorOrCshtml(x.FileName)))
+        foreach (var snippet in builder.Snippets)
         {
-            var filePath = Path.Combine(projectRoot, snippet.FileName);
+            var filePath = Path.Combine(projectRoot, snippet.FileName ?? $"snippet.{snippetCount++}{language.FileExtension}");
             File.WriteAllText(filePath, snippet.Content);
-            razorFiles.Add(filePath);
+            if (IsRazorOrCshtml(filePath))
+            {
+                razorFiles.Add(filePath);
+            }
         }
         return razorFiles;
     }