fix: filter non-lambda-rooted members in Moq1302 to eliminate false positives#1017
fix: filter non-lambda-rooted members in Moq1302 to eliminate false positives#1017
Conversation
…ositives Add IsRootedInLambdaParameter guard to prevent Moq1302 from flagging static members, constants, enum values, and external instance members in Mock.Of binary expressions. Thread MoqKnownSymbols through the call chain to eliminate per-operation allocation. Route default branch child operations through the guarded path. Fixes #1010 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
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 and resolves an issue where the Moq1302 analyzer incorrectly flagged static members, constants, and enum values within 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
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThreaded MoqKnownSymbols through LinqToMocksExpressionShouldBeValidAnalyzer analysis paths and added IOperationExtensions.IsRootedInLambdaParameter to determine whether operations originate from a lambda parameter. Refined member-operation traversal and guards; expanded theory-driven tests across multiple Moq reference assemblies. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Feedback
- Analyzer naming rule indiscriminately flags tests
- The Task-returning naming check is applied to test methods, which commonly omit an "Async" suffix; narrowing the rule to ignore recognised test attributes/frameworks or marking test projects as exempt will stop these false positives.
- Bulk test creation without naming normalization
- Many identical violations across a single test file indicate copy/paste or generated tests that never followed the project's naming convention; fixing the generator/templates or running a one-off codemod to append "Async" breaks the repetition.
- Tooling lacks an automated remediation path
- Repeated, trivial renames show there's no automated code-fix or template enforcement; adding an IDE/Roslyn code fix or an EditorConfig naming rule with auto-apply prevents recurrence and cleans existing occurrences quickly.
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| C# | Mar 7, 2026 5:49a.m. | Review ↗ |
There was a problem hiding this comment.
Code Review
This pull request effectively fixes a bug causing false positives in the Moq1302 analyzer by introducing a guard that verifies member accesses are rooted in the lambda parameter. The changes are well-implemented, including a new IsRootedInLambdaParameter extension method and significant improvements to the test suite with 13 new boundary-case tests and dual Moq version coverage. The refactoring to pass MoqKnownSymbols down the call stack is also a good improvement for efficiency. I've suggested a minor refactoring in IsRootedInLambdaParameter to reduce code duplication, but overall this is an excellent contribution.
There was a problem hiding this comment.
Pull request overview
Fixes Moq1302 false positives in LINQ-to-Mocks (Mock.Of<T>(...)) expressions by ensuring only member accesses rooted in the lambda parameter are analyzed/reported, avoiding flagging static/constant/enum/external-value expressions.
Changes:
- Add
IOperationExtensions.IsRootedInLambdaParameter(...)to determine whether a receiver chain originates from theMock.Oflambda parameter. - Thread
MoqKnownSymbolsthrough the analyzer call chain and route recursive analysis through a guarded entry point. - Expand Moq1302 tests with boundary cases and run them against both old/new Moq reference assemblies.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Analyzers/LinqToMocksExpressionShouldBeValidAnalyzer.cs |
Adds guarded recursion + threads MoqKnownSymbols to reduce allocations and avoid static-member false positives. |
src/Common/IOperationExtensions.cs |
Introduces receiver-chain helper IsRootedInLambdaParameter(...). |
tests/Moq.Analyzers.Test/LinqToMocksExpressionShouldBeValidAnalyzerTests.cs |
Adds boundary-case tests and dual-version Moq coverage for the new scenarios. |
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: Chained
&&/||comparisons silently skip member analysis- The fix narrows the IsRootedInLambdaParameter guard to only apply to leaf member operations, allowing IBinaryOperation nodes to pass through for recursive decomposition.
The guard was applied to all operations, causing false negatives for chained comparisons (&&, ||). Composite operations like IBinaryOperation have no receiver chain and silently failed the rooted check. Changes: - Narrow guard to IMemberReferenceOperation and IInvocationOperation only - Consolidate property/field/event cases into IMemberReferenceOperation - Add 5 regression tests for chained &&, ||, and null-coalescing - Update ternary test to expect diagnostic (no longer false negative) All 138 Moq1302 tests pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CI builds with PedanticMode=true (TreatWarningsAsErrors). The nested if statement in AnalyzeMemberOperations triggered S1066. Merged into a single condition using parenthesized pattern matching. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 |
- Remove unreachable null guard in AnalyzeMemberOperations by tightening parameter type from IOperation? to IOperation - Restructure IsRootedInLambdaParameter loop from while(current!=null) to while(true) eliminating unreachable post-loop return - Fold parameterless-lambda guard into inline null check at comparison site, removing uncoverable early-return block - Add tests: nested Mock.Of, explicit cast receiver chain, virtual instance method on lambda parameter Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 10 new test scenarios covering real-world patterns that triggered false positives in Moq1302 (LinqToMocksExpressionShouldBeValid): - Exact reporter pattern: nested Mock.Of inside method argument - Static lambda with external constant - Captured local variable as comparison value - Method parameter as comparison value - Instance field (this member) as comparison value - Inequality operators (!=) with external constants - Chained external instance property (multi-hop) - Non-virtual method with external arguments (true positive) - String concatenation on right side of comparison - || with all external constants (no false positives) These complement the existing 22 tests from PR #1017 to ensure the IsRootedInLambdaParameter guard handles every common customer pattern. Closes #1010 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…1020) ## Summary Adds 10 new test scenarios for Moq1302 (`LinqToMocksExpressionShouldBeValid`) to ensure the `IsRootedInLambdaParameter` guard from PR #1017 handles every common customer pattern. These tests run against both Moq 4.8.2 and 4.18.4. ### New test scenarios **False positive prevention (should NOT flag):** - Exact reporter pattern: nested `Mock.Of` inside method argument (#1010) - `static` lambda with external constant (reporter's exact syntax) - Captured local variable as comparison value - Method parameter as comparison value - Instance field (`this` member) as comparison value - Inequality operator (`!=`) with external constant - Chained external instance property (multi-hop: `settings.Service.DefaultName`) - String concatenation on right side (`Prefix.Value + "-default"`) - `||` with all external constants **True positive preservation (SHOULD flag):** - Non-virtual method with external arguments still flagged ### Test results - **Before**: 144 Moq1302 tests - **After**: 164 Moq1302 tests (+20, each runs against 2 Moq versions) - **Full suite**: 2892 tests, 0 failures ## Test plan - [x] All 164 Moq1302 tests pass - [x] Full suite (2892 tests) passes - [x] Build: 0 warnings, 0 errors Closes #1010 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Expanded test coverage with comprehensive scenarios for analyzer validation, including edge cases with external variables, method parameters, instance fields, static lambdas, chained properties, and comparison operations to improve reliability across complex patterns. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Richard Murillo <rjmurillo@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…ositives (#1017) Fixes #1010. The Moq1302 analyzer (`LinqToMocksExpressionShouldBeValidAnalyzer`) produced false positives when `Mock.Of<T>()` expressions contained static members, constants, or enum values in binary comparisons (e.g., `r => r.Status == StatusCodes.Status200OK`). **Root cause**: The analyzer walked both sides of binary expressions without checking whether the member access originated from the lambda parameter. Static members (`Instance == null`), constants, and external instance properties were incorrectly flagged as invalid mock members. **Fix**: - Add `IsRootedInLambdaParameter` extension method to `IOperationExtensions` that walks the receiver chain to verify a member access terminates in the lambda parameter - Apply the guard in `AnalyzeMemberOperations` only to leaf member operations (`IMemberReferenceOperation`, `IInvocationOperation`), allowing composite operations (`IBinaryOperation` for chained `&&`/`||`/`==`) to pass through for decomposition - Thread `MoqKnownSymbols` through the call chain to eliminate per-operation allocation - Route the `default` branch child operations through the guarded path to prevent bypass - Achieve 100% block code coverage on all changed lines - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature with breaking side effects) - [ ] Documentation update | File | Change | |------|--------| | `src/Analyzers/LinqToMocksExpressionShouldBeValidAnalyzer.cs` | Add `IsRootedInLambdaParameter` guard (leaf ops only), thread `MoqKnownSymbols`, route default branch through guarded path, tighten `AnalyzeMemberOperations` parameter to non-nullable | | `src/Common/IOperationExtensions.cs` | Add `IsRootedInLambdaParameter` extension method with `IMemberReferenceOperation` consolidated receiver chain walk, `while(true)` loop structure | | `tests/.../LinqToMocksExpressionShouldBeValidAnalyzerTests.cs` | Add 16 boundary case tests, convert existing `[Fact]` to `[Theory]` with dual Moq version coverage | - [x] 16 new boundary case tests covering: static const, static property, enum value, reversed operands, chained `&&`, static method call, non-virtual member (true positive), field on lambda parameter (true positive), external instance property, ternary with static member, null-coalescing, conditional with mixed sources, chained property access, nested Mock.Of, explicit cast receiver chain, virtual instance method - [x] All tests run against both Moq 4.8.2 and 4.18.4 via `[Theory]` with `MoqReferenceAssemblyGroups` - [x] Existing `ShouldNotAnalyzeNonMockOfInvocations` converted from `[Fact]` to `[Theory]` for dual-version coverage - [x] 100% block code coverage on all changed lines (verified via coverlet) - [x] All tests pass locally `IConditionalOperation` (ternary expressions) inside `Mock.Of` lambdas are not fully analyzed. Non-virtual members inside ternary branches are not flagged (false negative). This is a deliberate trade-off to prevent false positives. Documented in the `ShouldNotFlagConditionalWithMixedSources` test. `IParenthesizedOperation` is intentionally omitted from `IsRootedInLambdaParameter`. The C# compiler never emits it in IOperation trees (VB.NET only), and this analyzer targets C# exclusively via `[DiagnosticAnalyzer(LanguageNames.CSharp)]`. None. Changes are limited to compile-time Roslyn analyzer logic with no runtime attack surface. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Bug Fixes** * Improved Linq to Mocks expression validation to correctly handle comparisons, chained properties, and conditional expressions involving external members and static values. * **Tests** * Expanded test coverage across multiple Moq versions with scenarios for ternaries, null-coalescing operators, nested comparisons, and complex expression patterns. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Richard Murillo <rjmurillo@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…1020) ## Summary Adds 10 new test scenarios for Moq1302 (`LinqToMocksExpressionShouldBeValid`) to ensure the `IsRootedInLambdaParameter` guard from PR #1017 handles every common customer pattern. These tests run against both Moq 4.8.2 and 4.18.4. ### New test scenarios **False positive prevention (should NOT flag):** - Exact reporter pattern: nested `Mock.Of` inside method argument (#1010) - `static` lambda with external constant (reporter's exact syntax) - Captured local variable as comparison value - Method parameter as comparison value - Instance field (`this` member) as comparison value - Inequality operator (`!=`) with external constant - Chained external instance property (multi-hop: `settings.Service.DefaultName`) - String concatenation on right side (`Prefix.Value + "-default"`) - `||` with all external constants **True positive preservation (SHOULD flag):** - Non-virtual method with external arguments still flagged ### Test results - **Before**: 144 Moq1302 tests - **After**: 164 Moq1302 tests (+20, each runs against 2 Moq versions) - **Full suite**: 2892 tests, 0 failures ## Test plan - [x] All 164 Moq1302 tests pass - [x] Full suite (2892 tests) passes - [x] Build: 0 warnings, 0 errors Closes #1010 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Expanded test coverage with comprehensive scenarios for analyzer validation, including edge cases with external variables, method parameters, instance fields, static lambdas, chained properties, and comparison operations to improve reliability across complex patterns. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Richard Murillo <rjmurillo@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Fixes #1010. The Moq1302 analyzer (
LinqToMocksExpressionShouldBeValidAnalyzer) produced false positives whenMock.Of<T>()expressions contained static members, constants, or enum values in binary comparisons (e.g.,r => r.Status == StatusCodes.Status200OK).Root cause: The analyzer walked both sides of binary expressions without checking whether the member access originated from the lambda parameter. Static members (
Instance == null), constants, and external instance properties were incorrectly flagged as invalid mock members.Fix:
IsRootedInLambdaParameterextension method toIOperationExtensionsthat walks the receiver chain to verify a member access terminates in the lambda parameterAnalyzeMemberOperationsonly to leaf member operations (IMemberReferenceOperation,IInvocationOperation), allowing composite operations (IBinaryOperationfor chained&&/||/==) to pass through for decompositionMoqKnownSymbolsthrough the call chain to eliminate per-operation allocationdefaultbranch child operations through the guarded path to prevent bypassType of Change
Changes
src/Analyzers/LinqToMocksExpressionShouldBeValidAnalyzer.csIsRootedInLambdaParameterguard (leaf ops only), threadMoqKnownSymbols, route default branch through guarded path, tightenAnalyzeMemberOperationsparameter to non-nullablesrc/Common/IOperationExtensions.csIsRootedInLambdaParameterextension method withIMemberReferenceOperationconsolidated receiver chain walk,while(true)loop structuretests/.../LinqToMocksExpressionShouldBeValidAnalyzerTests.cs[Fact]to[Theory]with dual Moq version coverageTest Plan
&&, static method call, non-virtual member (true positive), field on lambda parameter (true positive), external instance property, ternary with static member, null-coalescing, conditional with mixed sources, chained property access, nested Mock.Of, explicit cast receiver chain, virtual instance method[Theory]withMoqReferenceAssemblyGroupsShouldNotAnalyzeNonMockOfInvocationsconverted from[Fact]to[Theory]for dual-version coverageKnown Limitations
IConditionalOperation(ternary expressions) insideMock.Oflambdas are not fully analyzed. Non-virtual members inside ternary branches are not flagged (false negative). This is a deliberate trade-off to prevent false positives. Documented in theShouldNotFlagConditionalWithMixedSourcestest.IParenthesizedOperationis intentionally omitted fromIsRootedInLambdaParameter. The C# compiler never emits it in IOperation trees (VB.NET only), and this analyzer targets C# exclusively via[DiagnosticAnalyzer(LanguageNames.CSharp)].Security Impact
None. Changes are limited to compile-time Roslyn analyzer logic with no runtime attack surface.
Summary by CodeRabbit
Bug Fixes
Tests