Skip to content

Commit

Permalink
Fix bug in virtual annotation logic for RequiresUnreferencedCode (#10…
Browse files Browse the repository at this point in the history
…0707)

The following virtual methods are correctly annotated and do not warn:

```csharp
class Base {
  virtual void M() {}
}

[RequiresUnreferencedCode("Derived")]
class Derived : Base {
  override void M() {}
}
```

However, if the method also happens to have DynamicallyAccessedMembers
annotations, there's a bug in the logic that causes this to produce
warnings in illink. This change fixes the bug.

Hit this while experimenting with ComponentModel annotations.

---------

Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com>
  • Loading branch information
sbomer and jtschuster authored Apr 8, 2024
1 parent c1a4c9c commit 8508806
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,15 @@ protected override void Process ()
void ValidateMethodRequiresUnreferencedCodeAreSame (MethodDefinition method, MethodDefinition baseMethod)
{
var annotations = Context.Annotations;
bool methodHasAttribute = annotations.IsInRequiresUnreferencedCodeScope (method, out _);
if (methodHasAttribute != annotations.IsInRequiresUnreferencedCodeScope (baseMethod, out _)) {
string message = MessageFormat.FormatRequiresAttributeMismatch (methodHasAttribute,
baseMethod.DeclaringType.IsInterface, nameof (RequiresUnreferencedCodeAttribute), method.GetDisplayName (), baseMethod.GetDisplayName ());
bool methodSatisfies = annotations.IsInRequiresUnreferencedCodeScope (method, out _);
bool baseRequires = annotations.DoesMethodRequireUnreferencedCode (baseMethod, out _);
if ((baseRequires && !methodSatisfies) || (!baseRequires && annotations.DoesMethodRequireUnreferencedCode (method, out _))) {
string message = MessageFormat.FormatRequiresAttributeMismatch (
methodSatisfies,
baseMethod.DeclaringType.IsInterface,
nameof (RequiresUnreferencedCodeAttribute),
method.GetDisplayName (),
baseMethod.GetDisplayName ());
Context.LogWarning (method, DiagnosticId.RequiresUnreferencedCodeAttributeMismatch, message);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public static void Main ()
StaticInterfaceMethods.Test ();
BaseInPreservedScope.Test ();
DirectCall.Test ();
RequiresAndDynamicallyAccessedMembersValidation.Test ();
}

static void RequirePublicMethods ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type type)
Expand Down Expand Up @@ -1019,6 +1020,61 @@ public static void Test ()
CallStaticGvm<ImplIGvmBase> ();
}
}

class RequiresAndDynamicallyAccessedMembersValidation
{
// These tests have both DynamicallyAccessedMembers annotations and Requires annotations.
// This is to reproduce a bug where the virtual method annotations would be validated due to
// the presence of DynamicallyAccessedMembers, but the logic for checking Requires annotations
// was incorrect. The bug didn't manifest with just Requires annotations because the methods wouldn't
// be validated at all for Requires on type.

class BaseMethodWithRequires
{
[RequiresUnreferencedCode (nameof (MethodWithRequires))]
[RequiresDynamicCode (nameof (MethodWithRequires))]
public virtual void MethodWithRequires ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t) {}
}

[RequiresUnreferencedCode (nameof (DerivedTypeWithRequires_BaseMethodWithRequires))]
[RequiresDynamicCode (nameof (DerivedTypeWithRequires_BaseMethodWithRequires))]
class DerivedTypeWithRequires_BaseMethodWithRequires : BaseMethodWithRequires
{
public override void MethodWithRequires ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t) {}
}

[ExpectedWarning ("IL2026", nameof (DerivedTypeWithRequires_BaseMethodWithRequires))]
[ExpectedWarning ("IL2026", nameof (DerivedTypeWithRequires_BaseMethodWithRequires.MethodWithRequires))]
[ExpectedWarning ("IL3050", nameof (DerivedTypeWithRequires_BaseMethodWithRequires), ProducedBy = Tool.NativeAot | Tool.Analyzer)]
[ExpectedWarning ("IL3050", nameof (DerivedTypeWithRequires_BaseMethodWithRequires.MethodWithRequires), ProducedBy = Tool.NativeAot | Tool.Analyzer)]
static void Test_DerivedTypeWithRequires_BaseMethodWithRequires ()
{
new DerivedTypeWithRequires_BaseMethodWithRequires ().MethodWithRequires (typeof (int));
}

class BaseMethodWithoutRequires
{
public virtual void MethodWithoutRequires ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t) {}
}

[RequiresUnreferencedCode (nameof (DerivedTypeWithRequires_BaseMethodWithoutRequires))]
class DerivedTypeWithRequires_BaseMethodWithoutRequires : BaseMethodWithoutRequires
{
public override void MethodWithoutRequires ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)] Type t) {}
}

[ExpectedWarning ("IL2026", nameof (DerivedTypeWithRequires_BaseMethodWithoutRequires))]
static void Test_DerivedTypeWithRequires_BaseMethodWithoutRequires ()
{
new DerivedTypeWithRequires_BaseMethodWithoutRequires ().MethodWithoutRequires (typeof (int));
}

public static void Test ()
{
Test_DerivedTypeWithRequires_BaseMethodWithRequires ();
Test_DerivedTypeWithRequires_BaseMethodWithoutRequires ();
}
}
}
}

Expand Down

0 comments on commit 8508806

Please sign in to comment.