Analyzer for methods with PlatformNotSupportedException body but missing platform attributes

[marek-safar]

Describe the problem you are trying to solve

In .NET5 new attributes were introduced to support CA1416 analyzer rule to detect when unsupported APIs are called. To make the analyzer more effective existing libraries need to annotate their code with UnsupportedOSPlatformAttribute or SupportedOSPlatformAttribute for the analyzer to be able to work.

Describe suggestions on how to achieve the rule

The implementation could be as simple as check for throw PlatformNotSupportedException body with missing UnsupportedOSPlatformAttribute or SupportedOSPlatformAttribute anywhere in the method hierarchy.

@jeffhandley @terrajobst

EDIT: This should also include the work applying the rule to the runtime repo and addressing the warnings that appear.

[ghost]

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

[jeffhandley]

Do I understand the following correctly?

• This would be a new analyzer, separate from CA1416
• Any time a method body throws PlatformNotSupportedException, and there are no [UnsupportedOSPlatform] or [SupportedOSPlatform] attributes, a diagnostic would be raised?
• The analyzer would not attempt to discern what platform(s) are (un)supported
• The diagnostic message would merely indicate that attributes should be applied, presumably with a link to the Platform Compatibility Analyzer documentation

[marek-safar]

@jeffhandley correct

[buyaa-n]

Plus to the @jeffhandley's list:

• Containing type/assemblies attributes also need to be checked
• Need to handle Throw helpers, probably if the method is not doing anything except throwing then should warn on the call site of those methods

Suggested Category: Interoperability
Suggested Severity: We might want to keep it hidden, it might be noisy

[buyaa-n]

Even the analyzer implementation could be small applying it to runtime repo and fixing the warnings might need a lot more work, increasing size to medium

[bartonjs]

Video

Because we don't currently have the ability to mark things as OS-specific based on parameter input, it can only be reliable for methods that are entirely throw new PlatformNotSupportedException(...). As such, we don't think there's sufficient value in it at this time.

When it is possible to do something more complicated the analyzer should not warn if the method is declared [Obsolete], in addition to the OS-based attributes.

[jeffhandley]

@marek-safar @terrajobst Until we get through #44922 and find ways to handle all of the conditional PNSE scenarios, an analyzer warning for PNSE bodies would be too noisy.

My understanding is that the APIs we've marked with [SupportedOSPlatform] and [UnsupportedOSPlatform] were largely discovered through static analysis that found method bodies throwing PNSE; is it correct to assume we have likely found the vast majority of the simple cases of a body that very simply throwing the exception?

If we do revisit this analyzer proposal, we would likely want the diagnostic to surface on the line of code throwing the PlatformNotSupportedException as opposed to the method definition too, as that would allow for a cleaner #pragma warning disable suppression around the throw.

[marek-safar]

is it correct to assume we have likely found the vast majority of the simple cases of a body that very simply throwing the exception?

That's not correct. We did a manual pass for one platform (and we missed many places) in .net5 and will be doing another one in .net6 for different platforms and another one in the future. Having a reliable tool/way to do that is beneficial over grep for throw and manual edits.

I'm also not sure I understand what would be noisy on a one-line throw PNSE body detection.

[jeffhandley]

The one-line throw PNSE body detection could be done if we also have the analyzer respect [Obsolete] attributes (for APIs not implemented in .NET Core, where PNSE is orthogonal to OSPlatform). It would not be noisy. I thought we had already most of the one-line throw PNSE bodies that were related to OSPlatform; now I understand that we haven't. I found over 4500 PNSEs thrown across runtime (any many appear to be one-liners on first glance).

Would the value of the analyzer for one-liners be mostly for the existing cases, or to guard against us writing more of the one-liners? I don't know common it's going to be with the mobile platforms for us to introduce new one-liners.

[bartonjs]

FWIW: If it makes sense to do as an internal analyzer there's nothing in the API review "no" that impedes that. Once it's done we can then decide that it makes sense as a public analyzer and move the code.

[terrajobst]

Right, but the attribute is viral and the leaf nodes are public APIs...

[marek-safar]

I don't know common it's going to be with the mobile platforms for us to introduce new one-liners.

Most PNSE in runtime are one liners and I think it's going to stay this way. I cannot tell how many are going to be added (wild guess between 10 and 100) but it won't be zero in every upcoming release.

[terrajobst]

Related: #47228

[jeffhandley]

Also related #44916.