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.

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

PersistedAssemblyBuilder missing methods on generated types #109320

Merged
merged 10 commits into from
Dec 5, 2024

Conversation

snakex64
Copy link
Contributor

@snakex64 snakex64 commented Oct 28, 2024

#108733

Add missing methods to PersistedAssemblyBuilder generated types:

  • GetProperties
  • GetProperty
  • GetInterface

Edit:

  • GetNestedType
  • GetNestedTypes
  • GetMember
  • GetMembers

Also provide bugfixes to GetMethods, GetFields, GetProperties, GetInterfaces not returning parent members

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

1 similar comment
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, 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.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 28, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

@snakex64
Copy link
Contributor Author

@dotnet-policy-service agree

}

return candidates;
}

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicNestedTypes | DynamicallyAccessedMemberTypes.NonPublicNestedTypes)]
public override Type[] GetNestedTypes(BindingFlags bindingAttr) => throw new NotSupportedException();
Copy link
Contributor

Choose a reason for hiding this comment

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

is nested type data available somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like it is available by going through the module. Should be easy. Will try to add that one too tomorrow. I'm not sure of all the different options of mapping between BindingFlags and the attribute enum used to create types so I will create a base version and we can augment it after

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, should be good now if you want to have a look

@snakex64
Copy link
Contributor Author

snakex64 commented Oct 30, 2024

Added GetMember and GetMembers too. It made me realize that there was some bugs related to parent members not being returned by GetMethods, GetFields, etc... By default unless BindingFlags.DeclaredOnly is specified, the members of the parent class should also be returned. I added a potential bugfix to all of them

@steveharter steveharter self-assigned this Dec 5, 2024
Copy link
Member

@steveharter steveharter 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 for the contribution.

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods)]
public override MethodInfo[] GetMethods(BindingFlags bindingAttr)
{
ThrowIfNotCreated();

List<MethodInfo> methods = new();
List<MethodInfo> methods = new List<MethodInfo>();
Copy link
Member

Choose a reason for hiding this comment

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

nit: unnecessary change here; using new() is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Reflection.Emit community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants