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

Resolve ILLink warnings in System.Linq.Expressions (Round 3) #47938

Merged
merged 4 commits into from
Feb 10, 2021
Merged

Resolve ILLink warnings in System.Linq.Expressions (Round 3) #47938

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
78623f6
Resolve ILLink warnings in System.Linq.Expressions (Round 3)
eerhardt Feb 3, 2021
a3d9bc8
Resolve a few more ILLink warnings in System.Linq.Expressions
eerhardt Feb 6, 2021
42ee7d3
Respond to PR feedback.
eerhardt Feb 9, 2021
2999861
PR feedback
eerhardt Feb 9, 2021
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
26 changes: 25 additions & 1 deletion src/libraries/Microsoft.CSharp/src/ILLink/ILLink.Suppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,5 +139,29 @@
<property name="Scope">member</property>
<property name="Target">M:Microsoft.CSharp.RuntimeBinder.SymbolTable.AddNamesInInheritanceHierarchy(System.String,System.Collections.Generic.List{System.Type})</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
Copy link
Member

Choose a reason for hiding this comment

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

How come these are necessary now? This is in Microsoft.CSharp which should not be affected by this PR, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

which should not be affected by this PR

This isn't true. Microsoft.CSharp references System.Linq.Expressions:

runtime/src/libraries/Microsoft.CSharp/src/Microsoft.CSharp.csproj

Line 245 in 118c0fd

<Reference Include="System.Linq.Expressions" />

Since I'm adding annotations to System.Linq.Expressions (specifically RequiresUnreferencedCode attributes), it is causing more warnings in the downstream assembly. For example, this is now unsafe:

runtime/src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/ComInterop/CurrencyArgBuilder.cs

Lines 24 to 27 in 1d9e50c

return Expression.Property(
Helpers.Convert(base.Marshal(parameter), typeof(CurrencyWrapper)),
nameof(CurrencyWrapper.WrappedObject)
);

Copy link
Member

Choose a reason for hiding this comment

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

Thanks - didn't realize that there are dependencies...

<argument>ILLink</argument>
<argument>IL2026</argument>
<property name="Scope">member</property>
<property name="Target">M:Microsoft.CSharp.RuntimeBinder.ComInterop.CurrencyArgBuilder.Marshal(System.Linq.Expressions.Expression)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2026</argument>
<property name="Scope">member</property>
<property name="Target">M:Microsoft.CSharp.RuntimeBinder.ComInterop.ErrorArgBuilder.Marshal(System.Linq.Expressions.Expression)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2026</argument>
<property name="Scope">member</property>
<property name="Target">M:Microsoft.CSharp.RuntimeBinder.ComInterop.VariantArray.GetStructField(System.Linq.Expressions.ParameterExpression,System.Int32)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2026</argument>
<property name="Scope">member</property>
<property name="Target">M:Microsoft.CSharp.RuntimeBinder.ExpressionTreeCallRewriter.GenerateConvert(Microsoft.CSharp.RuntimeBinder.Semantics.ExprCall)</property>
</attribute>
</assembly>
</linker>
</linker>
23 changes: 19 additions & 4 deletions src/libraries/System.Linq.Expressions/ref/System.Linq.Expressions.cs
View file
Open in desktop

Large diffs are not rendered by default.

68 changes: 1 addition & 67 deletions src/libraries/System.Linq.Expressions/src/ILLink/ILLink.Suppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,79 +5,13 @@
<argument>ILLink</argument>
<argument>IL2026</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Linq.Expressions.Compiler.AssemblyGen.DefineDelegateType(System.String)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2060</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Linq.Expressions.Expression.ApplyTypeArgs(System.Reflection.MethodInfo,System.Type[])</property>
<property name="Target">M:System.Linq.Expressions.Expression.Convert(System.Linq.Expressions.Expression,System.Type)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2070</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Dynamic.Utils.TypeExtensions.GetAnyStaticMethodValidated(System.Type,System.String,System.Type[])</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2072</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Linq.Expressions.Expression.ListInit(System.Linq.Expressions.NewExpression,System.Collections.Generic.IEnumerable{System.Linq.Expressions.Expression})</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2072</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Linq.Expressions.Expression.Call(System.Linq.Expressions.Expression,System.String,System.Type[],System.Linq.Expressions.Expression[]))</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2072</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Linq.Expressions.Expression.Property(System.Linq.Expressions.Expression,System.String,System.Linq.Expressions.Expression[])</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2075</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Dynamic.DynamicObject.MetaDynamic.IsOverridden(System.Reflection.MethodInfo)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2075</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Dynamic.Utils.TypeUtils.GetUserDefinedCoercionMethod(System.Type,System.Type)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2075</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Linq.Expressions.Expression.CheckMethod(System.Reflection.MethodInfo,System.Reflection.MethodInfo)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2075</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Linq.Expressions.Expression.Field(System.Linq.Expressions.Expression,System.String)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2075</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Linq.Expressions.Expression.GetProperty(System.Reflection.MethodInfo,System.String,System.Int32)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2075</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Linq.Expressions.Expression.Property(System.Linq.Expressions.Expression,System.String)</property>
</attribute>
<attribute fullname="System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute">
<argument>ILLink</argument>
<argument>IL2075</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Linq.Expressions.Expression.PropertyOrField(System.Linq.Expressions.Expression,System.String)</property>
</attribute>
</assembly>
</linker>
2 changes: 2 additions & 0 deletions src/libraries/System.Linq.Expressions/src/System/Dynamic/DynamicObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,8 @@ private DynamicMetaObject CallMethodNoResult<TBinder>(MethodInfo method, TBinder
/// implementation for the method provided then Dynamic falls back to the base class
/// behavior which lets the call site determine how the binder is performed.
/// </summary>
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2075:UnrecognizedReflectionPattern",
Justification = "This is looking if the method is overriden. An overriden method will never be trimmed if the virtual method exists.")]
eerhardt marked this conversation as resolved.
Show resolved Hide resolved
private bool IsOverridden(MethodInfo method)
{
MemberInfo[] methods = Value.GetType().GetMember(method.Name, MemberTypes.Method, BindingFlags.Public | BindingFlags.Instance);
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Linq.Expressions/src/System/Dynamic/Utils/TypeUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,7 @@ public static bool IsImplicitlyConvertibleTo(this Type source, Type destination)
|| IsImplicitBoxingConversion(source, destination)
|| IsImplicitNullableConversion(source, destination);

[RequiresUnreferencedCode(Expression.ExpressionRequiresUnreferencedCode)]
public static MethodInfo? GetUserDefinedCoercionMethod(Type convertFrom, Type convertToType)
{
Type nnExprType = GetNonNullableType(convertFrom);
Expand Down
27 changes: 22 additions & 5 deletions src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/BinaryExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Dynamic.Utils;
using System.Reflection;
using System.Runtime.CompilerServices;
Expand Down Expand Up @@ -404,21 +405,21 @@ internal Expression ReduceUserdefinedLifted()
new TrueReadOnlyCollection<Expression>(
Assign(left, Left),
Condition(
Property(left, "HasValue"),
GetHasValueProperty(left),
Condition(
Call(opTrueFalse, Call(left, "GetValueOrDefault", null)),
Call(opTrueFalse, CallGetValueOrDefault(left)),
left,
Block(
new TrueReadOnlyCollection<ParameterExpression>(right),
new TrueReadOnlyCollection<Expression>(
Assign(right, Right),
Condition(
Property(right, "HasValue"),
GetHasValueProperty(right),
Convert(
Call(
Method,
Call(left, "GetValueOrDefault", null),
Call(right, "GetValueOrDefault", null)
CallGetValueOrDefault(left),
CallGetValueOrDefault(right)
),
Type
),
Expand All @@ -432,6 +433,22 @@ internal Expression ReduceUserdefinedLifted()
)
);
}

[DynamicDependency("GetValueOrDefault", typeof(Nullable<>))]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "The method will be preserved by the DynamicDependency.")]
private static MethodCallExpression CallGetValueOrDefault(ParameterExpression nullable)
{
return Call(nullable, "GetValueOrDefault", null);
}

[DynamicDependency("HasValue", typeof(Nullable<>))]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "The property will be preserved by the DynamicDependency.")]
private static MemberExpression GetHasValueProperty(ParameterExpression nullable)
{
return Property(nullable, "HasValue");
}
}

// Optimized representation of simple logical expressions:
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Compiler/AssemblyGen.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ private TypeBuilder DefineType(string name, [DynamicallyAccessedMembers(Dynamica
return _myModule.DefineType(name, attr, parent);
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "MulticastDelegate has a ctor with RequiresUnreferencedCode, but the generated derived type doesn't reference this ctor, so this is trim compatible.")]
eerhardt marked this conversation as resolved.
Show resolved Hide resolved
internal static TypeBuilder DefineDelegateType(string name)
{
return Assembly.DefineType(
Expand Down
11 changes: 10 additions & 1 deletion ...System.Linq.Expressions/src/System/Linq/Expressions/Compiler/LambdaCompiler.Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Dynamic.Utils;
using System.Globalization;
using System.Reflection;
Expand Down Expand Up @@ -737,7 +738,7 @@ private bool TryEmitHashtableSwitch(SwitchExpression node, CompilationFlags flag
Expression.Equal(switchValue, Expression.Constant(null, typeof(string))),
Expression.Assign(switchIndex, Utils.Constant(nullCase)),
Expression.IfThenElse(
Expression.Call(dictInit, "TryGetValue", null, switchValue, switchIndex),
CallTryGetValue(dictInit, switchValue, switchIndex),
Utils.Empty,
Expression.Assign(switchIndex, Utils.Constant(-1))
)
Expand All @@ -750,6 +751,14 @@ private bool TryEmitHashtableSwitch(SwitchExpression node, CompilationFlags flag
return true;
}

[DynamicDependency("TryGetValue", typeof(Dictionary<,>))]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "The method will be preserved by the DynamicDependency.")]
private static MethodCallExpression CallTryGetValue(Expression dictInit, ParameterExpression switchValue, ParameterExpression switchIndex)
{
return Expression.Call(dictInit, "TryGetValue", null, switchValue, switchIndex);
}

#endregion

private void CheckRethrow()
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Compiler/LambdaCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ private MemberExpression CreateLazyInitializedField<T>(string name)
Debug.Assert(_method is DynamicMethod);
#endif
{
return Expression.Field(Expression.Constant(new StrongBox<T>()), "Value");
return Utils.GetStrongBoxValueField(Expression.Constant(new StrongBox<T>()));
}
#if FEATURE_COMPILE_TO_METHODBUILDER
else
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/DynamicExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ internal DynamicExpression(Type delegateType, CallSiteBinder binder)
/// Reduces the dynamic expression node to a simpler expression.
/// </summary>
/// <returns>The reduced expression.</returns>
[DynamicDependency("Target", typeof(CallSite<>))]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "The field will be preserved by the DynamicDependency")]
public override Expression Reduce()
{
var site = Expression.Constant(CallSite.Create(DelegateType, Binder));
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Expression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ namespace System.Linq.Expressions
/// </summary>
public abstract partial class Expression
{
internal const string ExpressionRequiresUnreferencedCode = "Creating Expressions requires unreferenced code because the members being referenced by the Expression may be trimmed.";

private static readonly CacheDict<Type, MethodInfo> s_lambdaDelegateCache = new CacheDict<Type, MethodInfo>(40);
private static volatile CacheDict<Type, Func<Expression, string?, bool, ReadOnlyCollection<ParameterExpression>, LambdaExpression>>? s_lambdaFactories;

Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/IndexExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ public static IndexExpression ArrayAccess(Expression array, IEnumerable<Expressi
/// <param name="propertyName">The name of the indexer.</param>
/// <param name="arguments">An array of <see cref="Expression"/> objects that are used to index the property.</param>
/// <returns>The created <see cref="IndexExpression"/>.</returns>
[RequiresUnreferencedCode(ExpressionRequiresUnreferencedCode)]
public static IndexExpression Property(Expression instance, string propertyName, params Expression[]? arguments)
{
ExpressionUtils.RequiresCanRead(instance, nameof(instance));
Expand Down
2 changes: 1 addition & 1 deletion ...braries/System.Linq.Expressions/src/System/Linq/Expressions/Interpreter/TypeOperations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,7 @@ protected internal override Expression VisitParameter(ParameterExpression node)
{
return node;
}
return Expression.Convert(Expression.Field(Expression.Constant(box), "Value"), node.Type);
return Expression.Convert(Utils.GetStrongBoxValueField(Expression.Constant(box)), node.Type);
}

private IStrongBox? GetBox(ParameterExpression variable)
Expand Down
5 changes: 5 additions & 0 deletions src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/ListInitExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Dynamic.Utils;
using System.Reflection;
using System.Runtime.CompilerServices;
Expand Down Expand Up @@ -103,6 +104,7 @@ public partial class Expression
/// <param name="newExpression">A <see cref="NewExpression"/> to set the <see cref="ListInitExpression.NewExpression"/> property equal to.</param>
/// <param name="initializers">An array of <see cref="Expression"/> objects to use to populate the <see cref="ListInitExpression.Initializers"/> collection.</param>
/// <returns>A <see cref="ListInitExpression"/> that has the <see cref="NodeType"/> property equal to <see cref="ExpressionType.ListInit"/> and the <see cref="ListInitExpression.NewExpression"/> property set to the specified value.</returns>
[RequiresUnreferencedCode(ExpressionRequiresUnreferencedCode)]
public static ListInitExpression ListInit(NewExpression newExpression, params Expression[] initializers)
{
return ListInit(newExpression, initializers as IEnumerable<Expression>);
Expand All @@ -114,6 +116,7 @@ public static ListInitExpression ListInit(NewExpression newExpression, params Ex
/// <param name="newExpression">A <see cref="NewExpression"/> to set the <see cref="ListInitExpression.NewExpression"/> property equal to.</param>
/// <param name="initializers">An <see cref="IEnumerable{T}"/> that contains <see cref="Expressions.ElementInit"/> objects to use to populate the <see cref="ListInitExpression.Initializers"/> collection.</param>
/// <returns>A <see cref="ListInitExpression"/> that has the <see cref="NodeType"/> property equal to <see cref="ExpressionType.ListInit"/> and the <see cref="ListInitExpression.NewExpression"/> property set to the specified value.</returns>
[RequiresUnreferencedCode(ExpressionRequiresUnreferencedCode)]
public static ListInitExpression ListInit(NewExpression newExpression, IEnumerable<Expression> initializers)
{
ContractUtils.RequiresNotNull(newExpression, nameof(newExpression));
Expand All @@ -136,6 +139,7 @@ public static ListInitExpression ListInit(NewExpression newExpression, IEnumerab
/// <param name="addMethod">A <see cref="MethodInfo"/> that represents an instance method named "Add" (case insensitive), that adds an element to a collection.</param>
/// <param name="initializers">An array of <see cref="Expression"/> objects to use to populate the <see cref="ListInitExpression.Initializers"/> collection.</param>
/// <returns>A <see cref="ListInitExpression"/> that has the <see cref="NodeType"/> property equal to <see cref="ExpressionType.ListInit"/> and the <see cref="ListInitExpression.NewExpression"/> property set to the specified value.</returns>
[RequiresUnreferencedCode(ExpressionRequiresUnreferencedCode)]
public static ListInitExpression ListInit(NewExpression newExpression, MethodInfo? addMethod, params Expression[] initializers)
{
return ListInit(newExpression, addMethod, initializers as IEnumerable<Expression>);
Expand All @@ -148,6 +152,7 @@ public static ListInitExpression ListInit(NewExpression newExpression, MethodInf
/// <param name="addMethod">A <see cref="MethodInfo"/> that represents an instance method named "Add" (case insensitive), that adds an element to a collection.</param>
/// <param name="initializers">An <see cref="IEnumerable{T}"/> that contains <see cref="Expression"/> objects to use to populate the Initializers collection.</param>
/// <returns>A <see cref="ListInitExpression"/> that has the <see cref="NodeType"/> property equal to <see cref="ExpressionType.ListInit"/> and the <see cref="ListInitExpression.NewExpression"/> property set to the specified value.</returns>
[RequiresUnreferencedCode(ExpressionRequiresUnreferencedCode)]
public static ListInitExpression ListInit(NewExpression newExpression, MethodInfo? addMethod, IEnumerable<Expression> initializers)
{
if (addMethod == null)
Expand Down
Loading