From ecee832f08d69c9dcabdd98b3a881619150622ac Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Wed, 22 Nov 2023 19:50:24 +0000 Subject: [PATCH] PR feedback - Move IsDynamicCodeCompiled goal down - Mention challenge with arbitrary feature analysis - Add future extension of validating FeatureSwitch --- accepted/2023/feature-switch-attributes.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/accepted/2023/feature-switch-attributes.md b/accepted/2023/feature-switch-attributes.md index a2150603c..50442a92f 100644 --- a/accepted/2023/feature-switch-attributes.md +++ b/accepted/2023/feature-switch-attributes.md @@ -57,7 +57,6 @@ The ILLink Roslyn analyzer has built-in support for treating `IsDynamicCodeSuppo ## Goals -- Teach the ILLink Roslyn analyzer to treat `IsDynamicCodeCompiled` as a guard for `RequiresDynamicCodeAttribute` - Allow libraries to define their own feature guard properties Libraries should be able to introduce their own properties that can act as guards for `RequiresDynamicCodeAttribute`, or for other features that might produce warnings in the analyzer @@ -68,6 +67,8 @@ The ILLink Roslyn analyzer has built-in support for treating `IsDynamicCodeSuppo We will explore what an attribute-based model for feature switches would look like to ensure that it interacts well with a model for feature guards. It's possible that we would design both in conjunction if they are naturally related. +- Teach the ILLink Roslyn analyzer to treat `IsDynamicCodeCompiled` as a guard for `RequiresDynamicCodeAttribute` using the same model we define for third-party libraries + ### Non-goals - Support branch elimination in the analyzer for all feature switches @@ -410,7 +411,7 @@ The analyzer would validate that `FeatureGuardAttribute` and `FeatureSwitchAttri We could use this model even for feature switches similar to `StartupHookSupport`, which don't currently have a `StartupHookSupportAttribute`. There's no need to actually annotate related APIs with the attribute if that is overkill for a small feature. In this case the attribute definition would just serve as metadata that allows us to define a feature switch in a uniform way. -Note that this makes it possible to define custom `Requires` attributes for arbitrary features. While the immediate goal of this proposal is not to enable analyzer support for analysis warnings based on such custom attributes, the model intentionally allows for this so that we could easily open up the analyzer to work for custom features. However, initially support for analysis warnings would be limited to `RequiresUnreferencedCodeAttribute`, `RequiresDynamicCodeAttribute`, and `RequiresAssemblyFilesAttribute`. +Note that this makes it possible to define custom `Requires` attributes for arbitrary features. While the immediate goal of this proposal is not to enable analyzer support for analysis warnings based on such custom attributes, the model intentionally allows for this so that we could easily open up the analyzer to work for custom features. However, initially support for analysis warnings would be limited to `RequiresUnreferencedCodeAttribute`, `RequiresDynamicCodeAttribute`, and `RequiresAssemblyFilesAttribute`. Analyzers currently require all supported diagnostic IDs to be declared up-front, so allowing analysis for arbitrary features would mean that all features share the same analyzer diagnostic ID. We would need to consider solutions to this problem. ## Comparison with "capability-based analyzer" @@ -550,6 +551,17 @@ We might eventually want to extend the semantics in a few directions: class RequiresNoDynamicCodeAttribute : RequiresNotAttribute {} ``` +- Validation or generation of feature switch implementation + + The recommended pattern for implementing feature switches is to check `AppContext`, for example: + + ```csharp + [FeatureSwitch(typeof(RequiresDynamicCodeAttribute))] + static bool IsDynamicCodeSupported => AppContext.TryGetSwitch("RuntimeFeature.IsDynamicCodeSupported", out bool isEnabled) ? isEnabled : false; + ``` + + We could consider adding validation that the body correctly checks `AppContext` for the feature name associated with the feature attribute, or adding a source generator that would generate the implementation from the `FeatureSwitchAttribute`. + - Versioning support for feature attributes/checks/guards The model here would extend naturally to include support for version checks the same way that the platform compatibility analyzer does. Versions would likely be represented as strings because they are encodable in custom attributes: