Changes per PR comments

src/Mvc/Mvc.Abstractions/src/ModelBinding/IPropertyFilterProvider.cs

@@ -6,12 +6,14 @@
 namespace Microsoft.AspNetCore.Mvc.ModelBinding
 {
     /// <summary>
-    /// Provides a predicate which can determines which model properties should be bound by model binding.
+    /// Provides a predicate which can determines which model properties or parameters should be bound by model binding.
     /// </summary>
     public interface IPropertyFilterProvider
     {
         /// <summary>
+        /// <para>
         /// Gets a predicate which can determines which model properties should be bound by model binding.
+        /// </para>
         /// <para>
         /// This predicate is also used to determine which parameters are bound when a model's constructor is bound.
         /// </para>

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

@@ -105,11 +105,10 @@ internal IReadOnlyList<ModelMetadata> BoundProperties
         {
             get
             {
-                // An item may appear as both a constructor parameter and a property. For instance, in record types,
-                // each constructor parameter is also a settable property and will have the same name, possibly with a difference in case.
+                // In record types, each constructor parameter in the primary constructor is also a settable property with the same name.
                 // Executing model binding on these parameters twice may have detrimental effects, such as duplicate validation entries,
                 // or failures if a model expects to be bound exactly ones.
-                // Consequently when a bound constructor is present, we only bind and validate the subset of properties whose names
+                // Consequently when binding to a constructor, we only bind and validate the subset of properties whose names
                 // haven't appeared as parameters.
                 if (BoundConstructor is null)
                 {
@@ -118,7 +117,7 @@ internal IReadOnlyList<ModelMetadata> BoundProperties
 
                 if (_boundProperties is null)
                 {
-                    var boundParameters = BoundConstructor.Parameters!;
+                    var boundParameters = BoundConstructor.BoundConstructorParameters!;
                     var boundProperties = new List<ModelMetadata>();
 
                     foreach (var metadata in Properties)
@@ -138,7 +137,7 @@ internal IReadOnlyList<ModelMetadata> BoundProperties
             }
         }
 
-        internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> ParameterMapping
+        internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> BoundConstructorParameterMapping
         {
             get
             {
@@ -153,7 +152,7 @@ internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> ParameterMapping
                     return _parameterMapping;
                 }
 
-                var boundParameters = BoundConstructor.Parameters!;
+                var boundParameters = BoundConstructor.BoundConstructorParameters!;
                 var parameterMapping = new Dictionary<ModelMetadata, ModelMetadata>();
 
                 foreach (var parameter in boundParameters)
@@ -179,10 +178,10 @@ internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> ParameterMapping
         public virtual ModelMetadata? BoundConstructor { get; }
 
         /// <summary>
-        /// Gets the collection of <see cref="ModelMetadata"/> instances for a constructor's parameters.
+        /// Gets the collection of <see cref="ModelMetadata"/> instances for parameters on a <see cref="BoundConstructor"/>.
         /// This is only available when <see cref="MetadataKind"/> is <see cref="ModelMetadataKind.Constructor"/>.
         /// </summary>
-        public virtual IReadOnlyList<ModelMetadata>? Parameters { get; }
+        public virtual IReadOnlyList<ModelMetadata>? BoundConstructorParameters { get; }
 
         /// <summary>
         /// Gets the name of a model if specified explicitly using <see cref="IModelNameProvider"/>.
@@ -491,9 +490,9 @@ internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> ParameterMapping
         public abstract Action<object, object> PropertySetter { get; }
 
         /// <summary>
-        /// Gets a delegate that invokes the constructor.
+        /// Gets a delegate that invokes the bound constructor <see cref="BoundConstructor" /> if non-<see langword="null" />.
         /// </summary>
-        public virtual Func<object[], object>? ConstructorInvoker => null;
+        public virtual Func<object[], object>? BoundConstructorInvoker => null;
 
         /// <summary>
         /// Gets a display name for the model.

src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelStateDictionary.cs

@@ -9,7 +9,6 @@
 using System.Runtime.CompilerServices;
 using Microsoft.AspNetCore.Mvc.Abstractions;
 using Microsoft.AspNetCore.Mvc.Formatters;
-using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
 using Microsoft.Extensions.Primitives;
 
 namespace Microsoft.AspNetCore.Mvc.ModelBinding

src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelMetadataTest.cs

@@ -731,9 +731,9 @@ public override Action<object, object> PropertySetter
 
             public override ModelMetadata BoundConstructor => throw new NotImplementedException();
 
-            public override Func<object[], object> ConstructorInvoker => throw new NotImplementedException();
+            public override Func<object[], object> BoundConstructorInvoker => throw new NotImplementedException();
 
-            public override IReadOnlyList<ModelMetadata> Parameters => throw new NotImplementedException();
+            public override IReadOnlyList<ModelMetadata> BoundConstructorParameters => throw new NotImplementedException();
         }
 
         private class CollectionImplementation : ICollection<string>

src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinder.cs

@@ -82,11 +82,11 @@ private async Task BindModelCoreAsync(ModelBindingContext bindingContext, int pr
                 var boundConstructor = modelMetadata.BoundConstructor;
                 if (boundConstructor != null)
                 {
-                    var values = new object[boundConstructor.Parameters.Count];
+                    var values = new object[boundConstructor.BoundConstructorParameters.Count];
                     var (attemptedParameterBinding, parameterBindingSucceeded) = await BindParameters(
                         bindingContext,
                         propertyData,
-                        boundConstructor.Parameters,
+                        boundConstructor.BoundConstructorParameters,
                         values);
 
                     attemptedBinding |= attemptedParameterBinding;
@@ -162,7 +162,7 @@ internal static bool CreateModel(ModelBindingContext bindingContext, ModelMetada
         {
             try
             {
-                bindingContext.Model = boundConstructor.ConstructorInvoker(values);
+                bindingContext.Model = boundConstructor.BoundConstructorInvoker(values);
                 return true;
             }
             catch (Exception ex)
@@ -552,7 +552,7 @@ private int CanBindAnyModelItem(ModelBindingContext bindingContext)
             var performsConstructorBinding = bindingContext.Model == null && modelMetadata.BoundConstructor != null;
 
             if (modelMetadata.Properties.Count == 0 &&
-                 (!performsConstructorBinding || modelMetadata.BoundConstructor.Parameters.Count == 0))
+                 (!performsConstructorBinding || modelMetadata.BoundConstructor.BoundConstructorParameters.Count == 0))
             {
                 Log.NoPublicSettableItems(_logger, bindingContext);
                 return NoDataAvailable;
@@ -617,7 +617,7 @@ private int CanBindAnyModelItem(ModelBindingContext bindingContext)
 
             if (performsConstructorBinding)
             {
-                var parameters = bindingContext.ModelMetadata.BoundConstructor.Parameters;
+                var parameters = bindingContext.ModelMetadata.BoundConstructor.BoundConstructorParameters;
                 for (var i = 0; i < parameters.Count; i++)
                 {
                     var parameterMetadata = parameters[i];

src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinderProvider.cs

@@ -48,11 +48,11 @@ private static IReadOnlyList<IModelBinder> GetParameterBinders(ModelBinderProvid
                 return Array.Empty<IModelBinder>();
             }
 
-            var parameterBinders = boundConstructor.Parameters.Count == 0 ? Array.Empty<IModelBinder>() : new IModelBinder[boundConstructor.Parameters.Count];
+            var parameterBinders = boundConstructor.BoundConstructorParameters.Count == 0 ? Array.Empty<IModelBinder>() : new IModelBinder[boundConstructor.BoundConstructorParameters.Count];
 
             for (var i = 0; i < parameterBinders.Length; i++)
             {
-                parameterBinders[i] = context.CreateBinder(boundConstructor.Parameters[i]);
+                parameterBinders[i] = context.CreateBinder(boundConstructor.BoundConstructorParameters[i]);
             }
 
             return parameterBinders;

src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs

@@ -104,8 +104,9 @@ private static ConstructorInfo GetRecordTypeConstructor(Type type, ConstructorIn
             }
 
             // For record types, we will support binding and validating the primary constructor.
-            // There isn't metadata to identify a primary constructor. We require that at most one constructor is defined on the type,
-            // and that every parameter on the constructor has a corresponding property.
+            // There isn't metadata to identify a primary constructor. Our heuristic is:
+            // We require exactly one constructor to be defined on the type, and that every parameter on
+            // that constructor is mapped to a property with the same name and type.
 
             if (constructors.Length > 1)
             {
@@ -135,7 +136,9 @@ private static ConstructorInfo GetRecordTypeConstructor(Type type, ConstructorIn
 
             static bool IsRecordType(Type type)
             {
-                return type.GetProperty("EqualityContract", BindingFlags.NonPublic | BindingFlags.Instance) != null;
+                // Based on the state of the art as described in https://github.com/dotnet/roslyn/issues/45777
+                var cloneMethod = type.GetMethod("<>Clone", BindingFlags.Public | BindingFlags.Instance);
+                return cloneMethod != null && cloneMethod.ReturnType == type;
             }
         }
 

src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultMetadataDetails.cs

@@ -57,7 +57,7 @@ public DefaultMetadataDetails(ModelMetadataIdentity key, ModelAttributes attribu
         /// <summary>
         /// Gets or sets the <see cref="ModelMetadata"/> entries for constructor parameters.
         /// </summary>
-        public ModelMetadata[] ConstructorParameters { get; set; }
+        public ModelMetadata[] BoundConstructorParameters { get; set; }
 
         /// <summary>
         /// Gets or sets a property getter delegate to get the property value from a model object.
@@ -70,7 +70,7 @@ public DefaultMetadataDetails(ModelMetadataIdentity key, ModelAttributes attribu
         public Action<object, object> PropertySetter { get; set; }
 
         /// <summary>
-        /// Gets or sets a delegate used to construct an object using the model binding constructor.
+        /// Gets or sets a delegate used to invoke the bound constructor for record types.
         /// </summary>
         public Func<object[], object> BoundConstructorInvoker { get; set; }
 

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

@@ -407,7 +407,7 @@ public override ModelMetadata BoundConstructor
             }
         }
 
-        public override IReadOnlyList<ModelMetadata> Parameters => _details.ConstructorParameters;
+        public override IReadOnlyList<ModelMetadata> BoundConstructorParameters => _details.BoundConstructorParameters;
 
         /// <inheritdoc />
         public override IPropertyFilterProvider PropertyFilterProvider => BindingMetadata.PropertyFilterProvider;
@@ -517,7 +517,7 @@ internal static bool CalculateHasValidators(HashSet<DefaultModelMetadata> visite
             }
             else if (defaultModelMetadata.IsComplexType)
             {
-                var parameters = defaultModelMetadata.BoundConstructor?.Parameters ?? Array.Empty<ModelMetadata>();
+                var parameters = defaultModelMetadata.BoundConstructor?.BoundConstructorParameters ?? Array.Empty<ModelMetadata>();
                 foreach (var parameter in parameters)
                 {
                     if (CalculateHasValidators(visited, parameter))
@@ -559,7 +559,7 @@ public override IReadOnlyList<object> ValidatorMetadata
         /// <inheritdoc />
         public override Action<object, object> PropertySetter => _details.PropertySetter;
 
-        public override Func<object[], object> ConstructorInvoker => _details.BoundConstructorInvoker;
+        public override Func<object[], object> BoundConstructorInvoker => _details.BoundConstructorInvoker;
 
         internal DefaultMetadataDetails Details => _details;
 

src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadataProvider.cs

@@ -5,11 +5,8 @@
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Diagnostics;
-using System.Linq;
 using System.Linq.Expressions;
 using System.Reflection;
-using Microsoft.AspNetCore.Mvc.Infrastructure;
-using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Internal;
 using Microsoft.Extensions.Options;
 
@@ -275,7 +272,7 @@ private DefaultMetadataDetails CreateConstructorDetails(ModelMetadataIdentity co
             }
 
             var constructorDetails = new DefaultMetadataDetails(constructorKey, ModelAttributes.Empty);
-            constructorDetails.ConstructorParameters = parameterMetadata;
+            constructorDetails.BoundConstructorParameters = parameterMetadata;
             constructorDetails.BoundConstructorInvoker = CreateObjectFactory(constructor);
 
             return constructorDetails;
@@ -453,13 +450,5 @@ public ModelMetadataCacheEntry(ModelMetadata metadata, DefaultMetadataDetails de
 
             public DefaultMetadataDetails Details { get; }
         }
-
-        private class NullServiceProvider : IServiceProvider
-        {
-            public static readonly NullServiceProvider Instance = new NullServiceProvider();
-
-            // We do not expect this to be invoked at all.
-            public object GetService(Type serviceType) => throw new NotSupportedException();
-        }
     }
 }

src/Mvc/Mvc.Core/src/ModelBinding/Validation/DefaultComplexObjectValidationStrategy.cs

@@ -4,10 +4,7 @@
 using System;
 using System.Collections;
 using System.Collections.Generic;
-using System.Linq;
-using System.Reflection;
 using Microsoft.AspNetCore.Mvc.Core;
-using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
 
 namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
 {
@@ -19,7 +16,7 @@ internal class DefaultComplexObjectValidationStrategy : IValidationStrategy
         /// <summary>
         /// Gets an instance of <see cref="DefaultComplexObjectValidationStrategy"/>.
         /// </summary>
-        public static readonly DefaultComplexObjectValidationStrategy Instance = new DefaultComplexObjectValidationStrategy();
+        public static readonly IValidationStrategy Instance = new DefaultComplexObjectValidationStrategy();
 
         private DefaultComplexObjectValidationStrategy()
         {
@@ -61,7 +58,7 @@ public Enumerator(
                 }
                 else
                 {
-                    _parameters = _modelMetadata.BoundConstructor.Parameters;
+                    _parameters = _modelMetadata.BoundConstructor.BoundConstructorParameters;
                 }
 
                 _properties = _modelMetadata.BoundProperties;
@@ -95,7 +92,7 @@ public bool MoveNext()
                     }
                     else
                     {
-                        if (!_modelMetadata.ParameterMapping.TryGetValue(parameter, out var property))
+                        if (!_modelMetadata.BoundConstructorParameterMapping.TryGetValue(parameter, out var property))
                         {
                             throw new InvalidOperationException(
                                 Resources.FormatValidationStrategy_MappedPropertyNotFound(parameter, _modelMetadata.ModelType));

src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs

@@ -1275,7 +1275,7 @@ public void CalculateHasValidators_RecordType_ParametersWithNoValidators()
             var parameterMetadata = CreateModelMetadata(parameterId, metadataProvider.Object, hasValidators: false);
 
             var constructorMetadata = CreateModelMetadata(ModelMetadataIdentity.ForConstructor(constructor, modelType), metadataProvider.Object, null);
-            constructorMetadata.Details.ConstructorParameters = new[]
+            constructorMetadata.Details.BoundConstructorParameters = new[]
             {
                 parameterMetadata,
             };
@@ -1316,7 +1316,7 @@ public void CalculateHasValidators_RecordType_ParametersWithValidators()
             var parameterMetadata = CreateModelMetadata(parameterId, metadataProvider.Object, hasValidators: true);
 
             var constructorMetadata = CreateModelMetadata(ModelMetadataIdentity.ForConstructor(constructor, modelType), metadataProvider.Object, null);
-            constructorMetadata.Details.ConstructorParameters = new[]
+            constructorMetadata.Details.BoundConstructorParameters = new[]
             {
                 parameterMetadata,
             };
@@ -1365,7 +1365,7 @@ public void CalculateHasValidators_RecordTypeWithProperty_NoValidators()
             var parameterMetadata = CreateModelMetadata(parameterId, metadataProvider.Object, hasValidators: false);
 
             var constructorMetadata = CreateModelMetadata(ModelMetadataIdentity.ForConstructor(constructor, modelType), metadataProvider.Object, null);
-            constructorMetadata.Details.ConstructorParameters = new[]
+            constructorMetadata.Details.BoundConstructorParameters = new[]
             {
                 parameterMetadata,
             };
@@ -1409,7 +1409,7 @@ public void CalculateHasValidators_RecordTypeWithProperty_PropertyHasValidators(
             var parameterMetadata = CreateModelMetadata(parameterId, metadataProvider.Object, hasValidators: true);
 
             var constructorMetadata = CreateModelMetadata(ModelMetadataIdentity.ForConstructor(constructor, modelType), metadataProvider.Object, null);
-            constructorMetadata.Details.ConstructorParameters = new[]
+            constructorMetadata.Details.BoundConstructorParameters = new[]
             {
                 parameterMetadata,
             };
@@ -1453,7 +1453,7 @@ public void CalculateHasValidators_RecordTypeWithProperty_MappedPropertyHasValid
             var parameterMetadata = CreateModelMetadata(parameterId, metadataProvider.Object, hasValidators: false);
 
             var constructorMetadata = CreateModelMetadata(ModelMetadataIdentity.ForConstructor(constructor, modelType), metadataProvider.Object, null);
-            constructorMetadata.Details.ConstructorParameters = new[]
+            constructorMetadata.Details.BoundConstructorParameters = new[]
             {
                 parameterMetadata,
             };