Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Attribute model for feature APIs #96859

Closed
Tracked by #96511
sbomer opened this issue Jan 11, 2024 · 8 comments
Closed
Tracked by #96511

[API Proposal]: Attribute model for feature APIs #96859

sbomer opened this issue Jan 11, 2024 · 8 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Milestone

Comments

@sbomer
Copy link
Member

sbomer commented Jan 11, 2024

Background and motivation

.NET has feature switches which can be set to turn on/off areas of functionality in our libraries, with optional support for removing unused features when trimming or native AOT compiling.

Feature switches suffer from a poor user experience:

  • trimming support requires embedding an unintuitive XML file into the library, and
  • there is no analyzer support

This document proposes an attribute-based model for feature switches that will significantly improve the user experience, by removing the need for this XML and enabling analyzer support.

More detail and discussion in dotnet/designs#305.
The attribute model is heavily inspired by the capability-based analyzer draft.

API Proposal

namespace System.Diagnostics.CodeAnalysis;

[AttributeUsage(AttributeTargets.Property, Inherited = false)]
public sealed class FeatureCheckAttribute : Attribute
{
    public Type FeatureType { get; }
    public FeatureCheckAttribute(Type featureType)
}

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Property, Inherited = false, AllowMultiple = true)]
public sealed class FeatureGuardAttribute : Attribute
{
    public Type FeatureType { get; }
    public FeatureGuardAttribute(Type featureType)
}

[AttributeUsage(AttributeTargets.Class, Inherited = false)]
public sealed class FeatureSwitchDefinitionAttribute : Attribute
{
    public string SwitchName { get; }
    public FeatureSwitchDefinitionAttribute(string switchName)
}

[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Class, Inherited=false, AllowMultiple=true)]
public sealed class RequiresFeatureAttribute : Attribute
{
    public Type FeatureType { get; }
    public RequiresFeatureAttribute(Type featureType)
}

API Usage

FeatureCheck may be placed on a static boolean property to indicate that it is a check for the referenced feature (represented by a type):

namespace System.Runtime.CompilerServices;

public static class RuntimeFeature
{
    [FeatureCheck(typeof(RequiresDynamicCodeAttribute))]
    public static bool IsDynamicCodeSupported { get; } = AppContext.TryGetSwitch("System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported", out bool isDynamicCodeSupported) ? isDynamicCodeSupported : true;

    [FeatureCheck(typeof(DynamicCodeCompilation))]
    public static bool IsDynamicCodeCompiled => IsDynamicCodeSupported;
}

FeatureGuard on a feature type may be used to express dependencies between features.

namespace System.Diagnostics.CodeAnalysis;

[FeatureGuard(typeof(RequiresDynamicCodeAttribute))]
internal static class DynamicCodeCompilation { }

This allows the property, or RequiresFeatureAttribute referencing the feature type, to guard calls to APIs annotated as requiring that feature:

if (RuntimeFeature.IsDynamicCodeSupported) {
    APIWhichRequiresDynamicCode(); // No warning, thanks to FeatureCheck
}

if (RuntimeFeature.IsDynamicCodeCompiled) {
    APIWhichRequiresDynamicCodeCompilation(); // No warning, thanks to FeatureCheck
    APIWhichRequiresDynamicCode(); // No warning, thanks to FeatureCheck/FeatureGuard
}

[RequiresDynamicCode("Does something with dynamic codegen")]
static void APIWhichRequiresDynamicCode() {
    // ...
}

[RequiresFeature(typeof(DynamicCodeCompilation))]
static void APIWhichRequiresDynamicCodeCompilation() {
    // ...
    APIWhichRequiresDynamicCode(); // No warning, thanks to RequiresFeature/FeatureGuard
}

FeatureGuard may also be placed directly on a static boolean property as a shorthand, to define a simple guard without a separate feature type. So the annotations on IsDynamicCodeCompiled could be simplified to:

namespace  System.Diagnostics.CodeAnalysis;

public static class RuntimeFeature
{
    // ...

    [FeatureGuard(typeof(RequiresDynamicCodeAttribute))]
    public static bool IsDynamicCodeCompiled => IsDynamicCodeSupported;
}
if (RuntimeFeature.IsDynamicCodeCompiled)
    APIWhichRequiresDynamicCode(); // No warning, thanks to FeatureGuard

For trimming support, FeatureSwitchDefinition may be applied to the attribute type to give the feature a name:

namespace System.Diagnostics.CodeAnalysis;

[FeatureSwitchDefinition("System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported")]
public sealed class RequiresDynamicCodeAttribute
{
    // ...
}

When the app is trimmed with the feature switch "System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported" set to false, the properties are rewritten to return false, and the guarded branches are removed.

The initial implementation will not support RequiresFeatureAttribute. Instead, analyzer warnings will initially be limited to RequiresUnreferencedCodeAttribute, RequiresDynamicCodeAttribute, and RequiresAssemblyFilesAttribute. It will still be possible to define an (otherwise unused) type for use in FeatureCheck and FeatureGuard, to influence branch elimination.

Applications

Aside from the analysis for RequiresUnreferencedCode, RequiresDynamicCode, RequiresAssemblyFiles, these semantics work well for protecting usage of hardware intrinsics or crypto hash algorithms: #96859 (comment).

Alternative Designs

Separate type to represent feature

There could be a level of indirection, so that the feature is represented not by the attribute type, but by a separate type that is linked to the attribute type:

[FeatureAttribute(typeof(RequiresFeatureAttribute))]
[FeatureSwitchDefinition("MyLibrary.Feature.IsSupported")]
class Feature {
    [FeatureCheck(typeof(Feature))]
    public static bool IsSupported => ...;

    [RequiresFeature(typeof(Feature))]
    public static void DoSomething() { ... }
}

class RequiresFeatureAttribute : Attribute { ... }

The current proposal allows attribute or non-attribute types.

String-based API

The attributes could instead use strings, making the usage slightly more analogous to preprocessor symbols. The difference is that callsites or code blocks within a method can't be annotated directly, so the IsSupported check serves the purpose that #if serves, but at trim time.

class Feature {
    [FeatureCheck("MY_LIBRARY_FEATURE")]
    public static bool IsSupported => ...;

    [RequiresFeature("MY_LIBRARY_FEATURE")]
    public static void DoSomething() { ... }
}

class Consumer {
    static void Main() {
        if (Feature.IsSupported)
            Feature.DoSomething();
    }
}

(compare to preprocessor symbols):

class Library {
#if MY_LIBRARY_FEATURE
    public static void DoSomething() { ... }
#endif
}

class Consumer {
    static void Main() {
#if MY_LIBRARY_FEATURE
        Feature.DoSomething();
#endif
    }
}

Risks

The proposed API doesn't cover every possible pattern that might be useful for feature switches. We are aiming to start with a small, well-defined set of behavior, but need to ensure this doesn't lock us out of future extensions. We can extend these by adding extra constructor parameters to the attributes in the future, as discussed in dotnet/designs#305.

Updates

  • Replaced FeatureGuardAttribute with FeatureDependsOnAttribute Changed back later
  • Lifted restriction that feature type must be an attribute type
  • Updated FeatureName to SwitchName in FeatureSwitchDefinition
  • Included RequiresFeatureAttribute in the proposal
  • Changed FeatureDepndsOnAttribute back to FeatureGuardAttribute, allowed on properties or classes
@sbomer sbomer added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 11, 2024
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 11, 2024
@ghost
Copy link

ghost commented Jan 11, 2024

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

.NET has feature switches which can be set to turn on/off areas of functionality in our libraries, with optional support for removing unused features when trimming or native AOT compiling.

Feature switches suffer from a poor user experience:

  • trimming support requires embedding an unintuitive XML file into the library, and
  • there is no analyzer support

This document proposes an attribute-based model for feature switches that will significantly improve the user experience, by removing the need for this XML and enabling analyzer support.

More detail and discussion in dotnet/designs#305.
The attribute model is heavily inspired by the capability-based analyzer draft.

API Proposal

namespace System.Diagnostics.CodeAnalysis;

[AttributeUsage(AttributeTargets.Property, Inherited = false)]
public sealed class FeatureCheckAttribute : Attribute
{
    public Type FeatureType { get; }
    public FeatureCheckAttribute(Type featureType)
}

[AttributeUsage(AttributeTargets.Property, Inherited = false, AllowMultiple = true)]
public sealed class FeatureGuardAttribute : Attribute
{
    public Type FeatureAttributeType { get; }
    public FeatureGuardAttribute(Type featureAttributeType)
}

[AttributeUsage(AttributeTargets.Class, Inherited = false)]
public sealed class FeatureSwitchDefinitionAttribute : Attribute
{
    public string FeatureName { get; }
    public FeatureSwitchDefinitionAttribute(string featureName)
}

API Usage

FeatureCheck or FeatureGuard may be placed on static boolean properties to indicate that they (respectively) are a direct check for, or depend on, the referenced feature (represented by an attribute type):

namespace System.Runtime.CompilerServices;

public static class RuntimeFeature
{
    [FeatureCheck(typeof(RequiresDynamicCodeAttribute))]
    public static bool IsDynamicCodeSupported { get; } = AppContext.TryGetSwitch("System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported", out bool isDynamicCodeSupported) ? isDynamicCodeSupported : true;

    [FeatureGuard(typeof(RequiresDynamicCodeAttribute))]
    public static bool IsDynamicCodeCompiled => IsDynamicCodeSupported;
}

FeatureSwitchDefinition should be applied to the attribute type to give the feature a name:

namespace System.Diagnostics.CodeAnalysis;

[FeatureSwitchDefinition("System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported")]
public sealed class RequiresDynamicCodeAttribute : RequiresFeatureAttribute
{
    // ...
}

This allows them to guard calls to annotated APIs, so that the callsites don't produce analyzer warnings:

if (RuntimeFeature.IsDynamicCodeSupported)
    APIWhichRequiresDynamicCode(); // No warnings

if (RuntimeFeature.IsDynamicCodeCompiled)
    APIWhichRequiresDynamicCode(); // No warnings

[RequiresDynamicCode("Does something with dynamic codegen")]
static void APIWhichRequiresDynamicCode() {
    // ...
}

When the app is trimmed with the feature switch "System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported" set to false, the properties are rewritten to return false, and the guarded branches are removed.

Analyzer warnings will initially be limited to RequiresUnreferencedCodeAttribute, RequiresDynamicCodeAttribute, and RequiresAssemblyFilesAttribute. It will still be possible to define an (otherwise unused) attribute type for use in FeatureCheck and FeatureGuard, to influence branch elimination.

Alternative Designs

Separate type to represent feature

There could be a level of indirection, so that the feature is represented not by the attribute type, but by a separate type that is linked to the attribute type:

[FeatureAttribute(typeof(RequiresFeatureAttribute))]
[FeatureSwitchDefinition("MyLibrary.Feature.IsSupported")]
class Feature {
    [FeatureCheck(typeof(Feature))]
    public static bool IsSupported => ...;

    [RequiresFeature(typeof(Feature))]
    public static void DoSomething() { ... }
}

class RequiresFeatureAttribute : Attribute { ... }

String-based API

The attributes could instead use strings, making the usage slightly more analogous to preprocessor symbols. The difference is that callsites or code blocks within a method can't be annotated directly, so the IsSupported check serves the purpose that #if serves, but at trim time.

class Feature {
    [FeatureCheck("MY_LIBRARY_FEATURE")]
    public static bool IsSupported => ...;

    [RequiresFeature("MY_LIBRARY_FEATURE")]
    public static void DoSomething() { ... }
}

class Consumer {
    static void Main() {
        if (Feature.IsSupported)
            Feature.DoSomething();
    }
}

(compare to preprocessor symbols):

class Library {
#if MY_LIBRARY_FEATURE
    public static void DoSomething() { ... }
#endif
}

class Consumer {
    static void Main() {
#if MY_LIBRARY_FEATURE
        Feature.DoSomething();
#endif
    }
}

Risks

The proposed API doesn't cover every possible pattern that might be useful for feature switches. We are aiming to start with a small, well-defined set of behavior, but need to ensure this doesn't lock us out of future extensions. We can extend these by adding extra constructor parameters to the attributes in the future, as discussed in dotnet/designs#305.

Author: sbomer
Assignees: -
Labels:

api-suggestion, area-System.Runtime.CompilerServices, untriaged

Milestone: -

@sbomer sbomer self-assigned this Jan 11, 2024
@sbomer sbomer added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Jan 23, 2024
@sbomer sbomer added this to the 9.0.0 milestone Jan 29, 2024
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 29, 2024
@bartonjs
Copy link
Member

bartonjs commented Jan 30, 2024

Video

  • FeatureSwitchDefinition: FeatureName => SwitchName, to align with the terminology from AppContext
  • Consider if FeatureCheck can be eliminated in terms of FeatureGuard (if FeatureCheck is intended for sourcegen then it has unique value).
  • FeatureGuardAttribute.FeatureAttributeType => FeatureType
  • We're concerned that the names are over-generalized but that we haven't proven out that it can really be used generally. If it can't be applied to RequiresDynamicCode, the Intrinsics/ISA types, and some third technology, we need to revisit these names.
namespace System.Diagnostics.CodeAnalysis;

[AttributeUsage(AttributeTargets.Property, Inherited = false)]
public sealed class FeatureCheckAttribute : Attribute
{
    public Type FeatureType { get; }
    public FeatureCheckAttribute(Type featureType)
}

[AttributeUsage(AttributeTargets.Property, Inherited = false, AllowMultiple = true)]
public sealed class FeatureGuardAttribute : Attribute
{
    public Type FeatureType { get; }
    public FeatureGuardAttribute(Type featureType)
}

[AttributeUsage(AttributeTargets.Class, Inherited = false)]
public sealed class FeatureSwitchDefinitionAttribute : Attribute
{
    public string SwitchName { get; }
    public FeatureSwitchDefinitionAttribute(string switchName)
}

@bartonjs bartonjs added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 30, 2024
@sbomer
Copy link
Member Author

sbomer commented Feb 9, 2024

Based on an exploration of the corelib intrinsics analyzer, I've updated the proposal to allow FeatureGuardAttribute on classes. This allows "Requires" attributes like [CompExactlyDependsOn(typeof(Avx2))] (and not just IsSupported checks) to serve as guards for features that they depend on.

Usage for intrinsics analyzer

These attributes can express the semantics of the IsSupported checks and dependencies between intrinsics APIs as follows:

