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.

Sign up for GitHub

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

Jump to bottom

Disable default and ambient attribute at runtime with a feature switch #100416

Merged
merged 3 commits into from
Apr 5, 2024
Merged

Disable default and ambient attribute at runtime with a feature switch #100416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b5a2240
Disable default and ambient attribute at runtime with a feature switch
LakshanF Mar 28, 2024
f4da5ba
Merge branch 'main' into DefaultValueFix
LakshanF Mar 28, 2024
acd6ca6
throwing at the getter at runtime
LakshanF Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/libraries/System.ComponentModel.TypeConverter/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,9 @@
<data name="NullableConverterBadCtorArg" xml:space="preserve">
<value>The specified type is not a nullable type.</value>
</data>
<data name="RuntimeInstanceNotAllowed" xml:space="preserve">
<value>Runtime instantiation of this attribute is not allowed.</value>
</data>
<data name="Text" xml:space="preserve">
<value>(Text)</value>
</data>
Expand Down
46 changes: 34 additions & 12 deletions ...es/System.ComponentModel.TypeConverter/src/System/ComponentModel/AmbientValueAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.ComponentModel.Design;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;

namespace System.ComponentModel
Expand All @@ -12,6 +14,11 @@ namespace System.ComponentModel
[AttributeUsage(AttributeTargets.All)]
public sealed class AmbientValueAttribute : Attribute
{
/// <summary>
/// This is the default value.
/// </summary>
private object? _value;

/// <summary>
/// Initializes a new instance of the <see cref='System.ComponentModel.AmbientValueAttribute'/> class, converting the
/// specified value to the specified type, and using the U.S. English culture as the
Expand All @@ -22,9 +29,15 @@ public AmbientValueAttribute([DynamicallyAccessedMembers(DynamicallyAccessedMemb
{
// The try/catch here is because attributes should never throw exceptions. We would fail to
// load an otherwise normal class.

if (!IDesignerHost.IsSupported)
{
return;
}

try
{
Value = TypeDescriptor.GetConverter(type).ConvertFromInvariantString(value);
_value = TypeDescriptor.GetConverter(type).ConvertFromInvariantString(value);
}
catch
{
Expand All @@ -37,7 +50,7 @@ public AmbientValueAttribute([DynamicallyAccessedMembers(DynamicallyAccessedMemb
/// </summary>
public AmbientValueAttribute(char value)
{
Value = value;
_value = value;
}

/// <summary>
Expand All @@ -46,7 +59,7 @@ public AmbientValueAttribute(char value)
/// </summary>
public AmbientValueAttribute(byte value)
{
Value = value;
_value = value;
}

/// <summary>
Expand All @@ -55,7 +68,7 @@ public AmbientValueAttribute(byte value)
/// </summary>
public AmbientValueAttribute(short value)
{
Value = value;
_value = value;
}

/// <summary>
Expand All @@ -64,7 +77,7 @@ public AmbientValueAttribute(short value)
/// </summary>
public AmbientValueAttribute(int value)
{
Value = value;
_value = value;
}

/// <summary>
Expand All @@ -73,7 +86,7 @@ public AmbientValueAttribute(int value)
/// </summary>
public AmbientValueAttribute(long value)
{
Value = value;
_value = value;
}

/// <summary>
Expand All @@ -82,7 +95,7 @@ public AmbientValueAttribute(long value)
/// </summary>
public AmbientValueAttribute(float value)
{
Value = value;
_value = value;
}

/// <summary>
Expand All @@ -91,7 +104,7 @@ public AmbientValueAttribute(float value)
/// </summary>
public AmbientValueAttribute(double value)
{
Value = value;
_value = value;
}

/// <summary>
Expand All @@ -100,15 +113,15 @@ public AmbientValueAttribute(double value)
/// </summary>
public AmbientValueAttribute(bool value)
{
Value = value;
_value = value;
}

/// <summary>
/// Initializes a new instance of the <see cref='System.ComponentModel.AmbientValueAttribute'/> class using a <see cref='string'/>.
/// </summary>
public AmbientValueAttribute(string? value)
{
Value = value;
_value = value;
}

/// <summary>
Expand All @@ -117,13 +130,22 @@ public AmbientValueAttribute(string? value)
/// </summary>
public AmbientValueAttribute(object? value)
{
Value = value;
_value = value;
}

/// <summary>
/// Gets the ambient value of the property this attribute is bound to.
/// </summary>
public object? Value { get; }
public object? Value {
get
{
if (!IDesignerHost.IsSupported)
{
throw new ArgumentException(SR.RuntimeInstanceNotAllowed);
}
return _value;
}
}

public override bool Equals([NotNullWhen(true)] object? obj)
{
Expand Down
5 changes: 4 additions & 1 deletion src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3359,6 +3359,9 @@
<data name="RFLCT_Targ_StatMethReqTarg" xml:space="preserve">
<value>Non-static method requires a target.</value>
</data>
<data name="RuntimeInstanceNotAllowed" xml:space="preserve">
<value>Runtime instantiation of this attribute is not allowed.</value>
</data>
<data name="RuntimeWrappedException" xml:space="preserve">
<value>An object that does not derive from System.Exception has been wrapped in a RuntimeWrappedException.</value>
</data>
Expand Down Expand Up @@ -4313,4 +4316,4 @@
<data name="WasmThreads_BlockingWaitNotSupportedOnJSInterop" xml:space="preserve">
<value>Blocking wait is not supported on the JS interop threads.</value>
</data>
</root>
</root>
23 changes: 22 additions & 1 deletion src/libraries/System.Private.CoreLib/src/System/ComponentModel/DefaultValueAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Reflection;
Expand All @@ -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;
Copy link
Member

@steveharter steveharter Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// <summary>
/// Initializes a new instance of the <see cref='DefaultValueAttribute'/>
/// class, converting the specified value to the specified type, and using the U.S. English
Expand All @@ -35,6 +39,12 @@ public DefaultValueAttribute(
// 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.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assert vs. throw approaches - was the Assert approach taken to be backwards compatible here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, respecting the comments on the type so as not to throw

return;
}

if (type == null)
{
return;
Expand Down Expand Up @@ -229,7 +239,18 @@ public DefaultValueAttribute(ulong value)
/// <summary>
/// Gets the default value of the property this attribute is bound to.
/// </summary>
public virtual object? Value => _value;
public virtual object? Value
{
get
{
if (!IsSupported)
{
throw new ArgumentException(SR.RuntimeInstanceNotAllowed);
}
return _value;
}
}


public override bool Equals([NotNullWhen(true)] object? obj)
{
Expand Down
Loading