Resolve ILLink warnings in System.Linq.Expressions (Round 1)

[eerhardt]

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

[ghost]

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

Issue Details

---

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

Author:	eerhardt
Assignees:	-
Labels:	area-System.Linq.Expressions
Milestone:	-

[eerhardt]

Thanks for the feedback! I believe I've addressed it, please take another look.

[MichalStrehovsky]

This suppression will only be correct if we replace the Activator.CreateInstance with GetUninitializedObject. Are we sure we won't forget to revisit this if the resolution of the new bug is unexpected?

[eerhardt]

Are you saying it isn't correct because trimming will cause Activator.CreateInstance to fail? Or are you saying it is incorrect based on if value types get a parameterless constructor, and the trimmer removes it, causing a behavior change between trimmed and untrimmed apps?

Either way I've called this out in #47647.

[MichalStrehovsky]

The latter. The annotation on the field is writing checks no consumer can cash (because the suppression that makes is possible is incorrect).

I would keep the suppression on the Run method where it was before because once we swap this for GetUnitializedObject we can say something along the lines of "this is okay to suppress because trimming always considers valuetypes boxed" - which is what we need for the suppression on GetUninitializedObject to be safe.

[eerhardt]

In the spirit of this comment (and remembering I made the same suppression elsewhere), I tried changing the following code from Activator.CreateInstance to RuntimeHelpers.GetUninitializedObject:

runtime/src/libraries/Common/src/Extensions/ParameterDefaultValue/ParameterDefaultValue.cs

Lines 42 to 50 in d1c0fa8

	// Workaround for https://github.com/dotnet/runtime/issues/18599
	if(defaultValue==null&&parameter.ParameterType.IsValueType)
	{
	defaultValue=CreateValueType(parameter.ParameterType);
	}
	[UnconditionalSuppressMessage("ReflectionAnalysis","IL2067:UnrecognizedReflectionPattern",
	Justification="CreateInstance is only called on a ValueType, which will always have a default constructor.")]
	staticobject?CreateValueType(Typet)=>Activator.CreateInstance(t);

However, unit tests started failing. This is because RuntimeHelpers.GetUninitializedObject using a Nullable<T> type doesn't return null, like Activator.CreateInstance:

t=typeof(Nullable<int>);Console.WriteLine($"Activator Is Null? {Activator.CreateInstance(t)==null}");Console.WriteLine($"GetUninitializedObject Is Null? {RuntimeHelpers.GetUninitializedObject(t)==null}");

prints

Activator Is Null? True
GetUninitializedObject Is Null? False

This won't be a problem for this specific site, since you can't have a Nullable<Nullable<T>>. But for ParameterDefaultVale (and potentially other places), these two APIs aren't interchangeable.

[MichalStrehovsky]

Good catch. Something we need to keep in mind as we go fixing incorrect usages of CreateInstance.

[eerhardt]

Test failure is #47728.

[eerhardt]

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

[ghost]

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

Issue Details

---

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

Author:	eerhardt
Assignees:	-
Labels:	area-System.Linq.Expressions
Milestone:	-

[eerhardt]

Thanks for the feedback! I believe I've addressed it, please take another look.

[MichalStrehovsky]

This suppression will only be correct if we replace the Activator.CreateInstance with GetUninitializedObject. Are we sure we won't forget to revisit this if the resolution of the new bug is unexpected?

[eerhardt]

Are you saying it isn't correct because trimming will cause Activator.CreateInstance to fail? Or are you saying it is incorrect based on if value types get a parameterless constructor, and the trimmer removes it, causing a behavior change between trimmed and untrimmed apps?

Either way I've called this out in #47647.

[MichalStrehovsky]

The latter. The annotation on the field is writing checks no consumer can cash (because the suppression that makes is possible is incorrect).

I would keep the suppression on the Run method where it was before because once we swap this for GetUnitializedObject we can say something along the lines of "this is okay to suppress because trimming always considers valuetypes boxed" - which is what we need for the suppression on GetUninitializedObject to be safe.

[eerhardt]

In the spirit of this comment (and remembering I made the same suppression elsewhere), I tried changing the following code from Activator.CreateInstance to RuntimeHelpers.GetUninitializedObject:

runtime/src/libraries/Common/src/Extensions/ParameterDefaultValue/ParameterDefaultValue.cs

Lines 42 to 50 in d1c0fa8

	// Workaround for https://github.com/dotnet/runtime/issues/18599
	if(defaultValue==null&&parameter.ParameterType.IsValueType)
	{
	defaultValue=CreateValueType(parameter.ParameterType);
	}
	[UnconditionalSuppressMessage("ReflectionAnalysis","IL2067:UnrecognizedReflectionPattern",
	Justification="CreateInstance is only called on a ValueType, which will always have a default constructor.")]
	staticobject?CreateValueType(Typet)=>Activator.CreateInstance(t);

However, unit tests started failing. This is because RuntimeHelpers.GetUninitializedObject using a Nullable<T> type doesn't return null, like Activator.CreateInstance:

t=typeof(Nullable<int>);Console.WriteLine($"Activator Is Null? {Activator.CreateInstance(t)==null}");Console.WriteLine($"GetUninitializedObject Is Null? {RuntimeHelpers.GetUninitializedObject(t)==null}");

prints

Activator Is Null? True
GetUninitializedObject Is Null? False

This won't be a problem for this specific site, since you can't have a Nullable<Nullable<T>>. But for ParameterDefaultVale (and potentially other places), these two APIs aren't interchangeable.

[MichalStrehovsky]

Good catch. Something we need to keep in mind as we go fixing incorrect usages of CreateInstance.

[eerhardt]

Test failure is #47728.

[eerhardt]

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

[ghost]

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

Issue Details

---

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

Author:	eerhardt
Assignees:	-
Labels:	area-System.Linq.Expressions
Milestone:	-

[eerhardt]

Thanks for the feedback! I believe I've addressed it, please take another look.

[MichalStrehovsky]

This suppression will only be correct if we replace the Activator.CreateInstance with GetUninitializedObject. Are we sure we won't forget to revisit this if the resolution of the new bug is unexpected?

[eerhardt]

Are you saying it isn't correct because trimming will cause Activator.CreateInstance to fail? Or are you saying it is incorrect based on if value types get a parameterless constructor, and the trimmer removes it, causing a behavior change between trimmed and untrimmed apps?

Either way I've called this out in #47647.

[MichalStrehovsky]

The latter. The annotation on the field is writing checks no consumer can cash (because the suppression that makes is possible is incorrect).

I would keep the suppression on the Run method where it was before because once we swap this for GetUnitializedObject we can say something along the lines of "this is okay to suppress because trimming always considers valuetypes boxed" - which is what we need for the suppression on GetUninitializedObject to be safe.

[eerhardt]

In the spirit of this comment (and remembering I made the same suppression elsewhere), I tried changing the following code from Activator.CreateInstance to RuntimeHelpers.GetUninitializedObject:

runtime/src/libraries/Common/src/Extensions/ParameterDefaultValue/ParameterDefaultValue.cs

Lines 42 to 50 in d1c0fa8

	// Workaround for https://github.com/dotnet/runtime/issues/18599
	if(defaultValue==null&&parameter.ParameterType.IsValueType)
	{
	defaultValue=CreateValueType(parameter.ParameterType);
	}
	[UnconditionalSuppressMessage("ReflectionAnalysis","IL2067:UnrecognizedReflectionPattern",
	Justification="CreateInstance is only called on a ValueType, which will always have a default constructor.")]
	staticobject?CreateValueType(Typet)=>Activator.CreateInstance(t);

However, unit tests started failing. This is because RuntimeHelpers.GetUninitializedObject using a Nullable<T> type doesn't return null, like Activator.CreateInstance:

t=typeof(Nullable<int>);Console.WriteLine($"Activator Is Null? {Activator.CreateInstance(t)==null}");Console.WriteLine($"GetUninitializedObject Is Null? {RuntimeHelpers.GetUninitializedObject(t)==null}");

prints

Activator Is Null? True
GetUninitializedObject Is Null? False

This won't be a problem for this specific site, since you can't have a Nullable<Nullable<T>>. But for ParameterDefaultVale (and potentially other places), these two APIs aren't interchangeable.

[MichalStrehovsky]

Good catch. Something we need to keep in mind as we go fixing incorrect usages of CreateInstance.

[eerhardt]

Test failure is #47728.

[eerhardt]

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

[ghost]

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

Issue Details

---

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

Author:	eerhardt
Assignees:	-
Labels:	area-System.Linq.Expressions
Milestone:	-

[eerhardt]

Thanks for the feedback! I believe I've addressed it, please take another look.

[MichalStrehovsky]

This suppression will only be correct if we replace the Activator.CreateInstance with GetUninitializedObject. Are we sure we won't forget to revisit this if the resolution of the new bug is unexpected?

[eerhardt]

Are you saying it isn't correct because trimming will cause Activator.CreateInstance to fail? Or are you saying it is incorrect based on if value types get a parameterless constructor, and the trimmer removes it, causing a behavior change between trimmed and untrimmed apps?

Either way I've called this out in #47647.

[MichalStrehovsky]

The latter. The annotation on the field is writing checks no consumer can cash (because the suppression that makes is possible is incorrect).

I would keep the suppression on the Run method where it was before because once we swap this for GetUnitializedObject we can say something along the lines of "this is okay to suppress because trimming always considers valuetypes boxed" - which is what we need for the suppression on GetUninitializedObject to be safe.

[eerhardt]

In the spirit of this comment (and remembering I made the same suppression elsewhere), I tried changing the following code from Activator.CreateInstance to RuntimeHelpers.GetUninitializedObject:

runtime/src/libraries/Common/src/Extensions/ParameterDefaultValue/ParameterDefaultValue.cs

Lines 42 to 50 in d1c0fa8

	// Workaround for https://github.com/dotnet/runtime/issues/18599
	if(defaultValue==null&&parameter.ParameterType.IsValueType)
	{
	defaultValue=CreateValueType(parameter.ParameterType);
	}
	[UnconditionalSuppressMessage("ReflectionAnalysis","IL2067:UnrecognizedReflectionPattern",
	Justification="CreateInstance is only called on a ValueType, which will always have a default constructor.")]
	staticobject?CreateValueType(Typet)=>Activator.CreateInstance(t);

However, unit tests started failing. This is because RuntimeHelpers.GetUninitializedObject using a Nullable<T> type doesn't return null, like Activator.CreateInstance:

t=typeof(Nullable<int>);Console.WriteLine($"Activator Is Null? {Activator.CreateInstance(t)==null}");Console.WriteLine($"GetUninitializedObject Is Null? {RuntimeHelpers.GetUninitializedObject(t)==null}");

prints

Activator Is Null? True
GetUninitializedObject Is Null? False

This won't be a problem for this specific site, since you can't have a Nullable<Nullable<T>>. But for ParameterDefaultVale (and potentially other places), these two APIs aren't interchangeable.

[MichalStrehovsky]

Good catch. Something we need to keep in mind as we go fixing incorrect usages of CreateInstance.

[eerhardt]

Test failure is #47728.

[eerhardt]

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

[ghost]

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

Issue Details

---

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

Author:	eerhardt
Assignees:	-
Labels:	area-System.Linq.Expressions
Milestone:	-

[eerhardt]

Thanks for the feedback! I believe I've addressed it, please take another look.

[MichalStrehovsky]

This suppression will only be correct if we replace the Activator.CreateInstance with GetUninitializedObject. Are we sure we won't forget to revisit this if the resolution of the new bug is unexpected?

[eerhardt]

Are you saying it isn't correct because trimming will cause Activator.CreateInstance to fail? Or are you saying it is incorrect based on if value types get a parameterless constructor, and the trimmer removes it, causing a behavior change between trimmed and untrimmed apps?

Either way I've called this out in #47647.

[MichalStrehovsky]

The latter. The annotation on the field is writing checks no consumer can cash (because the suppression that makes is possible is incorrect).

I would keep the suppression on the Run method where it was before because once we swap this for GetUnitializedObject we can say something along the lines of "this is okay to suppress because trimming always considers valuetypes boxed" - which is what we need for the suppression on GetUninitializedObject to be safe.

[eerhardt]

In the spirit of this comment (and remembering I made the same suppression elsewhere), I tried changing the following code from Activator.CreateInstance to RuntimeHelpers.GetUninitializedObject:

runtime/src/libraries/Common/src/Extensions/ParameterDefaultValue/ParameterDefaultValue.cs

Lines 42 to 50 in d1c0fa8

	// Workaround for https://github.com/dotnet/runtime/issues/18599
	if(defaultValue==null&&parameter.ParameterType.IsValueType)
	{
	defaultValue=CreateValueType(parameter.ParameterType);
	}
	[UnconditionalSuppressMessage("ReflectionAnalysis","IL2067:UnrecognizedReflectionPattern",
	Justification="CreateInstance is only called on a ValueType, which will always have a default constructor.")]
	staticobject?CreateValueType(Typet)=>Activator.CreateInstance(t);

However, unit tests started failing. This is because RuntimeHelpers.GetUninitializedObject using a Nullable<T> type doesn't return null, like Activator.CreateInstance:

t=typeof(Nullable<int>);Console.WriteLine($"Activator Is Null? {Activator.CreateInstance(t)==null}");Console.WriteLine($"GetUninitializedObject Is Null? {RuntimeHelpers.GetUninitializedObject(t)==null}");

prints

Activator Is Null? True
GetUninitializedObject Is Null? False

This won't be a problem for this specific site, since you can't have a Nullable<Nullable<T>>. But for ParameterDefaultVale (and potentially other places), these two APIs aren't interchangeable.

[MichalStrehovsky]

Good catch. Something we need to keep in mind as we go fixing incorrect usages of CreateInstance.

[eerhardt]

Test failure is #47728.

[eerhardt]

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

[ghost]

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

Issue Details

---

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

Author:	eerhardt
Assignees:	-
Labels:	area-System.Linq.Expressions
Milestone:	-

[eerhardt]

Thanks for the feedback! I believe I've addressed it, please take another look.

[MichalStrehovsky]

This suppression will only be correct if we replace the Activator.CreateInstance with GetUninitializedObject. Are we sure we won't forget to revisit this if the resolution of the new bug is unexpected?

[eerhardt]

Are you saying it isn't correct because trimming will cause Activator.CreateInstance to fail? Or are you saying it is incorrect based on if value types get a parameterless constructor, and the trimmer removes it, causing a behavior change between trimmed and untrimmed apps?

Either way I've called this out in #47647.

[MichalStrehovsky]

The latter. The annotation on the field is writing checks no consumer can cash (because the suppression that makes is possible is incorrect).

I would keep the suppression on the Run method where it was before because once we swap this for GetUnitializedObject we can say something along the lines of "this is okay to suppress because trimming always considers valuetypes boxed" - which is what we need for the suppression on GetUninitializedObject to be safe.

[eerhardt]

In the spirit of this comment (and remembering I made the same suppression elsewhere), I tried changing the following code from Activator.CreateInstance to RuntimeHelpers.GetUninitializedObject:

runtime/src/libraries/Common/src/Extensions/ParameterDefaultValue/ParameterDefaultValue.cs

Lines 42 to 50 in d1c0fa8

	// Workaround for https://github.com/dotnet/runtime/issues/18599
	if(defaultValue==null&&parameter.ParameterType.IsValueType)
	{
	defaultValue=CreateValueType(parameter.ParameterType);
	}
	[UnconditionalSuppressMessage("ReflectionAnalysis","IL2067:UnrecognizedReflectionPattern",
	Justification="CreateInstance is only called on a ValueType, which will always have a default constructor.")]
	staticobject?CreateValueType(Typet)=>Activator.CreateInstance(t);

However, unit tests started failing. This is because RuntimeHelpers.GetUninitializedObject using a Nullable<T> type doesn't return null, like Activator.CreateInstance:

t=typeof(Nullable<int>);Console.WriteLine($"Activator Is Null? {Activator.CreateInstance(t)==null}");Console.WriteLine($"GetUninitializedObject Is Null? {RuntimeHelpers.GetUninitializedObject(t)==null}");

prints

Activator Is Null? True
GetUninitializedObject Is Null? False

This won't be a problem for this specific site, since you can't have a Nullable<Nullable<T>>. But for ParameterDefaultVale (and potentially other places), these two APIs aren't interchangeable.

[MichalStrehovsky]

Good catch. Something we need to keep in mind as we go fixing incorrect usages of CreateInstance.

[eerhardt]

Test failure is #47728.

[eerhardt]

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

[ghost]

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

Issue Details

---

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

Author:	eerhardt
Assignees:	-
Labels:	area-System.Linq.Expressions
Milestone:	-

[eerhardt]

Thanks for the feedback! I believe I've addressed it, please take another look.

[MichalStrehovsky]

This suppression will only be correct if we replace the Activator.CreateInstance with GetUninitializedObject. Are we sure we won't forget to revisit this if the resolution of the new bug is unexpected?

[eerhardt]

Are you saying it isn't correct because trimming will cause Activator.CreateInstance to fail? Or are you saying it is incorrect based on if value types get a parameterless constructor, and the trimmer removes it, causing a behavior change between trimmed and untrimmed apps?

Either way I've called this out in #47647.

[MichalStrehovsky]

The latter. The annotation on the field is writing checks no consumer can cash (because the suppression that makes is possible is incorrect).

I would keep the suppression on the Run method where it was before because once we swap this for GetUnitializedObject we can say something along the lines of "this is okay to suppress because trimming always considers valuetypes boxed" - which is what we need for the suppression on GetUninitializedObject to be safe.

[eerhardt]

In the spirit of this comment (and remembering I made the same suppression elsewhere), I tried changing the following code from Activator.CreateInstance to RuntimeHelpers.GetUninitializedObject:

runtime/src/libraries/Common/src/Extensions/ParameterDefaultValue/ParameterDefaultValue.cs

Lines 42 to 50 in d1c0fa8

	// Workaround for https://github.com/dotnet/runtime/issues/18599
	if(defaultValue==null&&parameter.ParameterType.IsValueType)
	{
	defaultValue=CreateValueType(parameter.ParameterType);
	}
	[UnconditionalSuppressMessage("ReflectionAnalysis","IL2067:UnrecognizedReflectionPattern",
	Justification="CreateInstance is only called on a ValueType, which will always have a default constructor.")]
	staticobject?CreateValueType(Typet)=>Activator.CreateInstance(t);

However, unit tests started failing. This is because RuntimeHelpers.GetUninitializedObject using a Nullable<T> type doesn't return null, like Activator.CreateInstance:

t=typeof(Nullable<int>);Console.WriteLine($"Activator Is Null? {Activator.CreateInstance(t)==null}");Console.WriteLine($"GetUninitializedObject Is Null? {RuntimeHelpers.GetUninitializedObject(t)==null}");

prints

Activator Is Null? True
GetUninitializedObject Is Null? False

This won't be a problem for this specific site, since you can't have a Nullable<Nullable<T>>. But for ParameterDefaultVale (and potentially other places), these two APIs aren't interchangeable.

[MichalStrehovsky]

Good catch. Something we need to keep in mind as we go fixing incorrect usages of CreateInstance.

[eerhardt]

Test failure is #47728.

[eerhardt]

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

[ghost]

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

Issue Details

---

Contributes to #45623

This is just round 1. At least one more round will be made where we start annotating APIs as RequiresUnreferencedCode.

Author:	eerhardt
Assignees:	-
Labels:	area-System.Linq.Expressions
Milestone:	-

[eerhardt]

Thanks for the feedback! I believe I've addressed it, please take another look.

[MichalStrehovsky]

This suppression will only be correct if we replace the Activator.CreateInstance with GetUninitializedObject. Are we sure we won't forget to revisit this if the resolution of the new bug is unexpected?

[eerhardt]

Are you saying it isn't correct because trimming will cause Activator.CreateInstance to fail? Or are you saying it is incorrect based on if value types get a parameterless constructor, and the trimmer removes it, causing a behavior change between trimmed and untrimmed apps?

Either way I've called this out in #47647.

[MichalStrehovsky]

The latter. The annotation on the field is writing checks no consumer can cash (because the suppression that makes is possible is incorrect).

I would keep the suppression on the Run method where it was before because once we swap this for GetUnitializedObject we can say something along the lines of "this is okay to suppress because trimming always considers valuetypes boxed" - which is what we need for the suppression on GetUninitializedObject to be safe.

[eerhardt]

In the spirit of this comment (and remembering I made the same suppression elsewhere), I tried changing the following code from Activator.CreateInstance to RuntimeHelpers.GetUninitializedObject:

runtime/src/libraries/Common/src/Extensions/ParameterDefaultValue/ParameterDefaultValue.cs

Lines 42 to 50 in d1c0fa8

	// Workaround for https://github.com/dotnet/runtime/issues/18599
	if(defaultValue==null&&parameter.ParameterType.IsValueType)
	{
	defaultValue=CreateValueType(parameter.ParameterType);
	}
	[UnconditionalSuppressMessage("ReflectionAnalysis","IL2067:UnrecognizedReflectionPattern",
	Justification="CreateInstance is only called on a ValueType, which will always have a default constructor.")]
	staticobject?CreateValueType(Typet)=>Activator.CreateInstance(t);

However, unit tests started failing. This is because RuntimeHelpers.GetUninitializedObject using a Nullable<T> type doesn't return null, like Activator.CreateInstance:

t=typeof(Nullable<int>);Console.WriteLine($"Activator Is Null? {Activator.CreateInstance(t)==null}");Console.WriteLine($"GetUninitializedObject Is Null? {RuntimeHelpers.GetUninitializedObject(t)==null}");

prints

Activator Is Null? True
GetUninitializedObject Is Null? False

This won't be a problem for this specific site, since you can't have a Nullable<Nullable<T>>. But for ParameterDefaultVale (and potentially other places), these two APIs aren't interchangeable.

[MichalStrehovsky]

Good catch. Something we need to keep in mind as we go fixing incorrect usages of CreateInstance.

[eerhardt]

Test failure is #47728.