System.Net.NetworkInformation UnsupportedOSPlatform attributes #57281
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
ghost
added
|
Tagging subscribers to this area: @dotnet/ncl |
There was a problem hiding this comment.
Should this be specific OS? (I don't know)
There was a problem hiding this comment.
@buyaa-n is it necessary? Let's say if the target OS is Linux will it be considered as "unix" here?
I know that's the way it works in the build system, but I'm not sure if the analyzer behaves the same way...
There was a problem hiding this comment.
but I'm not sure if the analyzer behaves the same way...
No, there is no Unix is Linux kind of relation in the analyzer, why could not use [UnsupportedOSPlatform("Linux")] instead?
There was a problem hiding this comment.
This file is conditionally compiled in for all Unixes (as the build system recognizes them - TargetsUnix property):
The TargetsUnix property is defined for all *nixes, it's not just Linux, you can see where it's set to true here:
https://github.com/dotnet/runtime/blob/main/eng/targetframeworksuffix.props
It's a long list: Linux, Android, OSX, iOS, tvOS, MacCatalyst, FreeBSD, NetBSD, ... I wanted to avoid to repeat the long list here, especially if it means replicating the same thing that's defined by the build system.
There was a problem hiding this comment.
Replaced with unix-like OSes for which we target the dll.
There was a problem hiding this comment.
Replaced with unix-like OSes for which we target the dll.
Thanks, that is better
Sorry somehow my reply to your previous comment was not posted, here is what i wrote:
It's a long list: Linux, Android, OSX, iOS, tvOS, MacCatalyst, FreeBSD, NetBSD, ... I wanted to avoid to repeat the long list here, especially if it means replicating the same thing that's defined by the build system.
The analyzer doesn't know that Unix is related to Linux, Android, OSX, iOS, tvOS, MacCatalyst, FreeBSD, NetBSD, ..., therefore [UnsupportedOSPlatform("unix")] annotation would not work for Linux, Android, OSX, iOS, tvOS, MacCatalyst, FreeBSD, NetBSD, ...
There was a problem hiding this comment.
I'm not sure if we need to care about unsupported OSes.
There is really now way how somebody can target them right now AFAIK.
There was a problem hiding this comment.
We have them as targets for this project:
https://github.com/dotnet/runtime/blob/8e92a6fe4d89934eba274e2a247c800a725d6de9/src/libraries/System.Net.NetworkInformation/src/System.Net.NetworkInformation.csproj#L4
I assume that's the reason why it's firing the warnings.
@buyaa-n can probably share more detail about the inner working of the analyzer.
There was a problem hiding this comment.
I assume that's the reason why it's firing the warnings.
Right when there is a target platform in the TFM that build going to be for that platform for the analyzer. Sounds like the case the API is supported on certain platforms and throws on all others, if you cannot use SupportedOSPlatform attributes with the realistic platforms then we can skip the warnings for these unrealistic platforms.
The PNSE analyzer will not be added to the repo its just one time run to detect missing annotations
ManickaP
force-pushed
the
mapichov/51301_unsupported_attributes
branch
from
8e92a6f to
d39aa3f
Compare
All methods/properties unconditionally throwing PNSE annotated. Also all the call sites that reported an compilation error afterwards are annotated as well.
Fixes #51301
cc @buyaa-n