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

Add support for annotations in classes/interfaces/structs in the linker #1929

Merged
merged 32 commits into from
Apr 9, 2021

Conversation

tlakollo
Copy link
Contributor

@tlakollo tlakollo commented Mar 29, 2021

Adds support for DynamicallyAccessedMembers on types (class, struct, interface).

The desired behavior is that calling object.GetType() on a statically typed instance will look for the annotation on the static type (or its base class or interface). If it finds it there, it will apply the annotation to the static type and all of its derived types (which are marked). Then it will treat the return value of the object.GetType() as a Type annotated with the found annotation.

The implementation is somewhere in between the MarkStep and the ReflectionMethodBodyScanner so I put it into a new class DynamicallyAccessedMembersTypeHierarchy. It stores a cache of type -> annotation. This cache is built from MarkType for all types regardless if the reflection pattern to access it was seen or not. It also stores information if the annotation has already been applied (if so, newly marked types to which that annotation also applied will get it applied on them as well - this means there's no need for post-processing).

Added lot of tests to cover interesting cases.

LakshanF and others added 22 commits March 17, 2021 16:21
Use xml link attributes to test DynamicallyAccessedMembers in a type
Change ReflectionMethodBodyScanner to check for StaticType
Create function for checking DynamicallyAccessMembers on type, basetype
and interfaces
Mock tests and use xml to inject DynamicallyAccessMemberTypes
Fix Tests
Fix Custom Attribute cache
Add more functionality to Value Nodes
Intentionally avoided dependency on diagnostic values for functional code
Implement marking of type hierarchies
Fixes the build to actuall work.
Fixes the new tests to actually work - and improved some of them. I renamed/restructured the tests to make it easier to work with.

Functional change:
Fixes equality comparison for ValueNode - there was actually one case where the code didn't work correctly because of this (found by the tests).
Fixes build, tests and ValueNode equality
@vitek-karas
Copy link
Member

@tlakollo can you please take a look at the new tests I added - just so that it makes sense.


namespace Mono.Linker.Dataflow
{
class DynamicallyAccessedMembersTypeHierarchy
Copy link
Member

Choose a reason for hiding this comment

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

Naming: I would welcome any suggestion for a better name...

// because interfaces are marked late and in effectively random order.
// For this cache to be effective we need to be able to fill it for all base types and interfaces
// of a type which is currently being marked - at which point the interfaces are not yet marked.
readonly Dictionary<TypeDefinition, (DynamicallyAccessedMemberTypes annotation, bool applied)> _typesInDynamicallyAccessedMembersHierarchy;
Copy link
Member

Choose a reason for hiding this comment

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

This cache will include all interfaces reachable from marked types - which can be relatively large number (1000s). It is built regardless of the annotations being used or even present, since it acts as a negative cache as well.

Alternatively this could be implemented by re-computing the annotation for interface each type it's needed - basically trade memory or CPU. I went with the cache as some kind of cache would still be needed for the applied bit, and the memory impact doesn't seem that bad (couple of KB).

@@ -4,61 +4,792 @@

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nit: Add license

Copy link
Member

@MichalStrehovsky MichalStrehovsky 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 otherwise!

// One of the base/interface types is already marked as having the annotation applied
// so we need to apply the annotation to this type as well
var reflectionMethodBodyScanner = new ReflectionMethodBodyScanner (_context, _markStep);
var reflectionPatternContext = new ReflectionPatternContext (_context, true, type, type);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var reflectionPatternContext = new ReflectionPatternContext (_context, true, type, type);
using var reflectionPatternContext = new ReflectionPatternContext (_context, true, type, type);

Copy link
Member

Choose a reason for hiding this comment

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

It's actually not possible to use using since we're passing it as ref (and the compiler checks that the values in using is immutable).
But it's a valid concern that we're not calling Dispose even though it's just a debug check. I modified all call sites to call Dispose - without the try/finally as it doesn't seem necessary - But I can be convinced to add that as well.

}

[Kept]
class AllAnnotationsAreApplied
Copy link
Contributor

Choose a reason for hiding this comment

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

How well does the code handle complex or conflicting hierarchies? I couldn't find any test and it could be worth adding as well.

Examples like

[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
class X : A, I
{
}

[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.None)]
class A : I
{
	public virtual void M ();
}

[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.None)]
interface I
{
	void M ();
}

or

[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.None)]
class C : B
{
	public override  void Foo () {}
}

[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
class B : A
{
	public override  void Foo () {}
}

[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.None)]
class A
{
	public virtual void Foo () {}
}

Copy link
Member

Choose a reason for hiding this comment

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

Good suggestions - I added a bit more complicated case of both samples above and one more around diamond shapes with interfaces.

vitek-karas and others added 4 commits April 6, 2021 07:59
* Call Dispose on context struct to get debug checks
* Fix typos
* More tests
@vitek-karas vitek-karas merged commit 6944b7c into dotnet:main Apr 9, 2021
@vitek-karas vitek-karas deleted the TrimAttrSupportFlow branch April 9, 2021 20:28
agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
…er (dotnet/linker#1929)

Main goal:
Adding [DynamicallyAccessedMembers] on a type Base enables code like:
```C#
Base b;
b.GetType().GetMethods() ....
```
The object.GetType() call will treat the return value as annotated with the annotation from the Base type. Base and all derived types will then include all necessary members according to the annotation.

Same will work for interfaces (putting the annotation on an interface).

Changes:
- Data flow now tracks static type of all values
- New class which implements the annotation cache and marking for type hierarchies
- Integration from MarkStep and ReflectionMethodBodyScanner
- Lot new tests

Co-authored-by: Lakshan Fernando <[email protected]>
Co-authored-by: tlakollo <[email protected]>
Co-authored-by: vitek-karas <[email protected]>

Commit migrated from dotnet/linker@6944b7c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants