Skip to content
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

Enforce CA1854 performance analyzer #22100

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

symbiogenesis
Copy link
Contributor

@symbiogenesis symbiogenesis commented Apr 28, 2024

This is hopefully a straightforward change. Similar to #22092

See: CA1854

@symbiogenesis symbiogenesis requested a review from a team as a code owner April 28, 2024 02:38
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Apr 28, 2024
@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not compiling. Needs more changes in the ApplyPropertiesVisitor class.

D:\a\_work\1\s\src\Controls\src\Xaml\ApplyPropertiesVisitor.cs(362,61): error CS0165: Use of unassigned local variable 'value' [D:\a\_work\1\s\src\Controls\src\Xaml\Controls.Xaml.csproj::TargetFramework=net8.0-windows10.0.20348.0]
    21 Error(s)

@symbiogenesis
Copy link
Contributor Author

symbiogenesis commented Apr 29, 2024

Not compiling. Needs more changes in the ApplyPropertiesVisitor class.

Fixed.

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@danmoseley
Copy link
Member

@Eilon

@Eilon
Copy link
Member

Eilon commented May 6, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@Eilon Eilon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looks like there are some legitimate failures in the build now that the analyzer is an error:

/Users/builder/azdo/_work/1/s/src/Core/src/Platform/Tizen/WindowExtensions.cs(24,8): error CA1854: Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check to avoid double lookup (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1854) [/Users/builder/azdo/_work/1/s/src/Core/src/Core.csproj::TargetFramework=net8.0-tizen]
/Users/builder/azdo/_work/1/s/src/Core/src/Platform/Tizen/WindowExtensions.cs(65,8): error CA1854: Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check to avoid double lookup (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1854) [/Users/builder/azdo/_work/1/s/src/Core/src/Core.csproj::TargetFramework=net8.0-tizen]
/Users/builder/azdo/_work/1/s/src/Core/src/Platform/Tizen/WindowExtensions.cs(106,8): error CA1854: Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check to avoid double lookup (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1854) [/Users/builder/azdo/_work/1/s/src/Core/src/Core.csproj::TargetFramework=net8.0-tizen]
/Users/builder/azdo/_work/1/s/src/Core/src/Platform/Tizen/WindowExtensions.cs(112,8): error CA1854: Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check to avoid double lookup (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1854) [/Users/builder/azdo/_work/1/s/src/Core/src/Core.csproj::TargetFramework=net8.0-tizen]
/Users/builder/azdo/_work/1/s/src/Core/src/Platform/Tizen/StackNavigationManager.cs(196,8): error CA1854: Prefer a 'TryGetValue' call over a Dictionary indexer access guarded by a 'ContainsKey' check to avoid double lookup (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1854) [/Users/builder/azdo/_work/1/s/src/Core/src/Core.csproj::TargetFramework=net8.0-tizen]

@symbiogenesis
Copy link
Contributor Author

Looks like I should add the Tizen workload locally. Fixed those builds.

@Eilon
Copy link
Member

Eilon commented May 6, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Eilon
Copy link
Member

Eilon commented May 6, 2024

Some more errors 😢

D:\a\_work\1\s\src\Core\src\Platform\Tizen\StackNavigationManager.cs(205,8): error CS0128: A local variable or function named 'naviPage' is already defined in this scope [D:\a\_work\1\s\src\Core\src\Core.csproj::TargetFramework=net8.0-tizen]
D:\a\_work\1\s\src\Core\src\Platform\Tizen\StackNavigationManager.cs(211,21): error CS8601: Possible null reference assignment. [D:\a\_work\1\s\src\Core\src\Core.csproj::TargetFramework=net8.0-tizen]
D:\a\_work\1\s\src\Core\src\Platform\Tizen\StackNavigationManager.cs(213,11): error CS8603: Possible null reference return. [D:\a\_work\1\s\src\Core\src\Core.csproj::TargetFramework=net8.0-tizen]

@Eilon
Copy link
Member

Eilon commented May 6, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@symbiogenesis
Copy link
Contributor Author

Rerun failed tests?

@Eilon
Copy link
Member

Eilon commented May 8, 2024

Rerun failed tests?

Yup trying now.

@Eilon
Copy link
Member

Eilon commented May 8, 2024

This test seems to be failing consistently:

Expected string length 21 but was 12. Strings differ at index 0.
Expected: "parameterless factory"
But was:  "default ctor"
-----------^

Stack trace
   at Microsoft.Maui.Controls.Xaml.UnitTests.FactoryMethods.Tests.TestArgumentlessFactoryMethod(Boolean useCompiledXaml) in /Users/builder/azdo/_work/3/s/src/Controls/tests/Xaml.UnitTests/FactoryMethods.xaml.cs:line 109
   at InvokeStub_Tests.TestArgumentlessFactoryMethod(Object, Span`1)

1)    at Microsoft.Maui.Controls.Xaml.UnitTests.FactoryMethods.Tests.TestArgumentlessFactoryMethod(Boolean useCompiledXaml) in /Users/builder/azdo/_work/3/s/src/Controls/tests/Xaml.UnitTests/FactoryMethods.xaml.cs:line 109
   at InvokeStub_Tests.TestArgumentlessFactoryMethod(Object, Span`1)

So maybe there's some problem in the change itself?

@Eilon
Copy link
Member

Eilon commented May 8, 2024

Given that this PR does change code to do with finding factory methods, and that this particular test has failed multiple times in the PR, I think it warrants an investigation of the code in the PR. Maybe there's some subtle change in terms of how the code now locates matches for constructors somehow?

@Eilon Eilon added the area-architecture Issues with code structure, SDK structure, implementation details label May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-architecture Issues with code structure, SDK structure, implementation details community ✨ Community Contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants