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.

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

Analyze recursive interfaces in illink #99566

Closed
wants to merge 46 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
8454427
Refactor OverrideInformation to include interface implementing type
jtschuster Feb 11, 2024
54c326d
Add NotNullWhenAttribute for InterfaceImplementor
jtschuster Feb 11, 2024
6bfc58a
Don't allow null interfaceImpl when base and override are interface m…
jtschuster Feb 11, 2024
ecc5d0c
Add InterfaceType to InterfaceImplementor
jtschuster Feb 11, 2024
310de30
Make new members internal for compat warnings
jtschuster Feb 13, 2024
77195a3
Make InterfaceImplementor internal for API compat
jtschuster Feb 14, 2024
a73cc1f
Make TypeMapInfo internal for API Compat
jtschuster Feb 14, 2024
76bc255
Add InterfaceImplementor to suppressions.xml
jtschuster Feb 14, 2024
11577fa
Make types public again
jtschuster Feb 14, 2024
5fb3256
Use correct InterfaceImplementor in FindAndAddDims
jtschuster Feb 15, 2024
3b4a69d
InterfaceImplementor doesn't need to directly implement the interface
jtschuster Feb 15, 2024
cae111a
Add test case for interfaces without impls for whole hierarchy
jtschuster Feb 16, 2024
00d42b7
Undo source changes
jtschuster Feb 16, 2024
49b3464
Comment out all failing assertions
jtschuster Feb 16, 2024
14814ed
PR Feedback
jtschuster Feb 21, 2024
56f23e4
Don't need to resolve TypeDefinitions
jtschuster Feb 21, 2024
ccd0a8f
Merge branch 'main' of https://github.com/dotnet/runtime into CheckIm…
jtschuster Feb 22, 2024
1d1e883
Use correct InterfaceImpl
jtschuster Feb 22, 2024
7b218b6
Add argument to first call
jtschuster Feb 22, 2024
9addd27
Merge branch 'main' of https://github.com/dotnet/runtime into CheckIm…
jtschuster Feb 23, 2024
9b76b42
Merge branch 'main' into InterfacesWithoutImpls
jtschuster Feb 23, 2024
81ebc93
Look on base types and interfaces for interfaceImpl
jtschuster Feb 23, 2024
2828764
Merge branch 'main' of https://github.com/dotnet/runtime into Interfa…
jtschuster Feb 26, 2024
9a74024
Merge branch 'CheckImplementingType2' into InterfacesWithoutImpls
jtschuster Feb 26, 2024
6b2f99d
wip
jtschuster Feb 27, 2024
99164eb
wip
jtschuster Feb 28, 2024
e8c9319
Merge branch 'main' of https://github.com/dotnet/runtime into Analyze…
jtschuster Feb 29, 2024
295c099
wip
jtschuster Feb 29, 2024
40aa061
Merge branch 'main' of https://github.com/dotnet/runtime into Analyze…
jtschuster Feb 29, 2024
7189da5
Merge branch 'main' of https://github.com/dotnet/runtime into Analyze…
jtschuster Mar 1, 2024
8219095
Merge branch 'main' of https://github.com/dotnet/runtime into Analyze…
jtschuster Mar 4, 2024
a3a16df
Passing tests
jtschuster Mar 4, 2024
01d6f9c
wip
jtschuster Mar 5, 2024
8996d54
Passing tests
jtschuster Mar 6, 2024
6c51750
Make InterfaceImpl iterator a struct and unify resolved/unresolved if…
jtschuster Mar 6, 2024
d4f4ae8
Rename tests, reorder code, add DictionaryExtensions
jtschuster Mar 11, 2024
fafdda8
Clean up and add comments
jtschuster Mar 11, 2024
dd1feee
Rename tests, format code, add a few comments
jtschuster Mar 11, 2024
fae5bbd
Merge branch 'main' of https://github.com/dotnet/runtime into Analyze…
jtschuster Mar 11, 2024
cbab8b5
Remove some unnecessary changes, add 'HasExplicitImplementation' prop
jtschuster Mar 11, 2024
8b20918
Move property definition closer to use and make private
jtschuster Mar 11, 2024
999dc7d
Remove redundant test
jtschuster Mar 18, 2024
2b37c54
Merge branch 'main' into AnalyzeRecursiveInterfaces
jtschuster Mar 18, 2024
9aa7554
Add annotations for IReflection to ProjectingType and its hierarchy
jtschuster Mar 19, 2024
1fb7a4d
Merge branch 'AnalyzeRecursiveInterfaces' of https://github.com/jtsch…
jtschuster Mar 19, 2024
cfc1189
ifdef out DynamicallyAccessedMembers
jtschuster Mar 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
return IsAssignableFrom(objectType);
}

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties)]

Check failure on line 47 in src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs

View check run for this annotation

Azure Pipelines / runtime-dev-innerloop (Build linux-x64 debug Libraries_AllConfigurations)

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs#L47

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs(47,10): error CS0246: (NETCORE_ENGINEERING_TELEMETRY=Build) The type or namespace name 'DynamicallyAccessedMembersAttribute' could not be found (are you missing a using directive or an assembly reference?)

Check failure on line 47 in src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs

View check run for this annotation

Azure Pipelines / runtime-dev-innerloop (Build linux-x64 debug Libraries_AllConfigurations)

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs#L47

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs(47,10): error CS0246: (NETCORE_ENGINEERING_TELEMETRY=Build) The type or namespace name 'DynamicallyAccessedMembers' could not be found (are you missing a using directive or an assembly reference?)

Check failure on line 47 in src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs

View check run for this annotation

Azure Pipelines / runtime-dev-innerloop (Build linux-x64 debug Libraries_AllConfigurations)

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs#L47

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs(47,37): error CS0103: (NETCORE_ENGINEERING_TELEMETRY=Build) The name 'DynamicallyAccessedMemberTypes' does not exist in the current context

Check failure on line 47 in src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs

View check run for this annotation

Azure Pipelines / runtime-dev-innerloop (Build linux-x64 debug Libraries_AllConfigurations)

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs#L47

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs(47,87): error CS0103: (NETCORE_ENGINEERING_TELEMETRY=Build) The name 'DynamicallyAccessedMemberTypes' does not exist in the current context

Check failure on line 47 in src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs

View check run for this annotation

Azure Pipelines / runtime-dev-innerloop

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs#L47

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs(47,10): error CS0246: (NETCORE_ENGINEERING_TELEMETRY=Build) The type or namespace name 'DynamicallyAccessedMembersAttribute' could not be found (are you missing a using directive or an assembly reference?)

Check failure on line 47 in src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs

View check run for this annotation

Azure Pipelines / runtime-dev-innerloop

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs#L47

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs(47,10): error CS0246: (NETCORE_ENGINEERING_TELEMETRY=Build) The type or namespace name 'DynamicallyAccessedMembers' could not be found (are you missing a using directive or an assembly reference?)

Check failure on line 47 in src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs

View check run for this annotation

Azure Pipelines / runtime-dev-innerloop

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs#L47

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs(47,37): error CS0103: (NETCORE_ENGINEERING_TELEMETRY=Build) The name 'DynamicallyAccessedMemberTypes' does not exist in the current context

Check failure on line 47 in src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs

View check run for this annotation

Azure Pipelines / runtime-dev-innerloop

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs#L47

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Custom/CustomType.cs(47,87): error CS0103: (NETCORE_ENGINEERING_TELEMETRY=Build) The name 'DynamicallyAccessedMemberTypes' does not exist in the current context
public override PropertyInfo[] GetProperties(BindingFlags bindingAttr)
{
// list of properties on this type according to the underlying ReflectionContext
Expand Down Expand Up @@ -139,6 +140,7 @@
return binder.SelectProperty(bindingAttr, matchingProperties.ToArray(), returnType, types, modifiers);
}

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods)]
public override MethodInfo[] GetMethods(BindingFlags bindingAttr)
{
// list of methods on this type according to the underlying ReflectionContext
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -325,11 +325,17 @@ public override EventInfo[] GetEvents(BindingFlags bindingAttr)
return _typeInfo.GetEvents(bindingAttr);
}

#if NET8_0_OR_GREATER
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.NonPublicFields)]
#endif
public override FieldInfo? GetField(string name, BindingFlags bindingAttr)
{
return _typeInfo.GetField(name, bindingAttr);
}

#if NET8_0_OR_GREATER
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.NonPublicFields)]
#endif
public override FieldInfo[] GetFields(BindingFlags bindingAttr)
{
return _typeInfo.GetFields(bindingAttr);
Expand All @@ -345,6 +351,14 @@ public override Type[] GetInterfaces()
return _typeInfo.GetInterfaces();
}

#if NET8_0_OR_GREATER
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.NonPublicFields |
DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods |
DynamicallyAccessedMemberTypes.PublicEvents | DynamicallyAccessedMemberTypes.NonPublicEvents |
DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties |
DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors |
DynamicallyAccessedMemberTypes.PublicNestedTypes | DynamicallyAccessedMemberTypes.NonPublicNestedTypes)]
#endif
public override MemberInfo[] GetMembers(BindingFlags bindingAttr)
{
return _typeInfo.GetMembers(bindingAttr);
Expand All @@ -358,6 +372,9 @@ public override MemberInfo[] GetMembers(BindingFlags bindingAttr)
_typeInfo.GetMethod(name, bindingAttr, binder, callConvention, types, modifiers);
}

#if NET8_0_OR_GREATER
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods)]
#endif
public override MethodInfo[] GetMethods(BindingFlags bindingAttr)
{
return _typeInfo.GetMethods(bindingAttr);
Expand All @@ -373,6 +390,9 @@ public override Type[] GetNestedTypes(BindingFlags bindingAttr)
return _typeInfo.GetNestedTypes(bindingAttr);
}

#if NET8_0_OR_GREATER
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties)]
#endif
public override PropertyInfo[] GetProperties(BindingFlags bindingAttr)
{
return _typeInfo.GetProperties(bindingAttr);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,11 +206,13 @@
return _projector.Project(base.GetEvents(bindingAttr), _projector.ProjectEvent);
}

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.NonPublicFields)]
public override FieldInfo? GetField(string name, BindingFlags bindingAttr)
{
return _projector.ProjectField(base.GetField(name, bindingAttr));
}

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.NonPublicFields)]
public override FieldInfo[] GetFields(BindingFlags bindingAttr)
{
return _projector.Project(base.GetFields(bindingAttr), _projector.ProjectField);
Expand All @@ -226,6 +228,12 @@
return _projector.Project(base.GetInterfaces(), _projector.ProjectType);
}

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.NonPublicFields |
DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods |
DynamicallyAccessedMemberTypes.PublicEvents | DynamicallyAccessedMemberTypes.NonPublicEvents |
DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties |
DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors |
DynamicallyAccessedMemberTypes.PublicNestedTypes | DynamicallyAccessedMemberTypes.NonPublicNestedTypes)]
public override MemberInfo[] GetMembers(BindingFlags bindingAttr)
{
MethodInfo[] methods = GetMethods(bindingAttr);
Expand Down Expand Up @@ -264,6 +272,7 @@
return _projector.ProjectMethod(base.GetMethodImpl(name, bindingAttr, binder, callConvention, types, modifiers));
}

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods)]
public override MethodInfo[] GetMethods(BindingFlags bindingAttr)
{
return _projector.Project(base.GetMethods(bindingAttr), _projector.ProjectMethod);
Expand All @@ -279,6 +288,7 @@
return _projector.Project(base.GetNestedTypes(bindingAttr), _projector.ProjectType);
}

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties)]

