Fix warnings caused from build attributes for TFM target platforms added by SDK

[buyaa-n]

Related to the PR Previously we were stripping the target platforms during build of our csproj. Net5.0 sdk supports the targetPlatformMoniker string for net5.0 and above and hence we no longer need to strip it off for newer frameworks.

With the above change for the builds targeting a specific platform MSBuild will be adding assembly-level SupportedOSPlatform("platform") which is causing 2 types of CA1416 failures along shared files having some platform-specific annotations, see the failures here

1. Warning caused from platform attributes inconsistency for APIs having annotations different than the target platform. When a project has some APIs annotated with SupportedOSPlatform("windows") but also has a Linux target: net5.0-Linux MSBuild would produce assembly level SupportedOSPlatform("Linux") and simple repro looks like:

   [assembly:SupportedOSPlatform("Linux")]  // MSBuild would produces assembly level attribute 
   public class TestClass
   {
       [SupportedOSPlatform("windows")] // This annotation ignored as child API couldn't extend parend
       public TestApi() 
       {
           Console.Beep(10, 20); // call for windows only API, warns here because the method annotation ignored from inconsistency
       }
   }

2. When shared file has [UnsupportedOSPlatform("windows")] attribute for some of the APIs build fails for browser target (net5.0-Browser) because of [UnsupportedOSPlatform("windows")] APIs accessing some fields within the shared file. Example:

   [assembly:SupportedOSPlatform("Browser")]  // MSBuild would produces assembly-level attribute 
   public static class Console
   {
       ...
       private static readonly object s_syncObject = new object();
       private static TextReader? s_in;
       ...
   
       [UnsupportedOSPlatform("browser")]
       public static Encoding InputEncoding
       {
           get
           {
               Encoding? encoding = Volatile.Read(ref s_inputEncoding); // Warns: 'Console.s_inputEncoding' is supported on 'Browser'
               if (encoding == null)
               {
                   lock (s_syncObject) // Warns: 'Console.s_syncObject' is supported on 'Browser'
                   {
                       if (s_inputEncoding == null)
                       {
                           Volatile.Write(ref s_inputEncoding, ConsolePal.InputEncoding);
                       }
                       encoding = s_inputEncoding;
                   }
               }
               return encoding;
           }
           ...
       }

@Anipik added a rule to remove specified assembly level attributes. #44257
cc @jeffhandley @terrajobst

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

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

[ViktorHofer]

Why not just disable the logic in the sdk that adds the attributes as a workaround short-term instead of disabling the warning?

[buyaa-n]

Why not just disable the logic in the sdk that adds the attributes

You mean for runtime repo only? That could be the solution for scenario 1, if this could happen only for runtime repo.

Also, i think the 2nd warning need more proper handling as it could happen in general

[buyaa-n]

I reproduced the warnings by removing the section work around:

runtime/eng/versioning.targets

Lines 168 to 175 in c1b5ac0

	<!-- Removes specified assembly level attributes. https://github.com/dotnet/runtime/issues/44257-->
	<Target Name="RemoveSupportedPlatformAssemblyLevelAttribute"
	AfterTargets="GetAssemblyAttributes">
	<ItemGroup>
	<AssemblyAttribute Remove="System.Runtime.Versioning.SupportedOSPlatformAttribute" />
	<AssemblyAttribute Remove="System.Runtime.Versioning.TargetPlatformAttribute" />
	</ItemGroup>
	</Target>

Got 679 warnings, 6 of which caused from Scenario 1 mentioned above in the description, QuotaControl is accessing windows only APIs and attributed accordingly:

runtime/src/libraries/System.DirectoryServices.Protocols/src/System/DirectoryServices/Protocols/common/DirectoryControl.cs

Lines 1024 to 1025 in 5d702f8

	[SupportedOSPlatform("windows")]
	public class QuotaControl : DirectoryControl

But for non windows builds assembly level attribute added by MSBuild is causing platform attributes inconsistency and causing the warnings. I don't think we should fix it in the analyzer as it is against the attributes nesting rule and seems only runtime specific issue. My attempts to exclude the API from non windows builds was not successful as it is introducing ApiCompat warning: Type 'System.DirectoryServices.Protocols.QuotaControl' does not exist in the implementation but it does exist in the contract. So i could only use #if-def to exclude windows only API references from other targets builds.

Remaining 673 warnings related to Scenario 2, t first i thought it could happen anywhere, not only runtime, after looking into the warnings closely it is only happening for assemblies having browser target and having shared files with some APIs attributed with [UnsupportedOSPlatform("browser")]. Now it looks only runtime issue as it is caught only for browser builds in multitargeted shared files. Why it is not happening for other build targets? Because the files for other platform builds are separated as needed, for the clean build browser targets should NOT include any [UnsupportedOSPlatform("browser")] API. I think it is only happen in real/custom projects if they have such unrelated APIs in their build which i believe is not expected. So how do we fix this? We might want to separate all [UnsupportedOSPlatform("browser")] APIs from shared files referenced in the browser build, but it might require lots of work. By considering that Browser is not real platform and we already enabled unsupported on browser warnings on browser builds i think we can just remove the SupportedOSPlatformAttribute("Browser") from Browser targeted build only, just add the condition to the @Anipik's rule

<Target Name="RemoveSupportedPlatformAssemblyLevelAttribute" Condition="'$(TargetFrameworkSuffix)' == 'Browser'">
  <ItemGroup>
    <AssemblyAttribute Remove="System.Runtime.Versioning.SupportedOSPlatformAttribute" />
  </ItemGroup>
</Target>

cc @Anipik @jeffhandley @terrajobst @ViktorHofer

[buyaa-n]

For the scenario 1, where assembly level attribute added by MSBuild is causing attributes inconsistency with child attributes inside the assembly and overwriting/disabling them could be fixed by changing attributes evaluation order from parent to child into child to parent. This case the parent assembly level attribute is ignored. But for me the rule "Child cannot widen the parent support" implies that the attributes consistency should be evaluated from parent to child as we do it now

[buyaa-n]

Discussed with @jeffhandley offline, we gonna separate windows specific (scenario 1) and browser unsupported (scenario 2) APIs into separate files so that they are not warning in unrelated builds. Updates for scenario 2 would cover several files from several projects, so i am gonna cover those updates in different PR and for the first round i will keep Supported attribute removed from browser build and fix warning of scenario 1.

• Enable Supported attributes from sdk for non browser build, fix related warnings
• [] Fix browser build warnings and enable Supported attribute from sdk for browser EDIT: not doing this

[buyaa-n]

I was planning to fix browser build warnings by separating all APIs unsupported on the browser, but this issue is still could happen in the future when we add more annotations, which requiring clear separation of platform-specific APIs and forcing developers to duplicate their code to conform to the strict supported-platform name rules.

After having some discussions with the mono team and @terrajobst @jeffhandley we decided to fix this issue at the analyzer level.

[SupportedOSPlatform("Linux")]  // Type only accessible on Linux 
public class TestClass
{
    [SupportedOSPlatform("windows")] // Having windows only API within Linux only assembly makes this API unreachable
    public TestApi() 
    {
        Console.Beep(10, 20); // as this call is unreachable we don't need to warn
    }
}

[SupportedOSPlatform("Browser")] // Type only supported on Browser
public class Test
{
    private string program; 

    [UnsupportedOSPlatform("browser")] // Unsupports the only support causing empty callsite
    public string CurrentProgram
    {
        get
        {
            return program; // as this call is unreachable we don't need to warn
        }
    }
}

• The attributes combinations in the above scenarios causing the call site unreachable. We call it empty call-site which is not accessible from any platform with the above scenario, so we don't need to warn within this call site
• The above issues are runtime specific, only happening for multitargeted builds, should not happen in real-life scenario

So the tasks list updated to this:

• Enable Supported attributes from sdk for non browser build, fix related warnings
• Fix the browser build related warnings at the analyzer level
• Update the analyzer version in runtime and enable the TFM attributes on browser build