Conversation
Replace RegisterSyntaxNodeAction with RegisterCompilationStartAction and RegisterOperationAction using IInvocationOperation. Use MoqKnownSymbols for symbol-based detection of SetupGet, SetupSet, and SetupProperty methods instead of string-based matching. Add Mock1SetupGet, Mock1SetupSet, and Mock1SetupProperty properties to MoqKnownSymbols for resolving property setup method symbols. Fixes #337 Co-Authored-By: Claude Opus 4.6 <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 significantly refactors the NoMethodsInPropertySetupAnalyzer to improve its robustness and maintainability. By transitioning from syntax-node-based analysis to IOperation-based analysis and leveraging symbol-based method identification, the analyzer now more accurately detects invalid method calls within Moq property setups. This change enhances the reliability of the analyzer and standardizes its implementation with other existing analyzers. 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
|
📝 WalkthroughWalkthroughConverts NoMethodsInPropertySetupAnalyzer from syntax-node to compilation-start + IOperation analysis, adds semantic-symbol checks for Moq property setup methods, and exposes three new MoqKnownSymbols properties for SetupGet/SetupSet/SetupProperty. Adds tests and a reference-assembly entry for .NET 8 without Moq. Changes
Sequence Diagram(s)sequenceDiagram
participant Comp as CompilationStart
participant Analyzer as NoMethodsInPropertySetupAnalyzer
participant Known as MoqKnownSymbols
participant Op as InvocationOperation
participant SM as SemanticModel
participant Reporter as DiagnosticReporter
Comp->>Analyzer: RegisterCompilationStartAction(context)
Analyzer->>Known: Resolve Mock`1 and collect Setup* method symbols
Analyzer-->>Comp: If no Moq symbols, skip registering Op handler
Comp->>Analyzer: Invocation operation occurs (IInvocationOperation)
Analyzer->>Op: OperationAction invoked with IInvocationOperation
Analyzer->>SM: Obtain SemanticModel from operation
Op->>SM: Resolve mocked method symbol from invocation
Analyzer->>Reporter: Create/report diagnostic using resolved method symbol (if violation)
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 unit tests (beta)
Comment |
 |
|
Overall Grade Focus Area: Hygiene |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| C# |  |
Mar 1, 2026 9:17p.m. | Review ↗ |
Replace null-forgiving operator on SemanticModel with explicit null check, matching the established pattern used by all other IOperation analyzers in the codebase. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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 is a great step forward in modernizing the analyzers by converting NoMethodsInPropertySetupAnalyzer to use the IOperation API. This improves performance and correctness by relying on semantic symbols instead of string matching.
My review includes a suggestion to fully complete the transition to IOperation within the analyzer, which also resolves a potential issue with block-bodied lambdas. I've also noted a minor scope creep issue to help keep commits focused.
| // The lambda argument to SetupGet/SetupSet/SetupProperty contains the mocked member access. | ||
| // If the lambda body is an invocation (method call), that is invalid for property setup. | ||
| InvocationExpressionSyntax? mockedMethodCall = | ||
| (invocationOperation.Syntax as InvocationExpressionSyntax).FindMockedMethodInvocationFromSetupMethod(); | ||
|
|
||
| if (setupGetOrSetInvocation.Expression is not MemberAccessExpressionSyntax setupGetOrSetMethod) return; | ||
| if (!string.Equals(setupGetOrSetMethod.Name.ToFullString(), "SetupGet", StringComparison.Ordinal) | ||
| && !string.Equals(setupGetOrSetMethod.Name.ToFullString(), "SetupSet", StringComparison.Ordinal) | ||
| && !string.Equals(setupGetOrSetMethod.Name.ToFullString(), "SetupProperty", StringComparison.Ordinal)) return; | ||
| if (mockedMethodCall == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| InvocationExpressionSyntax? mockedMethodCall = setupGetOrSetInvocation.FindMockedMethodInvocationFromSetupMethod(); | ||
| if (mockedMethodCall == null) return; | ||
| SemanticModel? semanticModel = invocationOperation.SemanticModel; | ||
| if (semanticModel == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| ISymbol? mockedMethodSymbol = context.SemanticModel.GetSymbolInfo(mockedMethodCall, context.CancellationToken).Symbol; | ||
| if (mockedMethodSymbol == null) return; | ||
| ISymbol? mockedMethodSymbol = semanticModel.GetSymbolInfo(mockedMethodCall, context.CancellationToken).Symbol; | ||
| if (mockedMethodSymbol == null) | ||
| { | ||
| return; |
There was a problem hiding this comment.
This implementation for finding the mocked method call mixes IOperation with SyntaxNode analysis by falling back to FindMockedMethodInvocationFromSetupMethod. This partially defeats the purpose of refactoring to IOperation.
Additionally, the FindMockedMethodInvocationFromSetupMethod helper only works for expression-bodied lambdas (x => x.Method()) and will fail for block-bodied lambdas (x => { return x.Method(); }), as LambdaExpressionSyntax.Body would be a BlockSyntax.
You can implement this logic purely using the IOperation tree, which will be more robust and align better with the refactoring goal. This also provides an opportunity to correctly handle block-bodied lambdas.
// The lambda argument to SetupGet/SetupSet/SetupProperty contains the mocked member access.
// If the lambda body is an invocation (method call), that is invalid for property setup.
if (invocationOperation.Arguments.Length == 0)
{
return;
}
IOperation argument = invocationOperation.Arguments[0].Value.WalkDownImplicitConversion();
if (argument is not IAnonymousFunctionOperation lambda)
{
return;
}
IOperation? body = lambda.Body;
if (body is IBlockOperation block && block.Operations.Length == 1 && block.Operations[0] is IReturnOperation returnOp)
{
body = returnOp.ReturnedValue;
}
if (body is IInvocationOperation mockedMethodInvocation)
{
IMethodSymbol mockedMethodSymbol = mockedMethodInvocation.TargetMethod;
Diagnostic diagnostic = mockedMethodInvocation.Syntax.CreateDiagnostic(Rule, mockedMethodSymbol.Name);
context.ReportDiagnostic(diagnostic);
}| /// <summary> | ||
| /// Gets the interface <c>Microsoft.Extensions.Logging.ILogger</c>. | ||
| /// </summary> | ||
| internal INamedTypeSymbol? ILogger => TypeProvider.GetOrCreateTypeByMetadataName("Microsoft.Extensions.Logging.ILogger"); | ||
|
|
||
| /// <summary> | ||
| /// Gets the interface <c>Microsoft.Extensions.Logging.ILogger{T}</c>. | ||
| /// </summary> | ||
| internal INamedTypeSymbol? ILogger1 => TypeProvider.GetOrCreateTypeByMetadataName("Microsoft.Extensions.Logging.ILogger`1"); |
There was a problem hiding this comment.
These additions for ILogger and ILogger<T> seem unrelated to the primary goal of this pull request, which is to refactor NoMethodsInPropertySetupAnalyzer. To maintain clean and focused commits, it would be better to move these changes to a separate pull request.
Remove ILogger and ILogger1 properties that were accidentally included. These belong to a separate feature branch and should not be part of this refactoring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Refactors NoMethodsInPropertySetupAnalyzer to use IOperation-based analysis with compilation-start symbol precomputation, aligning with the repository’s symbol-based detection approach and avoiding string-based method name matching.
Changes:
- Converted
NoMethodsInPropertySetupAnalyzerfromRegisterSyntaxNodeActiontoRegisterCompilationStartAction+RegisterOperationActionusingIInvocationOperation. - Added
Mock1SetupGet,Mock1SetupSet, andMock1SetupPropertytoMoqKnownSymbolsto enable symbol-based identification of property setup APIs. - Introduced
ILogger/ILogger1symbol properties inMoqKnownSymbols(currently unused and not described in PR metadata).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/Common/WellKnown/MoqKnownSymbols.cs |
Adds well-known method symbols for Mock<T>.SetupGet/SetupSet/SetupProperty to support symbol-based analyzer matching; also adds ILogger/ILogger1 symbols. |
src/Analyzers/NoMethodsInPropertySetupAnalyzer.cs |
Refactors analyzer to operation-based invocation analysis and uses MoqKnownSymbols method symbols instead of string comparisons. |
Comments suppressed due to low confidence (2)
src/Common/WellKnown/MoqKnownSymbols.cs:430
- The new
ILogger/ILogger1symbols appear unrelated to this PR’s stated goal (property setup analyzer refactor) and are currently unused in the codebase. Please remove them fromMoqKnownSymbols(or move them into a separate PR that also adds the analyzer/code that consumes them) to avoid accumulating dead well-known symbols.
/// <summary>
/// Gets the interface <c>Microsoft.Extensions.Logging.ILogger</c>.
/// </summary>
internal INamedTypeSymbol? ILogger => TypeProvider.GetOrCreateTypeByMetadataName("Microsoft.Extensions.Logging.ILogger");
/// <summary>
/// Gets the interface <c>Microsoft.Extensions.Logging.ILogger{T}</c>.
/// </summary>
internal INamedTypeSymbol? ILogger1 => TypeProvider.GetOrCreateTypeByMetadataName("Microsoft.Extensions.Logging.ILogger`1");
src/Analyzers/NoMethodsInPropertySetupAnalyzer.cs:80
invocationOperation.Syntaxis cast withas InvocationExpressionSyntaxbut then immediately dereferenced. Since this path assumes the cast succeeds, use a direct cast instead (or add a null-check) to make the expectation explicit and avoid introducing a fragile null-dereference pattern.
InvocationExpressionSyntax? mockedMethodCall =
(invocationOperation.Syntax as InvocationExpressionSyntax).FindMockedMethodInvocationFromSetupMethod();
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Add Net80 reference assembly group (without Moq) to ReferenceAssemblyCatalog. Add ShouldNotAnalyzeWhenMoqNotReferenced test to exercise the IsMockReferenced() early exit in RegisterCompilationStartAction, covering the guard branches that were causing Codacy diff coverage to fall below the 95% threshold. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test harness injects a global using Moq directive, causing CS0246 when the Net80 reference assembly (without Moq) is used. Suppress compiler diagnostics since the test only validates analyzer behavior. Co-Authored-By: Claude Opus 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/Helpers/ReferenceAssemblyCatalog.cs`:
- Around line 41-43: The XML <remarks> for the public Catalog (in
ReferenceAssemblyCatalog) is out of date after adding the new key nameof(Net80)
=> ReferenceAssemblies.Net.Net80; update the Catalog XML remarks to list the
Net80 key alongside the existing keys so the public API docs accurately reflect
the available map entries (mention the symbol Net80 and the map entry
nameof(Net80) / ReferenceAssemblies.Net.Net80 when editing the remarks).
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/Moq.Analyzers.Test/Helpers/ReferenceAssemblyCatalog.cstests/Moq.Analyzers.Test/NoMethodsInPropertySetupAnalyzerTests.cs
| // .NET 8.0 without Moq, used to verify analyzers short-circuit when Moq is not referenced. | ||
| { nameof(Net80), ReferenceAssemblies.Net.Net80 }, | ||
|
|
There was a problem hiding this comment.
Update Catalog XML remarks to include the new Net80 key.
After adding Net80 to the map, the Catalog remarks still list only two key names, so the public API docs are now stale.
📝 Suggested doc fix
/// <remarks>
-/// The key is the name of the reference assembly group (<see cref="Net80WithOldMoq"/> and <see cref="Net80WithNewMoq"/>).
+/// The key is the name of the reference assembly group
+/// (<see cref="Net80"/>, <see cref="Net80WithOldMoq"/>, and <see cref="Net80WithNewMoq"/>).
/// </remarks>As per coding guidelines, "All public APIs must have complete XML documentation with accurate, up-to-date content."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Moq.Analyzers.Test/Helpers/ReferenceAssemblyCatalog.cs` around lines 41
- 43, The XML <remarks> for the public Catalog (in ReferenceAssemblyCatalog) is
out of date after adding the new key nameof(Net80) =>
ReferenceAssemblies.Net.Net80; update the Catalog XML remarks to list the Net80
key alongside the existing keys so the public API docs accurately reflect the
available map entries (mention the symbol Net80 and the map entry nameof(Net80)
/ ReferenceAssemblies.Net.Net80 when editing the remarks).
rjmurillo-bot
left a comment
rjmurillo-bot
left a comment
There was a problem hiding this comment.
All required checks passing. Code changes verified. LGTM.
rjmurillo
deleted the
refactor/convert-no-methods-in-property-setup-to-ioperation
branch
## Summary Main is broken. PRs #955 and #958 each independently added a `Net80` property and dictionary entry to `ReferenceAssemblyCatalog`. The merge created duplicates, failing with `CS0102: The type 'ReferenceAssemblyCatalog' already contains a definition for 'Net80'`. ## Changes Removed the duplicate property (lines 38-41) and duplicate dictionary entry (lines 73-74) from `ReferenceAssemblyCatalog.cs`. Kept the first copies which have the more descriptive doc comment. ## Test Plan - [x] `dotnet build` passes - [x] 2786 tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Removed .NET 8.0 reference assembly support from test infrastructure. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
NoMethodsInPropertySetupAnalyzerfromRegisterSyntaxNodeActiontoRegisterCompilationStartAction+RegisterOperationActionwithIInvocationOperation, matching the pattern established byAsShouldBeUsedOnlyForInterfaceAnalyzer"SetupGet","SetupSet","SetupProperty") with symbol-based detection usingMoqKnownSymbolsandIsInstanceOfMock1SetupGet,Mock1SetupSet, andMock1SetupPropertyproperties toMoqKnownSymbolsfor resolving property setup method symbolsFixes #337
Test plan
NoMethodsInPropertySetupAnalyzerTestspass without modification (behavior is identical)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests