Warn on called methods annotated with RequiresUnreferencedCode by mateoatr · Pull Request #1778 · dotnet/linker

mateoatr · 2021-01-23T03:16:30Z

See #1607. Most of the changes in this PR were already in #1639. We only run through ProcessRequiresUnreferencedCode whenever the annotated method was either called or found in a reflection scenario. A test case was added for making sure that methods marked with RequiresUnreferencedCodeAttribute in copied assemblies do not trigger any warnings (unless called or referenced through reflection).

src/ILLink.RoslynAnalyzer/Resources.resx

test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresUnreferencedCodeCapability.cs

src/linker/Linker.Steps/MarkStep.cs

vitek-karas · 2021-01-26T21:03:46Z

test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresUnreferencedCodeCapability.cs

+		}
+
+		[ExpectedWarning ("IL2026", "--MethodCalledThroughReflection--")]
+		static void TestRequiresThroughReflectionInMethodFromCopiedAssembly ()


Given the use case in DispatchProxy (that one is about DynamicallyAccessedMember, but it's similar) - can you please add another test case where we apply the All annotation to an interface and we validate that it warns if one of the methods on the interface is annotated with RequiresUnreferencedCode - maybe a more complex one if the method is on another interface, for example:

interface A { [RequiresUnreferencedCode] void DoA (); } interface B : A { void DoB (); } RequiresAll(typeof(B));

vitek-karas · 2021-01-28T14:56:29Z

src/linker/Linker.Steps/MarkStep.cs

+				}
+			}
+
+			DependencyInfo GetDependencyInfo (IMemberDefinition member, DependencyInfo reason)


With the change to use an explicit parameter, this should not be needed for functionality. I don't mind making the change as it makes it easier to understand dependencies, but maybe we should make that in a separate PR.

vitek-karas · 2021-01-28T15:05:01Z

src/linker/Linker.Steps/MarkStep.cs

 				foreach (InterfaceImplementation iface in type.Interfaces) {
-					MarkInterfaceImplementation (iface, type);
+					if (reason.Kind == DependencyKind.AccessedViaReflection)
+						MarkEntireTypeInternal (iface.InterfaceType.Resolve (), includeBaseTypes: true, reason, type, typesAlreadyVisited);


I think we need to call MarkInterfaceImplementation anyway. If the interface is a generic instantiation we would lose that information and for example would not correctly apply DynamicallyAccessedMembers.
I think we'll need to look into this a bit more - the same applies to base types, and potentially even methods. I filed https://github.com/mono/linker/issues/1787 to look into this some more.

Also - it's technically possible for Resole to return null, the should not crash on it. See handling of base types.

vitek-karas · 2021-01-28T15:08:03Z

src/linker/Linker.Steps/MarkStep.cs

+				if (reason.Kind == DependencyKind.AccessedViaReflection)
+					ProcessRequiresUnreferencedCode (method, new MessageOrigin (reason.Source as IMemberDefinition), reason.Kind);


This feels wrong - I understand it's because MarkEntireType doesn't call MarkMethod - maybe it should...
Can you also check if there are other places which side-step MarkMethod and end up here instead? Those could all be potential issues as well.

src/linker/Linker.Steps/MarkStep.cs

vitek-karas · 2021-01-28T21:11:19Z

src/linker/Linker.Steps/MarkStep.cs

-					Annotations.Mark (method, GetDependencyInfo (method, reason));
-					Annotations.SetAction (method, MethodAction.ForceParse);
-					EnqueueMethod (method, GetDependencyInfo (method, reason));
+					MarkMethod (method, new DependencyInfo (reason.Kind == DependencyKind.AccessedViaReflection ? reason.Kind : DependencyKind.MemberOfType, type), new MessageOrigin (reason.Source as IMemberDefinition));


We should keep the SetAction - the difference in behavior will be pretty small, but still - no reason to NOT do that.

vitek-karas

Other than the .Resolve returning null concern in #1778 (comment) this looks good.

mateoatr · 2021-01-28T21:41:12Z

Other than the .Resolve returning null concern in #1778 (comment) this looks good.

Doesn't

https://github.com/mono/linker/blob/bcc7dec192953e36ef9274c54f7a636d25b02f09/src/linker/Linker.Steps/MarkStep.cs#L277-L278

Solve this?

vitek-karas · 2021-01-28T21:46:50Z

Of course it does... sorry - my bad.

sbomer · 2021-01-29T00:15:37Z

src/linker/Linker.Steps/MarkStep.cs

-					Annotations.Mark (method, new DependencyInfo (DependencyKind.MemberOfType, type));
 					Annotations.SetAction (method, MethodAction.ForceParse);
-					EnqueueMethod (method, new DependencyInfo (DependencyKind.MemberOfType, type));
+					MarkMethod (method, new DependencyInfo (reason.Kind == DependencyKind.AccessedViaReflection ? reason.Kind : DependencyKind.MemberOfType, type), new MessageOrigin (reason.Source as IMemberDefinition));


Why the special case for AccessedViaReflection here - is it just to ensure the reason is one of the expected ones in ProcessRequiresUnreferencedCode?

Can we do this without changing the reason for the marked method?

If we go with the special case, should we do the same for DependencyKind.DynamicDependency - since we can get here from MarkDynamicDependency?

Why the special case for AccessedViaReflection here - is it just to ensure the reason is one of the expected ones in ProcessRequiresUnreferencedCode?

Yes.

Can we do this without changing the reason for the marked method?

We can do this if we add DynamicDependency to the list of reasons for marking a method... I think it makes sense and should make code cleaner/easier to read.

I'm not sure you'd need to add DynamicDependency if we continue passing MemberOfType - I think you'd need to add MemberOfType to the list of cases in ProcessRequiresUnreferencedCode.

Just tried not special casing. If we simple reuse reason.Kind then we can end up passing BaseType or NestedType, which then throws, since these are not valid reasons for marking a method.
The reason to special case here is so that we warn on annotated methods on a type that is kept because of DynamicallyAccessedMembers.All. We don't have MembersOfType on the list of reasons to process ProcessRequiresUnreferencedCode since we don't want to emit warnings on methods that are marked because of a copied assembly (we only want to warn if there's a possibility of the method being referenced through reflection or if it's directly called).
I think it's easier to just keep this special case and of course, special case DynamicDependency too.

Ah, that makes sense, to avoid warnings for copy assemblies. Thanks for the explanation!

Just another thought - now I am wondering if we could use sourceLocationMember only, instead of DependencyInfo, for this purpose - since DependencyInfo was designed to be more granular than source locations, and what we need here are source locations. But that might require more changes to the sourceLocationMember logic - I'm not sure. I'll leave it as a suggestion in case you think it's worth looking into, but I think special-casing AccessedViaReflection/DynamicDependency is fine too.

sbomer · 2021-01-29T00:47:40Z

src/linker/Linker.Steps/MarkStep.cs

+				// to be marked. This is important to avoid spurious warnings as virtual overrides can end up being
+				// marked without being referenced directly from user code. It is important to pass the base method
+				// here so that MarkMethod can see that it was marked because of an override.
+				MarkMethod (method, new DependencyInfo (DependencyKind.OverrideOnInstantiatedType, method.DeclaringType), new MessageOrigin (@base));


Is this logic still needed to avoid spurious warnings? It looks to me like it's not, since ProcessRequiresUnreferencedCode doesn't have a case for OverrideOnInstantiatedType (or maybe it's missing a case?)

More generally (not directly related to your change) - I'm not sure what the function of sourceLocationMember is here - I think we should just pass null. If I'm not missing something, it looks like:

At this point, method and @base are MethodDefinitions

MarkMethod will only use sourceLocationMember for instantiated generic types or methods (the latter in GetOriginalMethod)

I think the sourceLocationMember is only used for instantiated generics, but can't have instantiated generics here, so we shouldn't use it. Does that sound right to you? @vitek-karas I think you introduced these - what do you think?

You are right it's no longer needed; when calling MarkMethod from here we will never emit a warning.

More generally (not directly related to your change) - I'm not sure what the function of sourceLocationMember is here - I think we should just pass null.

Yes, since there's no way we will emit a warning in MarkMethod at this point, we can pass null here. Other than helping us report the warning, sourceLocationMember becomes relevant in the context of MarkMethod when dealing with generics, as you noted (we could end here:

https://github.com/mono/linker/blob/ba990ce8336dd278b31007aac30ac85f120d3683/src/linker/Linker.Steps/MarkStep.cs#L2267

I'll change the origin here to null, unless @vitek-karas disagrees / there a reason to pass @base/method.DeclaringType here.

sbomer

LGTM otherwise!

Pass null to origin's parameter when processing optimized overrides Add DynamicDependency to list of valid dependency kinds in ProcessRequiresUnreferencedCode

…t/linker#1778) * Only process direct calls and reflection method references. * Clean test * Add tests * Check for null origin * Warn on DynamicallyAccessedMember kind. * Update warnings message for Requires Unreferenced Code * Mark interfaces of types accessed via reflection * Fix whitespace * Add includeInterfaceTypes parameter * Keep set action for methods in MarkEntireType * Add test for dynamic dependency with RUC. * Fix DependencyKind in MarkTypeForDynamicallyAccessedMembers Pass null to origin's parameter when processing optimized overrides Add DynamicDependency to list of valid dependency kinds in ProcessRequiresUnreferencedCode * Fix formatting Commit migrated from dotnet/linker@6745019

mateoatr requested review from MichalStrehovsky, marek-safar and vitek-karas January 23, 2021 03:16

Mateo Torres Ruiz added 4 commits January 25, 2021 09:50

Only process direct calls and reflection method references.

3498e42

Clean test

01e3ef9

Add tests

81bf29e

Check for null origin

e7af1b8

mateoatr force-pushed the reqUnrefCode branch from f895748 to e7af1b8 Compare January 25, 2021 19:46

mateoatr requested a review from sbomer January 25, 2021 19:49

vitek-karas requested changes Jan 26, 2021

View reviewed changes

Mateo Torres Ruiz added 2 commits January 26, 2021 12:08

Warn on DynamicallyAccessedMember kind.

d0d1169

Update warnings message for Requires Unreferenced Code

ade3c03

vitek-karas approved these changes Jan 26, 2021

View reviewed changes

Mateo Torres Ruiz added 2 commits January 27, 2021 23:35

Mark interfaces of types accessed via reflection

f597eaa

Fix whitespace

d9a63a9

vitek-karas reviewed Jan 28, 2021

View reviewed changes

Add includeInterfaceTypes parameter

7da2247

vitek-karas reviewed Jan 28, 2021

View reviewed changes

Keep set action for methods in MarkEntireType

bcc7dec

vitek-karas approved these changes Jan 28, 2021

View reviewed changes

Add test for dynamic dependency with RUC.

675fa4c

sbomer reviewed Jan 29, 2021

View reviewed changes

sbomer approved these changes Jan 29, 2021

View reviewed changes

Fix DependencyKind in MarkTypeForDynamicallyAccessedMembers

3c3771c

Pass null to origin's parameter when processing optimized overrides Add DynamicDependency to list of valid dependency kinds in ProcessRequiresUnreferencedCode

mateoatr mentioned this pull request Jan 29, 2021

RequiresAssemblyFilesAttribute resource manager changes #1790

Merged

Fix formatting

7011ca6

mateoatr merged commit 6745019 into dotnet:master Feb 1, 2021

		if (reason.Kind == DependencyKind.AccessedViaReflection)
		ProcessRequiresUnreferencedCode (method, new MessageOrigin (reason.Source as IMemberDefinition), reason.Kind);

Conversation

mateoatr commented Jan 23, 2021

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!

Choose a reason for hiding this comment

Uh oh!

vitek-karas left a comment

Choose a reason for hiding this comment

Uh oh!

mateoatr commented Jan 28, 2021

Uh oh!

vitek-karas commented Jan 28, 2021

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!

sbomer Jan 29, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

sbomer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sbomer Jan 29, 2021 •

edited

Loading