From 7f58b7970d3bab6f687925bdbdf366ac14520dde Mon Sep 17 00:00:00 2001 From: Patrick Meinecke Date: Sat, 25 Aug 2018 07:55:53 -0400 Subject: [PATCH 01/11] Add ITuple checks to PSGetIndexBinder This change enables index operations on objects that implement ITuple. --- .../engine/runtime/Binding/Binders.cs | 18 +++++++++++++++++- .../Language/Scripting/Indexer.Tests.ps1 | 16 ++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs index f4ab7c73f6b..7fc286fe1d7 100644 --- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs +++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs @@ -3946,6 +3946,11 @@ public override DynamicMetaObject FallbackGetIndex(DynamicMetaObject target, Dyn return InvokeIndexer(target, indexes, errorSuggestion, defaultMember.MemberName).WriteToDebugLog(this); } + if (typeof(ITuple).IsAssignableFrom(target.LimitType)) + { + return InvokeIndexer(target, indexes, errorSuggestion, "Item").WriteToDebugLog(this); + } + return errorSuggestion ?? CannotIndexTarget(target, indexes).WriteToDebugLog(this); } @@ -4046,7 +4051,12 @@ internal static bool CanIndexFromEndWithNegativeIndex(DynamicMetaObject target) } // target implements IList? - return limitType.GetInterfaces().Any(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(IList<>)); + if (limitType.GetInterfaces().Any(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(IList<>))) + { + return true; + } + + return typeof(ITuple).IsAssignableFrom(limitType); } private DynamicMetaObject IndexWithNegativeChecks( @@ -4250,6 +4260,12 @@ private DynamicMetaObject InvokeIndexer(DynamicMetaObject target, PropertyInfo lengthProperty = target.LimitType.GetProperty("Count") ?? target.LimitType.GetProperty("Length"); // for string + // Default ITuple implementations explicitly implement ITuple.Length. + if (lengthProperty == null && typeof(ITuple).IsAssignableFrom(target.LimitType)) + { + lengthProperty = typeof(ITuple).GetProperty(nameof(ITuple.Length)); + } + if (lengthProperty != null) { return IndexWithNegativeChecks( diff --git a/test/powershell/Language/Scripting/Indexer.Tests.ps1 b/test/powershell/Language/Scripting/Indexer.Tests.ps1 index ded67dbdd01..bfc6656bb4a 100644 --- a/test/powershell/Language/Scripting/Indexer.Tests.ps1 +++ b/test/powershell/Language/Scripting/Indexer.Tests.ps1 @@ -24,4 +24,20 @@ Describe 'Tests for indexers' -Tags "CI" { $service = Get-WmiObject -List -Amended Win32_Service $service.Properties["Hello There"] | Should -BeNullOrEmpty } + + It 'ITuple implementations can be indexed' { + $tuple = [Tuple]::Create(10, 'Hello') + $tuple[0] | Should -Be 10 + $tuple[1] | Should -BeExactly 'Hello' + } + + It 'ITuple objects can be spliced' { + $tuple = [Tuple]::Create(10, 'Hello') + $tuple[0..1] | Should -Be @(10, 'Hello') + } + + It 'Index of -1 should return the last item for ITuple objects' { + $tuple = [Tuple]::Create(10, 'Hello') + $tuple[-1] | Should -BeExactly 'Hello' + } } From bdfea1643ee9bc49abe36ad033be20000015b6b3 Mon Sep 17 00:00:00 2001 From: Patrick Meinecke Date: Sun, 26 Aug 2018 08:56:32 -0400 Subject: [PATCH 02/11] Check for explicitly implemented indexers Remove the logic to specifically check for ITuple implementations. Instead, search all implemented interfaces for a DefaultMemberAttribute. This will ensure all explicitly implemented indexers are found. --- .../engine/runtime/Binding/Binders.cs | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs index 7fc286fe1d7..299448f92d6 100644 --- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs +++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs @@ -3928,6 +3928,8 @@ public override DynamicMetaObject FallbackGetIndex(DynamicMetaObject target, Dyn return GetIndexArray(target, indexes, errorSuggestion).WriteToDebugLog(this); } + var defaultMember = target.LimitType.GetCustomAttributes(true).FirstOrDefault(); + PropertyInfo lengthProperty = null; foreach (var i in target.LimitType.GetInterfaces()) { if (i.IsGenericType && i.GetGenericTypeDefinition() == typeof(IDictionary<,>)) @@ -3938,17 +3940,20 @@ public override DynamicMetaObject FallbackGetIndex(DynamicMetaObject target, Dyn return result.WriteToDebugLog(this); } } - } - var defaultMember = target.LimitType.GetCustomAttributes(true).FirstOrDefault(); - if (defaultMember != null) - { - return InvokeIndexer(target, indexes, errorSuggestion, defaultMember.MemberName).WriteToDebugLog(this); + if (defaultMember == null) + { + defaultMember = i.GetCustomAttribute(inherit: true); + if (defaultMember != null) + { + lengthProperty = i.GetProperty("Length") ?? i.GetProperty("Count"); + } + } } - if (typeof(ITuple).IsAssignableFrom(target.LimitType)) + if (defaultMember != null) { - return InvokeIndexer(target, indexes, errorSuggestion, "Item").WriteToDebugLog(this); + return InvokeIndexer(target, indexes, errorSuggestion, defaultMember.MemberName, lengthProperty).WriteToDebugLog(this); } return errorSuggestion ?? CannotIndexTarget(target, indexes).WriteToDebugLog(this); @@ -4207,7 +4212,8 @@ private DynamicMetaObject GetIndexMultiDimensionArray(DynamicMetaObject target, private DynamicMetaObject InvokeIndexer(DynamicMetaObject target, DynamicMetaObject[] indexes, DynamicMetaObject errorSuggestion, - string methodName) + string methodName, + PropertyInfo lengthProperty) { MethodInfo getter = PSInvokeMemberBinder.FindBestMethod(target, indexes, "get_" + methodName, false, _constraints); @@ -4257,13 +4263,10 @@ private DynamicMetaObject InvokeIndexer(DynamicMetaObject target, // PowerShell supports negative indexing for some types (specifically, types implementing IList or IList). // For those types, generate special code to check for negative indices, otherwise just generate // the call. - PropertyInfo lengthProperty = target.LimitType.GetProperty("Count") ?? - target.LimitType.GetProperty("Length"); // for string - - // Default ITuple implementations explicitly implement ITuple.Length. - if (lengthProperty == null && typeof(ITuple).IsAssignableFrom(target.LimitType)) + if (lengthProperty == null) { - lengthProperty = typeof(ITuple).GetProperty(nameof(ITuple.Length)); + lengthProperty = target.LimitType.GetProperty("Count") ?? + target.LimitType.GetProperty("Length"); // for string } if (lengthProperty != null) From b9270435c72f22294bb1028ffe0ececd782752d9 Mon Sep 17 00:00:00 2001 From: Patrick Meinecke Date: Sun, 26 Aug 2018 08:58:50 -0400 Subject: [PATCH 03/11] Fix Count/Length property search order --- .../engine/runtime/Binding/Binders.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs index 299448f92d6..fff7a848be2 100644 --- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs +++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs @@ -3946,7 +3946,7 @@ public override DynamicMetaObject FallbackGetIndex(DynamicMetaObject target, Dyn defaultMember = i.GetCustomAttribute(inherit: true); if (defaultMember != null) { - lengthProperty = i.GetProperty("Length") ?? i.GetProperty("Count"); + lengthProperty = i.GetProperty("Count") ?? i.GetProperty("Length"); } } } From ff21e59cefe413d77ba0b6ff46058f61afd4f08a Mon Sep 17 00:00:00 2001 From: Patrick Meinecke Date: Mon, 27 Aug 2018 21:04:21 -0400 Subject: [PATCH 04/11] Generalized can index with negative check Changed the method CanIndexFromEndWithNegativeIndex to determine if negative indexing should be allowed to be more generalized. Previously this was implemented by checking to see if the limit type matched a specific set of hard coded types. This was updated to allow negative indexing if the following is true. - The best matching index method takes only one parameter and it is typed as an int - The index parameter is not a generic type parameter of a constructed generic type - A "Count" or "Length" property it also found --- .../engine/runtime/Binding/Binders.cs | 35 ++++++------------- 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs index fff7a848be2..639db6a9f00 100644 --- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs +++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs @@ -4037,31 +4037,20 @@ private DynamicMetaObject GetIndexDictionary(DynamicMetaObject target, bindingRestrictions); } - internal static bool CanIndexFromEndWithNegativeIndex(DynamicMetaObject target) + internal static bool CanIndexFromEndWithNegativeIndex( + DynamicMetaObject target, + MethodInfo indexer, + ParameterInfo[] getterParams) { - var limitType = target.LimitType; - if (limitType.IsArray || limitType == typeof(string) || limitType == typeof(StringBuilder)) - { - return true; - } - - if (typeof(IList).IsAssignableFrom(limitType)) - { - return true; - } - - if (typeof(OrderedDictionary).IsAssignableFrom(limitType)) - { - return true; - } - - // target implements IList? - if (limitType.GetInterfaces().Any(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(IList<>))) + if (getterParams.Length != 1 || getterParams[0].ParameterType != typeof(int)) { - return true; + return false; } - return typeof(ITuple).IsAssignableFrom(limitType); + // Get the base method definition of the indexer to determine if the int + // parameter is a generic type parameter. Module.ResolveMethod is used + // because the indexer could be a method from a constructed generic type. + return indexer.Module.ResolveMethod(indexer.MetadataToken).ContainsGenericParameters; } private DynamicMetaObject IndexWithNegativeChecks( @@ -4070,8 +4059,6 @@ private DynamicMetaObject IndexWithNegativeChecks( PropertyInfo lengthProperty, Func generateIndexOperation) { - Diagnostics.Assert(CanIndexFromEndWithNegativeIndex(target), "Unexpected target type to index from end with negative value"); - // Generate: // try { // len = obj.Length @@ -4258,7 +4245,7 @@ private DynamicMetaObject InvokeIndexer(DynamicMetaObject target, } } - if (getterParams.Length == 1 && getterParams[0].ParameterType == typeof(int) && CanIndexFromEndWithNegativeIndex(target)) + if (CanIndexFromEndWithNegativeIndex(target, getter, getterParams)) { // PowerShell supports negative indexing for some types (specifically, types implementing IList or IList). // For those types, generate special code to check for negative indices, otherwise just generate From 44dcb5a9afb2c255bd98827dd4947d77fa54f8ea Mon Sep 17 00:00:00 2001 From: Patrick Meinecke Date: Mon, 27 Aug 2018 21:07:32 -0400 Subject: [PATCH 05/11] Added comment for additional default member check --- .../engine/runtime/Binding/Binders.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs index 639db6a9f00..434b746eb7c 100644 --- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs +++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs @@ -3941,6 +3941,9 @@ public override DynamicMetaObject FallbackGetIndex(DynamicMetaObject target, Dyn } } + // If the type explicitly implements an indexer specified by an interface + // then the DefaultMemberAttribute will not carry over to the implementation. + // This check will catch those cases. if (defaultMember == null) { defaultMember = i.GetCustomAttribute(inherit: true); From 6cd74058d9b5a81a27fc79c72d5888f30989a7c5 Mon Sep 17 00:00:00 2001 From: Patrick Meinecke Date: Mon, 27 Aug 2018 21:47:21 -0400 Subject: [PATCH 06/11] Fix is generic parameter logic for negative index --- .../engine/runtime/Binding/Binders.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs index d67be728166..73ec910d26e 100644 --- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs +++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs @@ -4053,7 +4053,8 @@ internal static bool CanIndexFromEndWithNegativeIndex( // Get the base method definition of the indexer to determine if the int // parameter is a generic type parameter. Module.ResolveMethod is used // because the indexer could be a method from a constructed generic type. - return indexer.Module.ResolveMethod(indexer.MetadataToken).ContainsGenericParameters; + MethodBase baseMethod = indexer.Module.ResolveMethod(indexer.MetadataToken); + return !baseMethod.GetParameters()[0].ParameterType.IsGenericParameter; } private DynamicMetaObject IndexWithNegativeChecks( From 77987167566b8fdde71ccbab1a3711485d2cf04d Mon Sep 17 00:00:00 2001 From: Patrick Meinecke Date: Tue, 28 Aug 2018 07:31:30 -0400 Subject: [PATCH 07/11] Add additional comments and fix extra new line --- .../engine/runtime/Binding/Binders.cs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs index 73ec910d26e..059f7b04e29 100644 --- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs +++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs @@ -4249,7 +4249,6 @@ private DynamicMetaObject InvokeIndexer(DynamicMetaObject target, } } - // Check return type after the argument conversion, so any no-conversion error can be thrown. if (getter.ReturnType.IsByRefLike) { @@ -4268,13 +4267,25 @@ private DynamicMetaObject InvokeIndexer(DynamicMetaObject target, if (CanIndexFromEndWithNegativeIndex(target, getter, getterParams)) { - // PowerShell supports negative indexing for some types (specifically, types implementing IList or IList). + // PowerShell supports negative indexing for for types that meet the following criteria: + // + // - Indexer method accepts one parameter that is typed as int + // + // - The int parameter is not a type argument from a constructed generic type (this is + // to exclude indexers for types that could use a negative index as a valid key + // like System.Linq.ILookup) + // + // - Declares a "Count" or "Length" property + // + // - Does not inherit from IDictionary<> as that is handled earlier in the binder + // // For those types, generate special code to check for negative indices, otherwise just generate // the call. if (lengthProperty == null) { + // Count is declared by most supported types, Length will catch some edge cases like strings. lengthProperty = target.LimitType.GetProperty("Count") ?? - target.LimitType.GetProperty("Length"); // for string + target.LimitType.GetProperty("Length"); } if (lengthProperty != null) From 1660c780e90f4bf915dc089c1097459d8213a052 Mon Sep 17 00:00:00 2001 From: Patrick Meinecke Date: Tue, 28 Aug 2018 13:15:57 -0400 Subject: [PATCH 08/11] Fix typo and change comment formatting --- .../engine/runtime/Binding/Binders.cs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs index 059f7b04e29..bd05f17c5e4 100644 --- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs +++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs @@ -4267,18 +4267,13 @@ private DynamicMetaObject InvokeIndexer(DynamicMetaObject target, if (CanIndexFromEndWithNegativeIndex(target, getter, getterParams)) { - // PowerShell supports negative indexing for for types that meet the following criteria: - // - // - Indexer method accepts one parameter that is typed as int - // - // - The int parameter is not a type argument from a constructed generic type (this is - // to exclude indexers for types that could use a negative index as a valid key - // like System.Linq.ILookup) - // - // - Declares a "Count" or "Length" property - // - // - Does not inherit from IDictionary<> as that is handled earlier in the binder - // + // PowerShell supports negative indexing for types that meet the following criteria: + // - Indexer method accepts one parameter that is typed as int + // - The int parameter is not a type argument from a constructed generic type + // (this is to exclude indexers for types that could use a negative index as + // a valid key like System.Linq.ILookup) + // - Declares a "Count" or "Length" property + // - Does not inherit from IDictionary<> as that is handled earlier in the binder // For those types, generate special code to check for negative indices, otherwise just generate // the call. if (lengthProperty == null) From 7a92042b78b4a1ea1f1ad7aa3fe8d52667163cef Mon Sep 17 00:00:00 2001 From: Patrick Meinecke Date: Wed, 29 Aug 2018 18:34:35 -0400 Subject: [PATCH 09/11] Add original type checks back to CanNegativeIndex --- .../engine/runtime/Binding/Binders.cs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs index bd05f17c5e4..5d512a88d89 100644 --- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs +++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs @@ -4045,6 +4045,28 @@ internal static bool CanIndexFromEndWithNegativeIndex( MethodInfo indexer, ParameterInfo[] getterParams) { + Type limitType = target.LimitType; + if (limitType.IsArray || limitType == typeof(string) || limitType == typeof(StringBuilder)) + { + return true; + } + + if (typeof(IList).IsAssignableFrom(limitType)) + { + return true; + } + + if (typeof(OrderedDictionary).IsAssignableFrom(limitType)) + { + return true; + } + + // target implements IList? + if (limitType.GetInterfaces().Any(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(IList<>))) + { + return true; + } + if (getterParams.Length != 1 || getterParams[0].ParameterType != typeof(int)) { return false; From 0a81b2ff1c5cb9d7992b10833c43d62737675f49 Mon Sep 17 00:00:00 2001 From: Patrick Meinecke Date: Wed, 29 Aug 2018 18:37:37 -0400 Subject: [PATCH 10/11] Change method to get attributes and move comment --- .../engine/runtime/Binding/Binders.cs | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs index 5d512a88d89..18eee08767a 100644 --- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs +++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs @@ -3946,7 +3946,7 @@ public override DynamicMetaObject FallbackGetIndex(DynamicMetaObject target, Dyn // This check will catch those cases. if (defaultMember == null) { - defaultMember = i.GetCustomAttribute(inherit: true); + defaultMember = i.GetCustomAttributes(inherit: false).FirstOrDefault(); if (defaultMember != null) { lengthProperty = i.GetProperty("Count") ?? i.GetProperty("Length"); @@ -4045,6 +4045,17 @@ internal static bool CanIndexFromEndWithNegativeIndex( MethodInfo indexer, ParameterInfo[] getterParams) { + // PowerShell supports negative indexing for types that meet the following criteria: + // - Indexer method accepts one parameter that is typed as int + // - The int parameter is not a type argument from a constructed generic type + // (this is to exclude indexers for types that could use a negative index as + // a valid key like System.Linq.ILookup) + // - Declares a "Count" or "Length" property + // - Does not inherit from IDictionary<> as that is handled earlier in the binder + // For those types, generate special code to check for negative indices, otherwise just generate + // the call. Before we test for the above criteria explicitly, we will determine if the + // target is of a type known to be compatible. This is done to avoid the call to Module.ResolveMethod + // when possible. Type limitType = target.LimitType; if (limitType.IsArray || limitType == typeof(string) || limitType == typeof(StringBuilder)) { @@ -4289,15 +4300,6 @@ private DynamicMetaObject InvokeIndexer(DynamicMetaObject target, if (CanIndexFromEndWithNegativeIndex(target, getter, getterParams)) { - // PowerShell supports negative indexing for types that meet the following criteria: - // - Indexer method accepts one parameter that is typed as int - // - The int parameter is not a type argument from a constructed generic type - // (this is to exclude indexers for types that could use a negative index as - // a valid key like System.Linq.ILookup) - // - Declares a "Count" or "Length" property - // - Does not inherit from IDictionary<> as that is handled earlier in the binder - // For those types, generate special code to check for negative indices, otherwise just generate - // the call. if (lengthProperty == null) { // Count is declared by most supported types, Length will catch some edge cases like strings. From 8ded034075459f6d48bb441c971ee4819e6e8029 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Wed, 29 Aug 2018 16:00:26 -0700 Subject: [PATCH 11/11] [Feature] Rearrange the check order in CanIndexFromEndWithNegativeIndex --- .../engine/runtime/Binding/Binders.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs index 18eee08767a..67e2fb8b899 100644 --- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs +++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs @@ -4056,6 +4056,12 @@ internal static bool CanIndexFromEndWithNegativeIndex( // the call. Before we test for the above criteria explicitly, we will determine if the // target is of a type known to be compatible. This is done to avoid the call to Module.ResolveMethod // when possible. + + if (getterParams.Length != 1 || getterParams[0].ParameterType != typeof(int)) + { + return false; + } + Type limitType = target.LimitType; if (limitType.IsArray || limitType == typeof(string) || limitType == typeof(StringBuilder)) { @@ -4078,11 +4084,6 @@ internal static bool CanIndexFromEndWithNegativeIndex( return true; } - if (getterParams.Length != 1 || getterParams[0].ParameterType != typeof(int)) - { - return false; - } - // Get the base method definition of the indexer to determine if the int // parameter is a generic type parameter. Module.ResolveMethod is used // because the indexer could be a method from a constructed generic type.