From 286f12a37c710006f75a2e6df70b5bfc3289fe39 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Sat, 14 Aug 2021 07:56:14 -0700 Subject: [PATCH] Update .NET adapter to handle interface static members properly (#15908) --- .../engine/CoreAdapter.cs | 284 +++++++----------- .../engine/LanguagePrimitives.cs | 9 - .../engine/runtime/Binding/Binders.cs | 14 +- .../Language/Scripting/Adapter.Tests.ps1 | 110 +++++++ 4 files changed, 213 insertions(+), 204 deletions(-) create mode 100644 test/powershell/Language/Scripting/Adapter.Tests.ps1 diff --git a/src/System.Management.Automation/engine/CoreAdapter.cs b/src/System.Management.Automation/engine/CoreAdapter.cs index bf9775b35e..1a623e92f1 100644 --- a/src/System.Management.Automation/engine/CoreAdapter.cs +++ b/src/System.Management.Automation/engine/CoreAdapter.cs @@ -2616,7 +2616,7 @@ namespace System.Management.Automation /// internal Func PSMethodCtor; - internal MethodCacheEntry(MethodBase[] methods) + internal MethodCacheEntry(IList methods) { methodInformationStructures = DotNetAdapter.GetMethodInformationArray(methods); } @@ -3075,9 +3075,8 @@ namespace System.Management.Automation private static void PopulateMethodReflectionTable(Type type, MethodInfo[] methods, CacheTable typeMethods) { - for (int i = 0; i < methods.Length; i++) + foreach (MethodInfo method in methods) { - MethodInfo method = methods[i]; if (method.DeclaringType == type) { string methodName = method.Name; @@ -3102,7 +3101,7 @@ namespace System.Management.Automation private static void PopulateMethodReflectionTable(ConstructorInfo[] ctors, CacheTable typeMethods) { - foreach (var ctor in ctors) + foreach (ConstructorInfo ctor in ctors) { var previousMethodEntry = (List)typeMethods["new"]; if (previousMethodEntry == null) @@ -3127,26 +3126,14 @@ namespace System.Management.Automation /// BindingFlags to use. private static void PopulateMethodReflectionTable(Type type, CacheTable typeMethods, BindingFlags bindingFlags) { - Type typeToGetMethod = type; + bool isStatic = bindingFlags.HasFlag(BindingFlags.Static); - // Assemblies in CoreCLR might not allow reflection execution on their internal types. In such case, we walk up - // the derivation chain to find the first public parent, and use reflection methods on the public parent. - if (!TypeResolver.IsPublic(type) && DisallowPrivateReflection(type)) - { - typeToGetMethod = GetFirstPublicParentType(type); - } - - // In CoreCLR, "GetFirstPublicParentType" may return null if 'type' is an interface - if (typeToGetMethod != null) - { - MethodInfo[] methods = typeToGetMethod.GetMethods(bindingFlags); - PopulateMethodReflectionTable(typeToGetMethod, methods, typeMethods); - } + MethodInfo[] methods = type.GetMethods(bindingFlags); + PopulateMethodReflectionTable(type, methods, typeMethods); Type[] interfaces = type.GetInterfaces(); - for (int interfaceIndex = 0; interfaceIndex < interfaces.Length; interfaceIndex++) + foreach (Type interfaceType in interfaces) { - var interfaceType = interfaces[interfaceIndex]; if (!TypeResolver.IsPublic(interfaceType)) { continue; @@ -3154,41 +3141,50 @@ namespace System.Management.Automation if (interfaceType.IsGenericType && type.IsArray) { - continue; // GetInterfaceMap is not supported in this scenario... not sure if we need to do something special here... + // A bit of background: Array doesn't directly support any generic interface at all. Instead, a stub class + // named 'SZArrayHelper' provides these generic interfaces at runtime for zero-based one-dimension arrays. + // This is why '[object[]].GetInterfaceMap([ICollection[object]])' throws 'ArgumentException'. + // (see https://stackoverflow.com/a/31883327) + // + // We had always been skipping generic interfaces for array types because 'GetInterfaceMap' doesn't work + // for it. Today, even though we don't use 'GetInterfaceMap' anymore, the same code is kept here because + // methods from generic interfaces of an array type could cause ambiguity in method overloads resolution. + // For example, "$objs = @(1,2,3,4); $objs.Contains(1)" would fail because there would be 2 overloads of + // the 'Contains' methods which are equally good matches for the call. + // bool IList.Contains(System.Object value) + // bool ICollection[Object].Contains(System.Object item) + continue; } - MethodInfo[] methods; - if (type.IsInterface) - { - methods = interfaceType.GetMethods(bindingFlags); - } - else - { - InterfaceMapping interfaceMapping = type.GetInterfaceMap(interfaceType); - methods = interfaceMapping.InterfaceMethods; - } + methods = interfaceType.GetMethods(bindingFlags); - for (int methodIndex = 0; methodIndex < methods.Length; methodIndex++) + foreach (MethodInfo interfaceMethod in methods) { - MethodInfo interfaceMethodDefinition = methods[methodIndex]; - - if ((!interfaceMethodDefinition.IsPublic) || - (interfaceMethodDefinition.IsStatic != ((BindingFlags.Static & bindingFlags) != 0))) + if (isStatic && interfaceMethod.IsVirtual) { + // Ignore static virtual/abstract methods on an interface because: + // 1. if it's implicitly implemented, which will be mostly the case, then the corresponding + // methods were already retrieved from the 'type.GetMethods' step above; + // 2. if it's explicitly implemented, we cannot call 'Invoke(null, args)' on the static method, + // but have to use 'type.GetInterfaceMap(interfaceType)' to get the corresponding target + // methods, and call 'Invoke(null, args)' on them. The target methods will be non-public + // in this case, which we always ignore. + // 3. The recommendation from .NET team is to ignore the static virtuals on interfaces, + // especially given that the APIs may change in .NET 7. continue; } - var previousMethodEntry = (List)typeMethods[interfaceMethodDefinition.Name]; + var previousMethodEntry = (List)typeMethods[interfaceMethod.Name]; if (previousMethodEntry == null) { - var methodEntry = new List { interfaceMethodDefinition }; - typeMethods.Add(interfaceMethodDefinition.Name, methodEntry); + var methodEntry = new List { interfaceMethod }; + typeMethods.Add(interfaceMethod.Name, methodEntry); } else { - if (!previousMethodEntry.Contains(interfaceMethodDefinition)) + if (!previousMethodEntry.Contains(interfaceMethod)) { - previousMethodEntry.Add(interfaceMethodDefinition); + previousMethodEntry.Add(interfaceMethod); } } } @@ -3211,7 +3207,7 @@ namespace System.Management.Automation for (int i = 0; i < typeMethods.memberCollection.Count; i++) { typeMethods.memberCollection[i] = - new MethodCacheEntry(((List)typeMethods.memberCollection[i]).ToArray()); + new MethodCacheEntry((List)typeMethods.memberCollection[i]); } } @@ -3224,38 +3220,24 @@ namespace System.Management.Automation /// BindingFlags to use. private static void PopulateEventReflectionTable(Type type, Dictionary typeEvents, BindingFlags bindingFlags) { - // Assemblies in CoreCLR might not allow reflection execution on their internal types. In such case, we walk up - // the derivation chain to find the first public parent, and use reflection events on the public parent. - if (!TypeResolver.IsPublic(type) && DisallowPrivateReflection(type)) + EventInfo[] events = type.GetEvents(bindingFlags); + var tempTable = new Dictionary>(StringComparer.OrdinalIgnoreCase); + + foreach (EventInfo typeEvent in events) { - type = GetFirstPublicParentType(type); + string eventName = typeEvent.Name; + if (!tempTable.TryGetValue(eventName, out List entryList)) + { + entryList = new List(); + tempTable.Add(eventName, entryList); + } + + entryList.Add(typeEvent); } - // In CoreCLR, "GetFirstPublicParentType" may return null if 'type' is an interface - if (type != null) + foreach (KeyValuePair> entry in tempTable) { - EventInfo[] events = type.GetEvents(bindingFlags); - var tempTable = new Dictionary>(StringComparer.OrdinalIgnoreCase); - for (int i = 0; i < events.Length; i++) - { - var typeEvent = events[i]; - string eventName = typeEvent.Name; - List previousEntry; - if (!tempTable.TryGetValue(eventName, out previousEntry)) - { - var eventEntry = new List { typeEvent }; - tempTable.Add(eventName, eventEntry); - } - else - { - previousEntry.Add(typeEvent); - } - } - - foreach (var entry in tempTable) - { - typeEvents.Add(entry.Key, new EventCacheEntry(entry.Value.ToArray())); - } + typeEvents.Add(entry.Key, new EventCacheEntry(entry.Value.ToArray())); } } @@ -3270,9 +3252,8 @@ namespace System.Management.Automation ParameterInfo[] propertyParameters = property.GetIndexParameters(); int propertyIndexLength = propertyParameters.Length; - for (int propertyIndex = 0; propertyIndex < previousProperties.Count; propertyIndex++) + foreach (PropertyInfo previousProperty in previousProperties) { - var previousProperty = previousProperties[propertyIndex]; ParameterInfo[] previousParameters = previousProperty.GetIndexParameters(); if (previousParameters.Length == propertyIndexLength) { @@ -3308,79 +3289,81 @@ namespace System.Management.Automation /// BindingFlags to use. private static void PopulatePropertyReflectionTable(Type type, CacheTable typeProperties, BindingFlags bindingFlags) { + bool isStatic = bindingFlags.HasFlag(BindingFlags.Static); var tempTable = new Dictionary>(StringComparer.OrdinalIgnoreCase); - Type typeToGetPropertyAndField = type; - // Assemblies in CoreCLR might not allow reflection execution on their internal types. In such case, we walk up the - // derivation chain to find the first public parent, and use reflection properties/fields on the public parent. - if (!TypeResolver.IsPublic(type) && DisallowPrivateReflection(type)) + PropertyInfo[] properties = type.GetProperties(bindingFlags); + foreach (PropertyInfo property in properties) { - typeToGetPropertyAndField = GetFirstPublicParentType(type); - } - - // In CoreCLR, "GetFirstPublicParentType" may return null if 'type' is an interface - PropertyInfo[] properties; - if (typeToGetPropertyAndField != null) - { - properties = typeToGetPropertyAndField.GetProperties(bindingFlags); - for (int i = 0; i < properties.Length; i++) - { - PopulateSingleProperty(type, properties[i], tempTable, properties[i].Name); - } + PopulateSingleProperty(type, property, tempTable, property.Name); } Type[] interfaces = type.GetInterfaces(); - for (int interfaceIndex = 0; interfaceIndex < interfaces.Length; interfaceIndex++) + foreach (Type interfaceType in interfaces) { - Type interfaceType = interfaces[interfaceIndex]; if (!TypeResolver.IsPublic(interfaceType)) { continue; } properties = interfaceType.GetProperties(bindingFlags); - for (int propertyIndex = 0; propertyIndex < properties.Length; propertyIndex++) + foreach (PropertyInfo property in properties) { - PopulateSingleProperty(type, properties[propertyIndex], tempTable, properties[propertyIndex].Name); + if (isStatic && + (property.GetMethod?.IsVirtual == true || property.SetMethod?.IsVirtual == true)) + { + // Ignore static virtual/abstract properties on an interface because: + // 1. if it's implicitly implemented, which will be mostly the case, then the corresponding + // properties were already retrieved from the 'type.GetProperties' step above; + // 2. if it's explicitly implemented, we cannot call 'GetValue(null)' on the static property, + // but have to use 'type.GetInterfaceMap(interfaceType)' to get the corresponding target + // get/set accessor methods, and call 'Invoke(null, args)' on them. The target methods will + // be non-public in this case, which we always ignore. + // 3. The recommendation from .NET team is to ignore the static virtuals on interfaces, + // especially given that the APIs may change in .NET 7. + continue; + } + + PopulateSingleProperty(type, property, tempTable, property.Name); } } - foreach (var pairs in tempTable) + foreach (KeyValuePair> entry in tempTable) { - var propertiesList = pairs.Value; + List propertiesList = entry.Value; PropertyInfo firstProperty = propertiesList[0]; if ((propertiesList.Count > 1) || (firstProperty.GetIndexParameters().Length != 0)) { - typeProperties.Add(pairs.Key, new ParameterizedPropertyCacheEntry(propertiesList)); + typeProperties.Add(entry.Key, new ParameterizedPropertyCacheEntry(propertiesList)); } else { - typeProperties.Add(pairs.Key, new PropertyCacheEntry(firstProperty)); + typeProperties.Add(entry.Key, new PropertyCacheEntry(firstProperty)); } } - // In CoreCLR, "GetFirstPublicParentType" may return null if 'type' is an interface - if (typeToGetPropertyAndField != null) + FieldInfo[] fields = type.GetFields(bindingFlags); + foreach (FieldInfo field in fields) { - FieldInfo[] fields = typeToGetPropertyAndField.GetFields(bindingFlags); - for (int i = 0; i < fields.Length; i++) + string fieldName = field.Name; + var previousMember = (PropertyCacheEntry)typeProperties[fieldName]; + if (previousMember == null) { - FieldInfo field = fields[i]; - string fieldName = field.Name; - var previousMember = (PropertyCacheEntry)typeProperties[fieldName]; - if (previousMember == null) - { - typeProperties.Add(fieldName, new PropertyCacheEntry(field)); - } - else - { - // A property/field declared with new in a derived class might appear twice - if (!string.Equals(previousMember.member.Name, fieldName)) - { - throw new ExtendedTypeSystemException("NotACLSComplaintField", null, - ExtendedTypeSystem.NotAClsCompliantFieldProperty, fieldName, type.FullName, previousMember.member.Name); - } - } + typeProperties.Add(fieldName, new PropertyCacheEntry(field)); + } + else if (!string.Equals(previousMember.member.Name, fieldName)) + { + // A property/field declared with 'new' in a derived class might appear twice, and it's OK to ignore + // the second property/field in that case. + // However, if the names of two properties/fields are different only in letter casing, then it's not + // CLS complaint and we throw an exception. + throw new ExtendedTypeSystemException( + "NotACLSComplaintField", + innerException: null, + ExtendedTypeSystem.NotAClsCompliantFieldProperty, + fieldName, + type.FullName, + previousMember.member.Name); } } } @@ -3411,68 +3394,6 @@ namespace System.Management.Automation } } - #region Handle_Internal_Type_Reflection_In_CoreCLR - - /// - /// The dictionary cache about if an assembly supports reflection execution on its internal types. - /// - private static readonly ConcurrentDictionary s_disallowReflectionCache = - new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); - - /// - /// Check if the type is defined in an assembly that disallows reflection execution on internal types. - /// - .NET Framework assemblies don't support reflection execution on their internal types. - /// - internal static bool DisallowPrivateReflection(Type type) - { - bool disallowReflection = false; - Assembly assembly = type.Assembly; - if (s_disallowReflectionCache.TryGetValue(assembly.FullName, out disallowReflection)) - { - return disallowReflection; - } - - var productAttribute = assembly.GetCustomAttribute(); - if (productAttribute != null && string.Equals(productAttribute.Product, "Microsoft® .NET Framework", StringComparison.OrdinalIgnoreCase)) - { - disallowReflection = true; - } - else - { - #pragma warning disable SYSLIB0015 - // Check for 'DisablePrivateReflectionAttribute'. It's applied at the assembly level, and allow an assembly to opt-out of private/internal reflection. - var disablePrivateReflectionAttribute = assembly.GetCustomAttribute(); - disallowReflection = disablePrivateReflectionAttribute != null; - #pragma warning restore SYSLIB0015 - } - - s_disallowReflectionCache.TryAdd(assembly.FullName, disallowReflection); - return disallowReflection; - } - - /// - /// Walk up the derivation chain to find the first public parent type. - /// - internal static Type GetFirstPublicParentType(Type type) - { - Dbg.Assert(!TypeResolver.IsPublic(type), "type should not be public."); - Type parent = type.BaseType; - while (parent != null) - { - if (parent.IsPublic) - { - return parent; - } - - parent = parent.BaseType; - } - - // Return null when type is an interface - return null; - } - - #endregion Handle_Internal_Type_Reflection_In_CoreCLR - /// /// Called from GetProperty and GetProperties to populate the /// typeTable with all public properties and fields @@ -4286,11 +4207,10 @@ namespace System.Management.Automation /// /// The methods to be converted. /// The MethodInformation[] corresponding to methods. - internal static MethodInformation[] GetMethodInformationArray(MethodBase[] methods) + internal static MethodInformation[] GetMethodInformationArray(IList methods) { - int methodCount = methods.Length; - MethodInformation[] returnValue = new MethodInformation[methodCount]; - for (int i = 0; i < methods.Length; i++) + var returnValue = new MethodInformation[methods.Count]; + for (int i = 0; i < methods.Count; i++) { returnValue[i] = new MethodInformation(methods[i], 0); } diff --git a/src/System.Management.Automation/engine/LanguagePrimitives.cs b/src/System.Management.Automation/engine/LanguagePrimitives.cs index 3caceb40bb..3176fbbead 100644 --- a/src/System.Management.Automation/engine/LanguagePrimitives.cs +++ b/src/System.Management.Automation/engine/LanguagePrimitives.cs @@ -5651,15 +5651,6 @@ namespace System.Management.Automation } } - // Assemblies in CoreCLR might not allow reflection execution on their internal types. - if (!TypeResolver.IsPublic(toType) && DotNetAdapter.DisallowPrivateReflection(toType)) - { - // If the type is non-public and reflection execution is not allowed on it, then we return - // 'ConvertNoConversion', because we won't be able to invoke constructor, methods or set - // properties on an instance of this type through reflection. - return CacheConversion(fromType, toType, ConvertNoConversion, ConversionRank.None); - } - PSConverter valueDependentConversion = null; ConversionRank valueDependentRank = ConversionRank.None; IConversionData conversionData = FigureLanguageConversion(fromType, toType, out valueDependentConversion, out valueDependentRank); diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs index 6cfc72fd16..a4917d5af6 100644 --- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs +++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs @@ -5407,18 +5407,6 @@ namespace System.Management.Automation.Language var type = castToType ?? ((value != null) ? value.GetType() : typeof(object)); - // Assemblies in CoreCLR might not allow reflection execution on their internal types. In such case, we walk up - // the derivation chain to find the first public parent, and use reflection methods on the public parent. - if (!TypeResolver.IsPublic(type) && DotNetAdapter.DisallowPrivateReflection(type)) - { - var publicType = DotNetAdapter.GetFirstPublicParentType(type); - if (publicType != null) - { - type = publicType; - } - // else we'll probably fail, but the error message might be more helpful than NullReferenceException - } - if (expr.Type != type) { // Unbox value types (or use Nullable.Value) to avoid a copy in case the value is mutated. @@ -5779,7 +5767,7 @@ namespace System.Management.Automation.Language } else { - DotNetAdapter.MethodCacheEntry method = new DotNetAdapter.MethodCacheEntry(candidateMethods.ToArray()); + DotNetAdapter.MethodCacheEntry method = new DotNetAdapter.MethodCacheEntry(candidateMethods); memberInfo = PSMethod.Create(this.Name, PSObject.DotNetInstanceAdapter, null, method); } } diff --git a/test/powershell/Language/Scripting/Adapter.Tests.ps1 b/test/powershell/Language/Scripting/Adapter.Tests.ps1 new file mode 100644 index 0000000000..db681192c4 --- /dev/null +++ b/test/powershell/Language/Scripting/Adapter.Tests.ps1 @@ -0,0 +1,110 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +Describe "Interface static members" -Tags "CI" { + BeforeAll { + $testType = "InterfaceStaticMemberTest.ITest" -as [Type] + if (-not $testType) { + Add-Type -TypeDefinition @' + using System; + + namespace InterfaceStaticMemberTest { + public interface ITest { + public const int MyConst = 17; + + public static string Type = "ITest-Type"; + public static string Name => "Joe"; + public static string GetPath() => "Path"; + public int GetAge(); + public int GetId(); + } + + public class Foo : ITest { + int ITest.GetId() { return 100; } + int ITest.GetAge() { return 2; } + } + + public class Zoo : Foo { + public static string Type = "Zoo-Type"; + } + } +'@ + } + } + + It "Access static/const members on the interface type" { + [InterfaceStaticMemberTest.ITest]::MyConst | Should -Be 17 + [InterfaceStaticMemberTest.ITest]::Type | Should -Be "ITest-Type" + [InterfaceStaticMemberTest.ITest]::Name | Should -Be "Joe" + [InterfaceStaticMemberTest.ITest]::GetPath() | Should -Be "Path" + + $type = [InterfaceStaticMemberTest.ITest] + $type::MyConst | Should -Be 17 + $type::Type | Should -Be "ITest-Type" + $type::Name | Should -Be "Joe" + $type::GetPath() | Should -Be "Path" + + { [InterfaceStaticMemberTest.ITest]::Name = 'Jane' } | Should -Throw -ErrorId 'PropertyAssignmentException' + { $type::Name = 'Jane' } | Should -Throw -ErrorId 'PropertyAssignmentException' + } + + It "Access interface static/const members on the implementation type" { + [InterfaceStaticMemberTest.Foo]::MyConst | Should -Be 17 + [InterfaceStaticMemberTest.Foo]::Type | Should -Be "ITest-Type" + [InterfaceStaticMemberTest.Foo]::Name | Should -Be "Joe" + [InterfaceStaticMemberTest.Foo]::GetPath() | Should -Be "Path" + [InterfaceStaticMemberTest.Foo]::get_Name() | Should -Be "Joe" + + $type = [InterfaceStaticMemberTest.Foo] + $type::MyConst | Should -Be 17 + $type::Type | Should -Be "ITest-Type" + $type::Name | Should -Be "Joe" + $type::GetPath() | Should -Be "Path" + $type::get_Name() | Should -Be "Joe" + + { [InterfaceStaticMemberTest.Foo]::Name = 'Jane' } | Should -Throw -ErrorId 'PropertyAssignmentException' + { $type::Name = 'Jane' } | Should -Throw -ErrorId 'PropertyAssignmentException' + } + + It "Static field with the same name on the implementation type overrides the one from the interface" { + [InterfaceStaticMemberTest.Zoo]::Type | Should -Be "Zoo-Type" + + $type = [InterfaceStaticMemberTest.Zoo] + $type::Type | Should -Be "Zoo-Type" + + $nameMember = $type | Get-Member -Static -Name Name + $nameMember | Should -Not -BeNullOrEmpty + $nameMember.Name | Should -Be "Name" + $nameMember.MemberType | Should -Be "Property" + + $getNameMember = $type | Get-Member -Static -Name 'get_Name' + $getNameMember | Should -BeNullOrEmpty + + $getNameMember = $type | Get-Member -Static -Name 'get_Name' -Force + $getNameMember | Should -Not -BeNullOrEmpty + $getNameMember.Name | Should -Be "get_Name" + $getNameMember.MemberType | Should -Be "Method" + + $type::Name | Should -Be "Joe" + $type::get_Name() | Should -Be "Joe" + } + + It "Explicitly implemented interface members are visible/accessible by default" { + $obj = [InterfaceStaticMemberTest.Zoo]::new() + + $ageMember = $obj | Get-Member -Name GetAge + $ageMember | Should -Not -BeNullOrEmpty + $ageMember.Name | Should -Be "GetAge" + $ageMember.MemberType | Should -Be "Method" + $ageMember.Definition | Should -Be "int ITest.GetAge()" + + $idMember = $obj | Get-Member -Name GetId + $idMember | Should -Not -BeNullOrEmpty + $idMember.Name | Should -Be "GetId" + $idMember.MemberType | Should -Be "Method" + $idMember.Definition | Should -Be "int ITest.GetId()" + + $obj.GetAge() | Should -Be 2 + $obj.GetId() | Should -Be 100 + } +}