-
Notifications
You must be signed in to change notification settings - Fork 480
Fix: .UseMauiCommunityToolkit() Analyzer when wrapped in preprocessor directives #2769
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
Conversation
Refactor `UseCommunityToolkitInitializationAnalyzerTests.cs` for better formatting and readability. Added a new test method to check for `UseMauiCommunityToolkit` usage within preprocessor directives. Updated the `VerifyMauiToolkitAnalyzer` method to include a new expected type. In `UseCommunityToolkitInitializationAnalyzer.cs`, modified the `AnalyzeNode` method to directly check the method's text for `UseMauiCommunityToolkit`, improving detection reliability even within preprocessor directives.
|
Wasn't able to test it via the toolkit .sln directly as it wouldn't build (build was running for 20m+ and not ending). The unit tests also weren't showing up in the test explorer, so I have no idea if the test will pass or fail. Was able to test it by building the nuget via powershell - $version = "99.0.0-preview2"
$outputDir = "$PSScriptRoot\nuget"
# Ensure the output directory exists
if (-not (Test-Path $outputDir)) {
New-Item -ItemType Directory -Path $outputDir | Out-Null
}
# Build solution
dotnet build "D:/WS/Github-Fork/MauiToolkit/src/CommunityToolkit.Maui.sln" -c Release
# Pack nuget
dotnet pack D:/WS/Github-Fork/MauiToolkit/src/CommunityToolkit.Maui/CommunityToolkit.Maui.csproj -c Release --no-restore -p:PackageVersion=$version -o $outputDir
dotnet pack D:/WS/Github-Fork/MauiToolkit/src/CommunityToolkit.Maui.Core/CommunityToolkit.Maui.Core.csproj -c Release --no-restore -p:PackageVersion=$version -o $outputDir
# Show the generated NuGet package(s)
Write-Host "Generated NuGet package(s) in ${outputDir}:"
Get-ChildItem -Path $outputDir -Filter "*.nupkg"
# Keep the PowerShell window open
Read-Host -Prompt "Press Enter to exit"Have tested that generated nuget against my current solution, and does fix the linked issue |
…izationAnalyzer.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
TheCodeTraveler
left a comment
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.
Thanks for getting this started @IeuanWalker!
I need your help optimizing this a bit. Using var methodText = methodDeclaration.ToString(); is very memory intensive in an Analyzer.
We've gotten in trouble with the Roslyn team for our analyzer performance in Visual Studio before and we need to ensure we don't make the performance of our analyzers worse.
I understand creating analyzers are tricky and using the built-in Roslyn types like InvocationExpressionSyntax and MemberAccessExpressionSyntax are not super intuitive. You can try running this through an agent like CoPilot or Claude to optimize the code using built-in Roslyn types to avoid using .ToString() and thus avoiding its additional memory allocation.
|
@TheCodeTraveler thanks, let me know how u get on the copilot. Atm, the solution isn't building correctly in VS for me to debug so I can see the types, etc. So if Copilot doesn't work, I'll see if I can spin up a VM in Azure to see if that works. |
You can use any LLM. Claude.ai is my preference. Just copy paste the analyzer code into Claude and tell it to optimize the code using Roslyn syntax. You can lean on the existing Unit Tests and the new Unit Test you've added to this PR to test the updated analyzer code without needing a debugger. For now, I cannot merge this PR until you update it. |
- Removed the `category` constant from the analyzer class. - Updated `AnalyzeNode` to retrieve `methodDeclaration` using `invocationExpression.Ancestors()`. - Added a check to ensure `methodDeclaration` is not null and verifies the absence of `UseMauiCommunityToolkit` calls. - Introduced `HasUseMauiCommunityToolkitCall` method to traverse the syntax tree for `UseMauiCommunityToolkit` calls. - Added `UseMauiCommunityToolkitVisitor` class to efficiently check for invocation expressions. - Improved diagnostic reporting logic to only report when `UseMauiCommunityToolkit` is not found.
- Added necessary `using` directives for testing in `UseCommunityToolkitInitializationAnalyzerTests.cs`. - Refactored `HasUseMauiCommunityToolkitCall` in `UseCommunityToolkitInitializationAnalyzer.cs` to remove the visitor pattern, improving memory efficiency by directly checking method text for the specified method name.
Refactor the method to check for the presence of the method name by iterating through each line of the method's text instead of retrieving the entire text span. This change reduces memory usage and enhances performance by only examining relevant lines that overlap with the method's span.
Refactor the `HasUseMauiCommunityToolkitCall` method to enhance efficiency by searching for the method name character by character within the method's span. This change eliminates unnecessary string allocations and improves performance. The new implementation uses a boolean flag to quickly determine if a match is found, returning `true` immediately upon a match and `false` if no match is found after checking all characters.
- Corrected URL for Style Cop SA1201 in documentation. - Clarified element order in file organization, adding "Classes." - Adjusted closing brace position in `VerifyMauiToolkitAnalyzer`. - Simplified null check in `AnalyzeNode` method and removed unnecessary call check. - Eliminated `HasUseMauiCommunityToolkitCall` method to streamline code.
Updated `AnalyzeNode` to report diagnostics if a method contains a call to `UseMauiCommunityToolkit`. Introduced `HasUseMauiCommunityToolkitCall` to efficiently search for the method call in the source text without string allocations.
|
@TheCodeTraveler Claude is awesome! been playing with Copilot in VS and been impressed, Claude seems to be on another level. Took a few iterations to get something to work, but all seems ok now 🤞 From Claude itself - |
TheCodeTraveler
left a comment
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.
Hey @IeuanWalker! It looks like Claude is hallucinating a bit. The code needs some more work to avoid degrading our benchmarks.
Analyzer Benchmark (main)
| Method | Mean | Error | StdDev | Median | Gen0 | Gen1 | Allocated |
|---|---|---|---|---|---|---|---|
| VerifyNoErrorsWhenUseMauiCommunityToolkit | 7.093 ms | 0.2889 ms | 0.7908 ms | 6.717 ms | 93.7500 | 31.2500 | 1.65 MB |
| VerifyNoErrorsWhenUseMauiCommunityToolkitHasAdditionalWhitespace | 7.288 ms | 0.5125 ms | 1.4116 ms | 6.503 ms | 93.7500 | 31.2500 | 1.65 MB |
| VerifyErrorsWhenMissingUseMauiCommunityToolkit | 11.119 ms | 1.0230 ms | 2.7830 ms | 9.683 ms | 125.0000 | 62.5000 | 2.35 MB |
Analyzer Benchmar (this PR)
| Method | Mean | Error | StdDev | Median | Gen0 | Gen1 | Gen2 | Allocated |
|---|---|---|---|---|---|---|---|---|
| VerifyNoErrorsWhenUseMauiCommunityToolkit | 23.946 ms | 0.4756 ms | 0.8203 ms | 23.945 ms | - | - | - | 3.54 MB |
| VerifyNoErrorsWhenUseMauiCommunityToolkitHasAdditionalWhitespace | 7.209 ms | 0.4074 ms | 1.1015 ms | 6.767 ms | 93.7500 | 31.2500 | - | 1.65 MB |
| VerifyErrorsWhenMissingUseMauiCommunityToolkit | 10.242 ms | 0.1855 ms | 0.5015 ms | 10.175 ms | 156.2500 | 93.7500 | 31.2500 | 2.29 MB |
Here's the steps to find the benchmark results from each automated CI build following each commit:
- In GithHub, click on the Actions tab
- On the Actions tab, select the most recent Benchmarks action for your PR
- On the benchmark action page, scroll to the bottom
- In the Artifacts section, click the download icon
Reformatted `UseCommunityToolkitInitializationAnalyzerTests.cs` for consistency and readability, adding a new test method `VerifyErrorsWhenUseMauiCommunityToolkitIsCommentedOut`. Updated the `VerifyMauiToolkitAnalyzer` method to include this new test case. Enhanced the logic in `UseCommunityToolkitInitializationAnalyzer.cs` to check the syntax tree before performing character-by-character searches, improving analysis efficiency.
|
@TheCodeTraveler hopefully this is better, im guessing there is still going to be an increase in memory but id think it be marginal. |
|
Excellent work! I'll copy this implementation across our other analyzers and merge the PR shortly. Analyzer Benchmark (this PR)
|
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.
Pull Request Overview
This pull request fixes the .UseMauiCommunityToolkit() analyzer to correctly handle cases when the corresponding calls are wrapped in preprocessor directives or commented out.
- Refactored analyzers to extract detection logic into helper methods that check both active syntax nodes and disabled code in trivia.
- Added unit tests to verify the analyzer behavior when the toolkit initialization calls are conditionally compiled or commented out.
- Adjusted type references in tests to align with recent changes.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/CommunityToolkit.Maui.MediaElement.Analyzers/UseCommunityToolkitMediaElementInitializationAnalyzer.cs | Refactored analyzer to use a helper method for call detection in both active code and disabled trivia. |
| src/CommunityToolkit.Maui.Camera.Analyzers/UseCommunityToolkitCameraInitializationAnalyzer.cs | Similar refactoring for camera analyzer with improved detection of conditional code. |
| src/CommunityToolkit.Maui.Analyzers/UseCommunityToolkitInitializationAnalyzer.cs | Updated analyzer logic to use a helper method for call detection in active and disabled code. |
| src/CommunityToolkit.Maui.Analyzers.UnitTests/UseCommunityToolkitMediaElementInitializationAnalyzerTests.cs | Added unit test to validate that preprocessor-wrapped calls do not trigger diagnostics and updated type reference. |
| src/CommunityToolkit.Maui.Analyzers.UnitTests/UseCommunityToolkitInitializationAnalyzerTests.cs | Added unit tests to verify analyzer behavior when the call is wrapped in preprocessor directives or commented out. |
| src/CommunityToolkit.Maui.Analyzers.UnitTests/UseCommunityToolkitCameraInitializationAnalyzerTests.cs | Added unit test for verifying analyzer behavior for camera initialization with preprocessor directives. |
TheCodeTraveler
left a comment
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.
Thanks again @IeuanWalker! Great job on fixing the bug and improving the Analyzer performance across the board 🙌
Analyzer Benchmarks (this PR)
| Method | Mean | Error | StdDev | Median | Gen0 | Gen1 | Allocated |
|---|---|---|---|---|---|---|---|
| VerifyNoErrorsWhenUseMauiCommunityToolkit | 6.802 ms | 0.2621 ms | 0.7040 ms | 6.454 ms | 93.7500 | 31.2500 | 1.65 MB |
| VerifyNoErrorsWhenUseMauiCommunityToolkitHasAdditionalWhitespace | 7.131 ms | 0.4847 ms | 1.3105 ms | 6.472 ms | 93.7500 | 31.2500 | 1.65 MB |
| VerifyErrorsWhenMissingUseMauiCommunityToolkit | 11.226 ms | 1.0566 ms | 2.8384 ms | 9.996 ms | 125.0000 | 62.5000 | 2.35 MB |
| VerifyNoErrorsWhenUseMauiCommunityToolkitCamera | 6.693 ms | 0.5236 ms | 1.4067 ms | 5.878 ms | 93.7500 | 31.2500 | - |
| VerifyNoErrorsWhenUseMauiCommunityToolkitCameraHasAdditionalWhitespace | 6.560 ms | 0.3950 ms | 1.0544 ms | 5.965 ms | 93.7500 | 31.2500 | - |
| VerifyErrorsWhenMissingUseMauiCommunityToolkitCamera | 9.735 ms | 0.3216 ms | 0.8748 ms | 9.417 ms | 156.2500 | 93.7500 | 31.2500 |
| VerifyNoErrorsWhenUseMauiCommunityToolkitMediaElement | 6.777 ms | 0.4193 ms | 1.1337 ms | 6.168 ms | 93.7500 | 31.2500 | - |
| VerifyNoErrorsWhenUseMauiCommunityToolkitMediaElementHasAdditionalWhitespace | 6.723 ms | 0.3987 ms | 1.0846 ms | 6.121 ms | 93.7500 | 31.2500 | - |
| VerifyErrorsWhenMissingUseMauiCommunityToolkitMediaElement | 9.753 ms | 0.2622 ms | 0.7089 ms | 9.479 ms | 156.2500 | 93.7500 | 31.2500 |
Analyzer Benchmarks (main)
| Method | Mean | Error | StdDev | Median | Gen0 | Gen1 | Allocated |
|---|---|---|---|---|---|---|---|
| VerifyNoErrorsWhenUseMauiCommunityToolkit | 7.093 ms | 0.2889 ms | 0.7908 ms | 6.717 ms | 93.7500 | 31.2500 | 1.65 MB |
| VerifyNoErrorsWhenUseMauiCommunityToolkitHasAdditionalWhitespace | 7.288 ms | 0.5125 ms | 1.4116 ms | 6.503 ms | 93.7500 | 31.2500 | 1.65 MB |
| VerifyErrorsWhenMissingUseMauiCommunityToolkit | 11.119 ms | 1.0230 ms | 2.7830 ms | 9.683 ms | 125.0000 | 62.5000 | 2.35 MB |
| VerifyNoErrorsWhenUseMauiCommunityToolkitCamera | 6.605 ms | 0.3626 ms | 0.9678 ms | 6.209 ms | 93.7500 | 31.2500 | - |
| VerifyNoErrorsWhenUseMauiCommunityToolkitCameraHasAdditionalWhitespace | 6.578 ms | 0.3306 ms | 0.8939 ms | 6.280 ms | 93.7500 | 31.2500 | - |
| VerifyErrorsWhenMissingUseMauiCommunityToolkitCamera | 9.803 ms | 0.3622 ms | 0.9915 ms | 9.389 ms | 156.2500 | 93.7500 | 31.2500 |
| VerifyNoErrorsWhenUseMauiCommunityToolkitMediaElement | 6.600 ms | 0.3916 ms | 1.0653 ms | 5.982 ms | 93.7500 | 31.2500 | - |
| VerifyNoErrorsWhenUseMauiCommunityToolkitMediaElementHasAdditionalWhitespace | 6.936 ms | 0.4109 ms | 1.1108 ms | 6.460 ms | 93.7500 | 31.2500 | - |
| VerifyErrorsWhenMissingUseMauiCommunityToolkitMediaElement | 10.101 ms | 0.3188 ms | 0.8563 ms | 9.834 ms | 156.2500 | 93.7500 | 31.2500 |

Description of Change
Linked Issues
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information