(see "Corelib intrinsics analyzer" in dotnet/designs#305 for a more detailed comparison)

namespace System.Runtime.Intrinsics.X86;

[RequiresFeature(typeof(Avx2))] // Similar to [CompExactlyDependsOn(typeof(Avx2))]
[FeatureGuard(typeof(Avx))]
public abstract class Avx2 : Avx
{
    [FeatureCheck(typeof(Avx2))]
    public static new bool IsSupported => // ...

    [RequiresFeature(typeof(X64))]
    [FeatureGuard(typeof(Avx.X64))]
    [FeatureGuard(typeof(Avx2))]
    public new abstract class X64 : Avx.X64
    {
        [FeatureCheck(typeof(X64))]
        public static new bool IsSupported => // ...
    }
}

This allows the IsSupported checks, or equivalent RequiresFeature attributes, to guard calls to any features they depend on:

if (Avx2.IsSupported) {
    Avx2.DoSomething();
    Avx.DoSomething();
}

if (Avx2.X64.IsSupported) {
    Avx2.DoSomething();
    Avx.X64.DoSomething();
}

[RequiresFeature(typeof(Avx2))]
static void VectorizationHelper1() {
    Avx2.DoSomething();
    Avx.DoSomething();
}

[RequiresFeature(typeof(Avx2.X64))]
static void VectorizationHelper2() {
    Avx2.DoSomething();
    Avx.X64.DoSomething();
}

Usage for crypto hash algorithms

These attribute definitions would work as follows for an analyzer designed to propagate requirements on the SHA3 family of hash algorithms:

[RequiresFeature(typeof(SHA3_256)]
public abstract class SHA3_256 : HashAlgorithm
{
    [FeatureCheck(typeof(SHA3_256))]
    public static bool IsSupported => // ...

    public static new SHA3_256 Create()
    {
        CheckSha3Support();
        return new Implementation();
    }

    // ...

    [RequiresFeature(typeof(SHA3_256))]
    private sealed class Implementation : SHA3_256
    {
        // ...
    }
}

Here, [RequiresFeature(typeof(SHA3_256))] at the type level protects calls to members of SHA3_256 (except for the IsSupported check), which are allowed only in a context that has an IsSupported check or equivalent RequiresFeature attribute:

var data = new byte[] { 0x01, 0x02 };

SHA3_256.HashData(data); // warning

if (SHA3_256.IsSupported)
    SHA3_256.HashData(data); // OK

[RequiresFeature(typeof(SHA3_256))]
static void HashHelper() {
    SHA3_256.HashData(data); // OK
}

Note on Requires attributes

[RequiresDynamicCode], [RequiresUnreferencedCode], [RequiresAssemblyFiles] and [CompExactlyDependsOn(typeof(Avx2))] have similar semantics for their respective analyzers.

We could provide a generalized analyzer that understands [RequiresFeature(typeof(MyFeature))] for arbitrary features. Analyzers for trimming support and intrinsics could share most of the implementation, and the generalized analyzer could be useful on its own, for example for protecting the SHA3 hash algorithms.

We believe the FeatureCheck/FeatureGuard/FeatureSwitchDefinition attributes are sufficient to be useful on their own for existing analyzers. But I've included RequiresFeatureAttribute in the proposal above to show what this would look like for a generalized analyzer.

Updates

  • Changed FeatureDependsOnAttribute back to FeatureGuardAttribute, allowed on properties or classes
  • Moved RequiresFeatureAttribute to main API proposal and used it in examples

@sbomer sbomer added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Feb 16, 2024
sbomer added a commit that referenced this issue Feb 29, 2024
Adds support for annotating static boolean properties with
`[FeatureCheckAttribute(typeof(RequiresDynamicCodeAttribute))]`,
causing the property to be treated as a guard for analyzer
warnings about the corresponding `Requires` attribute.

Adds two new warnings for:

- Invalid use of `FeatureCheckAttribute` applied to a non-static
  or non-bool property

- Implementation of the property doesn't obviously satisfy
  the "guard property" (it should return false whenever the
  guarded feature is disabled).

See #94625 for notes on the design.
See #96859 for the API proposal.

---------

Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
@rolfbjarne
Copy link
Member

Analyzer warnings will initially be limited to RequiresUnreferencedCodeAttribute, RequiresDynamicCodeAttribute, and RequiresAssemblyFilesAttribute.

Here's a scenario where supporting different attributes for analyzer warnings would be useful: xamarin/xamarin-macios#20059 (comment)

Basically we want to annotate APIs in the BCL so that if developers use those APIs they get a warning if they haven't configured their projects correctly (developers have to add a file to their apps explaining why they're using certain APIs, and we want to automatically be able to tell developers they need to add said file + we want to automate annotation of our API surface where we only need manual intervention when we use the native API that needs annotation, and then we have the analyzer tell us all the resulting managed APIs that use said native API, recursively).

@bartonjs
Copy link
Member

bartonjs commented Mar 5, 2024

Video

  • RequiresFeature was removed due to a clarified scope of need, and considering the complications with an analyzer (all the features using the same diagnostic ID)
  • FeatureGuard ("the dependency flow") was removed for simplicity
  • FeatureCheck ("I answer whether this thing is enabled") is renamed to FeatureGuard to be consistent with OSPlatformGuard.
  • We made both remaining attributes apply only to AttributeTargets.Property. There should ideally be an analyzer which restricts this to being applied to static get-only properties.
    • Potential enhancements, like methods, fields, and does-not-return-unless are deferred.
namespace System.Diagnostics.CodeAnalysis;

[AttributeUsage(AttributeTargets.Property, Inherited = false, AllowMultiple = true)]
public sealed class FeatureGuardAttribute : Attribute
{
    public Type FeatureType { get; }
    public FeatureGuardAttribute(Type featureType)
}

[AttributeUsage(AttributeTargets.Property, Inherited = false)]
public sealed class FeatureSwitchDefinitionAttribute : Attribute
{
    public string SwitchName { get; }
    public FeatureSwitchDefinitionAttribute(string switchName)
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 5, 2024
@KevinCathcart
Copy link
Contributor

KevinCathcart commented Mar 8, 2024

I've typed this up just to make sure I understand what was landed on here. Please let me know if any of the following is meaningfully incorrect.

FeatureSwitchDefinitionAttribute is something the Linker will understand, as an alternative to using the xml format. Placed on a boolean property, the linker will do something roughly along the lines of replacing calls to the attributed property with false if the specified feature switch is not provided to the linker, with dead code elimination thus removing any code guarded with this.

FeatureGuardAttribute is a theoretically generalizable attribute intended to be placed on boolean feature guard properties, designed for use by analyzers. It points at a type. The ILLink analyzer will support it with respect to a number of types known to it, like RequiresDynamicCodeAttribute, and RequiresUnreferencedCodeAttribute and presumably effectively ignore it when it does not recognize the feature type.

[FeatureGuard(...)] usage scenario by third party library

As a third party library author, I can put [FeatureGuard(typeof(RequiresDynamicCodeAttribute))] or similar on static Boolean properties where returning true would imply the presence of that underlying runtime feature. This would let me avoid checks against the canonical guards, or warnings from the analyzer for failing to do so.

Presumably, when I do so I should also && any condition in the property with the canonical feature check property like RuntimeFeature.IsDynamicCodeSupported. This would allow the linker to conclude my property always returns false if said feature is not passed to the linker. Failing to do so could potentially result in linker warnings if I don't use the canonical property checks deeper in my code, and even if I do, the linker may be unable to trim some other logic guarded by my property resulting in larger than necessary output.

[FeatureSwitchDefinition(...)] usage scenario by third party library

Let's assume my library has a not always used feature that pulls in a lot of code in a way the linker cannot automatically remove when an application doesn't need it. For this example support for a file format. but it is not fundamentally tied to something like RequiresDynamicCodeAttribute, I could define a property like the following, and wrap relevant code with the guard, to make it possible to trim away such features:

static internal class TrimmableFeatures
{
    [FeatureSwitchDefinition("Contoso.ImageProcessing.IsWebPSupported")]
    public bool IsWebPSupported => true;
}

This could be combined with a bit of MSBuild in my library's NuGet package to allow applications to say they don't want WebP support when trimmed by setting say a <ContosoTrimWebPSupport>true</...> property in the application's csproj file. Assuming the support works in all runtimes, and this trimming support is just for space reasons, there would be no strict need to have the property to really check an AppContext switch or anything like that, although such support could be included if say I wanted users to be able to simulate the trimmed experience when debugging the untrimmed version of the library.

This would of course provide no analyzer level checking, which might be fine for my library, for example if there is no WebP specific public API surface, and I was not seeking help ensuring I code the inside of my library correctly. If I did want analyzer checks, either for inside the library, or potentially even for callers of the library, I'd add a RequiresWebPSupportAttribute, and write an analyzer (and make my property public if needed). My analyzer could potentially even support [FeatureGuard(typeof(RequiresWebPSupportAttribute))] if so desired to allow downstream libraries to create their own guards that imply mine.

@sbomer
Copy link
Member Author

sbomer commented Mar 8, 2024

@KevinCathcart thanks for the write-up! The general idea is right. A few clarifications:

  • I think you meant to write FeatureGuard instead of FeatureCheck in a few places

  • Presumably, when I do so I should also && any condition in the property with the canonical feature check property like RuntimeFeature.IsDynamicCodeSupported. This would allow the linker to conclude my property always returns false if said feature is not passed to the linker.

    The FeatureGuard or FeatureSwitchDefinition is actually what allows the tools to conclude that the property returns false for disabled features, regardless of the body. But you should && the condition with the canonical feature check so that the check returns false even in non-trimmed apps (which could still have dynamic code support disabled via the runtimeconfig, for example).

    Instead of "is not passed to the linker", I would say "is disabled". For FeatureGuard on its own (without FeatureSwitchDefinition), this only applies to RequiresUnreferencedCode. If the linker command-line also has --feature System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported, then guards for RequiresDynamicCodeAttribute will also be treated as returning false.

    For properties that have FeatureSwitchDefinition, then the feature is disabled when the feature is explicitly set to false on the command-line.

  • although such support could be included if say I wanted users to be able to simulate the trimmed experience when debugging the untrimmed version of the library.

    This is actually an important invariant we try to maintain: untrimmed versions of a library that use the same runtimeconfig settings that would be used in a trimmed app should have the same behavior. So you should check AppContext.

@sbomer
Copy link
Member Author

sbomer commented Mar 12, 2024

@sbomer sbomer closed this as completed Mar 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Projects
None yet
Development

No branches or pull requests

4 participants