This is really weird - the place where the warning is reported....

I agree - although I'm not sure what to do in cases like this, where a method is kept because of a DynamicallyAccessedMember attribute. The method is not being called, we are just marking it in case the user later calls it, so I think maybe this should not warn.

You're right that we preserve it because it may be needed later - but that's kind of the point. Linker can't figure out when exactly it's going to be used, so instead we have data flow annotations and will "detect" the usage somewhere else. Since we can't tell where it's actually being used, there's no point trying to report it there. Instead we should report it where we know about it - it's not 100% but it's pretty close.

Not warning is not an option - if the method is going to be used for real (linker can't tell), that would break the promise and potentially the app. So we have to play it safe and warn.

I think the linker should warn at the place where it detects the need to preserve the method - in cases of DynamicallyAccessedMember, it's the same place where we would issue warnings about problems with the DynamicallyAccessedMember attribute itself (for example if the type being used is not statically known).

I don't think I understand the last check about the base methods.
If the method is in a direct call (can be non-virtual in fact) then the warning should be reported.
I think the interesting question is what happens if I have:

interface ITest
{
    [RequiresUnreferencedCode]
    void DoDanger();
}

class Test : ITest
{
    [RequiredUnreferencedCode]
    void DoDanger() { }
}

void TestIt()
{
    ITest it = new Test();
    it.DoDanger(); // This should warn, but that is the easy part
}

// There should not be another warning about DoDanger even though there will be a MarkMethod(Test.DoDanger) probably coming from the interface as it's detected to be instantiated.

I don't think I understand the last check about the base methods.

It should prevent us from producing two warnings in cases such as:

Bar(typeof(DerivedClass));

void Bar([[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type type) {}

class BaseClass
{
	[RequiresUnreferencedCode ("")]
	public virtual void Foo() {}
}

class DerivedClass : BaseClass
{
	[RequiresUnreferencedCode ("")]
	public override void Foo() {}
}

I finally figured out the counter example:

		abstract class CovariantReturnBase
		{
			[RequiresUnreferencedCode ("Message for --CovariantReturnBase.GetRequiresUnreferencedCode--")]
			public abstract BaseReturnType GetRequiresUnreferencedCode ();
		}

		class CovariantReturnDerived : CovariantReturnBase
		{
			[RequiresUnreferencedCode ("Message for --CovariantReturnBase.GetRequiresUnreferencedCode--")]
			public override DerivedReturnType GetRequiresUnreferencedCode ()
			{
				return null;
			}
		}

		[LogDoesNotContain("--CovariantReturnBase.GetRequiresUnreferencedCode--")]
		[ExpectedWarning ("IL2026", "--CovariantReturnBase.GetRequiresUnreferencedCode--")]
		static void TestCovariantReturnCallOnDerived ()
		{
			var tmp = new CovariantReturnDerived ();
			tmp.GetRequiresUnreferencedCode ();
		}

The problem is that the callvirt in this case is not to the base method. The code as written in the MarkStep basically assumes that if an override is marked, it is never a target of a call - it's only because of virtual hierarchy marking. Which is mostly true for C#, since the compiler makes sure it generates callvirt to the base method regardless of which type the method was called on. This might not be true for non-C#, but it's definitely not true with covariant returns as shown above, in this case C# intentionally generates call to the override method.

The above test sample fails because there's no warning generated. Which is weird for multiple reasons, I'm still looking into it.

Here is a change with the new test and modification to the markStep which makes all of the tests pass: mateoatr/linker@requiresUnreferencedCode...vitek-karas:requiresUnreferencedCode

I don't think testing the behavior with only partially annotated hierarchies is necessary. I think it's more important to test the case where the hierarchy is fully annotated (otherwise the user will get warnings anyway).

And just like in the interface case, I would validate that we don't get a warning due to the virtual override marking.

Why?
I thought we want to warn in this case - it was kind of the point of the bug which caused this whole change, wasn't it?

This comment needs updating. While it's technically true, it's not obvious how that matters for the if below - since the if doesn't have anything to do with virtual methods anymore:

• We rely on the fact that all overrides should have the same annotations as the base method (and we have a verification step which would warn if that's not the case)
• We rely on the fact that the base method is the "origin" for marking overrides
• And so if the override has the annotation, the base should have it as well -> we will not produce warning since the "origin" will have the annotation.

Ideally this warning would be generated in the TestStaticCctorRequiresUnreferencedCode method.
Basically I should be able to add RequiresUnreferencedCode onto the test method and not get any warnings.
The way it works now is problematic because there's no way to get rid of the warning without explicit suppression.

I don't understand why the warning shouldn't be generated. We will keep the method - it's marked as problematic - it may be actually used by the code (we don't know for sure, but that's why we included it) - we need to report warning to the user.
The warning should be generated on the caller - in this case the Main - just like it was before.