fix: resolve delegate-overload resolution for Moq1203 and Moq1206#919
fix: resolve delegate-overload resolution for Moq1203 and Moq1206#919
Conversation
GetSymbolInfo on MemberAccessExpressionSyntax lacks argument context, so Roslyn cannot resolve delegate-based overloads like ReturnsAsync((MyValue val) => val). Query the parent InvocationExpressionSyntax instead, and add a name-based fallback for cases where IsInstanceOf still cannot match the constructed generic method against known Moq symbols. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
IsReturnsMethodCallWithAsyncLambda queried GetSymbolInfo on the MemberAccessExpressionSyntax, missing argument context. Switch to querying the InvocationExpressionSyntax and add CandidateSymbols fallback (previously missing entirely), so delegate-typed async lambdas like Returns(async (string x) => x) are detected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR fixes two related analyzer false positives by improving symbol resolution when Moq's delegate-based overloads (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
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 significantly enhances the accuracy of the Moq.Analyzers by addressing specific issues related to how 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
|
  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}
tests/Moq.Analyzers.Test/ReturnsAsyncShouldBeUsedForAsyncMethodsAnalyzerTests.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 pull request effectively resolves two issues with delegate overload resolution in Moq analyzers. For Moq1203, it correctly gets symbol information from the invocation expression to provide argument context to Roslyn and adds a name-based fallback. For Moq1206, it also queries the invocation and adds a fallback to candidate symbols. The changes are logical, well-commented, and supported by new tests that confirm the fixes for both the false positive and false negative cases. I have one minor suggestion to improve code conciseness.
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 |
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: 2
🤖 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 167-180: Update the repository documentation to reflect the
analyzer behavior change introduced around return-value detection: document that
the analyzer now checks both HasReturnValueSymbol(symbolInfo, knownSymbols) and
falls back to
IsKnownReturnValueMethodName(memberAccess.Name.Identifier.ValueText,
knownSymbols) when resolving symbols via semanticModel for
invocation/memberAccess expressions, and add a note in docs/rules/ and README.md
describing the name-based fallback rationale and examples (mention invocation,
memberAccess, semanticModel, HasReturnValueSymbol and
IsKnownReturnValueMethodName) so readers understand the exact matching order and
when the fallback is used.
In
`@tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs`:
- Around line 367-376: The two delegate-based ReturnsAsync "no diagnostics" test
cases that use ReturnsAsync((MyValue val) => val) (the entries with "var
databaseMock = new Mock<IDatabase>(MockBehavior.Strict)...ReturnsAsync((MyValue
val) => val);" and "new Mock<IDatabase>().Setup(...).ReturnsAsync((MyValue val)
=> val);") should be executed via AllAnalyzersVerifier.VerifyAllAnalyzersAsync()
instead of the analyzer-specific verifier; update the test that currently
asserts no diagnostics for these cases to call
AllAnalyzersVerifier.VerifyAllAnalyzersAsync(...) with the combined test input
(including the "public record MyValue;" and "Task<MyValue> SaveAsync(MyValue
value);" snippets) so they run through all analyzers per the test guidelines.
tests/Moq.Analyzers.Test/MethodSetupShouldSpecifyReturnValueAnalyzerTests.cs
Show resolved
Hide resolved
## Summary - **Moq1203 false positive (#910):** `GetSymbolInfo` on `MemberAccessExpressionSyntax` lacks argument context, so Roslyn cannot resolve delegate-based overloads like `.ReturnsAsync((MyValue val) => val)`. Now queries the parent `InvocationExpressionSyntax` and adds a name-based fallback for cases where `IsInstanceOf` cannot match constructed generic methods. - **Moq1206 false negative (#911):** `IsReturnsMethodCallWithAsyncLambda` queried `GetSymbolInfo` on the member access instead of the invocation, and had no `CandidateSymbols` fallback. Now queries the invocation and falls back to candidate symbols, so delegate-typed async lambdas like `Returns(async (string x) => x)` are detected. Closes #910 Closes #911 ## Test plan - [x] `dotnet build` passes with zero errors and zero warnings - [x] All 288 Moq1203 tests pass (including 8 new delegate-overload cases) - [x] All 92 Moq1206 tests pass (including 4 new delegate-overload cases) - [x] Full suite of 2,497 tests passes with no regressions 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Moq analyzers now correctly identify return value specifications in complex method setup chains, with improved handling of chained method invocations and fallback matching. * Fixed detection of async delegate lambdas within Returns calls for more accurate recognition of valid Moq method calls. * **Tests** * Added test coverage for delegate-based ReturnsAsync scenarios in complex setups. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
GetSymbolInfoonMemberAccessExpressionSyntaxlacks argument context, so Roslyn cannot resolve delegate-based overloads like.ReturnsAsync((MyValue val) => val). Now queries the parentInvocationExpressionSyntaxand adds a name-based fallback for cases whereIsInstanceOfcannot match constructed generic methods.IsReturnsMethodCallWithAsyncLambdaqueriedGetSymbolInfoon the member access instead of the invocation, and had noCandidateSymbolsfallback. Now queries the invocation and falls back to candidate symbols, so delegate-typed async lambdas likeReturns(async (string x) => x)are detected.Closes #910
Closes #911
Test plan
dotnet buildpasses with zero errors and zero warnings🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests