Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix check for types interesting to dataflow #105642

Merged
merged 1 commit into from
Jul 31, 2024
Merged

Fix check for types interesting to dataflow #105642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,18 +148,18 @@ public override void Initialize (AnalysisContext context)
static void VerifyMemberOnlyApplyToTypesOrStrings (SymbolAnalysisContext context, ISymbol member)
{
var location = GetPrimaryLocation (member.Locations);
if (member is IFieldSymbol field && field.GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None && !field.Type.IsTypeInterestingForDataflow ())
if (member is IFieldSymbol field && field.GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None && !field.Type.IsTypeInterestingForDataflow (isByRef: field.RefKind is not RefKind.None))
context.ReportDiagnostic (Diagnostic.Create (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersOnFieldCanOnlyApplyToTypesOrStrings), location, member.GetDisplayName ()));
else if (member is IMethodSymbol method) {
if (method.GetDynamicallyAccessedMemberTypesOnReturnType () != DynamicallyAccessedMemberTypes.None && !method.ReturnType.IsTypeInterestingForDataflow ())
if (method.GetDynamicallyAccessedMemberTypesOnReturnType () != DynamicallyAccessedMemberTypes.None && !method.ReturnType.IsTypeInterestingForDataflow (isByRef: method.ReturnsByRef))
context.ReportDiagnostic (Diagnostic.Create (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersOnMethodReturnValueCanOnlyApplyToTypesOrStrings), location, member.GetDisplayName ()));
if (method.GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None && !method.ContainingType.IsTypeInterestingForDataflow ())
if (method.GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None && !method.ContainingType.IsTypeInterestingForDataflow (isByRef: false))
context.ReportDiagnostic (Diagnostic.Create (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersIsNotAllowedOnMethods), location));
foreach (var parameter in method.Parameters) {
if (parameter.GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None && !parameter.Type.IsTypeInterestingForDataflow ())
if (parameter.GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None && !parameter.Type.IsTypeInterestingForDataflow (isByRef: parameter.RefKind is not RefKind.None))
context.ReportDiagnostic (Diagnostic.Create (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersOnMethodParameterCanOnlyApplyToTypesOrStrings), location, parameter.GetDisplayName (), member.GetDisplayName ()));
}
} else if (member is IPropertySymbol property && property.GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None && !property.Type.IsTypeInterestingForDataflow ()) {
} else if (member is IPropertySymbol property && property.GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None && !property.Type.IsTypeInterestingForDataflow (isByRef: property.ReturnsByRef)) {
context.ReportDiagnostic (Diagnostic.Create (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersOnPropertyCanOnlyApplyToTypesOrStrings), location, member.GetDisplayName ()));
}
}
Expand Down
11 changes: 7 additions & 4 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/ITypeSymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,19 @@ private enum HierarchyFlags
IsSystemReflectionIReflect = 0x02,
}

public static bool IsTypeInterestingForDataflow (this ITypeSymbol type)
public static bool IsTypeInterestingForDataflow (this ITypeSymbol type, bool isByRef)
{
if (type.SpecialType == SpecialType.System_String)
if (type.SpecialType is SpecialType.System_String && !isByRef)
return true;

var flags = GetFlags (type);
if (type is not INamedTypeSymbol namedType)
return false;

var flags = GetFlags (namedType);
return IsSystemType (flags) || IsSystemReflectionIReflect (flags);
}

private static HierarchyFlags GetFlags (ITypeSymbol type)
private static HierarchyFlags GetFlags (INamedTypeSymbol type)
{
HierarchyFlags flags = 0;
if (type.IsTypeOf (WellKnownType.System_Reflection_IReflect)) {
Expand Down
6 changes: 3 additions & 3 deletions src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,8 @@ public override MultiValue VisitConversion (IConversionOperation operation, Stat
{
var value = base.VisitConversion (operation, state);

if (operation.OperatorMethod != null)
return operation.OperatorMethod.ReturnType.IsTypeInterestingForDataflow () ? new MethodReturnValue (operation.OperatorMethod, isNewObj: false) : value;
if (operation.OperatorMethod is IMethodSymbol method)
return method.ReturnType.IsTypeInterestingForDataflow (isByRef: method.ReturnsByRef) ? new MethodReturnValue (method, isNewObj: false) : value;

// TODO - is it possible to have annotation on the operator method parameters?
// if so, will these be checked here?
Expand Down Expand Up @@ -346,7 +346,7 @@ public override void HandleReturnValue (MultiValue returnValue, IOperation opera
if (OwningSymbol is not IMethodSymbol method)
return;

if (method.ReturnType.IsTypeInterestingForDataflow ()) {
if (method.ReturnType.IsTypeInterestingForDataflow (isByRef: method.ReturnsByRef)) {
var returnParameter = new MethodReturnValue (method, isNewObj: false);

TrimAnalysisPatterns.Add (
Expand Down
9 changes: 8 additions & 1 deletion src/tools/illink/src/linker/Linker.Dataflow/FlowAnnotations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,14 @@ public bool IsTypeInterestingForDataflow (TypeReference typeReference)
if (typeReference.MetadataType == MetadataType.String)
return true;

TypeDefinition? type = _context.TryResolve (typeReference);
// ByRef over an interesting type is itself interesting
if (typeReference.IsByReference)
typeReference = ((ByReferenceType) typeReference).ElementType;

if (!typeReference.IsNamedType ())
return false;

TypeDefinition? type = typeReference.ResolveToTypeDefinition (_context);
return type != null && (
_hierarchyInfo.IsSystemType (type) ||
_hierarchyInfo.IsSystemReflectionIReflect (type));
Expand Down
21 changes: 21 additions & 0 deletions src/tools/illink/src/linker/Linker/TypeReferenceExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,27 @@ public static TypeReference WithoutModifiers (this TypeReference type)
return type;
}

// Check whether this type represents a "named type" (i.e. a type that has a name and can be resolved to a TypeDefinition),
// not an array, pointer, byref, or generic parameter. Conceptually this is supposed to represent the same idea as Roslyn's
// INamedTypeSymbol, or ILC's DefType/MetadataType.
public static bool IsNamedType (this TypeReference typeReference) {
if (typeReference.IsDefinition || typeReference.IsGenericInstance)
return true;

if (typeReference.IsArray || typeReference.IsByReference || typeReference.IsPointer || typeReference.IsGenericParameter)
return false;

// Shouldn't get called for these cases
Debug.Assert (!typeReference.IsFunctionPointer);
Debug.Assert (!typeReference.IsRequiredModifier);
Debug.Assert (!typeReference.IsOptionalModifier);
Debug.Assert (!typeReference.IsPinned);
Debug.Assert (!typeReference.IsSentinel);

Debug.Assert (typeReference.GetType () == typeof (TypeReference));
return true;
}

// Array types that are dynamically accessed should resolve to System.Array instead of its element type - which is what Cecil resolves to.
// Any data flow annotations placed on a type parameter which receives an array type apply to the array itself. None of the members in its
// element type should be marked.
Expand Down
38 changes: 38 additions & 0 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AttributeConstructorDataflow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public static void Main ()
typeof (AttributeConstructorDataflow).GetMethod ("Main").GetCustomAttribute (typeof (KeepsPublicConstructorAttribute));
typeof (AttributeConstructorDataflow).GetMethod ("Main").GetCustomAttribute (typeof (KeepsPublicMethodsAttribute));
AllOnSelf.Test ();
AnnotationOnTypeArray.Test ();
}

[Kept]
Expand Down Expand Up @@ -160,6 +161,43 @@ public void Method () { }
}
}

[Kept]
class AnnotationOnTypeArray
{
[Kept]
[KeptBaseType (typeof (Attribute))]
class AttributeRequiresTypeArrayAttribute : Attribute
{
[Kept]
[ExpectedWarning ("IL2098")]
[UnexpectedWarning ("IL2067", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
public AttributeRequiresTypeArrayAttribute (
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
Type[] types)
{
RequirePublicFields (types);
}

[Kept]
[ExpectedWarning ("IL2098")]
static void RequirePublicFields (
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)]
Type[] types)
{
}
}

[Kept]
[KeptAttributeAttribute (typeof (AttributeRequiresTypeArrayAttribute))]
[UnexpectedWarning ("IL2062", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
[AttributeRequiresTypeArray (new Type[] { typeof (int) })]
public static void Test () {
typeof (AnnotationOnTypeArray).GetMethod ("Test").GetCustomAttribute (typeof (AttributeRequiresTypeArrayAttribute));
}
}

[Kept]
[KeptBaseType (typeof (Attribute))]
class TypeArrayAttribute : Attribute
Expand Down
43 changes: 42 additions & 1 deletion src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/FieldDataFlow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,11 +345,52 @@ static void TestFlowOutOfField ()
}

[UnexpectedWarning ("IL2074", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
public static void Test () {
static void TestUnsupportedType () {
var t = GetUnsupportedTypeInstance ();
unsupportedTypeInstance = t;
TestFlowOutOfField ();
}

ref struct StringRef
{
[ExpectedWarning ("IL2097")]
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
public ref string stringRef;

[UnexpectedWarning ("IL2069", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
public StringRef (ref string s)
{
stringRef = ref s;
}

static string GetString () => null;

[ExpectedWarning ("IL2098")]
static void RequirePublicFields (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)]
ref string s)
{
}

[UnexpectedWarning ("IL2077", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
void TestFlowOutOfField ()
{
RequirePublicFields (ref stringRef);
}

public static void Test ()
{
string s = GetString ();
var stringRef = new StringRef (ref s);
stringRef.TestFlowOutOfField ();
}
}

public static void Test ()
{
TestUnsupportedType ();
StringRef.Test ();
}
}

class WriteArrayField
Expand Down
148 changes: 146 additions & 2 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/MethodParametersDataFlow.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ namespace Mono.Linker.Tests.Cases.DataFlow
// - so the main validation is done by the ExpectedWarning attributes.
[SkipKeptItemsValidation]
[ExpectedNoWarnings]
[SetupCompileArgument ("/unsafe")]
[SetupLinkerArgument ("--keep-metadata", "parametername")]
public class MethodParametersDataFlow
{
Expand Down Expand Up @@ -44,6 +45,7 @@ public static void Main ()
TestVarargsMethod (typeof (TestType), __arglist (0, 1, 2));
#endif
AnnotationOnUnsupportedParameter.Test ();
AnnotationOnByRefParameter.Test ();
WriteCapturedParameter.Test ();
}

Expand Down Expand Up @@ -237,7 +239,7 @@ static void TestVarargsMethod ([DynamicallyAccessedMembers (DynamicallyAccessedM

class AnnotationOnUnsupportedParameter
{
class UnsupportedType ()
class UnsupportedType
{
}

Expand All @@ -260,10 +262,152 @@ static void RequirePublicFields (
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
public static void Test () {
static void TestUnsupportedType ()
{
var t = GetUnsupportedTypeInstance ();
RequirePublicMethods (t);
}

static Type[] GetTypeArray () => null;

[ExpectedWarning ("IL2098")]
[UnexpectedWarning ("IL2067", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void RequirePublicMethods (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
Type[] types)
{
RequirePublicFields (types);
}

[ExpectedWarning ("IL2098")]
static void RequirePublicFields (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)]
Type[] types)
{
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void TestTypeArray ()
{
var types = GetTypeArray ();
RequirePublicMethods (types);
}

static unsafe Type* GetTypePtr () => throw null;

[ExpectedWarning ("IL2098")]
[UnexpectedWarning ("IL2067", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static unsafe void RequirePublicMethods (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
Type* typePtr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understand what's going on here: we do not consider pointers of Type to be interesting, right? Or is #101211 saying that we shouldn't be doing that, but we are?

Copy link
Member Author

@sbomer sbomer Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type* should not be considered for dataflow analysis. All the tools correctly report IL2098 due to invalid annotations on uninteresting Type*.

#101211 is about the analyzer still considering Type* in dataflow analysis, and incorrectly reporting IL2067 about annotations on typePtr mismatching the requirements of RequirePublicFields. Basically the IsTypeInterestingForDataflow check isn't wired up correctly in the analyzer, so it only impacts the IL2098 warnings, not the dataflow warnings.

{
RequirePublicFields (typePtr);
}

[ExpectedWarning ("IL2098")]
static unsafe void RequirePublicFields (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)]
Type* typePtr)
{
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static unsafe void TestTypePointer ()
{
var typePtr = GetTypePtr ();
RequirePublicMethods (typePtr);
}

static T GetTConstrainedToType<T> () where T : Type => throw null;

[ExpectedWarning ("IL2098")]
[UnexpectedWarning ("IL2067", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void RequirePublicMethods<T> (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
T t) where T : Type
{
RequirePublicFields (t);
}

[ExpectedWarning ("IL2098")]
static void RequirePublicFields<T> (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)]
T t) where T : Type
{
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void TestTypeGenericParameter ()
{
var t = GetTConstrainedToType<Type> ();
RequirePublicMethods<Type> (t);
}

static ref string GetStringRef () => throw null;

[ExpectedWarning ("IL2098")]
[UnexpectedWarning ("IL2067", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void RequirePublicMethods (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
ref string stringRef)
{
RequirePublicFields (ref stringRef);
}

[ExpectedWarning ("IL2098")]
static void RequirePublicFields (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)]
ref string stringRef)
{
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void TestStringRef ()
{
var stringRef = GetStringRef ();
RequirePublicMethods (ref stringRef);
}

public static void Test () {
TestUnsupportedType ();
TestTypeArray ();
TestTypePointer ();
TestTypeGenericParameter ();
TestStringRef ();
}
}

class AnnotationOnByRefParameter
{
static ref Type GetTypeRef () => throw null;

[ExpectedWarning ("IL2067")]
[ExpectedWarning ("IL2067", Tool.NativeAot | Tool.Trimmer, "https://github.com/dotnet/runtime/issues/101734")]
static void RequirePublicMethods (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
ref Type typeRef)
{
RequirePublicFields (ref typeRef);
}

static void RequirePublicFields (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)]
ref Type typeRef)
{
}

[ExpectedWarning ("IL2062", Tool.NativeAot | Tool.Trimmer, "https://github.com/dotnet/runtime/issues/101734")]
[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101734")]
static void TestTypeRef ()
{
var typeRef = GetTypeRef ();
RequirePublicMethods (ref typeRef);
}

public static void Test ()
{
TestTypeRef ();
}
}

class WriteCapturedParameter
Expand Down
Loading
Loading