Separate interface method trimming logic and update for static interface methods #2868

jtschuster · 2022-06-27T21:40:59Z

~~The linker expected all static interface methods to be abstract. This fix makes sure static virtual interface methods are also marked.~~

~~Also updates the linker to consider all unresolved assemblies to be in a Preserve/Ignore scope.~~

~~I haven't been able to get static virtual interface methods to compile in tests yet, but will add tests once I can.~~

~~A test has been added to replicate the behavior that caused the issue, as well as the Override checking for tests. More static virtual interface tests will be added in #2859~~

Apologies for the massive diff, most of it is removing tests written in IL that are covered in C# tests now.

This PR now addresses marking of all static interface methods encompassing the changes from #2859, and also updates the way that all interface methods are marked. Whether or not we mark an interface method due to it's base method is now separated from marking other virtual methods and the marking is postponed to ProcessMarkedTypesWithInterface. In ProcessMarkedTypesWithInterfaces, interface implementations are marked, and methods that implement a marked/implemented interface are marked.

In addition, the assembly root mode for each assembly is tracked through LinkContext.AssemblyRootModes to allow the linker to make sure all interface implementations are marked and the methods that implement the interface are kept in library mode.

OverrideInformation was also modified to contain the logic for finding the corresponding interface implementation for a base/override pair.

The VarianceBasic.il test was decompiled to C#.

sbomer

The change looks good to me, but I am hoping we can get tests in the same PR. Happy to look at tests together to see if we can get them to compile.

MichalStrehovsky · 2022-06-28T23:30:15Z

src/linker/Linker.Steps/MarkStep.cs

@@ -2321,7 +2321,7 @@ bool IsMethodNeededByTypeDueToPreservedScope (MethodDefinition method)
 				// If the type is marked, we need to keep overrides of abstract members defined in assemblies
 				// that are copied.  However, if the base method is virtual, then we don't need to keep the override
 				// until the type could be instantiated
-				if (!@base.IsAbstract)
+				if (!(@base.IsAbstract || (@base.IsStatic && @base.IsVirtual)))


Not commenting on this fix because I don't have enough context anymore, but the comment 7 lines above:

// Just because the type is marked does not mean we need interface methods. // if the type is never instantiated, interfaces will be removed - but only if the optimization is enabled

indicates that either the comment is incorrect, or the logic in the foreach loop is incorrect for static methods - instantiating a type has no effect on whether the static methods are usable. We should probably not be in the foreach.

I think the comment is incorrect -- "instantiated" here probably means substituted into a constrained generic.

I agree with Michal on this one - the comment and code don't match my expectation.

@vitek-karas and I discussed this and came to an understanding of what this method is meant to do. It has been updated with a matrix of when to mark a method or leave it unmarked. This method also assumes that the type is not instantiated.

tannergooding · 2022-06-29T16:37:26Z

I haven't been able to get static virtual interface methods to compile in tests yet, but will add tests once I can.

Just noting that static virtuals in interfaces should be available in .NET 7 Preview 5 (the latest public SDK).

Alternatively, you could pull in the same toolset compiler that dotnet/runtime is using.

wip

agocke · 2022-07-01T17:50:37Z

src/linker/Linker.Steps/MarkStep.cs

@@ -2321,7 +2321,7 @@ bool IsMethodNeededByTypeDueToPreservedScope (MethodDefinition method)
 				// If the type is marked, we need to keep overrides of abstract members defined in assemblies
 				// that are copied.  However, if the base method is virtual, then we don't need to keep the override
 				// until the type could be instantiated
-				if (!@base.IsAbstract)
+				if (!(@base.IsAbstract || (@base.IsStatic && @base.IsVirtual)))


How could the base method not be static, given that the check above on line 2308 returns false if the current method isn't static?

I don't understand this comment - virtual instance method will pass all of the checks above and get here. This if here is:

Move onto potential marking for abstract or static virtual methods

Continue/Skip for instance virtual methods (as per the comments above).

Ah you're right, I misread, this is IsStatic && IsVirtual, above is IsStatic || IsVirtual

agocke · 2022-07-01T17:54:21Z

src/linker/Linker.Steps/MarkStep.cs

@@ -2321,7 +2321,7 @@ bool IsMethodNeededByTypeDueToPreservedScope (MethodDefinition method)
 				// If the type is marked, we need to keep overrides of abstract members defined in assemblies
 				// that are copied.  However, if the base method is virtual, then we don't need to keep the override
 				// until the type could be instantiated
-				if (!@base.IsAbstract)
+				if (!(@base.IsAbstract || (@base.IsStatic && @base.IsVirtual)))


I think the comment is incorrect -- "instantiated" here probably means substituted into a constrained generic.

agocke · 2022-07-01T17:58:16Z

src/linker/Linker.Steps/MarkStep.cs

@@ -2321,7 +2321,7 @@ bool IsMethodNeededByTypeDueToPreservedScope (MethodDefinition method)
 				// If the type is marked, we need to keep overrides of abstract members defined in assemblies
 				// that are copied.  However, if the base method is virtual, then we don't need to keep the override


Comment is now incorrect. I believe the rules for when we keep interface implementations for abstract or virtual members are identical. Is there a case where we might not need to keep an implementation for a virtual member when we would keep it for an abstract one?

Not sure if it applies here but linker must keep overrides of abstract base members if the type is kept - otherwise it's invalid IL (can't have a type which is not abstract and doesn't implement all abstract members of its base type). So non-instantiated types may keep some of their virtual members due to this (although I think linker will generate a throw body for such methods).

Well, these are static so I don't think the instantiated rule applies -- something like "substituted for a type argument" instead?

Do we wait until we see the type as substituted for a type argument? What about MakeGenericType/MakeGenericMethod at runtime?

We should treat reflected-over types as possibly substituted for type arguments. I think the IsRelevantToVariantCasting check covers this and the case where it's accessed via reflection.

agocke · 2022-07-01T17:59:16Z

...slynAnalyzer.Tests.TestCaseGenerator/Inheritance.Interfaces.StaticInterfaceMethodsTests.g.cs

+		[Fact]
+		public Task StaticVirtualInterfaceMethodsLibrary ()
+		{
+			return RunTest (allowMissingWarnings: true);


Probably shouldn't allow missing warnings. Move this into the proper test suite?

test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs

agocke · 2022-07-01T18:00:42Z

test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs

 						$"Method {linked} overrides method {overriddenMethod}, but {linked.DeclaringType} does not implement interface {overriddenMethod.DeclaringType}");
 				} else {
 					TypeReference baseType = linked.DeclaringType;
 					TypeReference overriddenType = overriddenMethod.DeclaringType;
 					while (baseType is not null) {
-						if (baseType.Equals (overriddenType))
+						if (baseType.FullName == overriddenType.FullName)


When could this be different?

I honestly don't remember what issue caused me to need to change this, but I think it was related to generics.

vitek-karas · 2022-07-04T17:24:59Z

src/linker/Linker.Steps/MarkStep.cs

+				if (!@base.DeclaringType.IsInterface && !@base.IsStatic && @base.IsVirtual && !@base.IsAbstract)
+					continue;


Do we need the @base.IsVirtual? I would think it's basically implied (since it's a base method on a class).

vitek-karas · 2022-07-04T17:25:18Z

src/linker/Linker.Steps/MarkStep.cs

+				// A virtual instance method with a default impl on the base doesn't need to be kept
+				if (!@base.DeclaringType.IsInterface && !@base.IsStatic && @base.IsVirtual && !@base.IsAbstract)
+					continue;
+				// base/iface | inst/stat  | virt/abst | iface has static | UnusedIFaceOpt | needed


Nit: Please separate the various cases with a newline.

vitek-karas · 2022-07-04T17:27:33Z

src/linker/Linker.Steps/MarkStep.cs

-				// that are copied.  However, if the base method is virtual, then we don't need to keep the override
-				// until the type could be instantiated
-				if (!@base.IsAbstract)
+				// ----------------------------------------------------------------------------------


Nit: I would preface this with comment which states the base is always virtual/abstract (since it's a "base" method for an override). I know it's kind of obvious, but I was struggling with it reading the table below.

vitek-karas · 2022-07-04T17:28:44Z

src/linker/Linker.Steps/MarkStep.cs

+				// iface      | instance   | abstract  | yes              | yes            | yes
+				// iface      | instance   | *         | no               | yes            | no
+				// iface      | static     | *         | always yes       | yes            | yes
+				// iface      | *          | *         | *                | no             | yes


The * values are a bit confusing. I guess it means "both", but it's hard to tell. Since each column is effectively a boolean mean just use both, or either or something like that.

vitek-karas · 2022-07-04T17:30:16Z

src/linker/Linker.Steps/MarkStep.cs

+				// iface      | *          | *         | *                | no             | yes
+
+
+				// base/iface | inst/stat  | virt/abst | iface has static | UnusedIFaceOpt | needed


vitek-karas · 2022-07-04T17:32:45Z

src/linker/Linker.Steps/MarkStep.cs

+				// Uninstantiated types will not need iface instance methods unless they are abstract
+				// Abstract iface methods must be implemented for valid IL, but not virtuals


This doesn't sound right.

Uninstantiated types may not even implement the interface

Typically interface methods are abstract, but if we remove the interface implementation the implementing methods are not needed (since they won't be overrides).

If no test failed on this then we're missing a test... and should add it.

Short example:

// Not kept interface IFace // In preserved scope { void Method(); // This is abstract effectively } [Kept] class MyType : IFace { // Not kept void Method() { } } void Test() { typeof(MyType); // The only use of MyType }

Thanks for pointing this out, the comment is very wrong and doesn't properly communicate what the code is meant to do.

vitek-karas · 2022-07-04T19:25:38Z

src/linker/Linker.Steps/MarkStep.cs

+				// Instance methods of interfaces without static methods don't need marking
+				// It is still very possible that the interface could be removed


This is unnecessarily conservative. For example:

interface IFace // In preserved scope { void Method(); // Abstract instance static void IfNotNull(IFace f) { if (f != null) f.Method(); } } class MyType : IFace { void Method() { } } void Test() { typeof(MyType); // The only use of the type }

I think in this case it's OK to remove the implementation of IFace on MyType and thus remove MyType.Method.

I think the interesting case is if an interface has a static virtual/abstract method, then we can't really remove implementation of such interface (in theory we could, but we would have to start tracking generic instantiations and constraints, which currently we don't).

Might be worth adding a helper like CanRemoveInterfaceImplementationForUninstantiatedType(TypeDefinition iface) which would return true for interfaces for which we could consider removing their implementation (if the type is not instantiated). It would return false if the interface has static virtual/abstract method I think.

The above example with non-virtual static on an interface: I think we'll need to figure out if it's OK to remove that (I could be wrong above). Basically what happens if I do this:

interface IFace { static void Hello () { Console.WriteLine("Hello"); } } class MyType : IFace { } class HelloThere<T> where T : IFace { public static void Greet() { T.Hello(); } } public static void Test() { HelloThere<MyType>.Greet(); }

It seems this doesn't compile, so if this is prohibited then the above holds true.

Actually I just found out that if I change it like this:

interface IFace { static void Hello () { Console.WriteLine("Hello"); } } class MyType : IFace { } class HelloThere<T> where T : IFace { public static void Greet() { // Removed the call here } } public static void Test() { HelloThere<MyType>.Greet(); }

It now compiles, but if I remove IFace implementation from MyType it doesn't compile anymore since MyType now doesn't fulfill the generic constraint. I wonder if we have issue in linker around this at all.

I played with this some more... linker is clever - if it sees interface in a generic constraint then such interface implementation is always preserved on all types (regardless of instantiation).
It's called "relevant to variant casting" (honestly no idea why, but that's what it does).

So I think my above statement holds true - presence of a simple non-virtual static method on an interface doesn't mean we can't remove the interface implementation from a type.
Given the above about "relevant to variant casting" - we will never remove static interface implementations (since we would see them in generic constraints). Which fits what we discussed.

In a way we may not need any checks for the static interface cases - due to the fact that linker will keep implementions of ifaces which are used for generic instantiation (and that is the only way static interfaces can be used really - other than reflection). We could keep the code here - as it doesn't hurt, but I would honestly expect it to work even without it (the iface implementation should be kept and since the iface is fully preserved I would expect that all implementation methods on the class are also kept - since they need to be).

docs/methods-kept-by-interface.md

docs/optimizations.md

src/linker/Linker.Steps/MarkStep.cs

Co-authored-by: Sven Boemer <sbomer@gmail.com>

…mentations

...ce.Interfaces/StaticInterfaceMethods/StaticVirtualInterfaceMethodsInPreservedScopeLibrary.cs

…nker into markStaticVirtuals

sbomer

Looking really good, I think this is almost ready!

src/linker/Linker.Steps/MarkStep.cs

docs/methods-kept-by-interface.md

src/linker/Linker.Steps/MarkStep.cs

Co-authored-by: Sven Boemer <sbomer@gmail.com>

…ticVirtuals

src/linker/Linker.Steps/MarkStep.cs

sbomer

The code looks good to me, I just think the doc needs to be updated.

docs/methods-kept-by-interface.md

sbomer

LGTM, thanks a lot!

…ace methods (dotnet/linker#2868) Fixes dotnet/linker#2865 Also addresses marking of all static interface methods encompassing the changes from dotnet/linker#2859, and updates the way that all interface methods are marked. Whether or not we mark an interface method due to its base method is now separated from marking other virtual methods and the marking is postponed to ProcessMarkedTypesWithInterface. In ProcessMarkedTypesWithInterfaces, interface implementations are marked, and methods that implement a marked/implemented interface are marked. Tests for static interface methods have also been updated. Co-authored-by: Sven Boemer <sbomer@gmail.com> Commit migrated from dotnet/linker@118bdca

Mark static virtual interface methods

da56f3d

jtschuster requested a review from marek-safar as a code owner June 27, 2022 21:40

jtschuster requested review from vitek-karas, sbomer and agocke June 27, 2022 21:41

Undo IgnoreScope changes

869748e

sbomer reviewed Jun 28, 2022

View reviewed changes

MichalStrehovsky reviewed Jun 28, 2022

View reviewed changes

jtschuster added 2 commits June 29, 2022 12:04

Add tests for virtual statics in preserve scope

f4af27f

add references folder to test output for skip assemblies

80ddcd2

wip

jtschuster requested a review from sbomer June 29, 2022 19:55

jtschuster and others added 8 commits June 29, 2022 15:46

Remove implicit implementations

deba9dd

formatting

26fdc82

Undo cecil and sdk changes

d66f76c

Update StaticVirtualInterfaceMethodsLibrary.cs

9b9cfae

clean up

c9c7e88

formatting

8dfbdc9

formatting

9076e0c

formatting

32978f3

agocke reviewed Jul 1, 2022

View reviewed changes

jtschuster added 4 commits July 1, 2022 18:14

New PreserveScope logic

fb5254a

Better wording

f522641

Move matrix below preserveScope check

6a24f50

Run dotetn-format latest version

9a5fd73

vitek-karas reviewed Jul 4, 2022

View reviewed changes

jtschuster added 3 commits July 6, 2022 16:39

whip, switching machines

471af7b

Merge remote-tracking branch 'upstream/main' into wip

57e7ea9

Wip

e6e69a7

jtschuster marked this pull request as draft July 8, 2022 01:33

Add docs

e554231

sbomer reviewed Jul 16, 2022

View reviewed changes

jtschuster and others added 2 commits July 25, 2022 11:09

Update docs/optimizations.md

840b2a4

Co-authored-by: Sven Boemer <sbomer@gmail.com>

PR Feedback and test for virtual base type methods with default imple…

59a2cc3

…mentations

sbomer reviewed Jul 26, 2022

View reviewed changes

...ce.Interfaces/StaticInterfaceMethods/StaticVirtualInterfaceMethodsInPreservedScopeLibrary.cs Outdated Show resolved Hide resolved

jtschuster and others added 6 commits July 26, 2022 12:06

Remove check for unusedinterfaces optimization

2c6a512

Move interface PreservedScope logic to single function

e4ece71

Move interface PreservedScope logic to single function

12486ee

Merge branch 'markStaticVirtuals' of https://github.com/jtschuster/li…

fe409cb

…nker into markStaticVirtuals

Update MarkStep.cs

9fa904f

wip

0960f91

sbomer reviewed Jul 29, 2022

View reviewed changes

jtschuster and others added 6 commits July 29, 2022 12:05

Update src/linker/Linker.Steps/MarkStep.cs

513ec3a

Co-authored-by: Sven Boemer <sbomer@gmail.com>

Update docs/methods-kept-by-interface.md

5ad5c34

Co-authored-by: Sven Boemer <sbomer@gmail.com>

Update src/linker/Linker.Steps/MarkStep.cs

a20ef79

Co-authored-by: Sven Boemer <sbomer@gmail.com>

Merge remote-tracking branch 'origin/markStaticVirtuals' into markSta…

4a2a4dc

…ticVirtuals

PR Feedback

0d6cef7

Formatting and link PR to commented ExpectedWarning

8bd3d7c

sbomer reviewed Jul 29, 2022

View reviewed changes

src/linker/Linker.Steps/MarkStep.cs Outdated Show resolved Hide resolved

jtschuster added 4 commits August 1, 2022 11:17

Add test for base provides static interface method

b9ac22c

Remove additions from another branch

f9f4c38

Remove commented code

1d0bddc

formatting

4f1252d

jtschuster requested a review from sbomer August 1, 2022 23:00

Merge branch 'main' into markStaticVirtuals

47cb5da

sbomer reviewed Aug 2, 2022

View reviewed changes

docs/methods-kept-by-interface.md Outdated Show resolved Hide resolved

docs/methods-kept-by-interface.md Outdated Show resolved Hide resolved

Update docs

cc870cb

sbomer approved these changes Aug 3, 2022

View reviewed changes

jtschuster merged commit 118bdca into dotnet:main Aug 3, 2022

		@@ -2321,7 +2321,7 @@ bool IsMethodNeededByTypeDueToPreservedScope (MethodDefinition method)
		// If the type is marked, we need to keep overrides of abstract members defined in assemblies
		// that are copied. However, if the base method is virtual, then we don't need to keep the override

		if (!@base.DeclaringType.IsInterface && !@base.IsStatic && @base.IsVirtual && !@base.IsAbstract)
		continue;

		// iface \| * \| * \| * \| no \| yes


		// base/iface \| inst/stat \| virt/abst \| iface has static \| UnusedIFaceOpt \| needed

		// Uninstantiated types will not need iface instance methods unless they are abstract
		// Abstract iface methods must be implemented for valid IL, but not virtuals

		// Instance methods of interfaces without static methods don't need marking
		// It is still very possible that the interface could be removed

Separate interface method trimming logic and update for static interface methods #2868

Separate interface method trimming logic and update for static interface methods #2868

Conversation

jtschuster commented Jun 27, 2022 • edited Loading

sbomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding commented Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbomer left a comment

Choose a reason for hiding this comment

sbomer left a comment

Choose a reason for hiding this comment

sbomer left a comment

Choose a reason for hiding this comment

jtschuster commented Jun 27, 2022 •

edited

Loading

tannergooding commented Jun 29, 2022 •

edited

Loading