Disable default and ambient attribute at runtime with a feature switch #100416
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-componentmodel |
| @@ -35,6 +39,12 @@ public class DefaultValueAttribute : Attribute | |||
| // The null check and try/catch here are because attributes should never throw exceptions. | |||
| // We would fail to load an otherwise normal class. | |||
|
|
|||
| if (!IsSupported) | |||
| { | |||
| Debug.Assert(!IsSupported, "Runtime instantiation of this attribute is not allowed."); | |||
There was a problem hiding this comment.
Assert vs. throw approaches - was the Assert approach taken to be backwards compatible here?
There was a problem hiding this comment.
Yes, respecting the comments on the type so as not to throw
| Debug.Assert(!IDesignerHost.IsSupported, "Runtime instantiation of this attribute is not allowed."); | ||
| return; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| Value = TypeDescriptor.GetConverter(type).ConvertFromInvariantString(value); |
There was a problem hiding this comment.
Was this (and the case for DefaultValueAttribute below) causing an error at runtime or is this to help with linkability (e.g. #97842)?
There was a problem hiding this comment.
Yes, this is to help with #97842 (trimming with the corresponding feature switch is set).
| @@ -22,6 +23,9 @@ public class DefaultValueAttribute : Attribute | |||
| // Delegate ad hoc created 'TypeDescriptor.ConvertFromInvariantString' reflection object cache | |||
| private static object? s_convertFromInvariantString; | |||
|
|
|||
| [FeatureSwitchDefinition("System.ComponentModel.DefaultValueAttribute.IsSupported")] | |||
| internal static bool IsSupported => AppContext.TryGetSwitch("System.ComponentModel.DefaultValueAttribute.IsSupported", out bool isSupported) ? isSupported : true; | |||
There was a problem hiding this comment.
If this is made readonly then the JIT should be able to remove code as well (for cases when the trimmer isn't used). (requires adding a readonly field + a property)
@sbomer is my readonly comment above still applicable? Do we have guidance for this?
There was a problem hiding this comment.
The sanctioned pattern that should be optimizable by the JIT is a static bool property with a getter (see original design at https://github.com/dotnet/designs/blob/main/accepted/2020/feature-switch.md). I don't believe a field is necessary, but haven't checked the JIT codegen. @vitek-karas
There was a problem hiding this comment.
Hmm, in addition to perhaps supporting JIT, a static readonly field accessed by the property would cache the value, so AppContext.TryGetSwitch() doesn't get called every time (again, only for non-trimmed scenarios).
|
|
||
| if (!IDesignerHost.IsSupported) | ||
| { | ||
| Debug.Assert(!IDesignerHost.IsSupported, "Runtime instantiation of this attribute is not allowed."); |
There was a problem hiding this comment.
Just curious - it feels weird asserting that the condition of the if is true in the true branch...
There was a problem hiding this comment.
The assert is meant to give an indication of the problem of using this attribute when the feature switch is set. Throwing (with an appropriate message) here would be ideal but it seems prudent to respect the comments on the type. Consumers (like WinForms) could also generate a warning via its code generator when it detects use of this attribute at runtime if the feature switch is set.
There was a problem hiding this comment.
So is it supposed to be Debug.Assert(false)? Currently it seems like the assert will never fail.
dotnet#100416) * Disable default and ambient attribute at runtime with a feature switch * throwing at the getter at runtime
Disable design time attributes being used at runtime via a feature switch.