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 Microsoft.VisualBasic.Core #51725

Merged
8 commits merged into from
Apr 27, 2021
Merged

Resolve ILLink warnings in Microsoft.VisualBasic.Core #51725

8 commits merged into from
Apr 27, 2021

Conversation

joperezr
Copy link
Member

@joperezr joperezr commented Apr 23, 2021 •
edited
Loading

cc: @eerhardt @vitek-karas @agocke

Addressing all Linker warnings on Microsoft.VisualBasic.Core assembly.

Contributes to #45623

@joperezr joperezr added area-Microsoft.VisualBasic linkable-framework Issues associated with delivering a linker friendly framework labels Apr 23, 2021
@ghost
Copy link

ghost commented Apr 23, 2021

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

Issue Details

cc: @eerhardt @vitek-karas @agocke

Addressing all Linker warnings on Microsoft.VisualBasic.Core assembly.

Changes are ready for review, but the RequiresUnreferencedCode messages are not final yet, I need to do another pass thorugh all of them to make sure we actually communicate what is actionable, so don't really pay much attention to those as they will be all changed in a commit that I'll add here. I'm just sending the PR now as the logic of it is ready and I want to start getting feedback.

Contributes to #45623

Author: joperezr
Assignees: -
Labels:

area-Microsoft.VisualBasic, linkable-framework
Milestone: -

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

@joperezr joperezr closed this Apr 23, 2021
@joperezr joperezr reopened this Apr 23, 2021
@agocke
Copy link
Member

agocke commented Apr 23, 2021

@cston FYI

@joperezr joperezr requested review from eerhardt and cston April 23, 2021 21:29
eerhardt
eerhardt reviewed Apr 23, 2021
View reviewed changes
src/libraries/Microsoft.VisualBasic.Core/src/MatchingRefApiCompatBaseline.txt
@@ -1,28 +1,4 @@
Compat issues with assembly Microsoft.VisualBasic:

# Loaded via reflection so needs to be public in the implementation but we don't want to expose them publicly in the ref assembly:
Copy link
Member

Choose a reason for hiding this comment

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

Are these changes intentional? What is the reasoning?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sort of, yeah. Not really related to this PR in particular but when adding the annotations on the src I started getting new APICompat warnings on since they didn't match the ref (as I hadn't updated that yet at that point) so I regenerated this file a few times until finally I regenerated it one last time after adding the changes back to the ref assembly again. Long story short, these baselines had gone way out of date since we don't regenerate them regularly, so this is just the result of regenerating them today. Of course I could remove this change from the PR and regenerate them outside of this effort if preferred.

eerhardt
eerhardt reviewed Apr 23, 2021
View reviewed changes
src/libraries/Microsoft.VisualBasic.Core/src/MatchingRefApiCompatBaseline.txt
TypesMustExist : Type 'Microsoft.VisualBasic.CompilerServices.SiteDelegate6' does not exist in the reference but it does exist in the implementation.
TypesMustExist : Type 'Microsoft.VisualBasic.CompilerServices.SiteDelegate7' does not exist in the reference but it does exist in the implementation.

# C# doesn't permit setting the parameter name of the setter.
Copy link
Member

Choose a reason for hiding this comment

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

Does this comment still add value? If so, it should be kept.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really sure why this was here but the whole purpose of these files is for them to be auto generated, and we were supposed to regenerate them every now and then (which clearly we haven't done in a while) so I wouldn't personally revert this change as comment will be removed next regeneration anyway.

eerhardt
eerhardt reviewed Apr 23, 2021
View reviewed changes
src/libraries/Microsoft.VisualBasic.Core/src/Microsoft/VisualBasic/FileIO/SpecialDirectories.vb
@@ -155,8 +166,9 @@ Namespace Microsoft.VisualBasic.FileIO
Return FileSystem.NormalizePath(Directory)
End Function

<RequiresUnreferencedCode("Cannot statically analyze the passed in type.")>
Private Shared Function GetWindowsFormsDirectory(typeName As String, propertyName As String) As String
Copy link
Member

Choose a reason for hiding this comment

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

Could we change this to have a DynamicallyAccessedMembers attribute on the typeName As String? And then pass the fully-qualified type in above. That way, when the assembly is present, the linker will see the full type name, and know to preserve properties on the type.

Copy link
Member Author

Choose a reason for hiding this comment

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

I remember I tried that, but then wasn't really sure if it would work since I was still getting the warning due to of course System.Windows.Forms not being present so I left it as RUC for now. I'll change it to be just annotated and suppress the warning for the missing assembly instead.

cston
cston reviewed Apr 23, 2021
View reviewed changes
...braries/Microsoft.VisualBasic.Core/src/Microsoft/VisualBasic/CompilerServices/LateBinding.vb
ByVal method As MethodBase,
ByVal Parameters As ParameterInfo(),
ByVal args As Object(),
ByVal objType As Type,
Copy link
Member

Choose a reason for hiding this comment

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

ByVal objType As Type

Should this parameter be annotated with <DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)>?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really required as the actual type that is being reflected on is objIReflect when we pass it down to InvokeMemberOnIReflect so just annotating that seems to be enough.

eerhardt
eerhardt reviewed Apr 26, 2021
View reviewed changes
src/libraries/Microsoft.VisualBasic.Core/src/Microsoft/VisualBasic/CompilerServices/Symbols.vb
@@ -932,6 +940,9 @@ Namespace Microsoft.VisualBasic.CompilerServices
Return result
End Function

<UnconditionalSuppressMessage("ReflectionAnalysis", "IL2075:UnrecognizedReflectionPattern",
Justification:="_type is annotated with .All, so it's BaseType will be annotated as well and it is safe to call GetMember on the BaseType.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this message. Where is BaseType used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies as it is not straight forward to figure this out. The variable classConstraint is created by calling GetClassConstraint(_type), which if you see it's implementation, all it's doing is:

runtime/src/libraries/Microsoft.VisualBasic.Core/src/Microsoft/VisualBasic/CompilerServices/Symbols.vb

Lines 522 to 531 in ac82799

Friend Shared Function GetClassConstraint(ByVal genericParameter As Type) As Type
'Returns the class constraint for the type parameter, Nothing if it has
'no class constraint.
Debug.Assert(IsGenericParameter(genericParameter), "expected type parameter")
'Type parameters with no class constraint have System.Object as their base type.
Dim classConstraint As Type = genericParameter.BaseType
If IsRootObjectType(classConstraint) Then Return Nothing
Return classConstraint
End Function

which basically means it is calling BaseType on _type and then the linker is warning because we are then calling GetMember into that and we haven't annotated the basetype, but because of the fact that _type itself is annotated with DAM.All then it's basetype and members should be preserved too.

eerhardt
eerhardt approved these changes Apr 27, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @joperezr!

@ghost
Copy link

ghost commented Apr 27, 2021

Hello @joperezr!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 0ee5cbd into dotnet:main Apr 27, 2021
@joperezr joperezr deleted the MVB1 branch April 27, 2021 16:14
@karelz karelz modified the milestones: 5.0.x, 6.0.0 May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cston cston cston left review comments

@eerhardt eerhardt eerhardt approved these changes

Assignees
No one assigned
Labels
area-Microsoft.VisualBasic linkable-framework Issues associated with delivering a linker friendly framework new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@joperezr @agocke @eerhardt @cston @karelz