GeezerVison

This doesn't feel like the right fix. ITuple specifies a DefaultMemberAttribute, but we don't find it above because GetCustomAttributes is only checking classes, not implemented interfaces.

The correct fix is to search interfaces for DefaultMemberAttribute - there are other interfaces that specify DefaultMemberAttribute, and some of those are not currently handled correctly:

System.Collections.IDictionary
System.Collections.IList
System.Collections.Generic.IDictionary`2
System.Collections.Generic.IList`1
System.Collections.Generic.IReadOnlyDictionary`2
System.Collections.Generic.IReadOnlyList`1
System.Runtime.CompilerServices.ITuple
System.Runtime.CompilerServices.IRuntimeVariables
System.Collections.Specialized.IOrderedDictionary
System.Linq.ILookup`2
System.Data.IColumnMappingCollection
System.Data.IDataParameterCollection
System.Data.IDataRecord
System.Data.ITableMappingCollection
System.ComponentModel.IDataErrorInfo
Newtonsoft.Json.Linq.IJEnumerable`1

Jason Shirk (@lzybkr) Good call! I had checked that none of the default ITuple implementations had that attribute but didn't think to check the interface itself.

I've updated the change to look for DefaultMemberAttribute in the loop that looks for IDictionary<,>. I tested the update with a custom ILookup<,> with an explicitly implemented indexer, that didn't work previously but does now.

Do now we support all these interfaces? If so, it's good to have a comment in the code and tests (or even extend the tests)
Also we need to document this.

Ilya (@iSazonov) They were already supported. What wasn't supported was indexing into classes that explicitly implement the indexer of an interface and define no other indexer.

All ITuple implementations explicitly implement the ITuple indexer, so while indexing tuples was technically supported, it wasn't working.

Patrick Meinecke (@SeeminglyScience) Sorry, I did not ask the question correctly. Your explanation is useful and I just believe that it will make it easier to understand the binder if it's in the code.

Ilya (@iSazonov) Ah yes absolutely, I'll add that tonight.

Ilya (@iSazonov) I've added a comment explaining purpose of the extra check in the loop.

I wonder if this whole method shouldn't just check for a Count or Length property.

I think that would be a breaking change. For example, if a Dictionary<,> could make it that far into the binder (I know it can't, but custom code might) then -1 could be a valid key.

I did a quick scan of the types visible by default in RC1, looking for types that:

1. Are decorated with DefaultMemberAttribute
2. Do not implement ICollection or ICollection<> (as the Count property coming from these make it likely enough to work as expected)
3. Do not implement IDictionary<,> (as they'll be handled earlier in the binder)

There's a couple types that jump out at a glance as possibly an issue, depending on how imperative it is to preserve that behavior.

Here's the list in full, a couple obvious ones that jump out are System.Linq.ILookup<,>, ReadOnlyPSMemberInfoCollection<>, and some CIM related classes like CimMethodParameterCollection.

System.String
System.ReadOnlySpan`1[T]
System.Span`1[T]
System.Numerics.Vector`1[T]
System.Text.StringBuilder
System.Runtime.CompilerServices.ITuple
System.Management.Automation.PSEventArgsCollection
System.Management.Automation.ReadOnlyPSMemberInfoCollection`1[T]
System.Management.Automation.Runspaces.InitialSessionStateEntryCollection`1[T]
System.Runtime.CompilerServices.IRuntimeVariables
Microsoft.Management.Infrastructure.CimMethodParametersCollection
Microsoft.Management.Infrastructure.Generic.CimKeyedCollection`1[T]
Microsoft.Management.Infrastructure.Generic.CimReadOnlyKeyedCollection`1[T]
System.Xml.XmlCDataSection
System.Xml.XmlCharacterData
System.Xml.XmlComment
System.Xml.XmlNodeList
System.Xml.XmlSignificantWhitespace
System.Xml.XmlText
System.Xml.XmlWhitespace
System.Xml.Serialization.XmlMembersMapping
System.Xml.Schema.XmlSchemaObjectTable
System.Collections.Specialized.StringDictionary
System.Linq.ILookup`2[TKey,TElement]
System.Linq.Lookup`2[TKey,TElement]
System.Data.SqlTypes.SqlChars
System.Data.SqlTypes.SqlBinary
System.Data.SqlTypes.SqlBytes
System.ComponentModel.MaskedTextProvider
Microsoft.CodeAnalysis.ChildSyntaxList
Microsoft.CodeAnalysis.SeparatedSyntaxList`1[TNode]
Microsoft.CodeAnalysis.SyntaxList`1[TNode]
Microsoft.CodeAnalysis.SyntaxNodeOrTokenList
Microsoft.CodeAnalysis.SyntaxTokenList
Microsoft.CodeAnalysis.SyntaxTriviaList
Microsoft.CodeAnalysis.Text.SourceText
Microsoft.CodeAnalysis.Text.TextLineCollection
System.Reflection.Metadata.GenericParameterHandleCollection
System.Reflection.Metadata.GenericParameterConstraintHandleCollection
Markdig.Helpers.StringSlice

While not incredibly likely to actually break existing code, perhaps it should be something the committee discusses?

The risk is that an indexer supports negative values as an index somehow, e.g. as the key in a dictionary.

Based on your list, StringDictionary, Lookup`2 and ILookup`2 do look like they'd support a negative value as the index.

Maybe the check should be - the indexer accepts a single int and is not a generic parameter. This avoids the problem with StringDictionary with the signature string Item(string key) {get;set;} and ILookup[int,T]] where even though the indexer takes an int, it was generic and therefore not always an int.

Jason Shirk (@lzybkr) I've refactored that method. It now checks that:

• The best overload match has one parameter and it's typed int (existing check)

• The type also defines a Count or Length property (existing check)

• The single int parameter is not a type argument of a constructed generic type (new check)

Turns out this parameter is not used within the method.

I think we probably should keep the existing checks for the common scenarios, since Module.ResolveMethod is relatively expensive.

Great comments! I think they should be moved to the method CanIndexFromEndWithNegativeIndex 😄

A minor comment: Attributes cannot be inherited from interfaces, so it should be inherit: false.