-
Notifications
You must be signed in to change notification settings - Fork 468
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
Don't emit CA1508 in Debug.Assert #7014
Conversation
Codecov Report
@@ Coverage Diff @@
## main #7014 +/- ##
=======================================
Coverage 96.42% 96.42%
=======================================
Files 1412 1412
Lines 336397 336421 +24
Branches 11114 11131 +17
=======================================
+ Hits 324382 324410 +28
+ Misses 9212 9207 -5
- Partials 2803 2804 +1 |
&& (op.Parent is not IArgumentOperation { Parent: IInvocationOperation invocation } || | ||
!debugAssertMethods.Contains(invocation.TargetMethod, SymbolEqualityComparer.Default)); |
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.
Will this work if Debug.Assert expressions is more complex, say Debug.Assert(MyConstant == 16 && MyConstant2 == 17);
where MyConstant2
is another constant with value 17
?
I think a better approach would be to revert the changes to ShouldAnalyze
and instead add a ShouldReport(IOperation op)
helper that gets called just prior to context.ReportDiagnostic
calls in this analyzer, and that one can do a walk up the operation tree and check if any of the ancestor IInvocationOperation
is a call to a debug assert method.
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.
I thought about that approach as well, but was hesitant about implementing a plain walk-up, since we could have something like this, which IMO should be flagged: Debug.Assert(MethodCall(MyConstant == 16));
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.
Sounds reasonable. We can revisit this in future if needed. Thanks for the fix.
Fixes #6983