Refactor Platforms Attribute to use Enums

[rprouse]

The Platforms attribute currently takes a string to specify the platforms that it supports. These are error prone and sometimes hard to guess. Switch to passing in a flags enum (so they can be or'd together).

• Evaluate minimum OS's, frameworks, etc that run .NET 4.5 and remove the others
• Name the enums after the most common supported string to make search/replace easier?
• Deprecate or remove the string constructor and support?
• Add new OS's and frameworks

[jnm2]

What would be the ideal way to specify versions along with the platforms?

Can an analyzer make the string-based version easy enough to use? Providing error diagnostics as you type is something we know how to do. Something I discovered last month at random when looking at Roslyn source code was that ExportCompletionProviderAttribute and the abstract class CompletionProvider are both public. Maybe there is a way for NUnit.Analyzers to add intellisense entries to bring the strings up to par with enums in user-friendliness. It's a wild guess for me at this point.

[jnm2]

Something that could be interesting to compare is the new string-based SupportedOSPlatformAttribute and UnsupportedOSPlatformAttribute in .NET, mentioned here:

• https://github.com/dotnet/designs/blob/main/accepted/2020/platform-exclusion/platform-exclusion.md
• https://devblogs.microsoft.com/dotnet/the-future-of-net-standard/
• https://docs.microsoft.com/en-us/dotnet/standard/analyzers/platform-compat-analyzer

[manfred-brands]

Note that these new attributes also work on strings and possibly complex ones at that. How detailed is NUnit?

Should the NUnit attributes be replaced with these? The problem is they are only supported on .NET5.0. Similar for the OperatingSystem.IsOSPlatform class which the runtime would need to check the strings. Although one could make the attributes available in a similar way to Nullable, the nunit runtime has to its own deciding.

@jnm2 An analyzer would certainly be able to check the string for correctness, if we know the rules. The CompletionProvider code in roslyn seems internal and the other one is Microsoft.Visual.Studio specific, which likely only works with a .vsix extension. Which CompletionProvider did you see?

[jnm2]

I wasn't thinking we would use the new attributes, only compare the string-based approach for consistency with our approach.

I saw this public class: http://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/Completion/CompletionProvider.cs,3632bba4d25ec249

It looks like Roslyn supports completion providers from NuGet packages: https://github.com/dotnet/roslyn/issues/30270

[manfred-brands]

@jnm2 Great. Now I only need to find documentation on the how (or a sample project). The question in the ticket you reference for documentation wasn't answered.

[mikkelbu]

@manfred-brands I think we can get some hints from src/EditorFeatures/Test/Completion/CompletionServiceTests.cs in the PR, but I'm unsure if we both need to implement a class that extends AnalyzerReference and implements ICompletionProviderFactory and returns the CompletionProvider and also the actual completion provider (or whether we only need the latter part).

[manfred-brands]

If all we want is less error prone names, with completion, q quicker solution would be to add a static SupportedPlatform class like below (only some examples and no Xml-Doc)

public static class SupportedPlatform
{
    public const string Win = "Win";
    public const string Win32 = "Win32";
    public const string Net45 = "Net-4.5";
}

We used something similar at my work for passing a string to the Category attribute.

[jnm2]

It would be cool to have SupportedPlatform.Win etc values show up in intellisense when you type [Platform( then, too, which is what it would take to gain parity with how enums would work. There's still a question for me about how to allow access to all the versions people might want to add to the end of the platform names. Define enum members/constants for every possible combination?

[rprouse]

Interestingly, I've been searching GitHub for usages of our PlatformsAttribute and so far I haven't found any. The word Platforms is used a lot, so I kept adding NOT queries to it.

The query Platforms NUnit NOT Unity NOT UnityTestAssemblyBuilder NOT PlatformAttribute NOT RequirePlatformSupportAttribute NOT RenderPlatforms NOT SupportedPlatformsAttribute NOT SupportedPlatforms extension:cs language:C# in:file gives me just over 4000 results and I still haven't found any usage.

Interstingly, I did find quite a few cases where people created their own versions of Platforms, but personally, I think theirs are better :)

Some examples,

• RunOnlyOnPlatformsAttribute by Microsoft
• WindowsAttribute
• WindowsAttribute

It looks like most people write their own attribute rather than use ours.

Should we deprecate in the next release of 3.13 and remove in 4.0 replacing it with proper attributes like above?

[jnm2]

Just to check, did you search for [Platform(? Ours is called PlatformAttribute, singular.

https://grep.app/search?q=%5BPlatform%28&filter[lang][0]=C%23 https://grep.app/search?q=%2C%20Platform%28&filter[lang][0]=C%23

[rprouse]

Thanks @jnm2 I did make a mistake. I searched again and there are only 135 usages including in NUnit, https://grep.app/search?q=%5BPlatform%28&filter[lang][0]=C%23

They only Include/Exclude the following platforms, Win, Mono, Linux, MacOSX, Unix, NET and NETCORE. I did not see any use of version.

[rprouse]

We are checking four things in the PlatformAttribute

1. The operating system (Win, Linux, Win7, etc)
2. The .NET runtime type (Net, NetCore, Mono), can include version but doesn't work for NetCore
3. The operating system bitness (32/64)
4. The running process bitness (32/64)

• Should these be separate attributes? Maybe combining OS bitness with OS.
• Should we have separate attributes for including and excluding to be explicit?

IsOperatingSystemAttribute(OSType osType, OSBits osBits = OSBits.Any)
IsntOperatingSystemAttribute(OSType osType, OSBits? osBits = null)
IsPlatform(PlatformType platformType)  // PlatformType.Net | PlatformType.NetCore
IsntPlatform(PlatformType platformType)
IsProcess(Bits bits) // Bits.32Bit | Bits.64Bit

All Enums are Flags so they can be combined. Platform could take a version string.

My only issue is that with Net 5.0+, Net/NetCore becomes confusing again. Personally, I'd prefer if people #pragmas to define those tests in/out.

We could add these attributes in the latest 3.x release and deprecate Platform.

[jnm2]

Would we want to be able to differentiate between x86, x64, ARM, and ARM64?

[rprouse]

Would we want to be able to differentiate between x86, x64, ARM, and ARM64?

Maybe, but based on the fact that people use the current attribute for so little and we don't support those, we could add another attribute? I'd argue for dropping the OSBits for Operating System, I didn't see anyone use it. Keep it simple and wait for people to request additions?

[stevenaw]

I second the notion of keeping it simple.

FWIW I like the idea of using strings over enums when it comes to OS names. I suspect it will be easier to extend (both within nunit and externally) as any change to an enum is technically considered breaking. Possibly the same for runtimes too. A few years ago I would've said netfx, core, and mono are all we'd ever need for runtime, but Blazor/wasm has made me second guess myself here. The push to "run .net everywhere" could make is hard for us to account for "everywhere".

OSbits are likely fairly static and I'd have no concerns being an enum.

Unfortunately, as we know, a string-based approach would complicate discoverability. I'm not sure what a "discoverable" string-based approach would could look like. Some parts of.net like httpclient's HttpMethod use "strongly type strings" but that requires a wrapping class which I don't think is an option for attributes.

[stevenaw]

Oh, actually, if we go with separate attributes, as @rprouse suggested, perhaps the "IsOSAttribute" could define 2 constructors (one for string representation and one for enum). If the internal representation stayed a string then the enum ctor would be a nice discoverable option while the string ctor could be used for extensibility or future as-yet-unpredictable values.

[rprouse]

I would argue that future values would be added to the enum so that it always represents the complete list of what we support.

[stevenaw]

"What we support" is a fair point. Would we use a semver-major release for that?

[rprouse]

Yee, this would be for 4.0

[rprouse]

Sorry, you mean changing the enum don't you? I'd assume that new values would be added not removed so it would not be a breaking change.

[stevenaw]

Sorry, I think I'd been misremembering something I'd read. I'd meant changing by adding values in the future.

I'd thought I read recommendation that adding values to a public enum should be considered breaking since a common pattern is to switch over a hard-coded list of values with a default case to throw an exception, and that a newly introduced value could cause such code to throw, but that pattern and recommendation really only applies to non-flag enums.

Since we're talking about a flagged enum, there's no concerns on my end about using flagged enums for all 4 scenarios. Sorry for the confusion!

[jnm2]

I hadn't heard of adding a non-flags enum member being considered a breaking change. Here's the list that the .NET runtime uses: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-change-rules.md

[stevenaw]

Thanks @jnm2 It had been in a print copy of "Framework Design Guidelines".

The other nuance I had forgotten about was that the switch/case/default concern would primarily be relevant when the enum is output or returned from the framework, rather than being used as input as was suggested above. As a result, the guideline actually was "CONSIDER adding values to enums, despite a small compatibility risk".

Seems I remembered the edge case better than the recommendation 😅