perf: add Moq1208 benchmarks for issue #777 completion#1089
perf: add Moq1208 benchmarks for issue #777 completion#1089
Conversation
Add performance benchmarks for the Moq1208 analyzer that detects Returns() delegate type mismatches on async method setups. Follows the established benchmark pattern from Moq1206. Refs: #777 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 completes the remaining deliverables for issue #777 by introducing performance benchmarks for the 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
|
📝 WalkthroughWalkthroughAdded a clarifying comment to the ReturnsDelegateShouldReturnTaskFixer and introduced a new BenchmarkDotNet test class Moq1208ReturnsDelegateBenchmarks that builds compilations and validates analyzer diagnostics across parameterized inputs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene  |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| C# |  |
Mar 31, 2026 4:58p.m. | Review ↗ |
![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 adds benchmarks for the Moq1208 analyzer, which is a great addition. The implementation is solid. The suggested improvements to the benchmark setup have been retained as they enhance clarity and accuracy of performance measurements, aligning with good practices for performance-sensitive code.
…markCategory
Moq's Returns() has no generic overload so the GenericNameSyntax path in
ReturnsDelegateShouldReturnTaskFixer.ReplaceReturnsWithReturnsAsync was
unreachable. Removing it eliminates the 50% branch coverage gap, simplifies
the fixer, and removes a maintenance liability.
Also adds [BenchmarkCategory("Moq1208")] to the new benchmark class to
match the Moq1201 convention.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Incorrect comment; Returns() does have generic overloads
- Restored GenericNameSyntax handling to preserve type arguments when replacing Returns with ReturnsAsync, since Moq's IReturns<TMock, TResult> interface does define method-level generic overloads like Returns(Func<T1, TResult>).
…with ReturnsAsync Moq's IReturns<TMock, TResult> interface defines method-level generic overloads like Returns<T1>(Func<T1, TResult>). When a user writes .Returns<string>(s => 42), Roslyn represents this as GenericNameSyntax. The previous code fix incorrectly dropped these type arguments. This restores the GenericNameSyntax handling to preserve the developer's explicit type arguments in the generated ReturnsAsync call.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Rename Moq1208WithoutDiagnostics to Moq1208Baseline and add Baseline = true to align with the convention used in other benchmark files (e.g., Moq1201AsyncResultBenchmarks). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@tests/Moq.Analyzers.Benchmarks/Moq1208ReturnsDelegateBenchmarks.cs`:
- Around line 12-20: Add XML documentation comments for the public benchmark API
surface: add a summary for the class Moq1208ReturnsDelegateBenchmarks and for
each public member FileCount and MoqKey, and add appropriate <summary>,
<returns> and <param> (if any) for the methods SetupCompilation,
Moq1208WithDiagnostics, and Moq1208Baseline so every public symbol listed has
complete XML docs; ensure summaries describe purpose, parameters and return
values where applicable and follow the project's XML doc style.
- Around line 69-70: The test currently uses null-forgiving on TestCompilation
(e.g., (await TestCompilation!.GetAnalysisResultAsync(CancellationToken.None)))
which can throw opaque NREs if setup didn't run; update the benchmark methods
that call TestCompilation!.GetAnalysisResultAsync (and any similar calls at
lines referenced) to first assert or guard that TestCompilation is not null (for
example throw a clear InvalidOperationException or use Assert.NotNull on
TestCompilation) and only then call GetAnalysisResultAsync; ensure all
occurrences (including the calls around line 86/87) have the same explicit guard
so failures surface as clear setup errors rather than NREs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7bc2a7f1-383f-4b79-8029-e0925520e896
📒 Files selected for processing (2)
src/CodeFixes/ReturnsDelegateShouldReturnTaskFixer.cstests/Moq.Analyzers.Benchmarks/Moq1208ReturnsDelegateBenchmarks.cs
| public class Moq1208ReturnsDelegateBenchmarks | ||
| { | ||
| #pragma warning disable ECS0900 | ||
| [Params(1, 1_000)] | ||
| #pragma warning restore ECS0900 | ||
| public int FileCount { get; set; } | ||
|
|
||
| [Params("Net80WithOldMoq", "Net80WithNewMoq")] | ||
| public string MoqKey { get; set; } = "Net80WithOldMoq"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add XML documentation for the public benchmark API surface.
This file introduces public members without XML docs (Moq1208ReturnsDelegateBenchmarks, FileCount, MoqKey, SetupCompilation, Moq1208WithDiagnostics, Moq1208Baseline).
As per coding guidelines, **/*.cs: "Ensure all public APIs have complete XML documentation".
Also applies to: 28-28, 66-66, 83-83
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[notice] 15-15: tests/Moq.Analyzers.Benchmarks/Moq1208ReturnsDelegateBenchmarks.cs#L15
Assign this magic number '1000' to a well-named variable or constant, and use that instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Moq.Analyzers.Benchmarks/Moq1208ReturnsDelegateBenchmarks.cs` around
lines 12 - 20, Add XML documentation comments for the public benchmark API
surface: add a summary for the class Moq1208ReturnsDelegateBenchmarks and for
each public member FileCount and MoqKey, and add appropriate <summary>,
<returns> and <param> (if any) for the methods SetupCompilation,
Moq1208WithDiagnostics, and Moq1208Baseline so every public symbol listed has
complete XML docs; ensure summaries describe purpose, parameters and return
values where applicable and follow the project's XML doc style.
| (await TestCompilation! | ||
| .GetAnalysisResultAsync(CancellationToken.None) |
There was a problem hiding this comment.
Replace null-forgiving dereferences with explicit setup guards.
Line 69 and Line 86 assume setup always ran; if not, benchmark failures degrade to opaque NREs. Add explicit state checks before analysis calls.
💡 Suggested change
[Benchmark]
public async Task Moq1208WithDiagnostics()
{
+ if (TestCompilation is null)
+ {
+ throw new InvalidOperationException($"{nameof(SetupCompilation)} must run before {nameof(Moq1208WithDiagnostics)}.");
+ }
+
ImmutableArray<Diagnostic> diagnostics =
- (await TestCompilation!
+ (await TestCompilation
.GetAnalysisResultAsync(CancellationToken.None)
.ConfigureAwait(false))
.AssertValidAnalysisResult()
.GetAllDiagnostics();
@@
[Benchmark(Baseline = true)]
public async Task Moq1208Baseline()
{
+ if (BaselineCompilation is null)
+ {
+ throw new InvalidOperationException($"{nameof(SetupCompilation)} must run before {nameof(Moq1208Baseline)}.");
+ }
+
ImmutableArray<Diagnostic> diagnostics =
- (await BaselineCompilation!
+ (await BaselineCompilation
.GetAnalysisResultAsync(CancellationToken.None)
.ConfigureAwait(false))
.AssertValidAnalysisResult()
.GetAllDiagnostics();Also applies to: 86-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Moq.Analyzers.Benchmarks/Moq1208ReturnsDelegateBenchmarks.cs` around
lines 69 - 70, The test currently uses null-forgiving on TestCompilation (e.g.,
(await TestCompilation!.GetAnalysisResultAsync(CancellationToken.None))) which
can throw opaque NREs if setup didn't run; update the benchmark methods that
call TestCompilation!.GetAnalysisResultAsync (and any similar calls at lines
referenced) to first assert or guard that TestCompilation is not null (for
example throw a clear InvalidOperationException or use Assert.NotNull on
TestCompilation) and only then call GetAnalysisResultAsync; ensure all
occurrences (including the calls around line 86/87) have the same explicit guard
so failures surface as clear setup errors rather than NREs.
Document that Returns<T>(lambda) with explicit generic type arguments is not detected by the analyzer due to Roslyn target-type inference masking the delegate return type mismatch. Added to ValidWithCompilerSuppression to prevent regressions if the analyzer behavior changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| ["""new Mock<AsyncService>().Setup(c => c.ProcessAsync(It.IsAny<string>())).Returns<string>(async s => s.Length);"""], | ||
| }; | ||
|
|
||
| return data.WithNamespaces().WithMoqReferenceAssemblyGroups(); |
|
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
Adds performance benchmarks for the Moq1208 analyzer and updates related tests/fixer commentary to support the rule’s completion work for issue #777.
Changes:
- Added a new BenchmarkDotNet benchmark suite for Moq1208 to measure analyzer overhead and allocations across multiple source-file counts and Moq versions.
- Extended Moq1208 analyzer test data with additional
Returns<T1>(...)scenarios under compiler-diagnostic suppression. - Updated/expanded inline commentary in the Moq1208 code fix about
GenericNameSyntaxhandling for genericReturns<T1>(...)calls.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/Moq.Analyzers.Test/ReturnsDelegateShouldReturnTaskAnalyzerTests.cs | Adds new “valid with compiler suppression” test inputs around generic Returns<T1> patterns. |
| tests/Moq.Analyzers.Benchmarks/Moq1208ReturnsDelegateBenchmarks.cs | Introduces a new Moq1208 benchmark class following the repo’s existing benchmark structure and parameters. |
| src/CodeFixes/ReturnsDelegateShouldReturnTaskFixer.cs | Adds explanatory comments around preserving type arguments when rewriting Returns → ReturnsAsync. |
You can also share your feedback on Copilot code review. Take the survey.
| // Generic Returns<T> with sync lambda: target-type inference masks the mismatch (analyzer gap, see PR #1089) | ||
| ["""new Mock<AsyncService>().Setup(c => c.ProcessAsync(It.IsAny<string>())).Returns<string>(s => s.Length);"""], |
| // Generic Returns<T> with async lambda (no mismatch) | ||
| ["""new Mock<AsyncService>().Setup(c => c.ProcessAsync(It.IsAny<string>())).Returns<string>(async s => s.Length);"""], |
| // Moq's IReturns<TMock, TResult> interface defines method-level generic overloads | ||
| // like Returns<T1>(Func<T1, TResult>). When a user writes .Returns<string>(s => 42), | ||
| // the syntax is GenericNameSyntax. Preserve type arguments to maintain developer intent. | ||
| if (oldName is GenericNameSyntax genericName) |
rjmurillo-bot
changed the title
|
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 1 medium |
| CodeStyle | 2 minor |
🟢 Metrics 6 complexity . 5 duplication
Metric Results Complexity 6 Duplication 5
🟢 Coverage ∅ diff coverage · +0.00% coverage variation
Metric Results Coverage variation ✅ +0.00% coverage variation (-1.00%) Diff coverage ✅ ∅ diff coverage (95.00%) Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (2a88555) 2660 2391 89.89% Head commit (dd0251f) 2660 (+0) 2391 (+0) 89.89% (+0.00%) 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 (#1089) 0 0 ∅ (not applicable) 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
Completes Moq1208 (ReturnsDelegateShouldReturnTask) by adding benchmarks and removing dead code from the fixer.
Closes #777
Changes
Moq1208ReturnsDelegateBenchmarks.csfollowing the established benchmark patternGenericNameSyntaxbranch fromReturnsDelegateShouldReturnTaskFixer.cs(Moq has noReturns<T>()generic overload)[BenchmarkCategory("Moq1208")]and[Benchmark(Baseline = true)]attributesPerformance
The Moq1208 benchmark is new (no baseline comparison available in CI). First baseline will be established on merge. For reference, comparable IOperation-based analyzers on the same CI runner (ARM64 Neoverse-N2):
Moq1208 performs similar work to Moq1200 (inspects invocation return types against the mocked method's return type). Expected overhead should be in the 2,000-6,000 us range per diagnostic file, with ~200 KB allocation. The baseline (empty analyzer) cost is ~235 us regardless of analyzer, dominated by Roslyn compilation setup.
Key performance characteristics:
IsMockReferenced()when Moq is not referencedEnableConcurrentExecution()enabledDead code removal justification
The
GenericNameSyntaxbranch inReplaceReturnsWithReturnsAsyncwas unreachable:Returns<T>()generic overloadValueText == "Returns") ensures onlyIdentifierNamenodes reach the methodThree defensive null guards in
RegisterCodeFixesAsyncare retained per project convention (Roslyn CodeFix contract pattern).Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Returns<T>patterns in mock setups with synchronous and asynchronous lambda scenarios.