-
Notifications
You must be signed in to change notification settings - Fork 4
Fix method check for callback and returns #429
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
ac6f57e
e1cfb87
6f4f8d6
44e7d33
4b7ecac
0482ea4
4407240
bdc2bcd
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 | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -120,14 +120,20 @@ private static bool IsCallbackOrReturnSymbol(ISymbol? symbol) | |||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| string? methodName = methodSymbol.ToString(); | ||||||||||||||||||
|
|
||||||||||||||||||
| if (string.IsNullOrEmpty(methodName)) | ||||||||||||||||||
| INamedTypeSymbol? containingType = methodSymbol.ContainingType; | ||||||||||||||||||
| if (containingType is null) | ||||||||||||||||||
| { | ||||||||||||||||||
| return false; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| return methodName.StartsWith("Moq.Language.ICallback", StringComparison.Ordinal) | ||||||||||||||||||
| || methodName.StartsWith("Moq.Language.IReturns", StringComparison.Ordinal); | ||||||||||||||||||
| string containingTypeName = containingType.ToDisplayString(); | ||||||||||||||||||
|
|
||||||||||||||||||
| bool isCallback = string.Equals(methodSymbol.Name, "Callback", StringComparison.Ordinal) | ||||||||||||||||||
| && containingTypeName.StartsWith("Moq.Language.ICallback", StringComparison.Ordinal); | ||||||||||||||||||
|
|
||||||||||||||||||
| bool isReturns = string.Equals(methodSymbol.Name, "Returns", StringComparison.Ordinal) | ||||||||||||||||||
|
Comment on lines
+131
to
+134
|
||||||||||||||||||
| bool isCallback = string.Equals(methodSymbol.Name, "Callback", StringComparison.Ordinal) | |
| && containingTypeName.StartsWith("Moq.Language.ICallback", StringComparison.Ordinal); | |
| bool isReturns = string.Equals(methodSymbol.Name, "Returns", StringComparison.Ordinal) | |
| bool isCallback = string.Equals(methodName, "Callback", StringComparison.Ordinal) | |
| && containingTypeName.StartsWith("Moq.Language.ICallback", StringComparison.Ordinal); | |
| bool isReturns = string.Equals(methodName, "Returns", StringComparison.Ordinal) |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,138 @@ | ||||||||||
| using System; | ||||||||||
|
Check failure on line 1 in tests/Moq.Analyzers.Benchmarks/CallbackReturnsSymbolBenchmarks.cs
|
||||||||||
| using System.Linq; | ||||||||||
| using System.Threading.Tasks; | ||||||||||
| using BenchmarkDotNet.Attributes; | ||||||||||
| using Microsoft.CodeAnalysis; | ||||||||||
| using Moq.Analyzers.Benchmarks.Helpers; | ||||||||||
|
|
||||||||||
| namespace Moq.Analyzers.Benchmarks; | ||||||||||
|
|
||||||||||
| [InProcess] | ||||||||||
| [MemoryDiagnoser] | ||||||||||
| public class CallbackReturnsSymbolBenchmarks | ||||||||||
| { | ||||||||||
| private IMethodSymbol? _callbackMethod; | ||||||||||
| private IMethodSymbol? _returnsMethod; | ||||||||||
|
|
||||||||||
| [GlobalSetup] | ||||||||||
| public void Setup() | ||||||||||
| { | ||||||||||
| (string Name, string Content)[] sources = | ||||||||||
| [ | ||||||||||
| ("Sample", @"namespace Moq.Language { public interface ICallback { void Callback(); } public interface IReturns { void Returns(); } }") | ||||||||||
| ]; | ||||||||||
|
|
||||||||||
| // Suppress VSTHRD002: Synchronously waiting on tasks or awaiters may cause deadlocks. | ||||||||||
| // This is required for BenchmarkDotNet compatibility and is safe in this context. | ||||||||||
| #pragma warning disable VSTHRD002 | ||||||||||
| Compilation? compilation = CSharpCompilationCreator.CreateAsync(sources).GetAwaiter().GetResult(); | ||||||||||
| #pragma warning restore VSTHRD002 | ||||||||||
| if (compilation is null) | ||||||||||
| { | ||||||||||
| throw new InvalidOperationException("Failed to create C# compilation for benchmark sources."); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| INamedTypeSymbol? callbackType = compilation.GetTypeByMetadataName("Moq.Language.ICallback"); | ||||||||||
| if (callbackType is null) | ||||||||||
| { | ||||||||||
| throw new InvalidOperationException("Type 'Moq.Language.ICallback' not found in compilation."); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| IMethodSymbol[] callbackMembers = callbackType.GetMembers("Callback").OfType<IMethodSymbol>().ToArray(); | ||||||||||
| if (callbackMembers.Length != 1) | ||||||||||
| { | ||||||||||
| throw new InvalidOperationException($"Expected exactly one 'Callback' method in 'Moq.Language.ICallback', found {callbackMembers.Length}."); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| _callbackMethod = callbackMembers[0]; | ||||||||||
|
|
||||||||||
| INamedTypeSymbol? returnsType = compilation.GetTypeByMetadataName("Moq.Language.IReturns"); | ||||||||||
| if (returnsType is null) | ||||||||||
| { | ||||||||||
| throw new InvalidOperationException("Type 'Moq.Language.IReturns' not found in compilation."); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| IMethodSymbol[] returnsMembers = returnsType.GetMembers("Returns").OfType<IMethodSymbol>().ToArray(); | ||||||||||
| if (returnsMembers.Length != 1) | ||||||||||
| { | ||||||||||
| throw new InvalidOperationException($"Expected exactly one 'Returns' method in 'Moq.Language.IReturns', found {returnsMembers.Length}."); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| _returnsMethod = returnsMembers[0]; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| [Benchmark(Baseline = true)] | ||||||||||
| public bool OldCallbackCheck() | ||||||||||
| { | ||||||||||
| if (_callbackMethod is null) | ||||||||||
| { | ||||||||||
| throw new InvalidOperationException("_callbackMethod is null. Ensure Setup completed successfully."); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return OldIsCallbackOrReturnSymbol(_callbackMethod); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| [Benchmark] | ||||||||||
| public bool NewCallbackCheck() | ||||||||||
| { | ||||||||||
| if (_callbackMethod is null) | ||||||||||
| { | ||||||||||
| throw new InvalidOperationException("_callbackMethod is null. Ensure Setup completed successfully."); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return NewIsCallbackOrReturnSymbol(_callbackMethod); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| [Benchmark] | ||||||||||
| public bool OldReturnsCheck() | ||||||||||
| { | ||||||||||
| if (_returnsMethod is null) | ||||||||||
| { | ||||||||||
| throw new InvalidOperationException("_returnsMethod is null. Ensure Setup completed successfully."); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return OldIsCallbackOrReturnSymbol(_returnsMethod); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| [Benchmark] | ||||||||||
| public bool NewReturnsCheck() | ||||||||||
| { | ||||||||||
| if (_returnsMethod is null) | ||||||||||
| { | ||||||||||
| throw new InvalidOperationException("_returnsMethod is null. Ensure Setup completed successfully."); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return NewIsCallbackOrReturnSymbol(_returnsMethod); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private static bool OldIsCallbackOrReturnSymbol(IMethodSymbol symbol) | ||||||||||
| { | ||||||||||
| string? methodName = symbol.ToString(); | ||||||||||
| if (string.IsNullOrEmpty(methodName)) | ||||||||||
| { | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return methodName.StartsWith("Moq.Language.ICallback", StringComparison.Ordinal) | ||||||||||
| || methodName.StartsWith("Moq.Language.IReturns", StringComparison.Ordinal); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private static bool NewIsCallbackOrReturnSymbol(IMethodSymbol symbol) | ||||||||||
| { | ||||||||||
| INamedTypeSymbol? containingType = symbol.ContainingType; | ||||||||||
| if (containingType is null) | ||||||||||
| { | ||||||||||
| return false; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| string containingTypeName = containingType.ToDisplayString(); | ||||||||||
|
Contributor
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. 🧹 Nitpick (assertive) Performance consideration: ToDisplayString() might be expensive.
Consider using symbol metadata directly: - string containingTypeName = containingType.ToDisplayString();
+ // Check namespace and type name separately for better performance
+ bool isInMoqLanguageNamespace = string.Equals(containingType.ContainingNamespace?.ToDisplayString(), "Moq.Language", StringComparison.Ordinal);
+ if (!isInMoqLanguageNamespace) return false;Then update the checks to use
🤖 Prompt for AI Agents
Owner
Author
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. @coderabbitai make an issue for this
Contributor
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. 🧩 Analysis chain@rjmurillo I've created a GitHub issue to track the performance optimization for replacing 🏁 Script executed: Length of output: 948 @rjmurillo Perfect! I've successfully created GitHub issue #460 to track the performance optimization for replacing The issue is now available at: #460 The issue includes:
This will help ensure the performance optimization gets proper attention in a future iteration. 🧬 Code Graph Analysis ResultsPossibly relevant snippets from
|
||||||||||
|
|
||||||||||
| bool isCallback = string.Equals(symbol.Name, "Callback", StringComparison.Ordinal) | ||||||||||
| && containingTypeName.StartsWith("Moq.Language.ICallback", StringComparison.Ordinal); | ||||||||||
|
Comment on lines
+130
to
+131
Contributor
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. Potential false positive matches with StartsWith check. The Use exact string comparison or check the full type name: - bool isCallback = string.Equals(symbol.Name, "Callback", StringComparison.Ordinal)
- && containingTypeName.StartsWith("Moq.Language.ICallback", StringComparison.Ordinal);
+ bool isCallback = string.Equals(symbol.Name, "Callback", StringComparison.Ordinal)
+ && string.Equals(containingTypeName, "Moq.Language.ICallback", StringComparison.Ordinal);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Owner
Author
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. @coderabbitai make an issue for this
Contributor
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. 🧩 Analysis chain@rjmurillo I've created a GitHub issue for the potential false positive bug in the StartsWith check. The issue has been assigned to you and includes all the relevant details about the problem, its impact, and references to this PR and comment thread. 🏁 Script executed: Length of output: 1451 @rjmurillo Perfect! I've successfully created the issue for you. Issue #461 has been created: #461 The issue includes:
The issue is now ready for tracking and can be addressed in a future PR. 🧬 Code Graph Analysis ResultsPossibly relevant snippets from
|
||||||||||
|
|
||||||||||
| bool isReturns = string.Equals(symbol.Name, "Returns", StringComparison.Ordinal) | ||||||||||
| && containingTypeName.StartsWith("Moq.Language.IReturns", StringComparison.Ordinal); | ||||||||||
|
Comment on lines
+133
to
+134
Contributor
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. Potential false positive matches with StartsWith check. Same issue as with the callback check - Use exact string comparison: - bool isReturns = string.Equals(symbol.Name, "Returns", StringComparison.Ordinal)
- && containingTypeName.StartsWith("Moq.Language.IReturns", StringComparison.Ordinal);
+ bool isReturns = string.Equals(symbol.Name, "Returns", StringComparison.Ordinal)
+ && string.Equals(containingTypeName, "Moq.Language.IReturns", StringComparison.Ordinal);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Owner
Author
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. @coderabbitai make an issue for this
Contributor
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. 🧩 Analysis chain@rjmurillo I'll create an issue to track this potential false positive problem. 🏁 Script executed: Length of output: 1093 Perfect! I've successfully created issue #459 to track the potential false positive matches in callback/returns symbol detection. Issue created: #459 The issue documents the problem with using 🧬 Code Graph Analysis ResultsPossibly relevant snippets from
|
||||||||||
|
|
||||||||||
| return isCallback || isReturns; | ||||||||||
| } | ||||||||||
| } | ||||||||||
Uh oh!
There was an error while loading. Please reload this page.