Add feature switch attribute design doc #94625
Conversation
ghost
added
the
linkable-framework
|
Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar Issue DetailsOpening this as a draft for early review. I wanted to get a few eyes on this before making an API proposal.
|
dotnet-issue-labeler
bot
added
the
needs-area-label
sbomer
force-pushed
the
featureSwitchAttributes
branch
from
cb5df07
to
a6a8428
Compare
teo-tsirpanis
removed
the
needs-area-label
|
|
||
| - Take into account how this would interact with an attribute-based model for feature switches | ||
|
|
||
| We will explore what an attribtute-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. |
There was a problem hiding this comment.
I agree that even if we don't actually implement the switch model, it would be great to have the design for it fully fleshed out. These feel very related.
| } | ||
| ``` | ||
|
|
||
| We could use this model even for feature switches similar to `StartupHookSupport`, which don't currently have a `StartupHoopSupportAttribute`. 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. |
There was a problem hiding this comment.
On the surface this doesn't sound right to me - declaring an attribute which will do nothing when applied to something.
On the other hand - it "might" work if we introduce the attribute as internal - in that case nobody would be able to apply it to anything anyway, but it would still serve the purpose as the ID of the feature.
Or similarly make the constructor of the attribute private -> same effect.
I'm still not sure I like this, but it would work.
One thing I do like about this - we're not using strings to identify things, we use IL metadata - which is nice.
There was a problem hiding this comment.
I added some notes about this under "Alternate API shapes". I'm not 100% in love with it either, but it's the best approach I can think of so far, and every alternative that comes to mind has worse problems. I am thinking of it as just an identifier, for the cases where we don't care about using attributes for the analysis. And the analysis could be useful for other feature areas even if we aren't using it today.
- Fix typos - Move feature guard example earlier - Replace feature guard notes with separate section on constant prop - Add comparison with capability API draft, including terminology - Add implementation notes section
|
|
||
| - Terminology: "check" vs "switch" | ||
|
|
||
| We use `FeatureSwitch` in essentially the same way that the capability API draft uses `CapabilityCheck`. Maybe `FeatureCheck` would be a better name for the attribute, since the "switch" is really the MSBuild property that can be toggled, while the attribute should indicate that the property is a check for that setting. |
There was a problem hiding this comment.
I actually like the "switch" personally - from the point of view of the code, the property acts as a switch. The check happens if I condition on the property value. Some of these properties are not solely driven by MSBuild, but also have env. variable overrides, or hard coded values on some targets. The only thing it doesn't have is mutability...
I just found the "check" confusing - to me "check" sounds like an action, so not a good description of a piece of data/state.
There was a problem hiding this comment.
I guess I'm thinking of "switch" as something that can be actively toggled, whereas the property is immutable from the code's point of view. So in a way it makes sense to declare "this property is a check for the presence of a certain feature". I don't feel strongly about it though - interested in other people's opinions.
|
|
||
| We rely on trim warnings to alert the app author to the problem, and also default `StartupHookSupport` to `false` for trimmed apps. If we have an attribute-based model for feature guards, we may want to consider inferring these defaults from the `FeatureGuard` instead (so a guard for `RequiresUnreferencedCode` that is also a feature switch would be `false` by default whenever "unreferenced code" is unavailable). | ||
|
|
||
| The proposal for now is not to infer any defaults and just take care to set appropriate defaults in the SDK. This means that custom feature guards that are also feature switches will need to do the same. For example, a library that defines a feature switch which guards `RequiresUnreferencedCode` will need to ship with MSBuild targets that disable the feature by default for trimmed apps. |
There was a problem hiding this comment.
I think we should add a sample for this - this part is probably the most complex topic in this document. Would be great to have a sample over startup hook, or something similar. How you tie a guard to a different feature switch and how it's supposed to work.
The defaults discussions is actually only important for illink/ilcompiler. The analyzer shouldn't really care as it needs to analyze both branches anyway. Also the correctness of this is not a problem because the "true" branch were the warnings are suppressed would still produce warnings in the app (illink/ilcompiler) if the feature switch is enabled. We basically need to be in a place where analyzer doesn't produce warnings in any case, but trimmer will produce them depending on the feature switch value.
There was a problem hiding this comment.
I expanded on this section a bit with an example
- FeatureAttribute -> RequiresFeatureAttribute - Add notes about analyzer support for Requires attributes - Mention feature guard validation in ILLink/ILCompiler - Expand on inferred defaults for feature guards/switches
|
Moved to dotnet/designs#305 |
Opening this as a draft for early review. I wanted to get a few eyes on this before making an API proposal.
edit: moved to dotnet/designs#305