Check failure on line 291 in src/libraries/System.Reflection.Context/src/System/Reflection/Context/Projection/ProjectingType.cs

View check run for this annotation

Azure Pipelines / runtime-dev-innerloop (Build linux-x64 debug Libraries_AllConfigurations)

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Projection/ProjectingType.cs#L291

src/libraries/System.Reflection.Context/src/System/Reflection/Context/Projection/ProjectingType.cs(291,10): error CS0246: (NETCORE_ENGINEERING_TELEMETRY=Build) The type or namespace name 'DynamicallyAccessedMembersAttribute' could not be found (are you missing a using directive or an assembly reference?)
public override PropertyInfo[] GetProperties(BindingFlags bindingAttr)
{
return _projector.Project(base.GetProperties(bindingAttr), _projector.ProjectProperty);
Expand Down
80 changes: 19 additions & 61 deletions src/tools/illink/src/linker/Linker.Steps/MarkStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Reflection.Runtime.TypeParsing;
using System.Runtime.CompilerServices;
using System.Text.RegularExpressions;
using ILLink.Shared;
using ILLink.Shared.TrimAnalysis;
Expand Down Expand Up @@ -779,53 +778,16 @@ void MarkMethodIfNeededByBaseMethod (MethodDefinition method)
}
}

/// <summary>
/// Returns true if <paramref name="type"/> implements <paramref name="interfaceType"/> and the interface implementation is marked,
/// or if any marked interface implementations on <paramref name="type"/> are interfaces that implement <paramref name="interfaceType"/> and that interface implementation is marked
/// </summary>
bool IsInterfaceImplementationMarkedRecursively (TypeDefinition type, TypeDefinition interfaceType)
{
if (type.HasInterfaces) {
foreach (var intf in type.Interfaces) {
TypeDefinition? resolvedInterface = Context.Resolve (intf.InterfaceType);
if (resolvedInterface == null)
continue;

if (Annotations.IsMarked (intf) && RequiresInterfaceRecursively (resolvedInterface, interfaceType))
return true;
}
}

return false;
}

bool RequiresInterfaceRecursively (TypeDefinition typeToExamine, TypeDefinition interfaceType)
{
if (typeToExamine == interfaceType)
return true;

if (typeToExamine.HasInterfaces) {
foreach (var iface in typeToExamine.Interfaces) {
var resolved = Context.TryResolve (iface.InterfaceType);
if (resolved == null)
continue;

if (RequiresInterfaceRecursively (resolved, interfaceType))
return true;
}
}

return false;
}

void ProcessDefaultImplementation (OverrideInformation ov)
{
Debug.Assert (ov.IsOverrideOfInterfaceMember);
if ((!ov.Override.IsStatic && !Annotations.IsInstantiated (ov.InterfaceImplementor.Implementor))
|| ov.Override.IsStatic && !Annotations.IsRelevantToVariantCasting (ov.InterfaceImplementor.Implementor))
return;

MarkInterfaceImplementation (ov.InterfaceImplementor.InterfaceImplementation);
foreach (var ifaceImpl in ov.InterfaceImplementor.MostDirectInterfaceImplementationPath) {
MarkInterfaceImplementation (ifaceImpl);
}
}

void MarkMarshalSpec (IMarshalInfoProvider spec, in DependencyInfo reason)
Expand Down Expand Up @@ -2450,37 +2412,35 @@ void MarkNamedProperty (TypeDefinition type, string property_name, in Dependency

void MarkInterfaceImplementations (TypeDefinition type)
{
if (!type.HasInterfaces)
return;

foreach (var iface in type.Interfaces) {
// Only mark interface implementations of interface types that have been marked.
// This enables stripping of interfaces that are never used
if (ShouldMarkInterfaceImplementation (type, iface))
MarkInterfaceImplementation (iface, new MessageOrigin (type));
foreach (var interfaceImplementor in Annotations.GetRecursiveInterfaces (type)) {
if (ShouldMarkInterfaceImplementation (interfaceImplementor)) {
foreach (InterfaceImplementation interfaceImpl in interfaceImplementor.MostDirectInterfaceImplementationPath) {
MarkInterfaceImplementation (interfaceImpl, new MessageOrigin (type));
}
}
}
}

protected virtual bool ShouldMarkInterfaceImplementation (TypeDefinition type, InterfaceImplementation iface)
protected virtual bool ShouldMarkInterfaceImplementation (InterfaceImplementor interfaceImplementor)
{
if (Annotations.IsMarked (iface))
if (interfaceImplementor.IsMostDirectImplementationMarked (Annotations))
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd make sense to separate this out into another change to make it clear what that impact of this optimization is on the test cases.

Copy link
Member

Choose a reason for hiding this comment

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

In fact, would it be possible to start with a simpler change that does something like IsInterfaceImplementationMarkedRecursively here, without tracking the instantiated interface types?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I could probably start with that.

return false;

if (!Context.IsOptimizationEnabled (CodeOptimizations.UnusedInterfaces, type))
if (!Context.IsOptimizationEnabled (CodeOptimizations.UnusedInterfaces, interfaceImplementor.Implementor))
return true;

if (Context.Resolve (iface.InterfaceType) is not TypeDefinition resolvedInterfaceType)
if (interfaceImplementor.InterfaceType is null)
return false;

if (Annotations.IsMarked (resolvedInterfaceType))
if (Annotations.IsMarked (interfaceImplementor.InterfaceType))
return true;

// It's hard to know if a com or windows runtime interface will be needed from managed code alone,
// so as a precaution we will mark these interfaces once the type is instantiated
if (resolvedInterfaceType.IsImport || resolvedInterfaceType.IsWindowsRuntime)
if (interfaceImplementor.InterfaceType.IsImport || interfaceImplementor.InterfaceType.IsWindowsRuntime)
return true;

return IsFullyPreserved (type);
return IsFullyPreserved (interfaceImplementor.Implementor);
}

void MarkGenericParameterProvider (IGenericParameterProvider provider)
Expand Down Expand Up @@ -2560,12 +2520,10 @@ bool IsInterfaceImplementationMethodNeededByTypeDueToInterface (OverrideInformat
if (Annotations.IsMarked (method))
return false;

// If the interface implementation is not marked, do not mark the implementation method
// A type that doesn't implement the interface isn't required to have methods that implement the interface.
InterfaceImplementation? iface = overrideInformation.InterfaceImplementor.InterfaceImplementation;
if (!((iface is not null && Annotations.IsMarked (iface))
|| IsInterfaceImplementationMarkedRecursively (method.DeclaringType, @base.DeclaringType)))
// If there is no chain of interface implementations that make the type implement the interface, the method isn't required
if (!overrideInformation.InterfaceImplementor.IsMarked(Annotations)) {
return false;
}

// If the interface method is not marked and the interface doesn't come from a preserved scope, do not mark the implementation method
// Unmarked interface methods from link assemblies will be removed so the implementing method does not need to be kept.
Expand Down
3 changes: 3 additions & 0 deletions src/tools/illink/src/linker/Linker/Annotations.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
Expand Down Expand Up @@ -717,5 +718,7 @@ public void EnqueueVirtualMethod (MethodDefinition method)
if (FlowAnnotations.RequiresVirtualMethodDataFlowAnalysis (method) || HasLinkerAttribute<RequiresUnreferencedCodeAttribute> (method))
VirtualMethodsWithAnnotationsToValidate.Add (method);
}

internal ImmutableArray<InterfaceImplementor> GetRecursiveInterfaces (TypeDefinition type) => TypeMapInfo.GetRecursiveInterfaces (type);
}
}