-
Notifications
You must be signed in to change notification settings - Fork 4
refactor: reduce CRAP scores in event handling and symbol extensions #633
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -207,38 +207,99 @@ | |
| /// </summary> | ||
| /// <param name="symbol">The symbol to check.</param> | ||
| /// <param name="knownSymbols">The known symbols for type checking.</param> | ||
| /// <returns>True if the symbol is a Raises or RaisesAsync method from Moq.Language.IRaiseable or IRaiseableAsync; otherwise false.</returns> | ||
| /// <returns>True if the symbol is a Raises or RaisesAsync method from Moq.Language interfaces; otherwise false.</returns> | ||
| internal static bool IsMoqRaisesMethod(this ISymbol symbol, MoqKnownSymbols knownSymbols) | ||
| { | ||
| if (symbol is not IMethodSymbol methodSymbol) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Check if this method symbol matches any of the known Raises methods | ||
| // Try the ICallback and IReturns interfaces which are more likely to contain Raises | ||
| bool symbolBasedResult = symbol.IsInstanceOf(knownSymbols.ICallbackRaises) || | ||
| symbol.IsInstanceOf(knownSymbols.ICallback1Raises) || | ||
| symbol.IsInstanceOf(knownSymbols.ICallback2Raises) || | ||
| symbol.IsInstanceOf(knownSymbols.IReturnsRaises) || | ||
| symbol.IsInstanceOf(knownSymbols.IReturns1Raises) || | ||
| symbol.IsInstanceOf(knownSymbols.IReturns2Raises) || | ||
| symbol.IsInstanceOf(knownSymbols.IRaiseableRaises) || | ||
| symbol.IsInstanceOf(knownSymbols.IRaiseableAsyncRaisesAsync); | ||
|
|
||
| if (symbolBasedResult) | ||
| // Primary: Use symbol-based detection for known Moq interfaces | ||
| if (IsKnownMoqRaisesMethod(symbol, knownSymbols)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| // Fallback: Check if it's a Raises/RaisesAsync method on any Moq.Language interface | ||
| // This provides compatibility until the correct interface names are identified | ||
| string? containingTypeName = methodSymbol.ContainingType?.ToDisplayString(); | ||
| // TODO: Remove this fallback once symbol-based detection is complete | ||
| // This is a temporary safety net for cases where symbol resolution fails | ||
| // but should be replaced with comprehensive symbol-based approach | ||
| return IsLikelyMoqRaisesMethodByName(methodSymbol); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Checks if the symbol matches any of the known Moq Raises method symbols. | ||
| /// This method handles all supported Moq interfaces that provide Raises functionality. | ||
| /// </summary> | ||
| /// <param name="symbol">The symbol to check.</param> | ||
| /// <param name="knownSymbols">The known symbols for type checking.</param> | ||
| /// <returns>True if the symbol matches a known Moq Raises method; otherwise false.</returns> | ||
| private static bool IsKnownMoqRaisesMethod(ISymbol symbol, MoqKnownSymbols knownSymbols) | ||
| { | ||
| return IsCallbackRaisesMethod(symbol, knownSymbols) || | ||
| IsReturnsRaisesMethod(symbol, knownSymbols) || | ||
| IsRaiseableMethod(symbol, knownSymbols); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Checks if the symbol is a Raises method from ICallback interfaces. | ||
| /// </summary> | ||
| /// <param name="symbol">The symbol to check.</param> | ||
| /// <param name="knownSymbols">The known symbols for type checking.</param> | ||
| /// <returns>True if the symbol is a callback Raises method; otherwise false.</returns> | ||
| private static bool IsCallbackRaisesMethod(ISymbol symbol, MoqKnownSymbols knownSymbols) | ||
| { | ||
| return symbol.IsInstanceOf(knownSymbols.ICallbackRaises) || | ||
| symbol.IsInstanceOf(knownSymbols.ICallback1Raises) || | ||
| symbol.IsInstanceOf(knownSymbols.ICallback2Raises); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Checks if the symbol is a Raises method from IReturns interfaces. | ||
| /// </summary> | ||
| /// <param name="symbol">The symbol to check.</param> | ||
| /// <param name="knownSymbols">The known symbols for type checking.</param> | ||
| /// <returns>True if the symbol is a returns Raises method; otherwise false.</returns> | ||
| private static bool IsReturnsRaisesMethod(ISymbol symbol, MoqKnownSymbols knownSymbols) | ||
| { | ||
| return symbol.IsInstanceOf(knownSymbols.IReturnsRaises) || | ||
| symbol.IsInstanceOf(knownSymbols.IReturns1Raises) || | ||
| symbol.IsInstanceOf(knownSymbols.IReturns2Raises); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Checks if the symbol is a Raises method from IRaiseable interfaces. | ||
| /// </summary> | ||
| /// <param name="symbol">The symbol to check.</param> | ||
| /// <param name="knownSymbols">The known symbols for type checking.</param> | ||
| /// <returns>True if the symbol is a raiseable method; otherwise false.</returns> | ||
| private static bool IsRaiseableMethod(ISymbol symbol, MoqKnownSymbols knownSymbols) | ||
| { | ||
| return symbol.IsInstanceOf(knownSymbols.IRaiseableRaises) || | ||
| symbol.IsInstanceOf(knownSymbols.IRaiseableAsyncRaisesAsync); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// TEMPORARY: Conservative fallback for Moq Raises method detection. | ||
| /// This should be removed once symbol-based detection is comprehensive. | ||
| /// Only matches methods that are clearly Moq Raises methods. | ||
| /// </summary> | ||
| /// <param name="methodSymbol">The method symbol to check.</param> | ||
| /// <returns>True if this is likely a Moq Raises method; otherwise false.</returns> | ||
| private static bool IsLikelyMoqRaisesMethodByName(IMethodSymbol methodSymbol) | ||
| { | ||
| string methodName = methodSymbol.Name; | ||
|
|
||
| return (string.Equals(methodName, "Raises", StringComparison.Ordinal) || | ||
| string.Equals(methodName, "RaisesAsync", StringComparison.Ordinal)) && | ||
| containingTypeName?.Contains("Moq.Language", StringComparison.Ordinal) == true; | ||
| // Only match exact "Raises" or "RaisesAsync" method names | ||
| if (!string.Equals(methodName, "Raises", StringComparison.Ordinal) && | ||
| !string.Equals(methodName, "RaisesAsync", StringComparison.Ordinal)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Must be in a type that contains "Moq" in its namespace to reduce false positives | ||
| string? containingTypeNamespace = methodSymbol.ContainingType?.ContainingNamespace?.ToDisplayString(); | ||
| return containingTypeNamespace?.StartsWith("Moq", StringComparison.Ordinal) == true; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Moq Raises Method Detection Namespace IssueThe fallback logic for Moq Raises method detection was altered. It changed from checking if the containing type name contained "Moq.Language" to checking if the containing namespace starts with "Moq". This introduces both false positives (matching methods in any namespace starting with "Moq", not just Locations (1) |
||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai Log an issue for this TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
I'll create a GitHub issue for this TODO comment about removing the fallback method once symbol-based detection is complete.
@rjmurillo I've created a GitHub issue to track the removal of the string-based fallback method in
IsMoqRaisesMethodonce the symbol-based detection is comprehensive. The issue includes the context, location details, and references back to this PR and comment.🏁 Script executed:
Length of output: 1169
Perfect! I've successfully created GitHub issue #637 for the TODO comment. The issue has been created with:
Issue URL: #637
Details included:
src/Common/ISymbolExtensions.cs,IsMoqRaisesMethodmethod)The issue is now tracked and ready for future work to clean up the fallback method
IsLikelyMoqRaisesMethodByNameonce the symbol-based approach viaIsKnownMoqRaisesMethodis verified to handle all scenarios.🐰✨
Creating organized tracking for future improvements!
🧬 Code Graph Analysis Results
Relevant code snippets for understanding
src/Common/ISymbolExtensions.csand the user comment about the TODO:File:
src/Common/ISymbolExtensions.cs(lines 1-230)The entire file content is relevant as it defines extension methods on
ISymbolrelated to Moq symbols and their identification, including the method containing the TODO comment:IsMoqRaisesMethod(this ISymbol symbol, MoqKnownSymbols knownSymbols)method (lines ~140-180) contains the TODO comment:This method checks if a symbol is a Moq Raises method, first by symbol-based detection, then by a fallback name-based heuristic (marked with a TODO to remove later).
IsMoqRaisesMethodare also relevant:These methods implement the logic for identifying Moq Raises methods by checking symbol equality against known Moq interface methods or by a fallback heuristic based on method name and namespace.
File:
src/Common/WellKnown/MoqKnownSymbols.cs(lines 6-286)This file defines the
MoqKnownSymbolsclass which provides cached references to known Moq symbols used inISymbolExtensionsfor symbol equality checks.These are the known Moq interface methods used by
ISymbolExtensionsto identify Raises methods.File:
src/Common/MoqVerificationHelpers.cs(lines 34-70)This file contains helper methods to extract symbols from Moq verification expressions, which may be relevant for understanding how symbols are used in the context of Moq analyzers and fixes:
File:
src/Common/IOperationExtensions.cs(lines 47-57 and 130-138)These extension methods help extract symbols from operation trees, which is useful in Moq analyzers and code fixes:
Summary
ISymbolExtensions.csinside theIsMoqRaisesMethodmethod, which currently uses a fallback heuristic by method name and namespace to detect Moq Raises methods when symbol-based detection fails.MoqKnownSymbols.cs.MoqVerificationHelpers.csandIOperationExtensions.csprovide context on how symbols are extracted and used in Moq analyzers and code fixes.These snippets provide the necessary context to understand the TODO and the related symbol detection logic.
🧠 Learnings used