It would be nice to add tests

@mrvoorhe I added two tests, including one to validate the current ability to remove interfaceimpls on uninstantiated types.

By the way - if this behavior is controversial, I'm happy to make a larger change that will preserve interfaceimpls in more cases - however I'd like to get this PR in as soon as possible since we are trying to enable it in coreclr/corefx.

@marek-safar PTAL.

I'm fine with it. I think someone is going to be a little confusing that uninstantiated types still get their interfaces swept at some point. But at the same time the optimizations made for uninstantiated types are in some ways their own separate feature.

The mono job failure doesn't appear related to the change:

16:09:01 Test Run Summary
16:09:01   Overall result: Warning
16:09:01   Test Count: 564, Passed: 540, Failed: 0, Warnings: 0, Inconclusive: 0, Skipped: 24
16:09:01     Skipped Tests - Ignored: 24, Explicit: 0, Other: 0
16:09:01   Start time: 2019-07-09 16:02:07Z
16:09:01     End time: 2019-07-09 16:09:01Z
16:09:01     Duration: 413.680 seconds

16:09:04   Could not unmount /mnt/jenkins/workspace/test-linker-pull-request, some programs might
16:09:04   still be using files in /proc (klogd?).
16:09:04   Please check and kill these processes manually
16:09:04   so that I can unmount /mnt/jenkins/workspace/test-linker-pull-request.  Last umount error was:
16:09:04 umount: /mnt/jenkins/buildplace/106009//mnt/jenkins/workspace/test-linker-pull-request: target is busy
16:09:04         (In some cases useful info about processes that
16:09:04          use the device is found by lsof(8) or fuser(1).)

@marek-safar here's an example from System.Collections. As I mentioned, corefx links individual assemblies, so the interface type doesn't get marked if it's from a referenced (not copied) assembly. The CanDisableInterfaceSweeping test illustrates this in a simpler case where the unmarked interface type is in the same assembly.

It might be actually bug and the option will not be needed.

+1. I agree that this looks like a bug and the option should not be really needed if the bug is fixed.

Would it be fine to sweep the implementation of an interface only if it's private? This seems to be removing them for public types implementing public interfaces that aren't used internally, which definitely sounds like a bug. For instance, in System.Private.CoreLib.dll, ThreadPoolBoundHandle is stripped of its implementation of IDisposable; this definitely shouldn't happen because IDisposable is public.

ThreadPoolBoundHandle is a bit of a special case - the interfaceimpl is removed because ThreadPoolBoundHandle isn't instantiated on unix (the linker currently only keeps interfaceimpls on types that it thinks have been instantiated).

I generally like the suggestion to treat implementations of public interfaces as public surface area - that would be the right thing to do for corefx. If we treat the current behavior as a bug, we need to consider what the desired behavior is, since the feature was originally designed to remove interfaceimpls when the interface type is not used (which is the case in the corefx setup). I'm working on this in #657.

Would it be fine to sweep the implementation of an interface only if it's private?

No. We want public interfaces removed if they are not used. If you wanted all public interfaces in an assembly to survive you would pass the assembly to the linker with -a (I think) so that the assembly is then processed by ResolveFromAssemblyStep which will cause all public types to be marked.

@sbomer I don't entirely follow what the issue is. The link you gave doesn't have any context so it's hard to know what should happen. Can you elaborate? or can you write a linker test case that produces invalid IL?

@mrvoorhe my apologies - hopefully I can clear up the context: Corefx is using the linker (with -r) to link individual libraries. They want to keep all public surface area of these libraries, including interface implementations of interfaces that are defined in other assemblies (referenced via -reference). The testcase in my other PR does the same (https://github.com/mono/linker/pull/657/files#diff-6992deef91516a2339611872e1d345cc).

-a/-r will mark public types including interfaces, but if like above the interface is from an assembly that's not rooted, the interface type itself isn't marked. The result is that the interfaceimpl can be removed (if it's not actually used by the -r assembly).

I think @lpereira's suggestion makes sense in that context (though in other circumstances like linking an app, we might still want to remove impls of public interfaces when they're not used).

In any case, this PR solved the problem by turning off the optimization. As @marek-safar and @jkotas suggested, from the corefx point of view this is a bug, so we might instead want to fix the behavior of -r to keep these cases, which is what I'm doing in #657.

@sbomer Thanks for clarifying. I was starting to piece it together after I saw the other PR.

@marek-safar I put the option to disable interface sweeping under a define like we discussed (there still needs to be a command-line option since we will flow the same build of the linker to both the SDK and coreclr/corefx), while we consider other approaches. @tlakollo is validating the diff with this change to ensure it will unblock corefx/coreclr's linker update. PTAL.

@marek-safar PTAL