Fix S1123 FN: The explanation should not be null or whitespace (#6890)

SonarSource · Mar 23, 2023 · f3c2ea1 · f3c2ea1
1 parent 8abd098
commit f3c2ea1
Show file tree

Hide file tree

Showing 8 changed files with 151 additions and 22 deletions.
diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/ObsoleteAttributes.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/ObsoleteAttributes.cs
@@ -24,4 +24,10 @@ namespace SonarAnalyzer.Rules.CSharp;
 public sealed class ObsoleteAttributes : ObsoleteAttributesBase<SyntaxKind>
 {
     protected override ILanguageFacade<SyntaxKind> Language => CSharpFacade.Instance;
+
+    protected override SyntaxNode GetExplanationExpression(SyntaxNode node) =>
+        node is AttributeSyntax { ArgumentList.Arguments: { Count: >= 1 } arguments }
+            && arguments[0] is { Expression: { } expression }
+                ? expression
+                : null;
 }
diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/ObsoleteAttributesBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/ObsoleteAttributesBase.cs
@@ -31,6 +31,8 @@ public abstract class ObsoleteAttributesBase<TSyntaxKind> : SonarDiagnosticAnaly
 
     protected abstract ILanguageFacade<TSyntaxKind> Language { get; }
 
+    protected abstract SyntaxNode GetExplanationExpression(SyntaxNode node);
+
     public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; }
 
     internal DiagnosticDescriptor ExplanationNeededRule { get; }
@@ -54,11 +56,15 @@ protected sealed override void Initialize(SonarAnalysisContext context) =>
                     var location = c.Node.GetLocation();
                     c.ReportIssue(Diagnostic.Create(RemoveRule, location));
 
-                    if (!attribute.GetParameters().Any())
+                    if (NoExplanation(c.Node, c.SemanticModel))
                     {
                         c.ReportIssue(Diagnostic.Create(ExplanationNeededRule, location));
                     }
                 }
             },
             Language.SyntaxKind.Attribute);
+
+    private bool NoExplanation(SyntaxNode node, SemanticModel model) =>
+        GetExplanationExpression(node) is not { } justification
+        || string.IsNullOrWhiteSpace(Language.FindConstantValue(model, justification) as string);
 }
diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/ObsoleteAttributes.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/ObsoleteAttributes.cs
@@ -24,4 +24,10 @@ namespace SonarAnalyzer.Rules.VisualBasic;
 public sealed class ObsoleteAttributes : ObsoleteAttributesBase<SyntaxKind>
 {
     protected override ILanguageFacade<SyntaxKind> Language => VisualBasicFacade.Instance;
+
+    protected override SyntaxNode GetExplanationExpression(SyntaxNode node) =>
+       node is AttributeSyntax { ArgumentList.Arguments: { Count: >= 1 } arguments }
+           && arguments[0] is SimpleArgumentSyntax { Expression: { } } argument
+               ? argument.Expression
+               : null;
 }
diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/ObsoleteAttributesTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/ObsoleteAttributesTest.cs
@@ -56,6 +56,11 @@ public void ObsoleteAttributesNeedExplanation_CSharp9() =>
     [TestMethod]
     public void ObsoleteAttributesNeedExplanation_CSharp10() =>
         explanationNeededCS.AddPaths("ObsoleteAttributesNeedExplanation.CSharp10.cs").WithOptions(ParseOptionsHelper.FromCSharp10).Verify();
+
+    [TestMethod]
+    public void ObsoleteAttributesNeedExplanation_VB14() =>
+        explanationNeededVB.AddPaths("ObsoleteAttributesNeedExplanation.VB14.vb").WithOptions(ParseOptionsHelper.FromVisualBasic14).Verify();
+
 #endif
 
     [TestMethod]

diff --git a/...yzers/tests/SonarAnalyzer.UnitTest/TestCases/ObsoleteAttributesNeedExplanation.CSharp9.cs b/...yzers/tests/SonarAnalyzer.UnitTest/TestCases/ObsoleteAttributesNeedExplanation.CSharp9.cs
@@ -18,3 +18,28 @@ static void LocalMethod()
         { }
     }
 }
