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

Respect the new DynamicDependencyAttribute #1148

Closed
eerhardt opened this issue Apr 27, 2020 · 5 comments
Closed

Respect the new DynamicDependencyAttribute #1148

eerhardt opened this issue Apr 27, 2020 · 5 comments
Assignees
Labels
Milestone

Comments

@eerhardt
Copy link
Member

The official public API for PreserveDependencyAttribute was renamed to DynamicDependencyAttribute. Also a MemberTypes property was added to DynamicDependencyAttribute.

We should respect this new attribute in the linker.

See dotnet/runtime#30902 for discussion.

@marek-safar marek-safar added this to the .NET5.0 milestone Apr 27, 2020
@marek-safar
Copy link
Contributor

I think for illink we could drop support for the internal PreserveDependencyAttribute and support only the new public attribute. It'd be also better to move the data extraction logic from MarkStep to PreserveDependencyLookupStep to allow printing info about wrong references consistently.

@sbomer could you work on this (/cc @vitek-karas) ?

@sbomer sbomer self-assigned this Apr 27, 2020
@sbomer
Copy link
Member

sbomer commented Apr 29, 2020

Part of this will involve defining the wildcard behavior. Currently:

  • ResolveFromXmlStep allows "*" anywhere in a type pattern, which will match any sequence of characters in a type's full name.
    • It doesn't support wildcards on member signatures - but it does support keeping all methods with the same name.
  • PreserveDependencyAttribute allows a signature equal to "*", which will cause the corresponding type to be kept in its entirety.
    • It doesn't support wildcards that match parts of a signature, or wildcards on the type part.

For consistency, I think we should make both support the same syntax. The design review for the attribute gives this example as a reason why we need wildcards for the attribute - in this case, on the type.

Does the following behavior sound ok? See the updated suggestion at the end.

  • Support "*" anywhere in a type pattern or member signature, for both ResolveFromXmlStep, and in DynamicDependencyAttribute when the type/member are given as strings.

This will allow selecting multiple types, and also multiple members of those types at the same time.

edit: Actually, since we're adding overloads to the attribute that take DynamicallyAccessedMemberTypes, that seems unnecessary. How does this sound?

  • Support "*" anywhere in a type pattern, but not member signatures. Remove support for wildcard in signatures from DynamicDependencyAttribute.

@vitek-karas
Copy link
Member

My understanding of the API review result was that we don't want to support wildcards and that we're replacing that functionality with the .ctor overloads which take DynamicallyAccessedMemberTypes - for example.

This probably means that it would be nice to refactor bits of the existing support in the data flow code such that we can use one place which understands the DynamicallyAccessedMemberTypes enum and knows how to apply it to types.

@marek-safar
Copy link
Contributor

I agree with Vitek that the wildcards support was not agreed/spec-ed enough to be implemented.

@vitek-karas
Copy link
Member

This should be fixed by #1215

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants