Provide a standard item group for OS platforms

[terrajobst]

We've built an analyzer that allows an API to be marked as unsupported for a given set of platforms. We're going to use this to mark APIs that we can't make work in Blazor WebAssembly.

In the Blazor case, there are many cuts and they cut deep (threading or file I/O). We don't want an experience where developers of normal cross-platform class libraries or console apps are confronted with a myriad of diagnostics these APIs don't work in WebAssembly -- most developers don't need their code to run in those environment.

We intend to address this experience by allowing the developer to include and exclude platforms that the analyzer will consider. Since we want developers to add an remove from the default set, it sees an item group is much more natural fit than a property:

Desired experience:

1. The base SDK includes a standard set, such as:

   <ItemGroup>
       <SupportedPlatform Include="android" />
       <SupportedPlatform Include="ios" />
       <SupportedPlatform Include="linux" />
       <SupportedPlatform Include="macos" />
       <SupportedPlatform Include="windows" />
   </ItemGroup>

2. The Blazor SDK changes that set:

   <ItemGroup>
       <SupportedPlatform Include="browser" />
   </ItemGroup>

3. Developers can do this manually in the project file, if, for example, they are building a class library for Blazor:

   <ItemGroup>
       <SupportedPlatform Include="browser" />
   </ItemGroup>

4. Developers can also manually remove platforms from the project file that they don't care about:

   <ItemGroup>
       <SupportedPlatform Remove="macos" />
   </ItemGroup>

Questions

• The name seems to conflict with the existing item groups in the SDK has: SupportedTargetFramework and SupportedTargetPlatform. So maybe we should call this one AnalyzedPlatform?

• The assumption is that there is a way to pass this to the Roslyn analyzer. @mavasani is looking into that.

/cc @dsplaisted @mavasani @buyaa-n @jeffhandley @eerhardt

[mavasani]

The assumption is that there is a way to pass this to the Roslyn analyzer. @mavasani is looking into that.

Tagging @jaredpar @chsienki. I was hoping Crhris' recent work on passing MSBuild stuff through global config would address this, but reading up on https://github.com/dotnet/roslyn/blob/421bf4ba651a669d0611e5d79af0f334b16e903b/docs/features/source-generators.cookbook.md#consume-msbuild-properties-and-metadata, that does not seem to be the case. We can implement this inside the roslyn-analyzers repo to transform the SupportedPlatform items into a special MSBuild property with concatenated values, that is then marked as CompilerVisibleProperty. However, I wonder if we can add this support inside the GenerateMSBuildEditorConfig task itself. @chsienki @jaredpar your thoughts?

[jaredpar]

@mavasani

Why don't you think the MSBuild support we added for SG would work here? I believe that should work. If not then I think we should focus on making sure it works because this is exactly the type of scenario we wanted to solve.

[mavasani]

Ah, then maybe I misunderstood. I thought the metadata support was only allowing passing metadata items specific to particular files (source or non-source), and cannot solve the generic scenario for items metadata required here. Would you or @chsienki we able to point out the MSBuild syntax needed to get this into global config option?

[chsienki]

Yes, this is possible:

You'll ship a .targets file with the analyzer that is included via usual NuGet mechanisms, that looks like this:

<?xml version="1.0" encoding="utf-8" ?>
<Project>
  <PropertyGroup>
    <SupportedPlatform>@(SupportedPlatform)</SupportedPlatform>
  </PropertyGroup>

  <ItemGroup>
    <CompilerVisibleProperty Include="SupportedPlatform" />
  </ItemGroup>
</Project>

You can access that in the analyzer via: context.AnalyzerConfigOptions.GlobalOptions.TryGetValue("build_property.SupportedPlatform", out var supportedPlatforms);

Note that this is concating the platforms together with a ; so you'll need to string.Split them back out into an array.

The evaluation happens just before build, so a user can add/remove from SupportedPlatform etc as needed and it should still flow through properly (caveat that you can always find ways to break MSBuild stuff with Before/After targets if you're determined to).

Why can't we pass the metadata directly?

Global configs aren't really designed to pass arbitrary item groups. You can pass the metadata from a specific ItemGroup type, but it's keyed off of the filename. In this case the 'filename' is is 'android', 'ios' etc.

If you request the 'Identity' metadata of the itemgroup it technically works, but you just end up with a bunch of items that you'd need to look up by value, rather than a set of values you can query. e.g.

<Project>
  <ItemGroup>
    <CompilerVisibleItemMetadata Include="SupportedPlatform" MetadataName="Identity" />
  </ItemGroup>
</Project>

Becomes:

[C:/projects/roslyn-sdk/samples/CSharp/SourceGenerators/GeneratedDemo/android]
build_metadata.SupportedPlatform.Identity = android

[C:/projects/roslyn-sdk/samples/CSharp/SourceGenerators/GeneratedDemo/ios]
build_metadata.SupportedPlatform.Identity = ios

[C:/projects/roslyn-sdk/samples/CSharp/SourceGenerators/GeneratedDemo/macos]
build_metadata.SupportedPlatform.Identity = macos

[C:/projects/roslyn-sdk/samples/CSharp/SourceGenerators/GeneratedDemo/windows]
build_metadata.SupportedPlatform.Identity = windows

[mavasani]

Thanks @chsienki! I'll get this implemented in the analyzers repo later today.

[mavasani]

I have a PR out to add this support to NuGet packages built out of analyzers repo: https://github.com/dotnet/roslyn-analyzers/pull/3950

@chsienki just as an FYI, ; separated list does not work as that is the comment character for editorconfig, so only the first entry gets retained in the analyzer options. I instead used the , char as separator. @terrajobst to confirm that we don't expect a , char in any supported platform name.

<Project>
  <!-- MSBuild item metadata to thread to the analyzers as options -->
  <PropertyGroup>
    <_SupportedPlatformList>@(SupportedPlatform, ',')</_SupportedPlatformList>
  </PropertyGroup>

  <!-- MSBuild properties to thread to the analyzers as options --> 
  <ItemGroup>
    <CompilerVisibleProperty Include="_SupportedPlatformList" />
  </ItemGroup>
</Project>

[chsienki]

@mavasani Ooh, that's an interesting bug. We're actually the analogous of it here https://github.com/dotnet/roslyn/issues/43970 but i'll update it to include this particular scenario too 👍

[mavasani]

Analyzer support is now checked into roslyn-analyzers repo. It uses the item name SupportedPlatform - if that is changed, @buyaa-n please rename the field declared at https://github.com/dotnet/roslyn-analyzers/blob/31fe7d4b7dbf85609426f49861ca58f0d2d04b3b/src/Utilities/Compiler/Options/MSBuildItemOptionNames.cs#L21 accordingly.

[dsplaisted]

Per our meeting, let's rename SupportedTargetPlatform to SdkSupportedTargetPlatform.

It would be nice to similarly rename SupportedTargetFramework, but that would require updating dotnet/project-system in sync, and isn't as critical.

@sfoslund, can you handle this?