Support DynamicDependencyAttribute by sbomer · Pull Request #1215 · dotnet/linker

sbomer · 2020-05-26T19:29:33Z

This adds support for DynamicDependencyAttribute, which uses the doc comment signature parser from #1197.

The behavior of the lookup step is pretty much kept the same - it will resolve assemblies from attributes, without recursively processing attributes from those assemblies. I originally started to do the recursive processing, but we agreed not to change it here since it is related to the broader work to enable adding new assemblies to the context in MarkStep, and there were concerns about the performance of loading extra unnecessary assemblies into the context.

marek-safar · 2020-05-28T09:01:01Z

+			}
+		}
+
+		public static IEnumerable<MethodDefinition> GetConstructorsOnType (TypeDefinition type, Func<MethodDefinition, bool> filter, BindingFlags? bindingFlags = null)


These are local helpers, right? Why are they public?

They are also used from ReflectionMethodBodyScanner - so maybe "internal" would be better.

DynamicallyAccessedMembersBinder is already internal because our copy of DynamicallyAccessedMemberTypes is internal, so I think they are effectively internal already. Generally I prefer to use class-based accessibility modifiers for consistency with the rest of the codebase, especially since we have the ref assembly to control externally-visible public surface.

marek-safar · 2020-05-28T09:10:00Z

+#endif
+			}
+
+			var dynamicDependencies = Context.Annotations.GetLinkerAttributes<DynamicDependency> (member);


I don't think you should use GetLinkerAttributes here, you already iterate all attributes above so this is doing the same loop and also building cache for member which might not be reached at all.

I originally optimized this a little more, but removed it based on this discussion: #1215 (comment). If you think it is worth it, I could add back the check for this callsite, and only call GetLinkerAttributes if the previous loop found any DynamicDependencyAttributes.

marek-safar · 2020-05-28T09:32:22Z

+				type = assembly.FindTypeByDocumentationSignature (typeName);
+				if (type == null) {
+					_context.LogMessage (MessageContainer.CreateWarningMessage ($"Unresolved type '{typeName}' in DynamicDependencyAttribute on '{context}'", 2032));
+					return;
+				}
+			} else if (dynamicDependency.Type is TypeReference typeReference) {
+				type = typeReference.Resolve ();
+				if (type == null) {
+					_context.LogMessage (MessageContainer.CreateWarningMessage ($"Unresolved type '{typeReference}' in DynamicDependencyAtribute on '{context}'", 2032));
+					return;
+				}
+			} else {
+				type = context.DeclaringType.Resolve ();
+				if (type == null) {
+					_context.LogMessage (MessageContainer.CreateWarningMessage ($"Unresolved type '{context.DeclaringType}' in DynamicDependencyAttribute on '{context}'", 2032));
+					return;
+				}


I'm tempted for this logic to respect --skip-unresolved

That is good point. Curious: do normal apps run linker with --skip-unresolved or is that more of a corner case option?

It's not a default option it's more about avoiding the aggressive context loading again

We do pass it when running the linker from the SDK. As I understand it, that option enables the linker to continue from resolution failures (see #260) instead of crashing. I don't think we need to respect it here since these warnings won't fail the build. I think that if --skip-unresolved is false, the call to Resolve() would cause an AssemblyResolutionException before we get a chance to log this warning.

marek-safar · 2020-06-01T11:03:29Z

Let's land #1223 before this PR

Instead of type itself for DynamicallyAccessedMemberTypes.All

- Use IMemberDefinition as key - Remove optimization for DynamicDependencyAttribute case - Add optimization to check for presence of attributes in the cache - Add checking of member types for the attributes

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>

- Remove assert - Move helpers to signature parser - Consolidate warning messages

We will keep supporting it for now, and remove support for it once runtime has been converted to DynamicDependency

sbomer · 2020-06-04T17:20:18Z

@marek-safar I've addressed your feedback. PTAL

marek-safar

Few minor issues, otherwise LGTM

marek-safar · 2020-06-05T07:47:38Z

+			if (!(args[2].Value is string assemblyName))
+				return null;
+
+			var dynamicDependency = memberSignature == null ? new DynamicDependency (memberTypes!.Value, typeName, assemblyName) : new DynamicDependency (memberSignature, typeName, assemblyName);


Suggested change

var dynamicDependency = memberSignature == null ? new DynamicDependency (memberTypes!.Value, typeName, assemblyName) : new DynamicDependency (memberSignature, typeName, assemblyName);

return new DynamicDependency (memberSignature ?? memberTypes!.Value, typeName, assemblyName) {

OriginalAttribute = ca

};

That doesn't type-check because memberTypes!.Value isn't a string.

What type check?

It needs to call a different ctor for string vs DynamicallyAccessedMemberTypes. memberSignature ?? memberTypes!.Value doesn't have a well-defined type - it produces error CS0019: Operator '??' cannot be applied to operands of type 'string' and 'DynamicallyAccessedMemberTypes'.

- Remove left-over dead code, imports - Fix typos - Don't respect Condition on DynamicDependencyAttribute

sbomer · 2020-06-05T18:40:44Z

@mateoatr could you review the last commit in particular? 82eca9c
I removed parameters to one of the ctors because I don't think we will have line numbers without a source file.

* Add DynamicDependencyAttribute * Add tests for DynamicDependencyAttribute * Test PreserveDependency on illink, disable on mono * Add more tests for DynamicDependency * Fix whitespace and failing test * Fix mono build with LangVersion latest * Use null sentinel Instead of type itself for DynamicallyAccessedMemberTypes.All * Changes to attribute caching - Use IMemberDefinition as key - Remove optimization for DynamicDependencyAttribute case - Add optimization to check for presence of attributes in the cache - Add checking of member types for the attributes * Use Attribute type in cache, and move helpers to DynamicDependency * Remove DynamicDependencyAttribute implementation * Don't disable flow analysis for Dynamic/PreserveDependency * Add tests for DynamicallyAccessedMemberTypes * Update docs/error-codes.md Co-authored-by: Vitek Karas <vitek.karas@microsoft.com> * PR feedback - Remove assert - Move helpers to signature parser - Consolidate warning messages * Fix up tests * Make PreserveDependency deprecated We will keep supporting it for now, and remove support for it once runtime has been converted to DynamicDependency * PR feedback - Remove left-over dead code, imports - Fix typos - Don't respect Condition on DynamicDependencyAttribute * Use new logging helpers * More PR feedback * Use Resolve () instead * Update one more logging callsite Co-authored-by: Vitek Karas <vitek.karas@microsoft.com> Commit migrated from dotnet/linker@6ec3d0f

vitek-karas reviewed May 27, 2020

View reviewed changes

sbomer force-pushed the dynamicDependencyAttribute branch from a636b5b to cd07ed1 Compare May 27, 2020 21:53

sbomer marked this pull request as ready for review May 28, 2020 00:24

sbomer requested a review from marek-safar as a code owner May 28, 2020 00:24

sbomer requested a review from MichalStrehovsky May 28, 2020 00:24

marek-safar requested changes May 28, 2020

View reviewed changes

sbomer force-pushed the dynamicDependencyAttribute branch from b8886a1 to 4668aae Compare May 29, 2020 18:56

sbomer mentioned this pull request May 29, 2020

Factor out binding logic for DynamicallyAccessedMemberTypes #1223

Merged

sbomer force-pushed the dynamicDependencyAttribute branch from 6382f4c to cd21177 Compare May 29, 2020 19:55

sbomer force-pushed the dynamicDependencyAttribute branch 2 times, most recently from 5ce7a13 to bff9585 Compare June 2, 2020 16:31

sbomer requested a review from marek-safar June 3, 2020 18:04

sbomer and others added 16 commits June 4, 2020 00:24

Add DynamicDependencyAttribute

3d4d0e0

Add tests for DynamicDependencyAttribute

9898afb

Test PreserveDependency on illink, disable on mono

3faac9e

Add more tests for DynamicDependency

51fa3b9

Fix whitespace and failing test

5dcfa86

Fix mono build with LangVersion latest

ebe8d9b

Use null sentinel

4ead2f0

Instead of type itself for DynamicallyAccessedMemberTypes.All

Changes to attribute caching

e85ce8f

- Use IMemberDefinition as key - Remove optimization for DynamicDependencyAttribute case - Add optimization to check for presence of attributes in the cache - Add checking of member types for the attributes

Use Attribute type in cache, and move helpers to DynamicDependency

39bb0dc

Remove DynamicDependencyAttribute implementation

60efa31

Don't disable flow analysis for Dynamic/PreserveDependency

aaa9ffa

Add tests for DynamicallyAccessedMemberTypes

bf5f423

Update docs/error-codes.md

b35fe9c

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>

PR feedback

0c4154b

- Remove assert - Move helpers to signature parser - Consolidate warning messages

Fix up tests

448e665

Make PreserveDependency deprecated

1136f0a

We will keep supporting it for now, and remove support for it once runtime has been converted to DynamicDependency

sbomer force-pushed the dynamicDependencyAttribute branch from bff9585 to 1136f0a Compare June 4, 2020 00:41

sbomer self-assigned this Jun 4, 2020

marek-safar mentioned this pull request Jun 5, 2020

Replace usage of PreserveDependencyAttribute with DynamicDependencyAttribute dotnet/runtime#37475

Closed

marek-safar approved these changes Jun 5, 2020

View reviewed changes

vitek-karas requested changes Jun 5, 2020

View reviewed changes

Comment thread src/linker/Linker/LinkerAttributesInformation.cs Outdated

Comment thread src/linker/Linker/TypeDefinitionExtensions.cs Outdated

Comment thread src/linker/System.Diagnostics.CodeAnalysis/DynamicDependencyAttribute.cs Outdated

sbomer added 2 commits June 5, 2020 17:39

PR feedback

c287d2a

- Remove left-over dead code, imports - Fix typos - Don't respect Condition on DynamicDependencyAttribute

Use new logging helpers

82eca9c

sbomer requested a review from mateoatr June 5, 2020 18:39

mateoatr approved these changes Jun 5, 2020

View reviewed changes

More PR feedback

ce02105

mateoatr reviewed Jun 5, 2020

View reviewed changes

Comment thread src/linker/Linker.Steps/MarkStep.cs Outdated

Use Resolve () instead

291ec0c

mateoatr reviewed Jun 5, 2020

View reviewed changes

Comment thread src/linker/Linker/DynamicDependency.cs Outdated

Update one more logging callsite

ddf6b6d

sbomer merged commit 6ec3d0f into dotnet:master Jun 5, 2020

vitek-karas mentioned this pull request Jun 8, 2020

Respect the new DynamicDependencyAttribute #1148

Closed

-			var dynamicDependency = memberSignature == null ? new DynamicDependency (memberTypes!.Value, typeName, assemblyName) : new DynamicDependency (memberSignature, typeName, assemblyName);
+			return new DynamicDependency (memberSignature ?? memberTypes!.Value, typeName, assemblyName) {
+				OriginalAttribute = ca
+			};

Conversation

sbomer commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbomer May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marek-safar commented Jun 1, 2020

Uh oh!

sbomer commented Jun 4, 2020

Uh oh!

marek-safar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbomer commented Jun 5, 2020

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sbomer commented May 26, 2020 •

edited

Loading

sbomer May 29, 2020 •

edited

Loading