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

Re-Enable CA1416 #20477

Closed
wants to merge 11 commits into from
Closed

Re-Enable CA1416 #20477

wants to merge 11 commits into from

Conversation

PureWeen
Copy link
Member

@PureWeen PureWeen commented Feb 9, 2024

Description of Change

Issues Fixed

Fixes #

@stephen-hawley
Copy link
Contributor

In finishing up this PR, I found that there were 3 cases that I ran into:

  1. The analyzer worked just fine, but the code needed correct or complete OS checks
  2. The analyzer lost its brains and couldn't handle the code inside the OS check, but if the offending code was moved into its own method with the OS restrictions put on it, it worked fine.
  3. Nothing, and I mean nothing could convince the analyzer that the code was correct, so the analyzer had to be shut off and for the offending code and turned back on after.

@PureWeen PureWeen marked this pull request as ready for review May 15, 2024 14:19
@PureWeen PureWeen requested a review from a team as a code owner May 15, 2024 14:19
@PureWeen PureWeen requested review from mattleibow and rmarinho May 15, 2024 14:19
@github-actions github-actions bot force-pushed the reenable_CA1416 branch 2 times, most recently from a64b6b9 to 13f106e Compare May 21, 2024 16:16
@PureWeen
Copy link
Member Author

/rebase

@stephen-hawley
Copy link
Contributor

/rebase

@PureWeen PureWeen added the area-infrastructure CI, Maestro / Coherency, upstream dependencies/versions label May 31, 2024
stephen-hawley and others added 4 commits June 5, 2024 10:10
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](https://gist.github.com/stephen-hawley/716761d318fbcddd7795ed53d331cdc8),
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 previously approved these changes Jun 24, 2024
@PureWeen
Copy link
Member Author

PureWeen commented Jul 9, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Aug 29, 2024
@PureWeen
Copy link
Member Author

Closing in favor of #26593

@PureWeen PureWeen closed this Dec 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jan 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-infrastructure CI, Maestro / Coherency, upstream dependencies/versions stale Indicates a stale issue/pr and will be closed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants