fix: Moq1203 false positives for ReturnsAsync and Callback chaining#886
fix: Moq1203 false positives for ReturnsAsync and Callback chaining#886
Conversation
Reproduces #849 where Moq1203 false positives occur when: - ReturnsAsync is used instead of Returns - Callback is chained before Returns/Throws/ReturnsAsync Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
HasReturnValueSpecification only checked the immediate parent of the Setup call and only recognized Returns/Throws. This caused false positives when Callback preceded Returns, or when ReturnsAsync/ThrowsAsync was used instead of Returns/Throws. Walk up through the full method chain and recognize all four return value specification methods. Fixes #849 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @rjmurillo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses false positives in the Moq1203 analyzer by refining its logic for detecting return value specifications. The changes ensure that method chains involving Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAnalyzer expanded to detect Moq return-value specification methods anywhere in a setup call chain (e.g., after intermediate calls like Callback), now recognizing Returns, ReturnsAsync, Throws, and ThrowsAsync, using chain-walking symbol resolution with cancellation and an initialized SupportedDiagnostics property. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
  DeepSource reviewed changes in the commit range For detailed review results, please see the PR on DeepSource ↗ PR Report Card
Code Review Summary
How are these analyzer statuses calculated?Administrators can configure which issue categories are reported and cause analysis to be marked as failed when detected. This helps prevent bad and insecure code from being introduced in the codebase. If you're an administrator, you can modify this in the repository's settings. |
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses false positives in the Moq1203 analyzer by correctly traversing the entire method chain to find return value specifications like Returns or Throws. The changes also properly recognize ReturnsAsync and ThrowsAsync. The implementation in HasReturnValueSpecification is robust, and the added regression tests are comprehensive and cover the reported issues well. I have one suggestion regarding code duplication in the test file to improve maintainability.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
There was a problem hiding this comment.
Pull request overview
This pull request attempts to fix false positives in the Moq1203 analyzer ("Method setup should specify a return value") that were introduced in v0.4.0. The issue (#849) reported that:
ReturnsAsync()was not being recognized as a valid return value specification- Method chaining with
Callback()beforeReturns()orThrows()was incorrectly flagged
Changes:
- Modified
HasReturnValueSpecificationto walk the entire method chain instead of only checking the immediate parent - Added
IsReturnValueMethodhelper to recognizeReturnsAsyncandThrowsAsyncalongsideReturnsandThrows - Added 20 regression tests (5 test cases × 4 namespace/Moq version combinations) covering all reported false positive patterns
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Analyzers/MethodSetupShouldSpecifyReturnValueAnalyzer.cs |
Updated HasReturnValueSpecification to walk full method chain and recognize ReturnsAsync/ThrowsAsync methods |
tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs |
Added regression test data and test method for Issue #849 false positive scenarios |
coderabbitai
Bot
added
analyzers
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs`:
- Around line 53-76: Add a regression test case for ThrowsAsync to
Issue849_FalsePositiveTestData so the analyzer test covers all four return-value
methods; edit the data array inside the Issue849_FalsePositiveTestData method to
include a new test string that calls Setup on a mock of IFoo for an async method
and specifies ThrowsAsync (also include a variant where Callback precedes
ThrowsAsync to mirror the existing patterns), then return the augmented data as
before with WithNamespaces() and WithMoqReferenceAssemblyGroups().
- Handle CandidateSymbols for overload resolution failure, matching the established pattern in SemanticModelExtensions - Pass CancellationToken through to GetSymbolInfo for cancellation support - Pass full MemberAccessExpressionSyntax (not .Name) to GetSymbolInfo, consistent with other call sites - Update stale Description field and class xmldoc to list all four recognized methods (Returns, ReturnsAsync, Throws, ThrowsAsync) - Add ThrowsAsync test coverage (direct and via Callback chaining) - Add negative test: Callback-only setup still flags Moq1203 - Add negative test: async method without return specification flags - Extract shared VerifyMock helper to eliminate duplicate test methods - Update docs/rules/Moq1203.md with async and Callback chain examples Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Cache SupportedDiagnostics with auto-property initializer (was allocating on every access via expression-bodied member) - Add helpLinkUri to DiagnosticDescriptor (missing vs all other analyzers) - Replace LINQ .Any() on CandidateSymbols with foreach to avoid ImmutableArray enumerator allocation in hot path - Remove unused knownSymbols out parameter from TryGetSetupInvocation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Analyzers/MethodSetupShouldSpecifyReturnValueAnalyzer.cs`:
- Around line 152-165: The loop in MethodSetupShouldSpecifyReturnValueAnalyzer
that walks from setupSyntax using the variable current only advances when the
parent is a MemberAccessExpressionSyntax, so wrapper nodes
(ParenthesizedExpressionSyntax, CastExpressionSyntax,
ConditionalAccessExpressionSyntax, etc.) can prematurely stop the walk and cause
false positives; update the while/advance logic around current (the chain-walk
that ultimately calls HasReturnValueSymbol) to first skip over wrapper nodes by
repeatedly setting current = current.Parent while the parent is one of those
wrapper node types, then continue checking for
MemberAccessExpressionSyntax/InvocationExpressionSyntax and calling
HasReturnValueSymbol, and add a regression test that covers a parenthesized
Setup invocation like (new Mock<IFoo>().Setup(...)).Returns(...).
Replace name-based string matching in IsReturnValueMethod with symbol-based detection using MoqKnownSymbols and ISymbolExtensions helpers, consistent with how other Moq methods are identified. - Add IThrows, ReturnsExtensions symbols to MoqKnownSymbols - Add IsMoqThrowsMethod, IsMoqReturnsAsyncMethod, IsMoqThrowsAsyncMethod, and IsMoqReturnValueSpecificationMethod helpers to ISymbolExtensions - Fix IsInstanceOf to handle reduced extension methods via ReducedFrom - Thread MoqKnownSymbols through the analyzer call chain - Delete dead IsReturnValueMethod string matcher Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add tests that exercise the candidate symbol fallback path in HasReturnValueSymbol, where Roslyn reports OverloadResolutionFailure instead of resolving the method directly. Uses deliberately mismatched argument types to force the fallback. - Add AnalyzerVerifier overload accepting CompilerDiagnostics - Add tests for return value method recognized via candidate symbols - Add tests for non-return-value candidates still flagging Moq1203 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs (1)
174-237: 🧹 Nitpick | 🔵 TrivialConsider extracting the common source template to reduce duplication.
Both
VerifyMockandVerifyMockIgnoringCompilerErrorsdefine identicalIFoointerfaces and test class structures. If the interface needs to change in the future (e.g., adding a new method for testing), it would need to be updated in both places.♻️ Proposed refactor to extract common template
+ private static string BuildTestSource(string `@namespace`, string mock) + { + return $$""" + {{`@namespace`}} + + public interface IFoo + { + bool DoSomething(string value); + int GetValue(); + int Calculate(int a, int b); + Task<int> BarAsync(); + void DoVoidMethod(); + void ProcessData(string data); + string Name { get; set; } + } + + internal class UnitTest + { + private void Test() + { + {{mock}} + } + } + """; + } + private async Task VerifyMock(string referenceAssemblyGroup, string `@namespace`, string mock) { - string source = $$""" - {{`@namespace`}} - - public interface IFoo - ... - """; + string source = BuildTestSource(`@namespace`, mock); output.WriteLine(source); await Verifier.VerifyAnalyzerAsync( source, referenceAssemblyGroup).ConfigureAwait(false); } private async Task VerifyMockIgnoringCompilerErrors(string referenceAssemblyGroup, string `@namespace`, string mock) { - string source = $$""" - {{`@namespace`}} - - public interface IFoo - ... - """; + string source = BuildTestSource(`@namespace`, mock); output.WriteLine(source); await Verifier.VerifyAnalyzerAsync( source, referenceAssemblyGroup, CompilerDiagnostics.None).ConfigureAwait(false); }
Simplify HasReturnValueSymbol to check resolved symbol first, then scan all candidates regardless of CandidateReason (not just OverloadResolutionFailure). This prevents false positives when Roslyn reports Ambiguous, Inaccessible, or other candidate reasons. Consolidate AnalyzerVerifier overloads by folding CompilerDiagnostics into the primary method as an optional parameter. Update AllAnalyzersVerifier reflection to match the new 5-parameter signature. Extract BuildSource in tests to eliminate duplicated source template. Add test cases for Throws<T>() on async methods and Throws(Exception) instance form with Callback chaining. Clean up documentation: remove implementation details from Moq1203.md, fix imprecise wording, remove redundant inline comments, fix inaccurate XML doc returns tags, remove coupling parenthetical from MoqKnownSymbols. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rjmurillo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs`:
- Around line 57-93: Add a regression entry to Issue849_FalsePositiveTestData to
cover parenthesized setup chains so the chain-walk fix doesn't regress on
wrapper nodes; specifically, insert a test string like "(new
Mock<IFoo>().Setup(x => x.GetValue())).Returns(42);" into the IEnumerable
returned by Issue849_FalsePositiveTestData (the same array that already contains
ReturnsAsync/ThrowsAsync/Callback cases) so the analyzer test verifies that
parenthesized chains do not trigger Moq1203.
Rename VerifyMock and VerifyMockIgnoringCompilerErrors to follow the project convention for private async methods (consistent with VerifyAnalyzerDynamicallyAsync, CreateProjectAsync, etc.). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…886) ## Summary - Walk the full method chain in `HasReturnValueSpecification` instead of only checking the immediate parent, fixing false positives when `Callback()` precedes `Returns()`/`Throws()` - Recognize `ReturnsAsync` and `ThrowsAsync` as valid return value specification methods alongside `Returns` and `Throws` - Replace string-based method name matching with symbol-based detection using `MoqKnownSymbols` and `ISymbolExtensions`, consistent with project conventions - Fix `IsInstanceOf` to handle reduced extension methods (via `ReducedFrom`) so extension methods like `ReturnsAsync`/`ThrowsAsync` are correctly identified - Add tests exercising the `OverloadResolutionFailure` candidate symbol fallback path - Consolidate `AnalyzerVerifier` overloads and fix `AllAnalyzersVerifier` reflection lookup - Simplify `HasReturnValueSymbol` to scan all candidate symbols, not just `OverloadResolutionFailure` Fixes #849 ## Test plan - [x] All 20 regression tests pass (ReturnsAsync, Callback+Returns, Callback+ReturnsAsync, Callback+Throws) - [x] All 122 Moq1203 tests pass (including 8 new OverloadResolutionFailure tests) - [x] Full test suite (1925 tests) passes with no regressions - [x] All changed lines have coverage; remaining gaps are pre-existing unreachable guard clauses - [ ] CI passes 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Analyzer now reliably detects return/exception specifications (Returns/ReturnsAsync/Throws/ThrowsAsync) across fluent chains (including Callback) and handles async and overload-resolution edge cases. * **Documentation** * Guidance and examples updated to cover async variants, chain-aware placement, and clarified applicability/edge cases. * **Tests** * Added extensive regression and scenario tests for async chains, callback combinations, overload-resolution situations, and custom mock scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
|
A code like this: validator.Setup(v => v.ValidateAsync(It.IsAny<ValidationContext<AsyncOperationResult>>(), It.IsAny<CancellationToken>()))
.ReturnsAsync(result);is flagged for Moq1203 after updating to |
Summary
HasReturnValueSpecificationinstead of only checking the immediate parent, fixing false positives whenCallback()precedesReturns()/Throws()ReturnsAsyncandThrowsAsyncas valid return value specification methods alongsideReturnsandThrowsMoqKnownSymbolsandISymbolExtensions, consistent with project conventionsIsInstanceOfto handle reduced extension methods (viaReducedFrom) so extension methods likeReturnsAsync/ThrowsAsyncare correctly identifiedOverloadResolutionFailurecandidate symbol fallback pathAnalyzerVerifieroverloads and fixAllAnalyzersVerifierreflection lookupHasReturnValueSymbolto scan all candidate symbols, not justOverloadResolutionFailureFixes #849
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit