Rename `ModelMetadata.IsCollectionType` and add "real" `ModelMetadata.IsCollectionType`

- #3022
- existing `IsCollectionType` -> `IsEnumerableType`
- use new `IsCollectionType` in a few places

src/Microsoft.AspNet.Mvc.Abstractions/ModelBinding/ModelMetadata.cs

@@ -132,7 +132,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
         /// <see cref="ModelMetadata"/> for <c>T</c> if <see cref="ModelType"/> implements
         /// <see cref="IEnumerable{T}"/>. <see cref="ModelMetadata"/> for <c>object</c> if <see cref="ModelType"/>
         /// implements <see cref="IEnumerable"/> but not <see cref="IEnumerable{T}"/>. <c>null</c> otherwise i.e. when
-        /// <see cref="IsCollectionType"/> is <c>false</c>.
+        /// <see cref="IsEnumerableType"/> is <c>false</c>.
         /// </value>
         public abstract ModelMetadata ElementMetadata { get; }
 
@@ -322,10 +322,28 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
         /// Gets a value indicating whether or not <see cref="ModelType"/> is a collection type.
         /// </summary>
         /// <remarks>
-        /// A collection type is defined as a <see cref="Type"/> which is assignable to
+        /// A collection type is defined as a <see cref="Type"/> which is assignable to <see cref="ICollection{T}"/>.
-        /// <see cref="IEnumerable"/>, and is not a <see cref="string"/>.
         /// </remarks>
         public bool IsCollectionType
