Skip to content

Commit

Permalink
Support for RequiresAttributeMismatch testing (#72496)
Browse files Browse the repository at this point in the history
The RequiresAttributeMismatch warnings were not being generated since one of the answers from the MultiFileSharedCompilatioModuleGroup was to generate Vtables from everything. The logic for checking for warning mismatch is gated by not having a vtable being generated so needed to create a copy of the MultiFileSharedCompilationModuleGroup for testing purposes. Adding testing also unveils some issues with the current way of producing warnings in NativeAOT that are addressed in this PR.

- Create a copy of the MultiFileSharedCompilationModuleGroup into the test infrastructure
- Default answer of ShouldProduceFullVTable to false in the TestInfraMultiFileSharedCompilationModuleGroup, otherwise, the logic for checking for mismatch in NativeAOT will not work as currently implemented.
- Property names don't need parenthesis, escape before they are added.
- Small fix when ExplicitInterface names are used, cannot rely on name starting with get_/set_ in all cases.
- Additional RUC warning check when a method is marked via DAM and is not TypeHierarchy marking
- Adds RequiresAttributeMismatch test file from linker (includes adding NativeAOT specific ProducedBy attributes)
  • Loading branch information
tlakollo committed Jul 21, 2022
1 parent bac2229 commit 2f02f12
Show file tree
Hide file tree
Showing 9 changed files with 822 additions and 215 deletions.
3 changes: 2 additions & 1 deletion src/coreclr/tools/Common/Compiler/DisplayNameHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public static string GetDisplayName(this MethodDesc method)
sb.Append(property.Name);
sb.Append('.');
sb.Append(property.GetMethod == method ? "get" : "set");
return sb.ToString();
}
else
{
Expand Down Expand Up @@ -104,7 +105,7 @@ public static string GetDisplayName(this PropertyPseudoDesc property)
.Append('.')
.Append(property.Name).ToString();
}

public static string GetDisplayName(this EventPseudoDesc @event)
{
return new StringBuilder(@event.OwningType.GetDisplayName())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,20 @@ void CheckAndWarnOnReflectionAccess(in MessageOrigin origin, TypeSystemEntity en
MessageFormat.FormatRequiresAttributeMessageArg(DiagnosticUtilities.GetRequiresAttributeMessage(requiresAttribute.Value)),
MessageFormat.FormatRequiresAttributeUrlArg(DiagnosticUtilities.GetRequiresAttributeUrl(requiresAttribute.Value)));
}
else
{
var diagnosticContext = new DiagnosticContext(
origin,
_logger.ShouldSuppressAnalysisWarningsForRequires(origin.MemberDefinition, DiagnosticUtilities.RequiresUnreferencedCodeAttribute),
_logger.ShouldSuppressAnalysisWarningsForRequires(origin.MemberDefinition, DiagnosticUtilities.RequiresDynamicCodeAttribute),
_logger.ShouldSuppressAnalysisWarningsForRequires(origin.MemberDefinition, DiagnosticUtilities.RequiresAssemblyFilesAttribute),
_logger);

string arg1 = MessageFormat.FormatRequiresAttributeMessageArg(DiagnosticUtilities.GetRequiresAttributeMessage(requiresAttribute.Value));
string arg2 = MessageFormat.FormatRequiresAttributeUrlArg(DiagnosticUtilities.GetRequiresAttributeUrl(requiresAttribute.Value));

diagnosticContext.AddDiagnostic(DiagnosticId.RequiresUnreferencedCode, entity.GetDisplayName(), arg1, arg2);
}
}

if (!Annotations.ShouldWarnWhenAccessedForReflection(entity))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ public sealed class UsageBasedMetadataManager : GeneratingMetadataManager

private readonly FeatureSwitchHashtable _featureSwitchHashtable;

private static (string AttributeName, DiagnosticId Id)[] _requiresAttributeMismatchNameAndId = new[]
{
(DiagnosticUtilities.RequiresUnreferencedCodeAttribute, DiagnosticId.RequiresUnreferencedCodeAttributeMismatch),
(DiagnosticUtilities.RequiresDynamicCodeAttribute, DiagnosticId.RequiresDynamicCodeAttributeMismatch),
(DiagnosticUtilities.RequiresAssemblyFilesAttribute, DiagnosticId.RequiresAssemblyFilesAttributeMismatch)
};

private readonly List<ModuleDesc> _modulesWithMetadata = new List<ModuleDesc>();
private readonly List<FieldDesc> _fieldsWithMetadata = new List<FieldDesc>();
private readonly List<MethodDesc> _methodsWithMetadata = new List<MethodDesc>();
Expand Down Expand Up @@ -783,15 +790,19 @@ public bool GeneratesAttributeMetadata(TypeDesc attributeType)