+
+class Noncompliant
+{
+
+    [Obsolete("", true, DiagnosticId = "42")] // Noncompliant
+    void WithDiagnostics() { }
+
+    [Obsolete("", true, DiagnosticId = "42", UrlFormat = "https://sonarsource.com")] // Noncompliant
+    void WithDiagnosticsAndUrlFormat() { }
+}
+
+class Compliant
+{
+    [Obsolete("explanation", true, DiagnosticId = "42")]
+    void WithDiagnostics() { }
+
+    [Obsolete("explanation", true, DiagnosticId = "42", UrlFormat = "https://sonarsource.com")]
+    void WithDiagnosticsAndUrlFormat() { }
+}
+
+class Ignore
+{
+    [Obsolete(UrlFormat = "https://sonarsource.com")]
+    void NamedParametersOnly() { } // FN
+}
diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ObsoleteAttributesNeedExplanation.VB14.vb b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ObsoleteAttributesNeedExplanation.VB14.vb
@@ -0,0 +1,27 @@
+' Commented line for concurrent namespace
+
+Class Noncompliant
+    <Obsolete("", True, DiagnosticId:="42")> ' Noncompliant
+    Sub WithDiagnostics()
+    End Sub
+
+    <Obsolete("", True, DiagnosticId:="42", UrlFormat:="https://sonarsource.com")> ' Noncompliant
+    Sub WithDiagnosticsAndUrlFormat()
+    End Sub
+End Class
+
+Class Compliant
+    <Obsolete("explanation", True, DiagnosticId:="42")>
+    Sub WithDiagnostics()
+    End Sub
+
+    <Obsolete("explanation", True, DiagnosticId:="42", UrlFormat:="https://sonarsource.com")>
+    Sub WithDiagnosticsAndUrlFormat()
+    End Sub
+End Class
+
+Class Ignore
+    <Obsolete(UrlFormat:="https://sonarsource.com")>
+    Private Sub NamedParametersOnly() ' FP
+    End Sub
+End Class
diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ObsoleteAttributesNeedExplanation.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ObsoleteAttributesNeedExplanation.cs
@@ -12,6 +12,18 @@ void FullyDeclaredNamespace() { }
     [global::System.Obsolete] // Noncompliant
     void GloballyDeclaredNamespace() { }
 
+    [Obsolete(null)] // Noncompliant
+    void WithNull() { }
+
+    [Obsolete("")] // Noncompliant
+    void WithEmptyString() { }
+
+    [Obsolete("  ")] // Noncompliant
+    void WithWhiteSpace() { }
+
+    [Obsolete("", true)] // Noncompliant
+    void WithTwoArguments() { }
+
     [Obsolete] // Noncompliant
     [CLSCompliant(false)]
     uint Multiple() { return 0; }
@@ -69,6 +81,9 @@ enum Enum { foo, bar }
     [Obsolete("explanation")]
     void Method() { }
 
+    [Obsolete("explanation", true)]
+    void WithTwoArguments() { }
+
     [Obsolete("explanation")]
     string Property { get; set; }
 
@@ -96,6 +111,16 @@ struct ComplaintStruct
     void Method() { }
 }
 
