Remove InternalsVisibleTo and Microsoft.Internals from S.ComponentModel.Composition

[ViktorHofer]

I suggest to turn off white-space changes when reviewing this PR.

• Remove InternalsVisibleTo from S.ComponentModel.Composition and disable tests that require internals access. 1100 tests are now executed instead of the previous 1500.
• Remove remaining Microsoft.Internal helpers (was already cleaned up in other Composition assemblies) and keeping the ones that have a strong dependency but grouping them into the right namespaces.
• Remove dead code + code cleanup with C# 6 and 7 features applied

I disabled all the tests rely on internals. I either commented out code paths or excluded the files from the project but didn't delete those code paths. Somebody could try to re-enable some of the tests with reflection calls.

[ViktorHofer]

@MichalStrehovsky currently 300 tests are failing out of 1100 on uapaot. These are the unique PNSEs:

PlatformNotSupportedException : AssemblyName.GetAssemblyName() is not supported on this platform.
PlatformNotSupportedException : Operation is not supported on this platform.
PlatformNotSupportedException : System.Reflection.Emit is not supported on this platform.

I need to dig deeper what exactly the reason for the "Operation is not supported on this platform." is. Is the Assembly.GetAssemblyName() expected and is there a workaround for it?

EDIT: attaching the test log
comperrors.txt

[MichalStrehovsky]

Is the Assembly.GetAssemblyName() expected

This is the flavor of the API that cracks open the file with the specified name and computes an AssemblyName from it. We could implement this by factoring out a subset of System.Reflection.Metadata and carrying that around if need be, but but this is the kind of API that is usually only useful if the next thing the app will do is to try to Assembly.Load the IL.

I really wonder what S.CM does with that. Whatever it's doing probably won't be compatible with the single-file deployment that is coming to .NET Core.

[ViktorHofer]

OK. One of the operation is not supported messages is because it tries to access Assembly.GetExecutingAssembly().CodeBase. It feeds that string into the AssemblyCatalog which will attempt to load the assembly: https://source.dot.net/#System.ComponentModel.Composition/System/ComponentModel/Composition/Hosting/AssemblyCatalog.cs,79

EDIT:

I really wonder what S.CM does with that.

Also using the AssemblyCatalog to load it:

    ---- System.PlatformNotSupportedException : AssemblyName.GetAssemblyName() is not supported on this platform.
    Stack Trace:
         at System.ComponentModel.Composition.Hosting.AssemblyCatalog.LoadAssembly(String) + 0xc4
         at System.ComponentModel.Composition.Hosting.AssemblyCatalog..ctor(String) + 0x4e

[ViktorHofer]

The Reflection.Emit PNSEs are because of the MetadataViewGenerator which uses AssemblyBuilder: https://github.com/dotnet/corefx/blob/master/src/System.ComponentModel.Composition/src/System/ComponentModel/Composition/MetadataViewGenerator.cs#L85

[ViktorHofer]

Other NullReferenceException are because of these static reflection calls:

corefx/src/System.ComponentModel.Composition/src/System/ComponentModel/Composition/ReflectionModel/ExportfactoryCreator.cs

Line 12 in c06df8e

private static readonly MethodInfo _createStronglyTypedExportFactoryOfT = typeof(ExportFactoryCreator).GetMethod("CreateStronglyTypedExportFactoryOfT", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);

[MichalStrehovsky]

OK. One of the operation is not supported messages is because it tries to access Assembly.GetExecutingAssembly().CodeBase. It does that because it won't to feed that string into the AssemblyCatalog which will attempt to load the assembly

I think I'll let @swaroop-sridhar solve this as part of dotnet/designs#52 :).

Other NullReferenceException are because of these static reflection calls

We need to make the classes/methods this is reflecting on public in the implementation and the problem will go away (we've done similar things in System.Linq.Expressions and Microsoft.CSharp).

[ViktorHofer]

We need to make the classes/methods this is reflecting on public in the implementation and the problem will go away (we've done similar things in System.Linq.Expressions and Microsoft.CSharp).

And add them to the corresponding rd.xml, right?

[MichalStrehovsky]

And add them to the corresponding rd.xml, right?

I think the ProjectN reflection analyzer should be able to get this right once this is no longer reflection blocked as an internal implementation detail.

[jkotas]

We need to make the classes/methods this is reflecting on public in the implementation and the problem will go away (we've done similar things in System.Linq.Expressions and Microsoft.CSharp).

You are right that we have used this workaround for ProjectN in the core framework, but we very much dislike it. S.ComponentModel.Composition should be rather marked as user assembly to make this work.

[jkotas]

1100 tests are now executed instead of the previous 1500.

What does this do to test code coverage?

[ViktorHofer]

With changes

+-----------------------------------+--------+--------+--------+
| Module | Line | Branch | Method |
+-----------------------------------+--------+--------+--------+
| System.ComponentModel.Composition | 78.7% | 74.8% | 79.7% |
+-----------------------------------+--------+--------+--------+

Without changes

85.2% |   |   |   | 81.3%

I can probably get the number higher with a bit more time (without using reflection).

[ViktorHofer]

You are right that we have used this workaround for ProjectN in the core framework, but we very much dislike it. S.ComponentModel.Composition should be rather marked as user assembly to make this work.

How would I do that? Does that mean that the assembly won't be assembly blocked?

[MichalStrehovsky]

How would I do that? Does that mean that the assembly won't be assembly blocked?

By adding <BlockReflectionAttribute>false</BlockReflectionAttribute> to the ComponentModel project.

Same reflection rules will apply as for user code (so you can reflect on anything).

It's worth trying out. One problem with that is that the reflection information becomes pay for play and we don't automatically reflection enable everything in the framework assemblies. Reflection use that we didn't predict will lead to exceptions. Blocked types have a special reflection experience (pretend type has no methods, or fields, pretend the type is not generic, pretend the name is a GUID, etc.). Reflection unblocked types without metadata just throw.

[sharwell]

Somebody could try to re-enable some of the tests with reflection calls.

This is always a bad idea. It leaves you with tests that hit the internal APIs but you lose all edit-time and compile-time assistance related to locating them (IDE navigation tools, refactoring tools, etc.).

[jkotas]

This is always a bad idea.

FWIW, it does not make it always a bad idea. It makes it a trade-off.

[karelz]

@ViktorHofer what's status of this PR? Looks like no update in last 3 weeks ...

[ViktorHofer]

I plan to finish this sometime next week.

[karelz]

@ViktorHofer looks like you did not have time. Can we close it for the time being. We can reopen once you have time again ...

[karelz]

1 month no update, let's close it for now - feel free to reopen it once you have time to come back to it. Thanks!

[ViktorHofer]

Sure. No prob.