public override void NoteOverridingMethod(MethodDesc baseMethod, MethodDesc overridingMethod)
{
// We validate that the various dataflow/Requires* annotations are consistent across virtual method overrides
if (HasMismatchingAttributes(baseMethod, overridingMethod, "RequiresUnreferencedCodeAttribute"))
bool baseMethodTypeIsInterface = baseMethod.OwningType.IsInterface;
foreach (var requiresAttribute in _requiresAttributeMismatchNameAndId)
{
Logger.LogWarning(overridingMethod, DiagnosticId.RequiresUnreferencedCodeAttributeMismatch, overridingMethod.GetDisplayName(), baseMethod.GetDisplayName());
}
// We validate that the various dataflow/Requires* annotations are consistent across virtual method overrides
if (HasMismatchingAttributes(baseMethod, overridingMethod, requiresAttribute.AttributeName))
{
string overridingMethodName = overridingMethod.GetDisplayName();
string baseMethodName = baseMethod.GetDisplayName();
string message = MessageFormat.FormatRequiresAttributeMismatch(overridingMethod.DoesMethodRequire(requiresAttribute.AttributeName, out _),
baseMethodTypeIsInterface, requiresAttribute.AttributeName, overridingMethodName, baseMethodName);

if (HasMismatchingAttributes(baseMethod, overridingMethod, "RequiresDynamicCodeAttribute"))
{
Logger.LogWarning(overridingMethod, DiagnosticId.RequiresDynamicCodeAttributeMismatch, overridingMethod.GetDisplayName(), baseMethod.GetDisplayName());
Logger.LogWarning(overridingMethod, requiresAttribute.Id, message);
}
}

bool baseMethodRequiresDataflow = FlowAnnotations.RequiresDataflowAnalysis(baseMethod);
Expand All @@ -802,7 +813,7 @@ public override void NoteOverridingMethod(MethodDesc baseMethod, MethodDesc over
}
}

public static bool HasMismatchingAttributes (MethodDesc baseMethod, MethodDesc overridingMethod, string requiresAttributeName)
public static bool HasMismatchingAttributes(MethodDesc baseMethod, MethodDesc overridingMethod, string requiresAttributeName)
{
bool baseMethodCreatesRequirement = baseMethod.DoesMethodRequire(requiresAttributeName, out _);
bool overridingMethodCreatesRequirement = overridingMethod.DoesMethodRequire(requiresAttributeName, out _);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,8 @@ class TypeWithWarnings
public void Method2 () { }

// https://github.com/dotnet/linker/issues/2273
[ExpectedWarning ("IL2026", "--Method1--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "--Method2--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "--Method1--", ProducedBy = ProducedBy.Trimmer | ProducedBy.NativeAot)]
[ExpectedWarning ("IL2026", "--Method2--", ProducedBy = ProducedBy.Trimmer | ProducedBy.NativeAot)]
public static void Test ()
{
Type.GetType ("Mono.Linker.Tests.Cases.DataFlow." + nameof (GetTypeDataFlow) + "+" + nameof (TypeWithWarnings)).RequiresPublicMethods ();
Expand All @@ -187,7 +187,7 @@ class OverConstTypeName
public void Method1 () { }

// https://github.com/dotnet/linker/issues/2273
[ExpectedWarning ("IL2026", "--Method1--", ProducedBy = ProducedBy.Trimmer)]
[ExpectedWarning ("IL2026", "--Method1--", ProducedBy = ProducedBy.Trimmer | ProducedBy.NativeAot)]
public static void Test ()
{
Type.GetType (s_ConstTypeName).RequiresPublicMethods ();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Mono.Linker.Tests.Cases.RequiresCapability.Dependencies
{
public class ReferenceInterfaces
{
public interface IBaseWithRequiresInReference
{
[RequiresUnreferencedCode ("Message")]
[RequiresAssemblyFiles ("Message")]
[RequiresDynamicCode ("Message")]
public void Method ();

public string PropertyAnnotationInAccesor {
[RequiresUnreferencedCode ("Message")]
[RequiresAssemblyFiles ("Message")]
[RequiresDynamicCode ("Message")]
get;
set;
}

[RequiresAssemblyFiles ("Message")]
public string PropertyAnnotationInProperty { get; set; }
}

public interface IBaseWithoutRequiresInReference
{
public void Method ();

public string PropertyAnnotationInAccesor {
get;
set;
}

public string PropertyAnnotationInProperty { get; set; }
}
}
}
Loading

0 comments on commit 2f02f12

Please sign in to comment.