fix: suppress Moq1203 for setup wrapped in extension methods#1086
fix: suppress Moq1203 for setup wrapped in extension methods#1086
Conversation
…ethods (#1067) When a Returns/ReturnsAsync call is wrapped inside an extension method that returns a Moq fluent interface type (e.g., IReturns<TMock, TResult>), the Moq1203 analyzer no longer reports a false positive. The analyzer now checks whether a non-Moq method in the invocation chain returns a type that implements a Moq fluent interface, indicating the method wraps the setup configuration internally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, 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 a specific false positive in the Moq1203 analyzer, which previously flagged valid Moq setups that utilized custom extension methods to wrap 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 extended to treat non‑Moq wrapper/extension methods that return Moq fluent interfaces as contributing return‑value specifications; added fluent‑interface detection helpers, two MoqKnownSymbols entries, and tests for wrapped fluent vs non‑fluent scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Tests as TestRunner
participant Analyzer as MethodSetupAnalyzer
participant SymbolHelpers as ISymbolExtensions.Moq
participant KnownSymbols as MoqKnownSymbols
rect rgba(0,128,255,0.5)
Tests->>Analyzer: run analysis on setup expression
end
rect rgba(0,200,83,0.5)
Analyzer->>SymbolHelpers: inspect member‑access chain / return types
SymbolHelpers->>KnownSymbols: query fluent‑interface symbols (IReturns2, IReturnsResult1, IThrowsResult, ...)
KnownSymbols-->>SymbolHelpers: return matched symbols
SymbolHelpers-->>Analyzer: ImplementsMoqFluentInterface result
end
rect rgba(255,128,0,0.5)
Analyzer->>Analyzer: evaluate IsFluentChainWrapperMethod / IsWrapperMethod -> decide HasReturnValueSpecification
Analyzer-->>Tests: produce diagnostic (flag or not)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| C# |  |
Mar 31, 2026 5:00p.m. | Review ↗ |
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs
Show resolved
Hide resolved
![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 is a great fix that addresses a tricky false positive with extension methods in Moq setups. The approach of inspecting the return type of chained methods to see if they implement a Moq fluent interface is a clever and robust solution. The new tests are very thorough, covering both cases where the diagnostic should be suppressed and where it should still be triggered. I have one minor suggestion to improve code clarity.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Incomplete Moq method skip logic in wrapper detection
- Added IsMoqRaisesMethod to the skip condition in IsFluentChainWrapperMethod to ensure Moq Raises methods are not incorrectly treated as user-defined wrappers.
Include IsMoqRaisesMethod check in IsFluentChainWrapperMethod to ensure Moq Raises methods are properly skipped when detecting user-defined wrapper methods. This prevents incorrect diagnostic suppression when a Raises method's return type implements a Moq fluent interface.
…sFluentChainWrapperMethod IsFluentChainWrapperMethod used FirstOrDefault when iterating candidate symbols, meaning overload ambiguity could cause a false negative if the first candidate's return type did not implement a Moq fluent interface while a later one did. HasReturnValueSymbol already uses Any() for the same scenario; align IsFluentChainWrapperMethod to the same pattern. Extract the per-symbol predicate into IsWrapperMethod so both the resolved-symbol and candidate-symbol paths share identical logic with no duplication. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Wrapper detection too broad for non-return-value fluent types
- Removed ICallback, ICallback1, and ICallback2 from IsMatchingFluentSymbol since these interfaces do not inherit from IReturns and do not indicate return value configuration.
The IsMatchingFluentSymbol method was incorrectly including ICallback, ICallback1, and ICallback2 as fluent types that suppress the Moq1203 diagnostic. These ICallback types do not inherit from IReturns and do not indicate that a return value has been configured. Types like ISetup<TMock, TResult> that inherit from IReturns are still caught via HasMoqFluentInterfaceInHierarchy when checking AllInterfaces. Fixes #1067
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…1067) Remove 2 phantom symbols from IsMatchingFluentSymbol that never resolve because they do not exist in Moq: - Moq.Language.IReturns (arity 0) - Moq.Language.IReturns`1 (arity 1) Add 2 missing symbols that are the actual return types of .Returns() and .Throws(): - Moq.Language.Flow.IReturnsResult`1 (return type of .Returns/.ReturnsAsync) - Moq.Language.Flow.IThrowsResult (return type of .Throws) Add test coverage for extension methods returning IReturnsResult<TMock> and IThrowsResult. Refs: #1067 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs`:
- Around line 637-711: BuildWrappedSetupSource is too long (75 lines); extract
the nested extension-method definitions from inside BuildWrappedSetupSource into
a separate reusable constant or helper to reduce method length. Move the entire
MoqExtensions class (including ReturnsTrue, ReturnsFortyTwo, ReturnsOneAsync,
ReturnsTrueViaResult, ThrowsInvalidOp, LogSetup, ToDescription) out of the
BuildWrappedSetupSource string builder and reference or concatenate that
extracted constant/helper when constructing the returned source string in
BuildWrappedSetupSource so the method body falls below the 50-line guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f22128f-00cc-480f-8220-93be3d5fa546
📒 Files selected for processing (4)
src/Analyzers/MethodSetupShouldSpecifyReturnValueAnalyzer.cssrc/Common/ISymbolExtensions.Moq.cssrc/Common/WellKnown/MoqKnownSymbols.cstests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs
tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs
Show resolved
Hide resolved
Adds a negative-path positive test case covering an extension method that returns Task (not a Moq fluent type) chained after Setup(). The analyzer must still flag Moq1203 in this case. Also adds the AsTask helper to the wrapped-setup source builder used by this test class. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs`:
- Around line 421-465: The regression test Issue1067_WrappedSetupTestData is
missing async wrapper cases; add two test entries: one where an extension wraps
ReturnsAsync (e.g., a call to ReturnsOneAsyncViaReturnsAsync()) and one for a
ValueTask<T> wrapper (e.g., ReturnsOneValueTask()) to exercise Task and
ValueTask branches. Update the IFoo test interface to include
BazValueTaskAsync() and add corresponding helper extension methods
ReturnsOneAsyncViaReturnsAsync and ReturnsOneValueTask to MoqExtensions so the
new test strings compile and the analyzer sees the ReturnsAsync and
ValueTask-wrapping paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3a17401-088b-42f9-b6eb-cb8ad3f65f2a
📒 Files selected for processing (1)
tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs
tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs
Show resolved
Hide resolved
…1067) Add missing test coverage for IReturnsResult1 and IThrowsResult properties added in PR #1086. Four tests follow the established pattern: - IReturnsResult1_WithMoqReference_ReturnsNamedTypeSymbol - IReturnsResult1_WithoutMoqReference_ReturnsNull - IThrowsResult_WithMoqReference_ReturnsNamedTypeSymbol - IThrowsResult_WithoutMoqReference_ReturnsNull Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Analyzers/MethodSetupShouldSpecifyReturnValueAnalyzer.cs`:
- Around line 268-278: IsWrapperMethod currently treats any method whose
ReturnType.ImplementsMoqFluentInterface(knownSymbols) as a return-value
specification, which wrongfully suppresses Moq1203 for passthrough wrappers
returning pre-configuration interfaces; change the suppression so it only
applies to post-configuration fluent interfaces (the types that represent an
already-configured Returns/Throws chain). Concretely, update IsWrapperMethod to
call a new or renamed helper (e.g.,
ImplementsMoqPostConfigurationInterface(knownSymbols)) instead of
ImplementsMoqFluentInterface, and implement that helper to detect only the
post-configuration interfaces (the interfaces representing configured
return-value specifications) while leaving pre-configuration interfaces like
ISetup<TMock, TResult>, IReturns<TMock, TResult>, IThrows<TMock, TResult>
uncovered; keep the existing checks for IsMoqReturnValueSpecificationMethod,
IsMoqCallbackMethod, and IsMoqRaisesMethod unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2c90bd5-467f-4c66-92a6-201f64b49b6d
📒 Files selected for processing (1)
src/Analyzers/MethodSetupShouldSpecifyReturnValueAnalyzer.cs
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Pull request overview
Fixes Moq1203 false positives by teaching MethodSetupShouldSpecifyReturnValueAnalyzer to treat fluent extension/wrapper methods as return-value specifications when they preserve the Moq fluent-chain return types.
Changes:
- Added detection for “wrapper” methods in the invocation chain whose return type implements a Moq fluent interface, to suppress Moq1203 in wrapped setup scenarios.
- Extended
MoqKnownSymbols/ symbol helpers withIReturnsResult<TMock>andIThrowsResultsupport plus anImplementsMoqFluentInterfacehelper. - Added regression tests for fluent extension wrappers (suppressed) and non-fluent wrappers (still flagged), plus new
MoqKnownSymbolstests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Analyzers/MethodSetupShouldSpecifyReturnValueAnalyzer.cs |
Suppresses Moq1203 when a non-Moq method in the chain returns a Moq fluent interface type. |
src/Common/ISymbolExtensions.Moq.cs |
Adds ImplementsMoqFluentInterface and matching logic for Moq fluent interface types. |
src/Common/WellKnown/MoqKnownSymbols.cs |
Adds known-symbol lookups for IReturnsResult<TMock> and IThrowsResult. |
tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs |
Adds issue #1067 regression tests for fluent/non-fluent extension wrappers. |
tests/Moq.Analyzers.Test/Common/MoqKnownSymbolsTests.cs |
Adds verification that the new known symbols resolve/null correctly with/without Moq references. |
You can also share your feedback on Copilot code review. Take the survey.
tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs
Show resolved
Hide resolved
tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs
Show resolved
Hide resolved
…sts (#1067) Remove dead comparisons against IReturns (arity 0) and IReturns1 (arity 1) from IsMatchingFluentSymbol. These types resolve to null in Moq 4.18+ and the comparisons always evaluated to false. Fix XML doc comments for accuracy: - IsMatchingFluentSymbol: correct type parameter names, clarify arity-0/1 resolve to null rather than claiming non-existence - ImplementsMoqFluentInterface: describe behavior, not caller intent - IsCallbackRaisesMethod: include ISetupGetter and ISetupSetter in summary - IsFluentChainWrapperMethod: state assumption rather than assertion - IsWrapperMethod: mention raises methods in exclusion list - MockRepositoryCreate: fix doc that said "Of" instead of "Create" Add test coverage for ReturnsAsync wrapper, ValueTask wrapper, and NoOpPassthrough (documenting accepted false-negative tradeoff). Deduplicate MoqKnownSymbolsTests added by upstream merge. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Comprehensibility | 1 minor |
| CodeStyle | 1 minor |
| Complexity | 1 medium |
🟢 Metrics 0 duplication
Metric Results Duplication 0
🟢 Coverage 96.88% diff coverage · +0.08% coverage variation
Metric Results Coverage variation ✅ +0.08% coverage variation (-1.00%) Diff coverage ✅ 96.88% diff coverage (95.00%) Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (2a88555) 2660 2391 89.89% Head commit (b933888) 2692 (+32) 2422 (+31) 89.97% (+0.08%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#1086) 32 31 96.88% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
TIP This summary will be updated as you push new changes. Give us feedback
Summary
Returns/ReturnsAsyncis called inside an extension method that wraps the setup configuration (fixes Improve invocation chain analysis to recognize wrapped setup configurations #1067)ImplementsMoqFluentInterfaceto detect when a non-Moq method in the invocation chain returns a Moq fluent interface type (IReturns,ICallback,ISetup, etc.), suppressing the diagnosticTest plan
dotnet build -c Release /p:PedanticMode=true(0 warnings, 0 errors)IReturns<TMock, TResult>do not trigger Moq1203voidorstringstill trigger Moq1203🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation