Fix xUnit3002 false positives

xunit · Aug 25, 2024 · bc0dbe4 · bc0dbe4
1 parent ac7cc01
commit bc0dbe4
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 89 deletions.
diff --git a/...t.analyzers.tests/Analyzers/X3000/DoNotTestForConcreteTypeOfJsonSerializableTypesTests.cs b/...t.analyzers.tests/Analyzers/X3000/DoNotTestForConcreteTypeOfJsonSerializableTypesTests.cs
@@ -6,67 +6,60 @@ namespace Xunit.Analyzers;
 
 public class DoNotTestForConcreteTypeOfJsonSerializableTypesTests
 {
-	[Fact(Skip = "xUnit3002 needs to be re-written")]
+	[Fact]
 	public async Task AcceptanceTest()
 	{
 		var code = /* lang=c#-test */ """
 			using Xunit;
 			using System.Collections.Generic;
 			using System.Linq;
 
-			public static class MessageFactory {
-			    public static MyMessage Create(int propertyValue = 42) =>
-			        new() {
-			            PropertyValue = propertyValue,
-			        };
-			}
+			public class GenericClass<T1,T2> { }
 
 			public class TheClass {
-			    public void TheMethod() {
-			        var message = new object();
-			        var collection = new List<object>();
+				/// <summary>
+				/// Testing XMLDOC references <see cref="MyMessage"/>.
+				/// </summary>
+				public void TheMethod() {
+					var message = new object();
+					var collection = new List<object>();
 
-			        // Not testing against a serializable type
-			        Assert.True(message is string);
-			        Assert.True(message is not string);
-			        Assert.NotNull(message as string);
-			        Assert.NotNull((string)message);
-			        Assert.Empty(collection.OfType<string>());
-			        Assert.IsType(typeof(string), message);
-			        Assert.IsType<string>(message);
-			        Assert.IsNotType(typeof(string), message);
-			        Assert.IsNotType<string>(message);
-			        Assert.IsAssignableFrom(typeof(string), message);
-			        Assert.IsAssignableFrom<string>(message);
-			        Assert.IsNotAssignableFrom(typeof(string), message);
-			        Assert.IsNotAssignableFrom<string>(message);
+					// Direct construction
+					_ = new MyMessage { PropertyValue = 2112 };
+					static MyMessage Create(int propertyValue) =>
+						new() { PropertyValue = propertyValue };
 
-			        // Construction should not be prohibited
-			        _ = new MyMessage { PropertyValue = 2112 };
-			        _ = MessageFactory.Create(2600);
+					// Non-serialized type
+					_ = message is IMyMessage;
+					_ = message is not IMyMessage;
+					_ = message as IMyMessage;
+					_ = (IMyMessage)message;
+					_ = typeof(IMyMessage);
+					_ = collection.OfType<IMyMessage>();
+					_ = default(IEnumerable<IMyMessage>);
+					_ = new GenericClass<IMyMessage, int>();
+					_ = new GenericClass<int, IMyMessage>();
 
-			        // Testing against a serializable type
-			        Assert.True([|message is MyMessage|]);
-			        Assert.True([|message is not MyMessage|]);
-			        Assert.NotNull([|message as MyMessage|]);
-			        Assert.NotNull([|(MyMessage)message|]);
-			        Assert.Empty([|collection.OfType<MyMessage>()|]);
-			        [|Assert.IsType(typeof(MyMessage), message)|];
-			        [|Assert.IsType<MyMessage>(message)|];
-			        [|Assert.IsNotType(typeof(MyMessage), message)|];
-			        [|Assert.IsNotType<MyMessage>(message)|];
-			        [|Assert.IsAssignableFrom(typeof(MyMessage), message)|];
-			        [|Assert.IsAssignableFrom<MyMessage>(message)|];
-			        [|Assert.IsNotAssignableFrom(typeof(MyMessage), message)|];
-			        [|Assert.IsNotAssignableFrom<MyMessage>(message)|];
-			    }
+					// Serialized type
+					_ = [|message is MyMessage|];
+					_ = [|message is not MyMessage|];
+					_ = [|message as MyMessage|];
+					_ = [|(MyMessage)message|];
+					_ = [|typeof(MyMessage)|];
+					_ = collection.[|OfType<MyMessage>|]();
+					_ = default([|IEnumerable<MyMessage>|]);
+					_ = new [|GenericClass<MyMessage, int>|]();
+					_ = new [|GenericClass<int, MyMessage>|]();
+				}
 			}
 			""";
 		var messagePartial1 = /* lang=c#-test */ """
 			using Xunit.Sdk;
 
+			public interface IMyMessage { }
+
 			[JsonTypeID("MyMessage")]
-			sealed partial class MyMessage { }
+			sealed partial class MyMessage : IMyMessage { }
 			""";
 		var messagePartial2 = /* lang=c#-test */ """
 			public partial class MyMessage {

diff --git a/src/xunit.analyzers/Utility/Descriptors.xUnit3xxx.cs b/src/xunit.analyzers/Utility/Descriptors.xUnit3xxx.cs
@@ -29,7 +29,7 @@ public static partial class Descriptors
 			"xUnit3002",
 			"Classes which are JSON serializable should not be tested for their concrete type",
 			Extensibility,
-			Hidden,
+			Warning,
 			"Class {0} is JSON serializable and should not be tested for its concrete type. Test for its primary interface instead."
 		);
 

diff --git a/src/xunit.analyzers/X3000/DoNotTestForConcreteTypeOfJsonSerializableTypes.cs b/src/xunit.analyzers/X3000/DoNotTestForConcreteTypeOfJsonSerializableTypes.cs
@@ -1,6 +1,8 @@
-using System.Collections.Generic;
+using System;
 using System.Linq;
 using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.CodeAnalysis.Operations;
 
@@ -9,13 +11,6 @@ namespace Xunit.Analyzers;
 [DiagnosticAnalyzer(LanguageNames.CSharp)]
 public class DoNotTestForConcreteTypeOfJsonSerializableTypes : XunitV3DiagnosticAnalyzer
 {
-	readonly static HashSet<string> matchingAssertions = [
-		Constants.Asserts.IsAssignableFrom,
-		Constants.Asserts.IsNotAssignableFrom,
-		Constants.Asserts.IsNotType,
-		Constants.Asserts.IsType,
-	];
-
 	public DoNotTestForConcreteTypeOfJsonSerializableTypes() :
 		base(Descriptors.X3002_DoNotTestForConcreteTypeOfJsonSerializableTypes)
 	{ }
@@ -43,7 +38,7 @@ public override void AnalyzeCompilation(
 			if (semanticModel is null)
 				return;
 
-			reportIfMessageType(context, semanticModel, isTypeOperation.TypeOperand, isTypeOperation.Syntax);
+			reportIfMessageType(context.ReportDiagnostic, semanticModel, isTypeOperation.TypeOperand, isTypeOperation.Syntax);
 		}, OperationKind.IsType);
 
 		// "value is not Type"
@@ -66,7 +61,7 @@ public override void AnalyzeCompilation(
 #endif
 				return;
 
-			reportIfMessageType(context, semanticModel, typePatternOperation.MatchedType, isPatternOperation.Syntax);
+			reportIfMessageType(context.ReportDiagnostic, semanticModel, typePatternOperation.MatchedType, isPatternOperation.Syntax);
 		}, OperationKind.IsPattern);
 
 		// "value as Type"
@@ -76,55 +71,43 @@ public override void AnalyzeCompilation(
 			if (context.Operation is not IConversionOperation conversionOperation)
 				return;
 
+			// We don't want to prohibit conversion that comes from "new(...)"
+			if (conversionOperation.Syntax is ImplicitObjectCreationExpressionSyntax)
+				return;
+
 			var semanticModel = conversionOperation.SemanticModel;
 			if (semanticModel is null)
 				return;
 
-			reportIfMessageType(context, semanticModel, conversionOperation.Operand.Type, conversionOperation.Syntax);
+			reportIfMessageType(context.ReportDiagnostic, semanticModel, conversionOperation.Type, conversionOperation.Syntax);
 		}, OperationKind.Conversion);
 
-		// "collection.OfType<Type>()"
-		// "Assert.IsType<Type>(value)"
-		// "Assert.IsNotType<Type>(value)"
-		// "Assert.IsAssignableFrom<Type>(value)"
-		// "Assert.IsNotAssignableFrom<Type>(value)"
+		// "typeof(Type)"
 		context.RegisterOperationAction(context =>
 		{
-			if (context.Operation is not IInvocationOperation invocationOperation)
+			if (context.Operation is not ITypeOfOperation typeOfOperation)
 				return;
 
-			var semanticModel = invocationOperation.SemanticModel;
+			var semanticModel = typeOfOperation.SemanticModel;
 			if (semanticModel is null)
 				return;
 
-			// We will match "OfType<>()" by convention; that is, no args, single type. This may be overly
-			// broad, but it seems like the best way to ensure every extension method gets convered.
-			var method = invocationOperation.TargetMethod;
-			if (method.Name == "OfType"
-					&& method.IsGenericMethod
-					&& method.TypeArguments.Length == 1
-					&& method.Parameters.Length == (method.IsExtensionMethod ? 1 : 0))
-				reportIfMessageType(context, semanticModel, method.TypeArguments[0], invocationOperation.Syntax);
-
-			// We also look for type-related calls to Assert
-			if (assertType is not null
-					&& matchingAssertions.Contains(method.Name)
-					&& SymbolEqualityComparer.Default.Equals(assertType, method.ContainingType))
-			{
-				var testType = default(ITypeSymbol);
-				if (method.IsGenericMethod && method.TypeArguments.Length == 1)
-					testType = method.TypeArguments[0];
-				else if (invocationOperation.Arguments.FirstOrDefault() is IArgumentOperation typeArgumentOperation
-						&& typeArgumentOperation.Value is ITypeOfOperation typeOfArgumentOperation)
-					testType = typeOfArgumentOperation.TypeOperand;
-
-				if (testType is not null)
-					reportIfMessageType(context, semanticModel, testType, invocationOperation.Syntax);
-			}
-		}, OperationKind.Invocation);
+			reportIfMessageType(context.ReportDiagnostic, semanticModel, typeOfOperation.TypeOperand, typeOfOperation.Syntax);
+		}, OperationKind.TypeOf);
+
+		// "SomeMethod<Type>()"
+		// "GenericType<Type>"
+		context.RegisterSyntaxNodeAction(context =>
+		{
+			if (context.Node.ChildNodes().FirstOrDefault() is not TypeArgumentListSyntax typeArgumentListSyntax)
+				return;
+
+			foreach (var identifierNameSyntax in typeArgumentListSyntax.ChildNodes().OfType<IdentifierNameSyntax>())
+				reportIfMessageType(context.ReportDiagnostic, context.SemanticModel, context.SemanticModel.GetTypeInfo(identifierNameSyntax).ConvertedType, context.Node);
+		}, SyntaxKind.GenericName);
 
 		void reportIfMessageType(
-			OperationAnalysisContext context,
+			Action<Diagnostic> reportDiagnostic,
 			SemanticModel semanticModel,
 			ITypeSymbol? typeSymbol,
 			SyntaxNode syntax)
@@ -134,7 +117,7 @@ void reportIfMessageType(
 
 			foreach (var attribute in typeSymbol.GetAttributes())
 				if (SymbolEqualityComparer.Default.Equals(attribute.AttributeClass, jsonTypeIDAttributeType))
-					context.ReportDiagnostic(
+					reportDiagnostic(
 						Diagnostic.Create(
 							Descriptors.X3002_DoNotTestForConcreteTypeOfJsonSerializableTypes,
 							syntax.GetLocation(),