+        {
+            get
+            {
+                // Ignore non-generic ICollection type. Would require an additional check and that interface is not
+                // used in MVC.
+                var collectionType = ClosedGenericMatcher.ExtractGenericInterface(ModelType, typeof(ICollection<>));
+
+                return collectionType != null;
+            }
+        }
+
+        /// <summary>
+        /// Gets a value indicating whether or not <see cref="ModelType"/> is an enumerable type.
+        /// </summary>
+        /// <remarks>
+        /// An enumerable type is defined as a <see cref="Type"/> which is assignable to
+        /// <see cref="IEnumerable"/>, and is not a <see cref="string"/>.
+        /// </remarks>
+        public bool IsEnumerableType
         {
             get
             {

src/Microsoft.AspNet.Mvc.Abstractions/project.json

@@ -10,6 +10,10 @@
   },
   "dependencies": {
     "Microsoft.AspNet.Routing": "1.0.0-*",
+    "Microsoft.Framework.ClosedGenericMatcher.Sources": {
+      "version": "1.0.0-*",
+      "type": "build"
+    },
     "Microsoft.Framework.CopyOnWriteDictionary.Sources": {
       "version": "1.0.0-*",
       "type": "build"

src/Microsoft.AspNet.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs

@@ -544,7 +544,7 @@ namespace Microsoft.AspNet.Mvc.ApiExplorer
                 //
                 //  3)  Types with no properties. Obviously nothing to explore there.
                 //
-                if (modelMetadata.IsCollectionType ||
+                if (modelMetadata.IsEnumerableType ||
                     !modelMetadata.IsComplexType ||
                     !modelMetadata.Properties.Any())
                 {

src/Microsoft.AspNet.Mvc.Core/Controllers/DefaultControllerActionArgumentBinder.cs

@@ -124,13 +124,11 @@ namespace Microsoft.AspNet.Mvc.Controllers
                 var propertyHelper = propertyHelpers.First(helper =>
                     string.Equals(helper.Name, property.Key, StringComparison.Ordinal));
                 var propertyType = propertyHelper.Property.PropertyType;
+                var metadata = _modelMetadataProvider.GetMetadataForType(propertyType);
                 var source = property.Value;
                 if (propertyHelper.Property.CanWrite && propertyHelper.Property.SetMethod?.IsPublic == true)
                 {
-                    // Handle settable property.
+                    // Handle settable property. Do not set the property to null if the type is a non-nullable type.
-                    var metadata = _modelMetadataProvider.GetMetadataForType(propertyType);
-
-                    // Do not set the property to null if the type is a non-nullable type.
                     if (source != null || metadata.IsReferenceOrNullableType)
                     {
                         propertyHelper.SetValue(controller, source);
@@ -153,12 +151,7 @@ namespace Microsoft.AspNet.Mvc.Controllers
                     continue;
                 }
 
-                // Determine T if this is an ICollection<T> property.
+                if (!metadata.IsCollectionType)
-                var collectionTypeArguments = ClosedGenericMatcher.ExtractGenericInterface(
-                        propertyType,
-                        typeof(ICollection<>))
-                    ?.GenericTypeArguments;
-                if (collectionTypeArguments == null)
                 {
                     // Not a collection model.
                     continue;
@@ -166,7 +159,7 @@ namespace Microsoft.AspNet.Mvc.Controllers
 
                 // Handle a read-only collection property.
                 var propertyAddRange = CallPropertyAddRangeOpenGenericMethod.MakeGenericMethod(
-                    collectionTypeArguments);
+                    metadata.ElementMetadata.ModelType);
                 propertyAddRange.Invoke(obj: null, parameters: new[] { target, source });
             }
         }

src/Microsoft.AspNet.Mvc.Core/ModelBinding/GenericModelBinder.cs

@@ -117,9 +117,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
                 // ICollection<T>. For example an IEnumerable<T> property may have a List<T> default value. Do not use
                 // IsAssignableFrom() because that does not handle explicit interface implementations and binders all
                 // perform explicit casts.
-                var closedGenericInterface =
+                if (!context.ModelMetadata.IsCollectionType)
-                    ClosedGenericMatcher.ExtractGenericInterface(context.Model.GetType(), typeof(ICollection<>));
-                if (closedGenericInterface == null)
                 {
                     return null;
                 }

src/Microsoft.AspNet.Mvc.Core/ModelBinding/Metadata/DefaultModelMetadata.cs

@@ -236,10 +236,10 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
                 if (!_haveCalculatedElementMetadata)
                 {
                     _haveCalculatedElementMetadata = true;
-                    if (!IsCollectionType)
+                    if (!IsEnumerableType)
                     {
-                        // Short-circuit checks below. If not IsCollectionType, ElementMetadata is null.
+                        // Short-circuit checks below. If not IsEnumerableType, ElementMetadata is null.
-                        // For example, as in IsCollectionType, do not consider strings collections.
+                        // For example, as in IsEnumerableType, do not consider strings collections.
                         return null;
                     }
 
@@ -261,7 +261,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
 
                     Debug.Assert(
                         elementType != null,
-                        $"Unable to find element type for '{ ModelType.FullName }' though IsCollectionType is true.");
+                        $"Unable to find element type for '{ ModelType.FullName }' though IsEnumerableType is true.");
 
                     // Success
                     _elementMetadata = _provider.GetMetadataForType(elementType);

src/Microsoft.AspNet.Mvc.Core/ModelBinding/MutableObjectModelBinder.cs

@@ -231,7 +231,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
                 return false;
             }
 
-            if (modelMetadata.IsCollectionType)
+            if (modelMetadata.IsEnumerableType)
             {
                 return false;
             }
@@ -522,17 +522,14 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             // Determine T if this is an ICollection<T> property. No need for a T[] case because CanUpdateProperty()
             // ensures property is either settable or not an array. Underlying assumption is that CanUpdateProperty()
             // and SetProperty() are overridden together.
-            var collectionTypeArguments = ClosedGenericMatcher.ExtractGenericInterface(
+            if (!propertyExplorer.Metadata.IsCollectionType)
-                    propertyExplorer.ModelType,
-                    typeof(ICollection<>))
-                ?.GenericTypeArguments;
-            if (collectionTypeArguments == null)
             {
                 // Not a collection model.
                 return;
             }
 
-            var propertyAddRange = CallPropertyAddRangeOpenGenericMethod.MakeGenericMethod(collectionTypeArguments);
+            var propertyAddRange = CallPropertyAddRangeOpenGenericMethod.MakeGenericMethod(
+                propertyExplorer.Metadata.ElementMetadata.ModelType);
             try
             {
                 propertyAddRange.Invoke(obj: null, parameters: new[] { target, source });

src/Microsoft.AspNet.Mvc.TagHelpers/SelectTagHelper.cs

@@ -90,7 +90,7 @@ namespace Microsoft.AspNet.Mvc.TagHelpers
 
             // Base allowMultiple on the instance or declared type of the expression to avoid a
             // "SelectExpressionNotEnumerable" InvalidOperationException during generation.
-            // Metadata.IsCollectionType() is similar but does not take runtime type into account.
+            // Metadata.IsEnumerableType is similar but does not take runtime type into account.
             var realModelType = For.ModelExplorer.ModelType;
             var allowMultiple = typeof(string) != realModelType &&
                 typeof(IEnumerable).GetTypeInfo().IsAssignableFrom(realModelType.GetTypeInfo());

src/Microsoft.AspNet.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs

@@ -818,7 +818,7 @@ namespace Microsoft.AspNet.Mvc.ViewFeatures
             modelExplorer = modelExplorer ??
                 ExpressionMetadataProvider.FromStringExpression(expression, viewContext.ViewData, _metadataProvider);
             var metadata = modelExplorer.Metadata;
-            if (allowMultiple && metadata.IsCollectionType)
+            if (allowMultiple && metadata.IsEnumerableType)
             {
                 metadata = metadata.ElementMetadata;
             }

test/Microsoft.AspNet.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs

@@ -50,7 +50,8 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             Assert.True(modelMetadata.IsComplexType);
         }
 
-        // IsCollectionType
+        // IsCollectionType / IsEnumerableType
+
         private class NonCollectionType
         {
         }
@@ -59,11 +60,49 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
         {
         }
 
+        private class JustEnumerable : IEnumerable
+        {
+            public IEnumerator GetEnumerator()
+            {
+                throw new NotImplementedException();
+            }
+        }
+
+        public static TheoryData<Type> NonCollectionNonEnumerableData
+        {
+            get
+            {
+                return new TheoryData<Type>
+                {
+                    typeof(object),
+                    typeof(int),
+                    typeof(NonCollectionType),
+                    typeof(string),
+                };
+            }
+        }
+
+        public static TheoryData<Type> CollectionAndEnumerableData
+        {
+            get
+            {
+                return new TheoryData<Type>
+                {
+                    typeof(int[]),
+                    typeof(List<string>),
+                    typeof(DerivedList),
+                    typeof(Collection<int>),
+                    typeof(Dictionary<object, object>),
+                    typeof(CollectionImplementation),
+                };
+            }
+        }
+
         [Theory]
-        [InlineData(typeof(object))]
+        [MemberData(nameof(NonCollectionNonEnumerableData))]
-        [InlineData(typeof(int))]
+        [InlineData(typeof(IEnumerable))]
-        [InlineData(typeof(NonCollectionType))]
+        [InlineData(typeof(IEnumerable<string>))]
-        [InlineData(typeof(string))]
+        [InlineData(typeof(JustEnumerable))]
         public void IsCollectionType_ReturnsFalseForNonCollectionTypes(Type type)
         {
             // Arrange
@@ -77,13 +116,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
         }
 
         [Theory]
-        [InlineData(typeof(int[]))]
+        [MemberData(nameof(CollectionAndEnumerableData))]
-        [InlineData(typeof(List<string>))]
-        [InlineData(typeof(DerivedList))]
-        [InlineData(typeof(IEnumerable))]
-        [InlineData(typeof(IEnumerable<string>))]
-        [InlineData(typeof(Collection<int>))]
-        [InlineData(typeof(Dictionary<object, object>))]
         public void IsCollectionType_ReturnsTrueForCollectionTypes(Type type)
         {
             // Arrange
@@ -96,6 +129,37 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
             Assert.True(modelMetadata.IsCollectionType);
         }
 
+        [Theory]
+        [MemberData(nameof(NonCollectionNonEnumerableData))]
+        public void IsEnumerableType_ReturnsFalseForNonEnumerableTypes(Type type)
+        {
+            // Arrange
+            var provider = new EmptyModelMetadataProvider();
+
+            // Act
+            var modelMetadata = new TestModelMetadata(type);
+
+            // Assert
+            Assert.False(modelMetadata.IsEnumerableType);
+        }
+
+        [Theory]
+        [MemberData(nameof(CollectionAndEnumerableData))]
+        [InlineData(typeof(IEnumerable))]
+        [InlineData(typeof(IEnumerable<string>))]
+        [InlineData(typeof(JustEnumerable))]
+        public void IsEnumerableType_ReturnsTrueForEnumerableTypes(Type type)
+        {
+            // Arrange
+            var provider = new EmptyModelMetadataProvider();
+
+            // Act
+            var modelMetadata = new TestModelMetadata(type);
+
+            // Assert
+            Assert.True(modelMetadata.IsEnumerableType);
+        }
+
         // IsNullableValueType
 
         [Theory]
@@ -481,5 +545,59 @@ namespace Microsoft.AspNet.Mvc.ModelBinding
                 }
             }
         }
+
+        private class CollectionImplementation : ICollection<string>
+        {
+            public int Count
+            {
+                get
+                {
+                    throw new NotImplementedException();
+                }
+            }
+
+            public bool IsReadOnly
+            {
+                get
+                {
+                    throw new NotImplementedException();
+                }
+            }
+
+            public void Add(string item)
+            {
+                throw new NotImplementedException();
+            }
+
+            public void Clear()
+            {
+                throw new NotImplementedException();
+            }
+
+            public bool Contains(string item)
+            {
+                throw new NotImplementedException();
+            }
+
+            public void CopyTo(string[] array, int arrayIndex)
+            {
+                throw new NotImplementedException();
+            }
+
+            public IEnumerator<string> GetEnumerator()
+            {
+                throw new NotImplementedException();
+            }
+
+            public bool Remove(string item)
+            {
+                throw new NotImplementedException();
+            }
+
+            IEnumerator IEnumerable.GetEnumerator()
+            {
+                throw new NotImplementedException();
+            }
+        }
     }
 }

test/Microsoft.AspNet.Mvc.Core.Test/ModelBinding/Metadata/DefaultModelMetadataTest.cs

@@ -40,8 +40,9 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
             Assert.True(metadata.HtmlEncode);
             Assert.True(metadata.IsBindingAllowed);
             Assert.False(metadata.IsBindingRequired);
-            Assert.False(metadata.IsComplexType);
             Assert.False(metadata.IsCollectionType);
+            Assert.False(metadata.IsComplexType);
+            Assert.False(metadata.IsEnumerableType);
             Assert.False(metadata.IsEnum);
             Assert.False(metadata.IsFlagsEnum);
             Assert.False(metadata.IsNullableValueType);
@@ -218,7 +219,7 @@ namespace Microsoft.AspNet.Mvc.ModelBinding.Metadata
 
             // Act
             var isBindingRequired = metadata.IsBindingRequired;
-            
+
             // Assert
             Assert.False(isBindingRequired);
         }