diff --git a/docs/rules/Moq1400.md b/docs/rules/Moq1400.md new file mode 100644 index 000000000..ca0f8c460 --- /dev/null +++ b/docs/rules/Moq1400.md @@ -0,0 +1,48 @@ +# Moq1400: Explicitly choose a mocking behavior instead of relying on the default (Loose) behavior + +| Item | Value | +| --- | --- | +| Enabled | True | +| Severity | Warning | +| CodeFix | False | +--- + +Mocks use the `MockBehavior.Loose` by default. Some people find this default behavior undesirable, as it can lead to +unexpected behavior if the mock is improperly set up. To fix, specify either `MockBehavior.Loose` or +`MockBehavior.Strict` to signify acknowledgement of the mock's behavior. + +## Examples of patterns that are flagged by this analyzer + +```csharp +interface ISample +{ + int Calculate() => 0; +} + +var mock = new Mock(); // Moq1400: Moq: Explicitly choose a mock behavior +var mock2 = Mock.Of(); // Moq1400: Moq: Explicitly choose a mock behavior +``` + +```csharp +interface ISample +{ + int Calculate() => 0; +} + +var mock = new Mock(MockBehavior.Default); // Moq1400: Moq: Explicitly choose a mock behavior +var mock2 = Mock.Of(MockBehavior.Default); // Moq1400: Moq: Explicitly choose a mock behavior +var repo = new MockRepository(MockBehavior.Default); // Moq1400: Moq: Explicitly choose a mock behavior +``` + +## Solution + +```csharp +interface ISample +{ + int Calculate() => 0; +} + +var mock = new Mock(MockBehavior.Strict); // Or `MockBehavior.Loose` +var mock2 = new Mock.Of(MockBehavior.Strict); // Or `MockBehavior.Loose` +var repo = new MockRepository(MockBehavior.Strict); // Or `MockBehavior.Loose` +``` diff --git a/src/Moq.Analyzers/AnalyzerReleases.Unshipped.md b/src/Moq.Analyzers/AnalyzerReleases.Unshipped.md index 6eea61900..fbe594b7b 100644 --- a/src/Moq.Analyzers/AnalyzerReleases.Unshipped.md +++ b/src/Moq.Analyzers/AnalyzerReleases.Unshipped.md @@ -1,2 +1,8 @@ ; Unshipped analyzer release ; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md + +### New Rules + +Rule ID | Category | Severity | Notes +--------|----------|----------|------- +Moq1400 | Moq | Warning | SetExplicitMockBehaviorAnalyzer, [Documentation](https://github.com/rjmurillo/moq.analyzers/blob/main/docs/rules/Moq1400.md) \ No newline at end of file diff --git a/src/Moq.Analyzers/AsShouldBeUsedOnlyForInterfaceAnalyzer.cs b/src/Moq.Analyzers/AsShouldBeUsedOnlyForInterfaceAnalyzer.cs index c5f8bedb2..099df996e 100644 --- a/src/Moq.Analyzers/AsShouldBeUsedOnlyForInterfaceAnalyzer.cs +++ b/src/Moq.Analyzers/AsShouldBeUsedOnlyForInterfaceAnalyzer.cs @@ -68,12 +68,10 @@ private static void Analyze(OperationAnalysisContext context, ImmutableArray typeArguments = targetMethod.TypeArguments; if (typeArguments.Length != 1) diff --git a/src/Moq.Analyzers/Common/DiagnosticIds.cs b/src/Moq.Analyzers/Common/DiagnosticIds.cs index 8e143e18b..bb9e85308 100644 --- a/src/Moq.Analyzers/Common/DiagnosticIds.cs +++ b/src/Moq.Analyzers/Common/DiagnosticIds.cs @@ -12,4 +12,5 @@ internal static class DiagnosticIds internal const string SetupOnlyUsedForOverridableMembers = "Moq1200"; internal const string AsyncUsesReturnsAsyncInsteadOfResult = "Moq1201"; internal const string AsShouldOnlyBeUsedForInterfacesRuleId = "Moq1300"; + internal const string SetExplicitMockBehavior = "Moq1400"; } diff --git a/src/Moq.Analyzers/Common/ISymbolExtensions.cs b/src/Moq.Analyzers/Common/ISymbolExtensions.cs index a0cfc1e0b..10041de45 100644 --- a/src/Moq.Analyzers/Common/ISymbolExtensions.cs +++ b/src/Moq.Analyzers/Common/ISymbolExtensions.cs @@ -15,19 +15,33 @@ internal static class ISymbolExtensions /// if is an instance of , either as a direct match, /// or as a specialaization; otherwise, . /// - /// - /// As an example, Type.Method<int>() is an instance of Type.Method<T>(). - /// + /// + /// MyType.MyMethod<int>() is an instance of MyType.MyMethod<T>(). + /// + /// + /// MyType<int>() is an instance of MyType<T>(). + /// public static bool IsInstanceOf(this ISymbol symbol, TSymbol other, SymbolEqualityComparer? symbolEqualityComparer = null) where TSymbol : class, ISymbol { symbolEqualityComparer ??= SymbolEqualityComparer.Default; - return symbol switch + if (symbol is IMethodSymbol methodSymbol) { - IMethodSymbol methodSymbol => symbolEqualityComparer.Equals(methodSymbol.OriginalDefinition, other), - _ => symbolEqualityComparer.Equals(symbol, other), - }; + return symbolEqualityComparer.Equals(methodSymbol.OriginalDefinition, other); + } + + if (symbol is INamedTypeSymbol namedTypeSymbol) + { + if (namedTypeSymbol.IsGenericType) + { + namedTypeSymbol = namedTypeSymbol.ConstructedFrom; + } + + return symbolEqualityComparer.Equals(namedTypeSymbol, other); + } + + return symbolEqualityComparer.Equals(symbol, other); } /// @@ -36,7 +50,7 @@ public static bool IsInstanceOf(this ISymbol symbol, TSymbol other, Sym /// The symbols to compare to. Returns if matches any of others. /// /// The to use for equality. - public static bool IsInstanceOf(this ISymbol symbol, IEnumerable others, SymbolEqualityComparer? symbolEqualityComparer = null) + public static bool IsInstanceOf(this ISymbol symbol, ImmutableArray others, SymbolEqualityComparer? symbolEqualityComparer = null) where TSymbol : class, ISymbol { symbolEqualityComparer ??= SymbolEqualityComparer.Default; diff --git a/src/Moq.Analyzers/SetExplicitMockBehaviorAnalyzer.cs b/src/Moq.Analyzers/SetExplicitMockBehaviorAnalyzer.cs new file mode 100644 index 000000000..98ff01624 --- /dev/null +++ b/src/Moq.Analyzers/SetExplicitMockBehaviorAnalyzer.cs @@ -0,0 +1,136 @@ +using Microsoft.CodeAnalysis.Operations; + +namespace Moq.Analyzers; + +/// +/// Mock should explicitly specify a behavior and not rely on the default. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class SetExplicitMockBehaviorAnalyzer : DiagnosticAnalyzer +{ + private static readonly LocalizableString Title = "Moq: Explicitly choose a mock behavior"; + private static readonly LocalizableString Message = "Explicitly choose a mocking behavior instead of relying on the default (Loose) behavior"; + + private static readonly DiagnosticDescriptor Rule = new( + DiagnosticIds.SetExplicitMockBehavior, + Title, + Message, + DiagnosticCategory.Moq, + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + helpLinkUri: $"https://github.com/rjmurillo/moq.analyzers/blob/{ThisAssembly.GitCommitId}/docs/rules/{DiagnosticIds.SetExplicitMockBehavior}.md"); + + /// + public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); + + /// + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + + context.RegisterCompilationStartAction(RegisterCompilationStartAction); + } + + private static void RegisterCompilationStartAction(CompilationStartAnalysisContext context) + { + // Ensure Moq is referenced in the compilation + ImmutableArray mockTypes = context.Compilation.GetMoqMock(); + if (mockTypes.IsEmpty) + { + return; + } + + // Look for the MockBehavior type and provide it to Analyze to avoid looking it up multiple times. + INamedTypeSymbol? mockBehaviorSymbol = context.Compilation.GetTypeByMetadataName(WellKnownTypeNames.MoqBehavior); + if (mockBehaviorSymbol is null) + { + return; + } + + // Look for the Mock.Of() method and provide it to Analyze to avoid looking it up multiple times. +#pragma warning disable ECS0900 // Minimize boxing and unboxing + ImmutableArray ofMethods = mockTypes + .SelectMany(mockType => mockType.GetMembers(WellKnownTypeNames.Of)) + .OfType() + .Where(method => method.IsGenericMethod) + .ToImmutableArray(); +#pragma warning restore ECS0900 // Minimize boxing and unboxing + + context.RegisterOperationAction( + context => AnalyzeNewObject(context, mockTypes, mockBehaviorSymbol), + OperationKind.ObjectCreation); + + if (!ofMethods.IsEmpty) + { + context.RegisterOperationAction( + context => AnalyzeInvocation(context, ofMethods, mockBehaviorSymbol), + OperationKind.Invocation); + } + } + + private static void AnalyzeNewObject(OperationAnalysisContext context, ImmutableArray mockTypes, INamedTypeSymbol mockBehaviorSymbol) + { + if (context.Operation is not IObjectCreationOperation creationOperation) + { + return; + } + + if (creationOperation.Type is not INamedTypeSymbol namedType) + { + return; + } + + if (!namedType.IsInstanceOf(mockTypes)) + { + return; + } + + foreach (IArgumentOperation argument in creationOperation.Arguments) + { + if (argument.Value is IFieldReferenceOperation fieldReferenceOperation) + { + ISymbol field = fieldReferenceOperation.Member; + if (field.ContainingType.IsInstanceOf(mockBehaviorSymbol) && IsExplicitBehavior(field.Name)) + { + return; + } + } + } + + context.ReportDiagnostic(creationOperation.Syntax.GetLocation().CreateDiagnostic(Rule)); + } + + private static void AnalyzeInvocation(OperationAnalysisContext context, ImmutableArray wellKnownOfMethods, INamedTypeSymbol mockBehaviorSymbol) + { + if (context.Operation is not IInvocationOperation invocationOperation) + { + return; + } + + IMethodSymbol targetMethod = invocationOperation.TargetMethod; + if (!targetMethod.IsInstanceOf(wellKnownOfMethods)) + { + return; + } + + foreach (IArgumentOperation argument in invocationOperation.Arguments) + { + if (argument.Value is IFieldReferenceOperation fieldReferenceOperation) + { + ISymbol field = fieldReferenceOperation.Member; + if (field.ContainingType.IsInstanceOf(mockBehaviorSymbol) && IsExplicitBehavior(field.Name)) + { + return; + } + } + } + + context.ReportDiagnostic(invocationOperation.Syntax.GetLocation().CreateDiagnostic(Rule)); + } + + private static bool IsExplicitBehavior(string symbolName) + { + return string.Equals(symbolName, "Loose", StringComparison.Ordinal) || string.Equals(symbolName, "Strict", StringComparison.Ordinal); + } +} diff --git a/tests/Moq.Analyzers.Test/Helpers/ReferenceAssemblyCatalog.cs b/tests/Moq.Analyzers.Test/Helpers/ReferenceAssemblyCatalog.cs index 383be5fbb..afed25e34 100644 --- a/tests/Moq.Analyzers.Test/Helpers/ReferenceAssemblyCatalog.cs +++ b/tests/Moq.Analyzers.Test/Helpers/ReferenceAssemblyCatalog.cs @@ -24,6 +24,7 @@ internal static class ReferenceAssemblyCatalog // implementation of `.As()` (see https://github.com/devlooped/moq/commit/b552aeddd82090ee0f4743a1ab70a16f3e6d2d11). { nameof(Net80WithOldMoq), ReferenceAssemblies.Net.Net80.AddPackages([new PackageIdentity("Moq", "4.8.2")]) }, + // This must be 4.12.0 or later in order to have the new `Mock.Of(MockBehavior)` method (see https://github.com/devlooped/moq/commit/1561c006c87a0894c5257a1e541da44e40e33dd3). // 4.18.4 is currently the most downloaded version of Moq. { nameof(Net80WithNewMoq), ReferenceAssemblies.Net.Net80.AddPackages([new PackageIdentity("Moq", "4.18.4")]) }, }; diff --git a/tests/Moq.Analyzers.Test/SetExplicitMockBehaviorAnalyzerTests.cs b/tests/Moq.Analyzers.Test/SetExplicitMockBehaviorAnalyzerTests.cs new file mode 100644 index 000000000..0af47f6bc --- /dev/null +++ b/tests/Moq.Analyzers.Test/SetExplicitMockBehaviorAnalyzerTests.cs @@ -0,0 +1,58 @@ +using Verifier = Moq.Analyzers.Test.Helpers.AnalyzerVerifier; + +namespace Moq.Analyzers.Test; + +public class SetExplicitMockBehaviorAnalyzerTests +{ + public static IEnumerable TestData() + { + IEnumerable mockConstructors = new object[][] + { + ["""{|Moq1400:new Mock()|};"""], + ["""{|Moq1400:new Mock(MockBehavior.Default)|};"""], + ["""new Mock(MockBehavior.Loose);"""], + ["""new Mock(MockBehavior.Strict);"""], + }.WithNamespaces().WithMoqReferenceAssemblyGroups(); + + IEnumerable fluentBuilders = new object[][] + { + ["""{|Moq1400:Mock.Of()|};"""], + ["""{|Moq1400:Mock.Of(MockBehavior.Default)|};"""], + ["""Mock.Of(MockBehavior.Loose);"""], + ["""Mock.Of(MockBehavior.Strict);"""], + }.WithNamespaces().WithNewMoqReferenceAssemblyGroups(); + + IEnumerable mockRepositories = new object[][] + { + ["""{|Moq1400:new MockRepository(MockBehavior.Default)|};"""], + ["""new MockRepository(MockBehavior.Loose);"""], + ["""new MockRepository(MockBehavior.Strict);"""], + }.WithNamespaces().WithNewMoqReferenceAssemblyGroups(); + + return mockConstructors.Union(fluentBuilders).Union(mockRepositories); + } + + [Theory] + [MemberData(nameof(TestData))] + public async Task ShouldAnalyzeMocksWithoutExplictMockBehavior(string referenceAssemblyGroup, string @namespace, string mock) + { + await Verifier.VerifyAnalyzerAsync( + $$""" + {{@namespace}} + + public interface ISample + { + int Calculate(int a, int b); + } + + internal class UnitTest + { + private void Test() + { + {{mock}} + } + } + """, + referenceAssemblyGroup); + } +}