Merge pull request #641 from manfred-brands/issue639_formatspec

Detect Assert.That invocations where CallerMemberExpression parameter has been passed in.

documentation/NUnit2050.md

@@ -28,9 +28,15 @@ This analyzer needs to be run when still building against NUnit3 as otherwise yo
 When usages of the new methods with `params` are detected, the associated CodeFix will convert the format specification
 into an interpolated string.
 
+Once you moved to NUnit4 the analyzer has some limited functionality as there are a few
+cases where your NUnit3 code will compile on NUnit4, but not the way you want it.
+Here what you think are parameters to a format specification are actually interpreted as 
+the _actual_ and _constraint_ expression strings.
+Unfortunately you only find that out when the test fails, which could be never.
+
 ## How to fix violations
 
-The following code, valid in NUNit3:
+The following code, valid in NUnit3:
 
 ```csharp
 [TestCase(4)]
@@ -60,13 +66,68 @@ public void MustBeMultipleOf3(int value)
 
 The failure message for NUnit4 becomes:
 
-```
+```csharp
 Expected value (4) to be multiple of 3
 Assert.That(value % 3, Is.Zero)
 Expected: 0
 But was:  1
 ```
 
+As the `[CallerMemberExpression]` parameters are `string`, some of NUnit 3.x code compiles, but when failing show the wrong message:
+
+```csharp
+[TestCase("NUnit 4", "NUnit 3")]
+public void TestMessage(string actual, string expected)
+{
+    Assert.That(actual, Is.EqualTo(expected), "Expected '{0}', but got: '{1}'", expected, actual);
+}
+```
+
+When using NUnit3, this results in:
+
+```
+  Expected 'NUnit 3', but got: 'NUnit 4'
+  String lengths are both 7. Strings differ at index 6.
+  Expected: "NUnit 3"
+  But was:  "NUnit 4"
+  -----------------^
+```
+
+But when using NUnit4, we get:
+
+```
+ Message: 
+  Expected '{0}', but got: '{1}'
+Assert.That(NUnit 3, NUnit 4)
+  String lengths are both 7. Strings differ at index 6.
+  Expected: "NUnit 3"
+  But was:  "NUnit 4"
+  -----------------^
+```
+
+Where the format string is treated as the _message_ and its arguments are interpreted as the _actual_ and _expected_ expressions!
+After applying the code fix the code looks like:
+
+```csharp
+[TestCase("NUnit 4", "NUnit 3")]
+public void TestMessage(string actual, string expected)
+{
+    Assert.That(actual, Is.EqualTo(expected), $"Expected '{expected}', but got: '{actual}'");
+}
+```
+
+and the output:
+
+```
+ Message: 
+  Expected 'NUnit 3', but got: 'NUnit 4'
+Assert.That(actual, Is.EqualTo(expected))
+  String lengths are both 7. Strings differ at index 6.
+  Expected: "NUnit 3"
+  But was:  "NUnit 4"
+  -----------------^
+```
+
 <!-- start generated config severity -->
 ## Configure severity
 

src/nunit.analyzers.tests/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringAnalyzerTests.cs

@@ -12,9 +12,7 @@ namespace NUnit.Analyzers.Tests.UpdateStringFormatToInterpolatableString
     public sealed class UpdateStringFormatToInterpolatableStringAnalyzerTests
     {
         private readonly DiagnosticAnalyzer analyzer = new UpdateStringFormatToInterpolatableStringAnalyzer();
-#if !NUNIT4
         private readonly ExpectedDiagnostic diagnostic = ExpectedDiagnostic.Create(AnalyzerIdentifiers.UpdateStringFormatToInterpolatableString);
-#endif
 
         private static IEnumerable<string> NoArgumentsAsserts { get; } = new[]
         {
@@ -92,7 +90,6 @@ public void AnalyzeAssertBoolWhenOnlyMessageArgumentIsUsed()
             RoslynAssert.Valid(this.analyzer, testCode);
         }
 
-#if !NUNIT4
         [Test]
         public void AnalyzeAssertBoolWhenFormatAndArgumentsAreUsed()
         {
@@ -101,7 +98,6 @@ public void AnalyzeAssertBoolWhenFormatAndArgumentsAreUsed()
             ");
             RoslynAssert.Diagnostics(this.analyzer, this.diagnostic, testCode);
         }
-#endif
 
         [Test]
         public void AnalyzeAssertBoolWhenFormattableStringIsUsed()
@@ -132,6 +128,19 @@ public void AnalyzeAssertThatWhenOnlyMessageArgumentIsUsed()
             RoslynAssert.Valid(this.analyzer, testCode);
         }
 
+        [Test]
+        public void AnalyzeAssertThatWhenFormatAndStringArgumentsAreUsed()
+        {
+            var testCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
+        [TestCase(""NUnit 4.0"", ""NUnit 3.14"")]
+        public void AssertSomething(string actual, string expected)
+        {
+            ↓Assert.That(actual, Is.EqualTo(expected).IgnoreCase, ""Expected '{0}', but got: {1}"", expected, actual);
+        }");
+
+            RoslynAssert.Diagnostics(this.analyzer, this.diagnostic, testCode);
+        }
+
 #if !NUNIT4
         [Test]
         public void AnalyzeAssertThatWhenFormatAndArgumentsAreUsed()

src/nunit.analyzers.tests/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringCodeFixTests.cs

@@ -1,5 +1,3 @@
-#if !NUNIT4
-
 using Gu.Roslyn.Asserts;
 using Microsoft.CodeAnalysis.CodeFixes;
 using Microsoft.CodeAnalysis.Diagnostics;
@@ -25,6 +23,25 @@ public void VerifyGetFixableDiagnosticIds()
             Assert.That(ids, Is.EquivalentTo(new[] { AnalyzerIdentifiers.UpdateStringFormatToInterpolatableString }));
         }
 
+#if NUNIT4
+        [Test]
+        public void AccidentallyUseFormatSpecification()
+        {
+            var code = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
+        [TestCase(""NUnit 4.0"", ""NUnit 3.14"")]
+        public void AssertSomething(string actual, string expected)
+        {
+            ↓Assert.That(actual, Is.EqualTo(expected).IgnoreCase, ""Expected '{0}', but got: {1}"", expected, actual);
+        }");
+            var fixedCode = TestUtility.WrapMethodInClassNamespaceAndAddUsings(@"
+        [TestCase(""NUnit 4.0"", ""NUnit 3.14"")]
+        public void AssertSomething(string actual, string expected)
+        {
+            Assert.That(actual, Is.EqualTo(expected).IgnoreCase, $""Expected '{expected}', but got: {actual}"");
+        }");
+            RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
+        }
+#else
         [TestCase(NUnitFrameworkConstants.NameOfAssertIgnore)]
         public void ConvertWhenNoMinimumParameters(string method)
         {
@@ -107,6 +124,7 @@ public void TestEscapedBraces()
 
             RoslynAssert.CodeFix(analyzer, fix, expectedDiagnostic, code, fixedCode);
         }
+#endif
+
     }
 }
-#endif

src/nunit.analyzers.tests/nunit.analyzers.tests.csproj

@@ -26,7 +26,7 @@
   </ItemGroup>
 
   <ItemGroup Condition="'$(NUnitVersion)' == '3'">
-    <PackageReference Include="NUnit" Version="3.13.3" />
+    <PackageReference Include="NUnit" Version="3.14.0" />
   </ItemGroup>
 
   <ItemGroup>

src/nunit.analyzers/UpdateStringFormatToInterpolatableString/UpdateStringFormatToInterpolatableStringAnalyzer.cs

