-
Notifications
You must be signed in to change notification settings - Fork 466
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
Improved performance of CA1041 #6671
base: main
Are you sure you want to change the base?
Conversation
Reimplemented CA1041 as a syntax node analyzer to improve performance.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6671 +/- ##
==========================================
- Coverage 96.40% 96.39% -0.01%
==========================================
Files 1379 1381 +2
Lines 322253 322340 +87
Branches 10460 10479 +19
==========================================
+ Hits 310657 310730 +73
- Misses 9103 9114 +11
- Partials 2493 2496 +3 |
This alone isn't really sufficient. it was mostly a "hypothesis". Profiling is really needed in real-world scenarios. |
while (name is QualifiedNameSyntax qualifiedName) | ||
name = qualifiedName.Right; | ||
|
||
return (IdentifierNameSyntax)name; |
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.
This might crash for AliasQualifiedNameSyntax or GenericNameSyntax
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.
@NewellClark - thank you for putting the efforts here and creating this PR, it is much appreciated.
However, I believe we need to root cause the performance issue before taking this path. We definitely want analyzer authors to be able to write performant IOperation analyzers when analyzing executable code. I have added a comment in the original issue to help us move forward with the performance investigations.
💡 We should now be able to use |
This might not work for local functions I think (assuming the original implementation works for local functions) |
I would expect it to work for local functions? |
@sharwell |
@Youssef1313 it seems like that would not be problematic. The handling will differ slightly for different contexts where this can occur, but in all cases it should be workable. |
Fix #6143
I've reimplemented it as a syntax-node analyzer, as suggested.