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 https://github.com/dotnet/designs/pull/261, but this approaches the problem more specifically with trimming support in mind.

[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.

[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