Skip to content

Fixed all remaining CA1416 errors#22868

Merged
PureWeen merged 1 commit intodotnet:reenable_CA1416from
stephen-hawley:reenable_CA1416
Jun 21, 2024
Merged

Fixed all remaining CA1416 errors#22868
PureWeen merged 1 commit intodotnet:reenable_CA1416from
stephen-hawley:reenable_CA1416

Conversation

@stephen-hawley
Copy link
Contributor

This should take care of all the remaining CA1416 errors.

Here's what I found about the analyzer for this -

  • It understands when code from Xamarin/macios is reachable from maui code and will flag an error
  • It never understands the pattern if (!OperatingSystem.IsZZZZZZVersionAtLeast(major, minor)) return; that implies that the rest of the code in the method is safe, flagging a false failure
  • Compound OS tests or complex code inside the test will flag a false failure
  • In the case of this code, the analyzer appears to hang, although to be clear, I don't have evidence that it is the analyzer doing this

In order to work around this, the simplest way is to refactor all the OS version specific code into its own method and flag it with [SupportedOSPlatform("xxxyy.zz")]. In this case, the above false failures go away.

In rare cases, I was unable to do this and instead used a #pragma to suppress the warning around the code that triggered the false failure. I tried to avoid this at all costs since this is source of bugs waiting to happen.

@stephen-hawley stephen-hawley requested a review from a team as a code owner June 5, 2024 19:31
@stephen-hawley stephen-hawley requested review from PureWeen and jsuarezruiz and removed request for a team June 5, 2024 19:31
@jfversluis
Copy link
Member

@stephen-hawley should we also add this to warnings as errors so we won't introduce any new ones?

@stephen-hawley
Copy link
Contributor Author

@stephen-hawley should we also add this to warnings as errors so we won't introduce any new ones?

The PR that this branch merges into already turns these into errors so that's not needed here.

@rmarinho
Copy link
Member

rmarinho commented Jun 6, 2024

/azp run

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@PureWeen PureWeen added the area-core-platform Integration with platforms label Jun 8, 2024
[SupportedOSPlatform("maccatalyst13.1")]
bool UIStyleMatches (UITraitCollection target)
{
return target.UserInterfaceStyle == TraitCollection.UserInterfaceStyle;
Copy link

@Domik234 Domik234 Jun 11, 2024

Choose a reason for hiding this comment

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

Are these nullchecks (previousTraitCollection?.UserInterfaceStyle) removed on purpose?

Also I have found that there is method "UITraitCollection.HasDifferentColorAppearanceComparedTo(UITraitCollection previous)" but have not tried.
Maybe this could be cheaper (passing C# referenced object to exported method vs. accessing and comparing two obj-c objects in C# referenced objects and comparing), but I am not sure how it handles "Unspecified" one or null.

(Edit: Second part is bad idea, can compare idiom and more + maybe fooling myself ... UIUserInterfaceStyle contains only "Light", "Dark" and "Unspecified" 🤦‍♂️, but still this should be in diff. topic, sorry)

@PureWeen PureWeen merged commit 0941c4f into dotnet:reenable_CA1416 Jun 21, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants