Merge pull request #93 from Microsoft/dev/andarno/issue91

Fix support for multiple internal metadata view interfaces defined from multiple assemblies

src/Microsoft.VisualStudio.Composition/Configuration/MetadataViewGenerator.cs

@@ -10,6 +10,7 @@ namespace Microsoft.VisualStudio.Composition
 {
     using System;
     using System.Collections.Generic;
+    using System.Collections.Immutable;
     using System.Globalization;
     using System.Linq;
     using System.Reflection;
@@ -79,36 +80,48 @@ internal static class MetadataViewGenerator
         private const string MetadataViewFactoryName = "Create";
 
         private static readonly Dictionary<Type, MetadataViewFactory> MetadataViewFactories = new Dictionary<Type, MetadataViewFactory>();
-        private static readonly AssemblyName ProxyAssemblyName = new AssemblyName(string.Format(CultureInfo.InvariantCulture, "MetadataViewProxies_{0}", Guid.NewGuid()));
 
         private static readonly Type[] CtorArgumentTypes = new Type[] { typeof(IReadOnlyDictionary<string, object>), typeof(IReadOnlyDictionary<string, object>) };
         private static readonly MethodInfo MdvDictionaryTryGet = CtorArgumentTypes[0].GetTypeInfo().GetMethod("TryGetValue");
         private static readonly MethodInfo MdvDictionaryIndexer = CtorArgumentTypes[0].GetTypeInfo().GetMethod("get_Item");
         private static readonly MethodInfo ObjectGetType = typeof(object).GetTypeInfo().GetMethod("GetType", Type.EmptyTypes);
         private static readonly ConstructorInfo ObjectCtor = typeof(object).GetTypeInfo().GetConstructor(Type.EmptyTypes);
 
-        private static ModuleBuilder transparentProxyModuleBuilder;
-        private static SkipClrVisibilityChecks skipClrVisibilityChecks;
+        private static readonly Dictionary<ImmutableHashSet<AssemblyName>, ModuleBuilder> TransparentProxyModuleBuilderByVisibilityCheck = new Dictionary<ImmutableHashSet<AssemblyName>, ModuleBuilder>(new ByContentEqualityComparer());
 
         public delegate object MetadataViewFactory(IReadOnlyDictionary<string, object> metadata, IReadOnlyDictionary<string, object> defaultMetadata);
 
         private static AssemblyBuilder CreateProxyAssemblyBuilder(ConstructorInfo constructorInfo)
         {
-            return AssemblyBuilder.DefineDynamicAssembly(ProxyAssemblyName, AssemblyBuilderAccess.Run);
+            var proxyAssemblyName = new AssemblyName(string.Format(CultureInfo.InvariantCulture, "MetadataViewProxies_{0}", Guid.NewGuid()));
+            return AssemblyBuilder.DefineDynamicAssembly(proxyAssemblyName, AssemblyBuilderAccess.Run);
         }
 
-        private static ModuleBuilder GetProxyModuleBuilder()
+        /// <summary>
+        /// Gets the <see cref="ModuleBuilder"/> to use for generating a proxy for the given type.
+        /// </summary>
+        /// <param name="viewType">The type of the interface to generate a proxy for.</param>
+        /// <returns>The <see cref="ModuleBuilder"/> to use.</returns>
+        private static ModuleBuilder GetProxyModuleBuilder(TypeInfo viewType)
         {
+            Requires.NotNull(viewType, nameof(viewType));
             Assumes.True(Monitor.IsEntered(MetadataViewFactories));
-            if (transparentProxyModuleBuilder == null)
+
+            // Dynamic assemblies are relatively expensive. We want to create as few as possible.
+            // For each unique set of skip visibility check assemblies, we need a new dynamic assembly
+            // because the CLR will not honor any additions to that set once the first generated type is closed.
+            // So maintain a dictionary to point at dynamic modules based on the set of skip visiblity check assemblies they were generated with.
+            var skipVisibilityCheckAssemblies = SkipClrVisibilityChecks.GetSkipVisibilityChecksRequirements(viewType);
+            if (!TransparentProxyModuleBuilderByVisibilityCheck.TryGetValue(skipVisibilityCheckAssemblies, out ModuleBuilder moduleBuilder))
             {
-                // make a new assemblybuilder and modulebuilder
                 var assemblyBuilder = CreateProxyAssemblyBuilder(typeof(SecurityTransparentAttribute).GetTypeInfo().GetConstructor(Type.EmptyTypes));
-                transparentProxyModuleBuilder = assemblyBuilder.DefineDynamicModule("MetadataViewProxiesModule");
-                skipClrVisibilityChecks = new SkipClrVisibilityChecks(assemblyBuilder, transparentProxyModuleBuilder);
+                moduleBuilder = assemblyBuilder.DefineDynamicModule("MetadataViewProxiesModule");
+                var skipClrVisibilityChecks = new SkipClrVisibilityChecks(assemblyBuilder, moduleBuilder);
+                skipClrVisibilityChecks.SkipVisibilityChecksFor(skipVisibilityCheckAssemblies);
+                TransparentProxyModuleBuilderByVisibilityCheck.Add(skipVisibilityCheckAssemblies, moduleBuilder);
             }
 
-            return transparentProxyModuleBuilder;
+            return moduleBuilder;
         }
 
         public static MetadataViewFactory GetMetadataViewFactory(Type viewType)
@@ -141,9 +154,7 @@ private static TypeInfo GenerateInterfaceViewProxyType(Type viewType)
             TypeBuilder proxyTypeBuilder;
             Type[] interfaces = { viewType };
 
-            var proxyModuleBuilder = GetProxyModuleBuilder();
-            skipClrVisibilityChecks.SkipVisibilityChecksFor(viewType.GetTypeInfo());
-
+            var proxyModuleBuilder = GetProxyModuleBuilder(viewType.GetTypeInfo());
             proxyTypeBuilder = proxyModuleBuilder.DefineType(
                 string.Format(CultureInfo.InvariantCulture, "_proxy_{0}_{1}", viewType.FullName, Guid.NewGuid()),
                 TypeAttributes.Public,
@@ -269,5 +280,29 @@ private static IEnumerable<PropertyInfo> GetAllProperties(this Type type)
         {
             return type.GetTypeInfo().GetInterfaces().Concat(new Type[] { type }).SelectMany(itf => itf.GetTypeInfo().GetProperties());
         }
+
+        private class ByContentEqualityComparer : IEqualityComparer<ImmutableHashSet<AssemblyName>>
+        {
+            public bool Equals(ImmutableHashSet<AssemblyName> x, ImmutableHashSet<AssemblyName> y)
+            {
+                if (x.Count != y.Count)
+                {
+                    return false;
+                }
+
+                return !x.Except(y).Any();
+            }
+
+            public int GetHashCode(ImmutableHashSet<AssemblyName> obj)
+            {
+                int hashCode = 0;
+                foreach (var item in obj)
+                {
+                    hashCode += item.GetHashCode();
+                }
+
+                return hashCode;
+            }
+        }
     }
 }

src/Microsoft.VisualStudio.Composition/Reflection/SkipClrVisibilityChecks.cs

@@ -4,6 +4,7 @@ namespace Microsoft.VisualStudio.Composition.Reflection
 {
     using System;
     using System.Collections.Generic;
+    using System.Collections.Immutable;
     using System.Linq;
     using System.Reflection;
     using System.Reflection.Emit;
@@ -64,33 +65,64 @@ internal SkipClrVisibilityChecks(AssemblyBuilder assemblyBuilder, ModuleBuilder
         }
 
         /// <summary>
-        /// Ensures the CLR will skip visibility checks when accessing
-        /// the assembly that contains the specified member.
+        /// Gets the set of assemblies that a generated assembly must be granted the ability to skip visiblity checks for
+        /// in order to access the specified type.
         /// </summary>
-        /// <param name="memberInfo">The member that may not be publicly accessible.</param>
-        internal void SkipVisibilityChecksFor(MemberInfo memberInfo)
+        /// <param name="typeInfo">The type which may be internal.</param>
+        /// <returns>The set of names of assemblies to skip visibility checks for.</returns>
+        internal static ImmutableHashSet<AssemblyName> GetSkipVisibilityChecksRequirements(TypeInfo typeInfo)
         {
-            this.SkipVisibilityChecksFor(memberInfo.Module.Assembly);
+            Requires.NotNull(typeInfo, nameof(typeInfo));
+
+            if (ReflectionHelpers.IsPublic(typeInfo.AsType()))
+            {
+                return ImmutableHashSet<AssemblyName>.Empty;
+            }
+            else
+            {
+                var result = ImmutableHashSet<AssemblyName>.Empty
+                    .Add(typeInfo.Assembly.GetName());
+
+                // If this type has a base type defined in another assembly that is also internal
+                // (with InternalsVisibleTo to the assembly hosting the original type)
+                // then we'll need to add that.
+                // TODO: Learn we somehow we don't need to do this, even given our test defines an internal interface
+                //       that derives from another internal interface defined in another assembly.
+                ////if (typeInfo.BaseType != null)
+                ////{
+                ////    result = result.Union(GetSkipVisibilityChecksRequirements(typeInfo.BaseType.GetTypeInfo()));
+                ////}
+
+                ////foreach (var iface in typeInfo.GetInterfaces())
+                ////{
+                ////    result = result.Union(GetSkipVisibilityChecksRequirements(iface.GetTypeInfo()));
+                ////}
+
+                return result;
+            }
         }
 
         /// <summary>
-        /// Add an attribute to the dynamic assembly so that the CLR will skip visibility checks
-        /// for the specified assembly.
+        /// Add attributes to a dynamic assembly so that the CLR will skip visibility checks
+        /// for the assemblies with the specified names.
         /// </summary>
-        /// <param name="assembly">The assembly to skip visibility checks for.</param>
-        private void SkipVisibilityChecksFor(Assembly assembly)
+        /// <param name="assemblyNames">The names of the assemblies to skip visibility checks for.</param>
+        internal void SkipVisibilityChecksFor(IEnumerable<AssemblyName> assemblyNames)
         {
-            Requires.NotNull(assembly, nameof(assembly));
-            var assemblyName = assembly.GetName();
-            this.SkipVisibilityChecksFor(assemblyName);
+            Requires.NotNull(assemblyNames, nameof(assemblyNames));
+
+            foreach (var assemblyName in assemblyNames)
+            {
+                this.SkipVisibilityChecksFor(assemblyName);
+            }
         }
 
         /// <summary>
-        /// Add an attribute to the dynamic assembly so that the CLR will skip visibility checks
+        /// Add an attribute to a dynamic assembly so that the CLR will skip visibility checks
         /// for the assembly with the specified name.
         /// </summary>
         /// <param name="assemblyName">The name of the assembly to skip visibility checks for.</param>
-        private void SkipVisibilityChecksFor(AssemblyName assemblyName)
+        internal void SkipVisibilityChecksFor(AssemblyName assemblyName)
         {
             Requires.NotNull(assemblyName, nameof(assemblyName));
 

src/tests/Microsoft.VisualStudio.Composition.Tests/ExportMetadataTests.cs

@@ -130,6 +130,17 @@ public void ImportManyWithInternalMetadataInterface(IContainer container)
             Assert.IsType<PartWithExportMetadata>(importingPart.ImportingProperty.Single().Value);
         }
 
+        /// <summary>
+        /// Verifies that we can skip visibility checks for more than one assembly.
+        /// </summary>
+        [Trait("Access", "NonPublic")]
+        [MefFact(CompositionEngines.V3EmulatingV1, typeof(ImportManyPartWithInternalMetadataInterface), typeof(ImportManyPartWithInternalMetadataInterface2), typeof(PartWithExportMetadata))]
+        public void InternalMetadataViewInterfacesAcrossMultipleAssemblies(IContainer container)
+        {
+            container.GetExportedValue<ImportManyPartWithInternalMetadataInterface>();
+            container.GetExportedValue<ImportManyPartWithInternalMetadataInterface2>();
+        }
+
         [MefFact(CompositionEngines.Unspecified, typeof(ImportingPartWithMetadataInterface), typeof(PartWithExportMetadata))]
         [Trait("Efficiency", "InstanceReuse")]
         public void MetadataViewInterfaceInstanceSharedAcrossImports(IContainer container)
@@ -645,6 +656,14 @@ public class ImportManyPartWithInternalMetadataInterface
             internal IEnumerable<Lazy<PartWithExportMetadata, IMetadataInternal>> ImportingProperty { get; set; }
         }
 
+        [MefV1.Export, MefV1.PartCreationPolicy(MefV1.CreationPolicy.NonShared)]
+        [Export]
+        public class ImportManyPartWithInternalMetadataInterface2
+        {
+            [ImportMany, MefV1.ImportMany]
+            internal IEnumerable<Lazy<PartWithExportMetadata, AssemblyDiscoveryTests.IInternalMetadataView>> ImportingProperty { get; set; }
+        }
+
         [MefV1.Export, MefV1.PartCreationPolicy(MefV1.CreationPolicy.NonShared)]
         [Export]
         public class ImportManyPartWithMetadataClass