fix: replace string-based detection with symbol-based detection#1030
Merged
fix: replace string-based detection with symbol-based detection#1030
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors Moq analyzer symbol detection to replace string-based name matching with symbol-based type checking. It consolidates event argument validation logic, removes redundant invocation detection methods, and strengthens test coverage with data-driven scenarios targeting validation across multiple analyzer paths. 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 |
Contributor
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 enhances the robustness and performance of several Moq analyzers by transitioning from brittle string-based type and method detection to more accurate and reliable symbol-based comparisons. It centralizes the initialization of 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
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
 |
|
Overall Grade Focus Area: Hygiene |
Security Reliability Complexity Hygiene  |
Feedback
- Inconsistent style enforcement across projects
- Naming, redundant-type and parameter-order violations keep reappearing because coding rules aren't applied uniformly; codify formatting and naming in EditorConfig and promote Roslyn analyzers to enforced rules so fixes run automatically.
- Test code lives outside automated fixes
- The bulk of violations is concentrated in test folders, showing formatters/analyzers or auto-fixes aren't targeting tests; apply the same analyzer and formatting configuration to test projects and enable bulk code-fixes there.
- No automated guardrails for public-API shape
- Misordered parameters and naming slips indicate no analyzer catching API surface conventions; implement and enable analyzers that detect and offer code-fixes for parameter ordering and Task-naming to prevent recurrence.
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| C# |  |
Mar 9, 2026 1:17a.m. | Review ↗ |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, refactoring several analyzers to use the more performant and robust RegisterCompilationStartAction pattern. Replacing string-based type detection with symbol-based detection significantly improves the reliability of the analyzers. The code is cleaner and more efficient. I have one minor suggestion regarding test coverage.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens analyzer correctness and consistency by removing remaining string-based fallbacks and standardizing on symbol-based detection patterns (with per-compilation MoqKnownSymbols), while also reducing per-node overhead in one analyzer.
Changes:
- Removed string-based
Action/EventHandlerdelegate detection fallbacks inEventSyntaxExtensionsand requiredKnownSymbolsfor delegate checks. - Updated
LinqToMocksExpressionShouldBeValidAnalyzerto detectMock.Ofvia symbol identity (IsInstanceOf(knownSymbols.MockOf)) and moved to the compilation-start pattern withIsMockReferenced()guard. - Hoisted Moq version scanning in
SetupShouldNotIncludeAsyncResultAnalyzertoRegisterCompilationStartActionto avoid repeated per-node scanning, and updated tests accordingly (including removing now-duplicate variants).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Moq.Analyzers.Test/Common/EventSyntaxExtensionsTests.cs | Updates tests to pass KnownSymbols explicitly and removes duplicate test variants after API consolidation. |
| src/Common/EventSyntaxExtensions.cs | Removes string-based fallbacks and makes delegate-type checks require KnownSymbols for symbol-based comparisons. |
| src/Analyzers/SetupShouldNotIncludeAsyncResultAnalyzer.cs | Moves Moq reference/version checks to compilation start and closure-captures MoqKnownSymbols for node actions. |
| src/Analyzers/RaisesEventArgumentsShouldMatchEventSignatureAnalyzer.cs | Switches to compilation-start pattern and threads knownSymbols through event-argument extraction. |
| src/Analyzers/LinqToMocksExpressionShouldBeValidAnalyzer.cs | Replaces method-name string check with symbol-based Mock.Of detection and adopts compilation-start registration. |
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}
src/Analyzers/RaisesEventArgumentsShouldMatchEventSignatureAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/RaisesEventArgumentsShouldMatchEventSignatureAnalyzer.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Common/SemanticModelExtensions.cs (1)
78-91:⚠️ Potential issue | 🟠 MajorAdd a cheap
Raises/RaisesAsyncsyntax guard beforeGetSymbolInfo().
RaisesEventArgumentsShouldMatchEventSignatureAnalyzernow reaches this helper from a syntax-node action on every invocation. Without a simple identifier prefilter, every member-access call in a Moq-referencing compilation pays the semantic lookup cost even when it is obviously unrelated.Suggested fix
if (raisesInvocation.Expression is not MemberAccessExpressionSyntax raisesMethod) { return false; } + string methodName = raisesMethod.Name.Identifier.ValueText; + if (!string.Equals(methodName, "Raises", StringComparison.Ordinal) + && !string.Equals(methodName, "RaisesAsync", StringComparison.Ordinal)) + { + return false; + } + SymbolInfo symbolInfo = semanticModel.GetSymbolInfo(raisesMethod); return symbolInfo.CandidateReason switch {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Common/SemanticModelExtensions.cs` around lines 78 - 91, The IsRaisesInvocation method currently calls semanticModel.GetSymbolInfo for every member-access invocation; add a cheap syntax-level guard before that to bail out early for anything whose member name is not "Raises" or "RaisesAsync". Concretely, inspect raisesMethod.Name (handle both IdentifierNameSyntax and GenericNameSyntax) and if the identifier text is neither "Raises" nor "RaisesAsync" return false immediately; only then call semanticModel.GetSymbolInfo and continue with the existing CandidateReason/IsRaisesSymbol logic. This keeps the rest of the method (including IsRaisesSymbol and knownSymbols usage) unchanged.src/Common/EventSyntaxExtensions.cs (1)
17-23:⚠️ Potential issue | 🟠 MajorValidation skips typeless literals, creating false negatives for non-nullable value-type event parameters.
Line 53 guards validation with
if (argumentType != null && ...), which means whenGetTypeInfo(...).Typeis null—as it is for typeless literals likenullanddefault—the entire validation block is skipped. This allows cases likeRaises(..., null)to pass without a diagnostic even when the event parameter is a non-nullable value type (e.g.,Action<int>).Add explicit handling: either classify the expression against
expectedTypebefore skipping, or emit a diagnostic for typeless literals passed to non-nullable value-type parameters. Include a regression test covering this edge case (e.g.,Raises(x => x.IntEvent += null, null)whereIntEvent: Action<int>).
🤖 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/IsRaisesMethodTests.cs`:
- Around line 3-6: The test summary overstates coverage: current
IsRaisesMethodTests only asserts snippets are diagnostic-free so a failing
IsRaisesInvocation could still go undetected; either narrow the class summary to
state it only verifies absence of diagnostics for symbol-based Raises patterns,
or add a positive test in IsRaisesMethodTests that intentionally triggers the
symbol-based detection path (a snippet calling Raises with the specific pattern
your analyzer detects) and assert the analyzer reports the expected
Raises-specific diagnostic (use the same test helpers/assertion methods your
suite uses to expect a diagnostic) so the detection path is actually exercised.
---
Outside diff comments:
In `@src/Common/SemanticModelExtensions.cs`:
- Around line 78-91: The IsRaisesInvocation method currently calls
semanticModel.GetSymbolInfo for every member-access invocation; add a cheap
syntax-level guard before that to bail out early for anything whose member name
is not "Raises" or "RaisesAsync". Concretely, inspect raisesMethod.Name (handle
both IdentifierNameSyntax and GenericNameSyntax) and if the identifier text is
neither "Raises" nor "RaisesAsync" return false immediately; only then call
semanticModel.GetSymbolInfo and continue with the existing
CandidateReason/IsRaisesSymbol logic. This keeps the rest of the method
(including IsRaisesSymbol and knownSymbols usage) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 049a41ed-139f-4605-9b99-c8ca97b0db2b
📒 Files selected for processing (10)
src/Analyzers/LinqToMocksExpressionShouldBeValidAnalyzer.cssrc/Analyzers/RaisesEventArgumentsShouldMatchEventSignatureAnalyzer.cssrc/Analyzers/SetupShouldNotIncludeAsyncResultAnalyzer.cssrc/Common/EventSyntaxExtensions.cssrc/Common/ISymbolExtensions.cssrc/Common/InvocationExpressionSyntaxExtensions.cssrc/Common/SemanticModelExtensions.cstests/Moq.Analyzers.Test/Common/EventSyntaxExtensionsTests.cstests/Moq.Analyzers.Test/Common/InvocationExpressionSyntaxExtensionsTests.cstests/Moq.Analyzers.Test/IsRaisesMethodTests.cs
💤 Files with no reviewable changes (2)
- src/Common/InvocationExpressionSyntaxExtensions.cs
- tests/Moq.Analyzers.Test/Common/InvocationExpressionSyntaxExtensionsTests.cs
Remove string-based fallbacks that bypassed symbol-based detection: - EventSyntaxExtensions: Remove string-based IsActionDelegate and IsEventHandlerDelegate fallbacks. Require KnownSymbols parameter for all delegate type checks. - LinqToMocksExpressionShouldBeValidAnalyzer: Replace string comparison for "Of" method name with IsInstanceOf(knownSymbols.MockOf). - SetupShouldNotIncludeAsyncResultAnalyzer: Hoist assembly version check to RegisterCompilationStartAction with IsMockReferenced guard. All three analyzers now use RegisterCompilationStartAction with IsMockReferenced guard and closure-captured MoqKnownSymbols, matching the established pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merge GetEventParameterTypes_ActionWithSingleTypeArg_ReturnsSingleType and GetEventParameterTypes_EventHandlerGeneric_ReturnsSingleTypeArgument into a single [Theory] with [InlineData] to eliminate 16 lines of duplicated test scaffolding (qlty similar-code, mass=78). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
IsRaisesMethodCall and IsRaisesInvocation performed identical logic: extract MemberAccessExpressionSyntax, call GetSymbolInfo, check IsMoqRaisesMethod, and handle OverloadResolutionFailure. The analyzer called both in sequence, which was a DRY violation. Remove IsRaisesMethodCall and its tests. Use IsRaisesInvocation as the single implementation for Raises detection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Action<int, string> is Action`2, which IsActionDelegate does not match (it only matches Action and Action<T>). The test actually exercises the custom delegate fallback path via TryGetCustomDelegateParameters. Rename from GetEventParameterTypes_ActionDelegate_ReturnsTypeArguments to GetEventParameterTypes_MultiArgActionDelegate_FallsBackToCustomDelegateInvoke. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover all code paths in ValidateEventArgumentTypes via the analyzer pipeline: too few args, too many args, wrong types, exact match, EventHandler subclass, Action<T>, custom delegates, zero-param delegates, many-param delegates, and CreateEventDiagnostic branching. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove ValidateArgumentTypesWithEventName from RaiseEventArgumentsShouldMatchEventSignatureAnalyzer; reuse EventSyntaxExtensions.ValidateEventArgumentTypes instead - Add static modifier to lambda in TryGetRaiseMethodArguments for consistency - Consolidate 10 identical Theory test methods into 2 in EventSyntaxExtensionsTests - Merge 6 CreateEventDiagnostic Fact tests into 2 covering same assertions - Consolidate 4 duplicate positive-case Fact tests into 2 Theory tests in IsTaskOrValueResultPropertyTests and remove duplicate IsGenericResultProperty tests - Fix SA1512 violations (comment followed by blank line) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolved 3 conflict files from main merge: - RaiseEventArgumentsShouldMatchEventSignatureAnalyzer: use extension method syntax - RaisesEventArgumentsShouldMatchEventSignatureAnalyzer: keep GetEventNameFromSelector helper - EventSyntaxExtensions: consolidate overloads with nullable optional KnownSymbols, retain CreateEventDiagnostic helper and improved docs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
src/Analyzers/RaisesEventArgumentsShouldMatchEventSignatureAnalyzer.cs
Outdated
Show resolved
Hide resolved
![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: Redundant null coalescing on non-nullable return value
- Removed the redundant
?? "event"from line 81 since GetEventNameFromSelector already returns a non-nullable string with its own fallback to "event".
- Removed the redundant
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract shared event name extraction and lambda-based argument extraction into EventSyntaxExtensions. Both Raise and Raises analyzers now call the shared methods instead of maintaining identical private implementations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/Common/EventSyntaxExtensions.cs`:
- Around line 61-75: GetEventParameterTypes lacks special handling for plain
System.EventHandler, causing it to fall back to TryGetCustomDelegateParameters
and return both (object, EventArgs) so Moq Raise/Raises are misvalidated; add a
dedicated handler (e.g., TryGetNonGenericEventHandlerParameters) that detects
when namedType.ConstructedFrom equals knownSymbols.EventHandler0 and returns
only the EventArgs parameter (invokeMethod.Parameters[1].Type), call this
handler before TryGetCustomDelegateParameters in GetEventParameterTypes, and
make KnownSymbols usage non-nullable in that path so EventHandler and
EventHandler<T> are resolved correctly; update tests to expect a single
parameter for non-generic EventHandler and add analyzer tests for direct
Raise/Raises with plain EventHandler.
In `@tests/Moq.Analyzers.Test/Common/IsTaskOrValueResultPropertyTests.cs`:
- Around line 161-180: Two tests,
IsTaskOrValueResultProperty_CustomClassWithResultProperty_ReturnsFalse and
IsGenericResultProperty_ResultOnUnrelatedGenericType_ReturnsFalse, exercise the
same scenario (a user-defined generic type with a Result property) and should be
consolidated or made distinct; update the tests so only one covers the generic
user-type case (keeping the existing assertion that
ISymbol.IsTaskOrValueResultProperty returns false) and either remove the
duplicate test or change the second test to target a different scenario (e.g., a
non-generic type, a nested type, or a Result property with a Task/ValueTask
wrapper) while using the existing helpers GetPropertyFromMemberAccess and
MoqKnownSymbols to fetch the property for assertion.
- Around line 242-253: The helper GetPropertyFromMemberAccess currently casts
model.GetSymbolInfo(memberAccess).Symbol! to IPropertySymbol without checking
for null; add a defensive null check after retrieving the symbol (e.g., var
symbol = model.GetSymbolInfo(memberAccess).Symbol) and throw or Assert.Fail with
a clear message if symbol is null or not an IPropertySymbol, then cast to
IPropertySymbol (prop) so test failures show a clear diagnostic; update
references to memberAccess and prop accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 229461b6-d19f-4020-b3a5-f479ab1f02f7
📒 Files selected for processing (11)
src/Analyzers/RaiseEventArgumentsShouldMatchEventSignatureAnalyzer.cssrc/Analyzers/RaisesEventArgumentsShouldMatchEventSignatureAnalyzer.cssrc/Common/EventSyntaxExtensions.cssrc/Common/ISymbolExtensions.Moq.cstests/Moq.Analyzers.Test/Common/EventSyntaxExtensionsTests.cstests/Moq.Analyzers.Test/Common/IsTaskOrValueResultPropertyTests.cstests/Moq.Analyzers.Test/Common/SemanticModelExtensionsTests.cstests/Moq.Analyzers.Test/PackageTests.Baseline#contents.verified.txttests/Moq.Analyzers.Test/PackageTests.Baseline_main#contents.verified.txttests/Moq.Analyzers.Test/PackageTests.Baseline_symbols#contents.verified.txttests/Moq.Analyzers.Test/RaisesEventArgumentsShouldMatchEventSignatureAnalyzerTests.cs
Source: extract IsMoqFluentInvocation shared helper from duplicate IsCallbackOrReturnInvocation/IsRaisesInvocation methods. Replace LINQ allocation with direct loop in TryGetCustomDelegateParameters. Tests: extract IsRaisesInvocationForMemberAccessAsync helper to remove repeated compilation boilerplate. Consolidate multi-param delegate tests into Theory-based parameterization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nullable dead code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move static field initializations into a static constructor to satisfy ECS1300. Suppress S3963 which conflicts with ECS1300. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rjmurillo
added a commit
that referenced
this pull request
Mar 9, 2026
The analyzer now correctly detects type mismatches in generic Callback<T>() syntax after symbol-based detection was introduced in PR #1030. The previous 'known limitation' test expected no diagnostic, but the analyzer now correctly produces Moq1100. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 tasks
rjmurillo
added a commit
that referenced
this pull request
Mar 9, 2026
rjmurillo
added a commit
that referenced
this pull request
Mar 9, 2026
…tch (#1057) ## Summary - Renamed `GenericCallbackValidation_CurrentLimitation_IsDocumented` to `GenericCallbackWithWrongType_ProducesDiagnostic` - Updated the test to expect Moq1100 diagnostic on `.Callback<int>(wrongTypeParam => { })` where the mocked method takes `string` - The "known limitation" no longer exists after PR #1030 introduced symbol-based detection, which correctly resolves generic type arguments ## Test plan - [x] Both `Net80WithOldMoq` and `Net80WithNewMoq` variants pass locally (2/2 passed) - [ ] CI passes on this branch 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Richard Murillo <rjmurillo@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
IsActionDelegate/IsEventHandlerDelegatefallbacks inEventSyntaxExtensions. All delegate type checks now requireKnownSymbolsand use symbol comparison viaNamedTypeSymbolExtensions.string.Equals(targetMethod.Name, "Of")inLinqToMocksExpressionShouldBeValidAnalyzerwithtargetMethod.IsInstanceOf(knownSymbols.MockOf)for proper symbol-based detection.ReferencedAssemblyNamesstring scan inSetupShouldNotIncludeAsyncResultAnalyzerfrom per-nodeAnalyzeintoRegisterCompilationStartAction, performing the version check once per compilation instead of per syntax node.RegisterCompilationStartAction+IsMockReferenced()guard + closure-capturedMoqKnownSymbols.WithKnownSymbolstest variants that became identical after consolidation.Test plan
EventSyntaxExtensionstests updated to useKnownSymbolsparameterCloses #981
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor
Tests