Expression.Default and Expression.New with structs with parameterless constructors #57034

cston · 2021-08-07T21:45:37Z

Fixes #45397.

First commit based on GrabYourPitchforks@1a22b6a.

Relates to parameterless struct constructors: dotnet/roslyn#51698 (test plan)

ghost · 2021-08-07T21:45:45Z

Tagging subscribers to this area: @cston
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #45397.

First commit based on GrabYourPitchforks@1a22b6a.

Author:	cston
Assignees:	-
Labels:	`area-System.Linq.Expressions`
Milestone:	-

… constructors

cston · 2021-08-10T00:19:33Z

src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Interpreter/LocalAccess.cs

@@ -424,7 +424,7 @@ public override int Run(InterpretedFrame frame)

                try
                {
-                    value = Activator.CreateInstance(_type);
+                    value = RuntimeHelpers.GetUninitializedObject(_type);


@GrabYourPitchforks, does RuntimeHelpers.GetUnitializedObject(_type) support Nullable<T> or should this method use Activator.CreateInstance(_type) or perhaps just null if _type.IsNullableType()?

GetUninitializedObject allows typeof(Nullable<T>) to be passed in. It extracts the T and returns a boxed default(T).

runtime/src/coreclr/vm/reflectioninvocation.cpp

Lines 2240 to 2242 in 1011edc

// If it is a nullable, return the underlying type instead.

if (pMT->IsNullable())

pMT = pMT->GetInstantiation()[0].GetMethodTable();

GetUninitializedObject allows typeof(Nullable<T>) to be passed in. It extracts the T and returns a boxed default(T).

Thanks. For MutableBox and MutableValue, it seems the value should be (T?)null rather than (T?)default(T).

eerhardt

LGTM. Thanks for fixing this.

Just a request to update the suppress message justifications.

eerhardt · 2021-08-10T15:55:52Z

...s/System.Linq.Expressions/src/System/Linq/Expressions/Interpreter/DefaultValueInstruction.cs

@@ -26,7 +27,7 @@ internal DefaultValueInstruction(Type type)
            Justification = "_type is a ValueType. You can always create an instance of a ValueType.")]


Can we update these justifications to match the new code? (here and below)

Suggested change

Justification = "_type is a ValueType. You can always create an instance of a ValueType.")]

Justification = "_type is a ValueType. You can always get an uninitialized ValueType.")]

cston · 2021-08-12T16:56:04Z

@dotnet/roslyn-compiler, please review.

jaredpar · 2021-08-12T21:58:07Z

src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Interpreter/TypeOperations.cs

            public override int Run(InterpretedFrame frame)
            {
                if (frame.Peek() == null)
                {
                    frame.Pop();
-                    frame.Push(Activator.CreateInstance(_defaultValueType));
+                    frame.Push(RuntimeHelpers.GetUninitializedObject(_defaultValueType));


There is no assertion in the ctor here that _defaultValueType is not nullable as there is in the ctor for DefaultValueInstruction. Would expect an assert in both places. #Resolved

jaredpar · 2021-08-12T21:58:36Z

src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Interpreter/LocalAccess.cs

+                    value = _type.IsNullableType() ?
+                        Activator.CreateInstance(_type) :
+                        RuntimeHelpers.GetUninitializedObject(_type);


This pattern is repeated, did you consider adding a helper?

Will leave as is to avoid moving suppression messages, etc.

eerhardt · 2021-08-12T23:21:58Z

src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Interpreter/LocalAccess.cs

@@ -439,6 +439,13 @@ public override int Run(InterpretedFrame frame)

            public override string InstructionName => "InitMutableBox";
        }
+
+        private static object? GetUninitializedObjectOrNull(Type type)


You are going to have to move the suppress messages now... #Resolved

RikkiGibson · 2021-08-13T00:05:51Z

...s/System.Linq.Expressions/src/System/Linq/Expressions/Interpreter/DefaultValueInstruction.cs

@@ -23,10 +24,10 @@ internal DefaultValueInstruction(Type type)
        public override string InstructionName => "DefaultValue";

        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2077:UnrecognizedReflectionPattern",


Do we still get a warning after this change if the attribute is removed?

You should, yes, because RuntimeHelpers.GetUninitializedObject is annotated as requiring Constructors on the Type:

runtime/src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs

Lines 145 to 152 in c0662e8

public static object GetUninitializedObject(

// This API doesn't call any constructors, but the type needs to be seen as constructed.

// A type is seen as constructed if a constructor is kept.

// This obviously won't cover a type with no constructor. Reference types with no

// constructor are an academic problem. Valuetypes with no constructors are a problem,

// but IL Linker currently treats them as always implicitly boxed.

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]

Type type)

RikkiGibson · 2021-08-13T00:09:06Z

src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Interpreter/LocalAccess.cs

            public override int Run(InterpretedFrame frame)
            {
                object? value;

                try
                {
-                    value = Activator.CreateInstance(_type);
+                    value = _type.IsNullableType() ?


It wasn't clear to me why we want to use Activator.CreateInstance when the type is a System.Nullable<T>. Does something different/unexpected happen if we use RuntimeHelpers.GetUninitializedObject(_type) here unconditionally? #Resolved

Does something different/unexpected happen if we use RuntimeHelpers.GetUninitializedObject(_type) here unconditionally?

Yes, GetUninitializedObject(_type) will return (ValueType?)default(ValueType) - that is, a boxed instance of ValueType - rather than null when _type == typeof(ValueType?).

RikkiGibson · 2021-08-13T00:19:39Z

src/libraries/System.Linq.Expressions/tests/Block/BlockTests.cs

+        {
+            // f = () =>
+            //     {
+            //         ValueTypeWithParameterlessConstructorThatThrows p;


I feel like I might not understand the semantics of this interpreter. Do we just implicitly initialize locals to default? Do you think it would improve clarity to throw in an = default here? #ByDesign

Yes, we implicitly initialize the value to default in MutableValue. I was not including = default to ensure we caught regressions where the behavior of MutableValue changes. Since we're using a value type that throws an exception in the parameterless constructor, that might also catch the same regressions though.

RikkiGibson · 2021-08-13T00:24:38Z

src/libraries/System.Linq.Expressions/tests/Default/DefaultTests.cs

+
+        [Theory]
+        [ClassData(typeof(CompilationTypes))]
+        public static void GetValueOrDefault(bool useInterpreter)


I didn't understand whether there was significance to this being static. #Resolved

It looks like we have a mix of static and instance tests. The only reason this one is static is it was originally added in a location where others were static. Removed the static here for consistency.

dotnet-issue-labeler bot added the area-System.Linq.Expressions label Aug 7, 2021

cston force-pushed the 45397 branch 2 times, most recently from e870d78 to c661b31 Compare August 9, 2021 22:13

cston added 2 commits August 9, 2021 16:57

Expression.Default and Expression.New with structs with parameterless…

75a44e8

… constructors

Use GetUninitializedObject() for GetValueOrDefault(), MutableValue()

02e2f89

cston force-pushed the 45397 branch from c661b31 to 02e2f89 Compare August 9, 2021 23:59

cston commented Aug 10, 2021

View reviewed changes

cston marked this pull request as ready for review August 10, 2021 00:19

cston requested review from GrabYourPitchforks and eerhardt August 10, 2021 00:20

eerhardt approved these changes Aug 10, 2021

View reviewed changes

cston added 3 commits August 11, 2021 13:27

Add Nullable<T> tests

0c77bac

Handle Nullable<T> in MutableValue and MutableBox

6dbe610

Update UnconditionalSuppressMessage.Justification message

a590219

cston requested a review from a team August 11, 2021 23:13

jaredpar approved these changes Aug 12, 2021

View reviewed changes

PR feedback

1d69f3b

eerhardt reviewed Aug 12, 2021

View reviewed changes

Remove added InitializeLocalInstruction helper

f6b1803

RikkiGibson approved these changes Aug 13, 2021

View reviewed changes

PR feedback

266fa28

cston merged commit 8ca505f into dotnet:main Aug 13, 2021

cston deleted the 45397 branch August 13, 2021 04:23

ghost locked as resolved and limited conversation to collaborators Sep 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expression.Default and Expression.New with structs with parameterless constructors #57034

Expression.Default and Expression.New with structs with parameterless constructors #57034

cston commented Aug 7, 2021 •

edited by jcouv

Loading

ghost commented Aug 7, 2021

cston Aug 10, 2021

GrabYourPitchforks Aug 11, 2021

cston Aug 11, 2021 •

edited

Loading

eerhardt left a comment

eerhardt Aug 10, 2021 •

edited by cston

Loading

cston commented Aug 12, 2021

jaredpar Aug 12, 2021 •

edited by cston

Loading

jaredpar Aug 12, 2021 •

edited by cston

Loading

cston Aug 12, 2021

eerhardt Aug 12, 2021 •

edited by cston

Loading

RikkiGibson Aug 13, 2021

eerhardt Aug 13, 2021

RikkiGibson Aug 13, 2021 •

edited by cston

Loading

cston Aug 13, 2021 •

edited

Loading

RikkiGibson Aug 13, 2021 •

edited by cston

Loading

cston Aug 13, 2021

RikkiGibson Aug 13, 2021 •

edited by cston

Loading

cston Aug 13, 2021

	// If it is a nullable, return the underlying type instead.
	if (pMT->IsNullable())
	pMT = pMT->GetInstantiation()[0].GetMethodTable();

		@@ -26,7 +27,7 @@ internal DefaultValueInstruction(Type type)
		Justification = "_type is a ValueType. You can always create an instance of a ValueType.")]

		@@ -23,10 +24,10 @@ internal DefaultValueInstruction(Type type)
		public override string InstructionName => "DefaultValue";

		[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2077:UnrecognizedReflectionPattern",

	public static object GetUninitializedObject(
	// This API doesn't call any constructors, but the type needs to be seen as constructed.
	// A type is seen as constructed if a constructor is kept.
	// This obviously won't cover a type with no constructor. Reference types with no
	// constructor are an academic problem. Valuetypes with no constructors are a problem,
	// but IL Linker currently treats them as always implicitly boxed.
	[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors \| DynamicallyAccessedMemberTypes.NonPublicConstructors)]
	Type type)

Expression.Default and Expression.New with structs with parameterless constructors #57034

Expression.Default and Expression.New with structs with parameterless constructors #57034

Conversation

cston commented Aug 7, 2021 • edited by jcouv Loading

ghost commented Aug 7, 2021

cston Aug 10, 2021

Choose a reason for hiding this comment

GrabYourPitchforks Aug 11, 2021

Choose a reason for hiding this comment

cston Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

eerhardt Aug 10, 2021 • edited by cston Loading

Choose a reason for hiding this comment

cston commented Aug 12, 2021

jaredpar Aug 12, 2021 • edited by cston Loading

Choose a reason for hiding this comment

jaredpar Aug 12, 2021 • edited by cston Loading

Choose a reason for hiding this comment

cston Aug 12, 2021

Choose a reason for hiding this comment

eerhardt Aug 12, 2021 • edited by cston Loading

Choose a reason for hiding this comment

RikkiGibson Aug 13, 2021

Choose a reason for hiding this comment

eerhardt Aug 13, 2021

Choose a reason for hiding this comment

RikkiGibson Aug 13, 2021 • edited by cston Loading

Choose a reason for hiding this comment

cston Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Aug 13, 2021 • edited by cston Loading

Choose a reason for hiding this comment

cston Aug 13, 2021

Choose a reason for hiding this comment

RikkiGibson Aug 13, 2021 • edited by cston Loading

Choose a reason for hiding this comment

cston Aug 13, 2021

Choose a reason for hiding this comment

cston commented Aug 7, 2021 •

edited by jcouv

Loading

cston Aug 11, 2021 •

edited

Loading

eerhardt Aug 10, 2021 •

edited by cston

Loading

jaredpar Aug 12, 2021 •

edited by cston

Loading

jaredpar Aug 12, 2021 •

edited by cston

Loading

eerhardt Aug 12, 2021 •

edited by cston

Loading

RikkiGibson Aug 13, 2021 •

edited by cston

Loading

cston Aug 13, 2021 •

edited

Loading

RikkiGibson Aug 13, 2021 •

edited by cston

Loading

RikkiGibson Aug 13, 2021 •

edited by cston

Loading