@@ -42,13 +42,21 @@ protected override bool IsAssert(bool hasClassicAssert, IInvocationOperation inv
 
         protected override void AnalyzeAssertInvocation(Version nunitVersion, OperationAnalysisContext context, IInvocationOperation assertOperation)
         {
-            if (nunitVersion.Major >= 4)
+            if (nunitVersion.Major < 4)
             {
-                // Too late, this won't work as the method with the `params` parameter doesn't exists
-                // and won't be resolved by the compiler.
-                return;
+                AnalyzeNUnit3AssertInvocation(context, assertOperation);
+            }
+            else
+            {
+                AnalyzeNUnit4AssertInvocation(context, assertOperation);
             }
+        }
 
+        /// <summary>
+        /// This looks to see if the `params` overload is called.
+        /// </summary>
+        private static void AnalyzeNUnit3AssertInvocation(OperationAnalysisContext context, IInvocationOperation assertOperation)
+        {
             int lastParameterIndex = assertOperation.TargetMethod.Parameters.Length - 1;
             if (lastParameterIndex > 0 && assertOperation.TargetMethod.Parameters[lastParameterIndex].IsParams)
             {
@@ -57,21 +65,10 @@ protected override void AnalyzeAssertInvocation(Version nunitVersion, OperationA
                 {
                     string methodName = assertOperation.TargetMethod.Name;
 
-                    if (!ObsoleteParamsMethods.Contains(methodName))
+                    if (ObsoleteParamsMethods.Contains(methodName))
                     {
-                        return;
+                        ReportDiagnostic(context, assertOperation, methodName, lastParameterIndex - 1);
                     }
-
-                    int minimumNumberOfArguments = lastParameterIndex - 1;
-
-                    context.ReportDiagnostic(Diagnostic.Create(
-                        updateStringFormatToInterpolatableString,
-                        assertOperation.Syntax.GetLocation(),
-                        new Dictionary<string, string?>
-                        {
-                            [AnalyzerPropertyKeys.ModelName] = methodName,
-                            [AnalyzerPropertyKeys.MinimumNumberOfArguments] = minimumNumberOfArguments.ToString(CultureInfo.InvariantCulture),
-                        }.ToImmutableDictionary()));
                 }
             }
 
@@ -87,5 +84,60 @@ static bool IsNonEmptyParamsArrayArgument(IArgumentOperation argument)
                 return argument.ArgumentKind == ArgumentKind.Explicit;
             }
         }
+
+        /// <summary>
+        /// This looks to see if the `CallerMemberExpression` parameters are explicitly specified.
+        /// </summary>
+        private static void AnalyzeNUnit4AssertInvocation(OperationAnalysisContext context, IInvocationOperation assertOperation)
+        {
+            string methodName = assertOperation.TargetMethod.Name;
+
+            if (methodName != NUnitFrameworkConstants.NameOfAssertThat)
+            {
+                // Only Assert.That has CallerMemberExpression that could be accidentally used as format paramaters
+                return;
+            }
+
+            // Find the 'message' parameter.
+            ImmutableArray<IParameterSymbol> parameters = assertOperation.TargetMethod.Parameters;
+            int formatParameterIndex = 1;
+            for (;  formatParameterIndex < parameters.Length; formatParameterIndex++)
+            {
+                IParameterSymbol parameter = parameters[formatParameterIndex];
+                if (parameter.IsOptional)
+                {
+                    if (parameter.Name == "message")
+                        break;
+
+                    // Overload with FormattableString or Func<string> overload
+                    return;
+                }
+            }
+
+            ImmutableArray<IArgumentOperation> arguments = assertOperation.Arguments;
+            if (arguments.Length > formatParameterIndex && arguments[formatParameterIndex + 1].ArgumentKind == ArgumentKind.Explicit)
+            {
+                // The argument after the message is explicitly specified
+                // Most likely the user thought it was using a format specification with a parameter.
+                // Or it copied code from some NUnit 3.x source into an NUNit 4 project.
+                ReportDiagnostic(context, assertOperation, methodName, formatParameterIndex);
+            }
+        }
+
+        private static void ReportDiagnostic(
+            OperationAnalysisContext context,
+            IInvocationOperation assertOperation,
+            string methodName,
+            int minimumNumberOfArguments)
+        {
+            context.ReportDiagnostic(Diagnostic.Create(
+                updateStringFormatToInterpolatableString,
+                assertOperation.Syntax.GetLocation(),
+                new Dictionary<string, string?>
+                {
+                    [AnalyzerPropertyKeys.ModelName] = methodName,
+                    [AnalyzerPropertyKeys.MinimumNumberOfArguments] = minimumNumberOfArguments.ToString(CultureInfo.InvariantCulture),
+                }.ToImmutableDictionary()));
+        }
     }
 }