Add feature switch attribute design doc

[sbomer]

Adds a design for an attribute-based model for feature switches, to be supported in the trim/AOT analyzers and in ILLink/ILCompiler. There's significant overlap with @terrajobst's capability API analyzer draft in #261, but this approaches the problem more specifically with trimming support in mind.

[akoeplinger]

one slight complication with this is that we have some more complex logic in the platform compatibility checks, e.g. we know that MacCatalyst is a subset of iOS so checking OperatingSystem.IsIOS() is enough to also check for MacCatalyst.

[sbomer]

The platform subsets (versions aside) are semantically almost the same idea as feature guards - IsIOS is annotated with SupportedOSPlatformGuard("maccatalyst") which effectively declares that iOS is a subset of MacCatalyst.

[terrajobst]

It would help if you could list all the features you'll want to guard today. The only examples that currently exist seem to be around dynamic code. That doesn't seem to be too compelling, especially because those already have attributes.

Also, do you imagine that users can extend the set of features? Or are the features being guarded a closed set that is defined by the .NET platform?

[sbomer]

I added a section with example use cases.

Also, do you imagine that users can extend the set of features?

Yes, with some limitations. I'm suggesting that users should be able to define:

• guards for existing features supported by the trim analyzer
• new feature switches with trimming support (but not analyzer support, initially)

So the initial implementation would allow defining guards for RequiresDynamicCodesAttribute and RequiresUnreferencedCodeAttribute, and defining new features with trimming/AOT branch removal support but no analyzer warnings.

But the two seem naturally related, so my goal is for the existing analysis attributes to be defined in terms of a more general model, whether or not we actually teach the analyzer to produce warnings for arbitrary feature attributes.

[terrajobst]

Using a type seems unfortunate because that would require on attribute per feature and would disallow more complex shapes (eg an attribute with flags).

[sbomer]

This is mentioned under "Feature attribute schemas" in the possible future extensions. I expanded a bit on the example to show what it might look like to define a feature switch where the feature attribute takes extra parameters.

I don't think we're going to be able to cover every possible attribute shape, so my goal is to start with a restricted model, while showing that it can be extended in various directions. I'm not attached to this particular API shape, but defining features in terms of types seemed like a pretty natural starting point. Happy to take other suggestions.

[sbomer]

The "Feature switches without feature attributes" has my favorite alternative so far - basically using the feature name string as the uniform representation.

[jtschuster]

Personally, I think it makes the most sense to have a single identifier to associate guards, switches, and requirements to a feature. To me, using the feature name everywhere rather than an attribute makes the most sense, and I don't think we'd get an issue with correlation in the analyzer, trimmer, or Native AOT if we can change the existing Requires attributes.

I think this generalizes well to other feature switches, and fits well with AppContext.TryGetSwitch. Any guarded block (or method body with a RequiresFeatureAttribute) is assumed to have the guarded features "enabled" within it. Calls to APIs with the RequiresFeatureAttribute would compare the required feature name to the list of enabled feature names for that block.

The trimmer and Native AOT could hard code the (new) "UnreferencedCode" and "DynamicCode" feature names as false and could accept other feature names and values at the command line to do branch elimination for arbitrary feature switches.

I also like the proposed NamedFeatureAttribute as the base RequiresFeatureAttribute so we don't need to require FeatureNameAttribute to only be on RequiresFeatureAttribute-derived classes, and can apply the attribute ad-hoc without defining a new derived attribute.

class RequiresFeatureAttribute : Attribute
{
    public string FeatureName { get; }

    public NamedFeatureAttribute(string name) => FeatureName = name;
}

// [FeatureName("MyLibrary.Features.MyFeature")]
// class RequiresMyFeature : RequiresFeatureAttribute { }

class RequiresMyFeature : RequiresFeatureAttribute
{
    public RequiresMyFeature() : base("MyLibrary.Features.MyFeature") { }
}

class Features
{
    [FeatureSwitch("MyLibrary.Features.MyFeature")]
    public static bool MyFeature => AppContext.TryGetSwitch("MyLibrary.Features.MyFeature" out bool val) && val;

    [FeatureSwitch("MyLibrary.Features.MyOtherFeature")]
    public static bool MyOtherFeature => AppContext.TryGetSwitch("MyLibrary.Features.MyOtherFeature" out bool val) && val;

    [FeatureGuard("MyLibrary.Features.MyOtherFeature")]
    [FeatureGuard("MyLibrary.Features.MyFeature")]
    public static bool BothFeatures => MyFeature && MyOtherFeature;
}

public class Program
{
    [RequiresMyFeature]
    public static void MyFeatureMethod() { }

    [Requires("MyLibrary.Features.MyOtherFeature")]
    public static void MyOtherFeatureMethod() { }
}

[sbomer]

Thanks @jtschuster, I incorporated those suggestions into the "Feature switches without feature attributes" section.

I think the main tradeoff is that the string-based model requires giving a string name to every feature (which we may or may not want for RequiresUnreferencedCodeAttribute), whereas the attribute-based model requires defining an attribute for each feature. At least the attribute types can be private, while the string names are effectively public. However, I do like that the string-based API shape resembles preprocessor symbols.

[sbomer]

I had to resist calling this UnconditionalConditionalAttribute 😅

[vitek-karas]

I'm going to play devil's advocate and only provide negative feedback ;-)

For the proposal from Jackson - feature name model:

• The feature name string is the least visible part of the feature. It's rarely used in MSBuild (where it's typically aliased with a property). It's basically never used in C# where one uses the real property. It's only visible in the internal code (MSBuild prop definition, and C# property definition) and in runtime config/AppContext where nobody looks.
• Would force us to define a feature string name for RequiresUnreferencedCode

For the attribute based model:

• This requires each feature to define a "Requires" attribute - even if it's not needed and not wanted. For example, we would not really want to define RequiresEventSource attribute, or lot of the others.

If I had to pick one of these two, I would prefer the string based model. I honestly like the "Separate type to define the feature" more... but neither is perfect :-)

[sbomer]

Per today's discussion:

• Changed FeatureSwitch to FeatureCheck
• Changed FeatureName to FeatureSwitchDefinition, in case we want to make the string argument optional and infer the feature name from the type name in the future. (Or should this be just FeatureDefinition?)
• We agreed on the API shape where FeatureCheck directly references the feature attribute type for now