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

merged 4 commits into from
Feb 10, 2021

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Feb 5, 2021

Contributes to #45623

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Feb 5, 2021

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

Issue Details

Contributes to #45623

Author: eerhardt
Assignees: -
Labels:

area-System.Linq.Expressions, new-api-needs-documentation
Milestone: -

@eerhardt eerhardt added linkable-framework Issues associated with delivering a linker friendly framework and removed new-api-needs-documentation labels Feb 6, 2021
@ghost
Copy link

ghost commented Feb 6, 2021

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #45623

Author: eerhardt
Assignees: -
Labels:

area-System.Linq.Expressions, linkable-framework
Milestone: -

vitek-karas
vitek-karas reviewed Feb 6, 2021
View reviewed changes
src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/MemberExpression.cs Outdated Show resolved Hide resolved
src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/MemberExpression.cs
@@ -150,6 +150,7 @@ public static MemberExpression Field(Expression? expression, FieldInfo field)
/// <param name="expression">The containing object of the field. This can be null for static fields.</param>
/// <param name="fieldName">The field to be accessed.</param>
/// <returns>The created <see cref="MemberExpression"/>.</returns>
[RequiresUnreferencedCode(ExpressionRequiresUnreferencedCode)]
Copy link
Member

Choose a reason for hiding this comment

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

I haven't realizes that linker has intrinsic handling for some of the Expression. static creation methods. For example field (and property) are here: https://github.com/mono/linker/blob/319ed02ed9a25ec3800f723ff72a1eb92ef050c7/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs#L722

I don't think there's anything to be done here - linker will not report the warning if the intrinsic handling kicks in - we should leave the method annotated like this for sure.

src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/MemberExpression.cs Outdated Show resolved Hide resolved
This was referenced Feb 9, 2021
Closed
Closed
vitek-karas
vitek-karas reviewed Feb 9, 2021
View reviewed changes
src/libraries/Microsoft.CSharp/src/ILLink/ILLink.Suppressions.xml
@@ -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...

src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/MethodCallExpression.cs
@@ -1162,6 +1163,7 @@ public static MethodCallExpression Call(Expression instance, string methodName,
/// <exception cref="ArgumentNullException">
/// <paramref name="type"/> or <paramref name="methodName"/> is null.</exception>
/// <exception cref="InvalidOperationException">No method whose name is <paramref name="methodName"/>, whose type parameters match <paramref name="typeArguments"/>, and whose parameter types match <paramref name="arguments"/> is found in <paramref name="type"/> or its base types.-or-More than one method whose name is <paramref name="methodName"/>, whose type parameters match <paramref name="typeArguments"/>, and whose parameter types match <paramref name="arguments"/> is found in <paramref name="type"/> or its base types.</exception>
[RequiresUnreferencedCode(GenericMethodRequiresUnreferencedCode)]
public static MethodCallExpression Call(
Copy link
Member

Choose a reason for hiding this comment

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

There's an intrinsic for Call in linker, so the annotations are not actually used by the linker (but they should remain for sure). Unfortunately there's a bug which will make all the generic method cases not warn: dotnet/linker#1819

Needs to be fixed,

@runfoapp runfoapp bot mentioned this pull request Feb 9, 2021
Closed
vitek-karas
vitek-karas approved these changes Feb 9, 2021
View reviewed changes
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

Looks good to me - not sure if there are some tests we should add, at least a little bit.

@runfoapp runfoapp bot mentioned this pull request Feb 9, 2021
Closed
@eerhardt
Copy link
Member Author

eerhardt commented Feb 9, 2021

allConfigurations failure is #48075.

@eerhardt eerhardt merged commit 451d67f into dotnet:master Feb 10, 2021
@eerhardt eerhardt deleted the LinqExpressionsILLinkWarnings3 branch February 10, 2021 00:19
@ghost ghost locked as resolved and limited conversation to collaborators Mar 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MichalStrehovsky MichalStrehovsky MichalStrehovsky left review comments

@vitek-karas vitek-karas vitek-karas approved these changes

@stephentoub stephentoub Awaiting requested review from stephentoub

@joperezr joperezr Awaiting requested review from joperezr

Assignees
No one assigned
Labels
area-System.Linq.Expressions linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eerhardt @vitek-karas @MichalStrehovsky