+class Ignore
+{
+    // FP the value of error is taken.
+    [Obsolete(error: true, message: "explanation")] // Noncompliant
+    public void NamedParmetersDifferentOrderFP() { }
+
+    [Obsolete(error: true, message: "")] // Noncompliant for wrong reason 
+    public void NamedParmetersDifferentOrder() { }
+}
+
 class NotApplicable
 {
     [CLSCompliant(false)]

diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ObsoleteAttributesNeedExplanation.vb b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ObsoleteAttributesNeedExplanation.vb
@@ -2,25 +2,41 @@
 
 <Obsolete>  ' Noncompliant^2#8 {{Add an explanation.}}
 Class Noncompliant
-    <Obsolete()> Private Sub WithBrackets() ' Noncompliant {{Add an explanation.}}
+    <Obsolete()> Sub WithBrackets() ' Noncompliant {{Add an explanation.}}
 '    ^^^^^^^^^^
     End Sub
 
     <System.Obsolete> ' Noncompliant
-    Private Sub FullyDeclaredNamespace()
+    Sub FullyDeclaredNamespace()
     End Sub
 
     <Global.System.Obsolete> ' Noncompliant
-    Private Sub GloballyDeclaredNamespace()
+    Sub GloballyDeclaredNamespace()
+    End Sub
+
+    <Obsolete(Nothing)> ' Noncompliant
+    Sub WithNothing() { }
+    End Sub
+
+    <Obsolete("")> ' Noncompliant
+    Sub WithEmptyString() { }
+    End Sub
+
+    <Obsolete("  ")> ' Noncompliant
+    Sub WithWhiteSpace() { }
+    End Sub
+
+    <Obsolete("", True)> ' Noncompliant
+    Sub WithTwoArguments()
     End Sub
 
     <Obsolete> ' Noncompliant
     <CLSCompliant(False)>
-    Private Function Multiple() As UInteger
+    Function Multiple() As UInteger
         Return 0
     End Function
 
-    <Obsolete, CLSCompliant(False)> Private Function Combined() As UInteger ' Noncompliant
+    <Obsolete, CLSCompliant(False)> Function Combined() As UInteger ' Noncompliant
         Return 0
     End Function
 
@@ -31,19 +47,22 @@ Class Noncompliant
     End Enum
 
     <Obsolete> ' Noncompliant
-    Private Sub New()
+    Sub New()
     End Sub
 
     <Obsolete> ' Noncompliant
-    Private Sub Method()
+    Sub Method()
     End Sub
 
     <Obsolete> ' Noncompliant
-    Private Property [Property] As Integer
+    Property [Property] As Integer
+
     <Obsolete> ' Noncompliant
     Private Field As Integer
+
     <Obsolete> ' Noncompliant
-    Private Event [Event] As EventHandler
+    Event [Event] As EventHandler
+
     <Obsolete> ' Noncompliant
     Delegate Sub [Delegate]()
 End Class
@@ -57,7 +76,7 @@ End Interface
 <Obsolete> ' Noncompliant
 Structure ProgramStruct
     <Obsolete> ' Noncompliant
-    Private Sub Method()
+    Sub Method()
     End Sub
 End Structure
 
@@ -70,19 +89,26 @@ Class Compliant
     End Enum
 
     <Obsolete("explanation")>
-    Private Sub New()
+    Sub New()
     End Sub
 
     <Obsolete("explanation")>
-    Private Sub Method()
+    Sub Method()
+    End Sub
+
+    <Obsolete("explanation", True)>
+    Sub WithTwoArguments()
     End Sub
 
     <Obsolete("explanation")>
-    Private Property [Property] As String
+    Property [Property] As String
+
     <Obsolete("explanation", True)>
     Private Field As Integer
+
     <Obsolete("explanation", False)>
-    Private Event [Event] As EventHandler
+    Event [Event] As EventHandler
+
     <Obsolete("explanation")>
     Delegate Sub [Delegate]()
 End Class
@@ -96,7 +122,7 @@ End Interface
 <Obsolete("explanation")>
 Structure ComplaintStruct
     <Obsolete("explanation")>
-    Private Sub Method()
+    Sub Method()
     End Sub
 End Structure
 
@@ -107,24 +133,27 @@ Class NotApplicable
         bar
     End Enum
 
-    Private Sub New()
+    Sub New()
     End Sub
 
-    Private Sub Method()
+    Sub Method()
     End Sub
 
-    Private Property [Property] As Integer
+    Property [Property] As Integer
+
     Private Field As Integer
-    Private Event [Event] As EventHandler
+
+    Event [Event] As EventHandler
+
     Delegate Sub [Delegate]()
 
     <NotSystem.ObsoleteAttribute>
-    Private Sub SameName()
+    Sub SameName()
     End Sub
 End Class
 
 Namespace NotSystem
-    Public Class ObsoleteAttribute
+    Class ObsoleteAttribute
         Inherits Attribute
     End Class
 End Namespace