Rework `ViewDataDictionary` constructors to ensure `ModelMetadata` is never `null`

- `ViewDataDictionary<TModel>` constructors now pass `typeof(TModel)` to base
  `protected` constructors
 - move type compatibility checks into base `ViewDataDictionary`
 - remove `ViewDataDictionary<TModel>.ModelMetadata` override
- don't retrieve `ModelMetadata` twice in a single constructor invocation
- remove newly-unused `protected` properties and use `private` fields in copy
  constructors

Address longstanding problems found (see #1466)
- avoid reusing `ModelMetadata` after `Model` value changes
 - reset `ModelMetadata` backing field in `Model` setter
 - `Model` and `ModelMetadata.Model` could previously get out of sync
- carry `ModelMetadata` forward from an outer scope only if `Model` matches
 - previously two scopes could have different `Model` values but share their
   `ModelMetadata` (and `ModelMetadata.Model`)
 - related to previous item but didn't require `Model` setting; switching
   to a property of the same type as containing `Model` was enough
- avoid NRE if `ViewDataDictionary<int>.Model` is read before it's written
 - problem affected all non-`Nullable` value types
- `ViewDataDictionary.ModelMetadata` setter should throw if value is `null`

nits:
- add and reword doc and code comments
 - `ViewDataDictionary<TModel>` constructors should only inherit base's
   parameter descriptions; have more information in the derived class
- make a few `ViewDataDictionary` properties get-only
- clean up `using` statements in `ViewDataDictionary<TModel>`
- make two constructors `internal`

src/Microsoft.AspNet.Mvc.Core/Properties/AssemblyInfo.cs

@@ -4,4 +4,6 @@
 using System.Runtime.CompilerServices;
 
 [assembly: InternalsVisibleTo("Microsoft.AspNet.Mvc.Core.Test")]
+[assembly: InternalsVisibleTo("Microsoft.AspNet.Mvc.Razor.Test")]
+[assembly: InternalsVisibleTo("Microsoft.AspNet.Mvc.TagHelpers.Test")]
 [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2")]

src/Microsoft.AspNet.Mvc.Core/ViewDataDictionary.cs

@@ -14,78 +14,238 @@ namespace Microsoft.AspNet.Mvc
     public class ViewDataDictionary : IDictionary<string, object>
     {
         private readonly IDictionary<string, object> _data;
+        private readonly Type _declaredModelType;
+        private readonly IModelMetadataProvider _metadataProvider;
+
         private object _model;
         private ModelMetadata _modelMetadata;
-        private IModelMetadataProvider _metadataProvider;
 
-        public ViewDataDictionary([NotNull] IModelMetadataProvider metadataProvider)
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ViewDataDictionary"/> class.
+        /// </summary>
+        /// <param name="metadataProvider">
+        /// <see cref = "IModelMetadataProvider" /> instance used to calculate
+        /// <see cref="ViewDataDictionary.ModelMetadata"/> values.
+        /// </param>
+        /// <param name="modelState"><see cref="ModelStateDictionary"/> instance for this scope.</param>
+        /// <remarks>For use when creating a <see cref="ViewDataDictionary"/> for a new top-level scope.</remarks>
+        public ViewDataDictionary([NotNull] IModelMetadataProvider metadataProvider,
+                                  [NotNull] ModelStateDictionary modelState)
+            : this(metadataProvider, modelState, declaredModelType: typeof(object))
+        {
+        }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ViewDataDictionary"/> class based entirely on an existing
+        /// instance.
+        /// </summary>
+        /// <param name="source"><see cref="ViewDataDictionary"/> instance to copy initial values from.</param>
+        /// <remarks>
+        /// For use when copying a <see cref="ViewDataDictionary"/> instance and the declared <see cref="Model"/>
+        /// <see cref="Type"/> will not change e.g. when copying from a <see cref="ViewDataDictionary{TModel}"/>
+        /// instance to a base <see cref="ViewDataDictionary"/> instance.
+        /// </remarks>
+        public ViewDataDictionary([NotNull] ViewDataDictionary source)
+            : this(source, source.Model, source._declaredModelType)
+        {
+        }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ViewDataDictionary"/> class based in part on an existing
+        /// instance. This constructor is careful to avoid exceptions <see cref="SetModel"/> may throw when
+        /// <paramref name="model"/> is <c>null</c>.
+        /// </summary>
+        /// <param name="source"><see cref="ViewDataDictionary"/> instance to copy initial values from.</param>
+        /// <param name="model">Value for the <see cref="Model"/> property.</param>
+        /// <remarks>
+        /// For use when the new instance's declared <see cref="Model"/> <see cref="Type"/> is unknown but its
+        /// <see cref="Model"/> is known. In this case, <see cref="object"/> is the best possible guess about the
+        /// declared type when <paramref name="model"/> is <c>null</c>.
+        /// </remarks>
+        public ViewDataDictionary([NotNull] ViewDataDictionary source, object model)
+            : this(source, model, declaredModelType: typeof(object))
+        {
+        }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ViewDataDictionary"/> class.
+        /// </summary>
+        /// <param name="metadataProvider">
+        /// <see cref="IModelMetadataProvider"/> instance used to calculate
+        /// <see cref="ViewDataDictionary.ModelMetadata"/> values.
+        /// </param>
+        /// <remarks>Internal for testing.</remarks>
+        internal ViewDataDictionary([NotNull] IModelMetadataProvider metadataProvider)
             : this(metadataProvider, new ModelStateDictionary())
         {
         }
 
-        public ViewDataDictionary([NotNull] IModelMetadataProvider metadataProvider,
+        /// <summary>
-                                  [NotNull] ModelStateDictionary modelState)
+        /// Initializes a new instance of the <see cref="ViewDataDictionary"/> class.
+        /// </summary>
+        /// <param name="metadataProvider">
+        /// <see cref = "IModelMetadataProvider" /> instance used to calculate
+        /// <see cref="ViewDataDictionary.ModelMetadata"/> values.
+        /// </param>
+        /// <param name="declaredModelType">
+        /// <see cref="Type"/> of <see cref="Model"/> values expected. Used to set
+        /// <see cref="ViewDataDictionary.ModelMetadata"/> when <see cref="Model"/> is <c>null</c>.
+        /// </param>
+        /// <remarks>
+        /// For use when creating a derived <see cref="ViewDataDictionary"/> for a new top-level scope.
+        /// </remarks>
+        protected ViewDataDictionary(
+            [NotNull] IModelMetadataProvider metadataProvider,
+            [NotNull] Type declaredModelType)
+            : this(metadataProvider, new ModelStateDictionary(), declaredModelType)
+        {
+        }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ViewDataDictionary"/> class.
+        /// </summary>
+        /// <param name="metadataProvider">
+        /// <see cref = "IModelMetadataProvider" /> instance used to calculate
+        /// <see cref="ViewDataDictionary.ModelMetadata"/> values.
+        /// </param>
+        /// <param name="modelState"><see cref="ModelStateDictionary"/> instance for this scope.</param>
+        /// <param name="declaredModelType">
+        /// <see cref="Type"/> of <see cref="Model"/> values expected. Used to set
+        /// <see cref="ViewDataDictionary.ModelMetadata"/> when <see cref="Model"/> is <c>null</c>.
+        /// </param>
+        /// <remarks>
+        /// For use when creating a derived <see cref="ViewDataDictionary"/> for a new top-level scope.
+        /// </remarks>
+        protected ViewDataDictionary(
+            [NotNull] IModelMetadataProvider metadataProvider,
+            [NotNull] ModelStateDictionary modelState,
+            [NotNull] Type declaredModelType)
             : this(metadataProvider,
-                   modelState: modelState,
+                   modelState,
+                   declaredModelType,
                    data: new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase),
                    templateInfo: new TemplateInfo())
         {
+            // This is the core constructor called when Model is unknown. Base ModelMetadata on the declared type.
+            ModelMetadata = _metadataProvider.GetMetadataForType(() => null, _declaredModelType);
         }
 
         /// <summary>
-        /// <see cref="ViewDataDictionary"/> copy constructor for use when model type does not change or caller will
+        /// Initializes a new instance of the <see cref="ViewDataDictionary"/> class based in part on an existing
-        /// immediately set the <see cref="Model"/> property.
+        /// instance.
         /// </summary>
-        public ViewDataDictionary([NotNull] ViewDataDictionary source)
+        /// <param name="source"><see cref="ViewDataDictionary"/> instance to copy initial values from.</param>
-            : this(source, source.Model)
+        /// <param name="declaredModelType">
+        /// <see cref="Type"/> of <see cref="Model"/> values expected. Used to set
+        /// <see cref="ViewDataDictionary.ModelMetadata"/> when <see cref="Model"/> is <c>null</c>.
+        /// </param>
+        /// <remarks>
+        /// <para>
+        /// For use when copying a <see cref="ViewDataDictionary"/> instance and new instance's declared
+        /// <see cref="Model"/> <see cref="Type"/> is known but <see cref="Model"/> should be copied from the existing
+        /// instance e.g. when copying from a base <see cref="ViewDataDictionary"/> instance to a
+        /// <see cref="ViewDataDictionary{TModel}"/> instance.
+        /// </para>
+        /// <para>
+        /// This constructor may <c>throw</c> if <c>source.Model</c> is non-<c>null</c> and incompatible with
+        /// <paramref name="declaredModelType"/>. Pass <c>model: null</c> to
+        /// <see cref="ViewDataDictionary(ViewDataDictionary, object, Type)"/> to ignore <c>source.Model</c>.
+        /// </para>
+        /// </remarks>
+        protected ViewDataDictionary([NotNull] ViewDataDictionary source, Type declaredModelType)
+            : this(source, model: source.Model, declaredModelType: declaredModelType)
         {
         }
 
         /// <summary>
-        /// <see cref="ViewDataDictionary"/> copy constructor for use when model type may change. This avoids
+        /// Initializes a new instance of the <see cref="ViewDataDictionary"/> class based in part on an existing
-        /// exceptions a derived class may throw when <see cref="SetModel"/> is called.
+        /// instance. This constructor is careful to avoid exceptions <see cref="SetModel"/> may throw when
+        /// <paramref name="model"/> is <c>null</c>.
         /// </summary>
-        public ViewDataDictionary([NotNull] ViewDataDictionary source, object model)
+        /// <param name="source"><see cref="ViewDataDictionary"/> instance to copy initial values from.</param>
-            : this(source.MetadataProvider,
+        /// <param name="model">Value for the <see cref="Model"/> property.</param>
+        /// <param name="declaredModelType">
+        /// <see cref="Type"/> of <see cref="Model"/> values expected. Used to set
+        /// <see cref="ViewDataDictionary.ModelMetadata"/> when <see cref="Model"/> is <c>null</c>.
+        /// </param>
+        /// <remarks>
+        /// <para>
+        /// For use when copying a <see cref="ViewDataDictionary"/> instance and new instance's declared
+        /// <see cref="Model"/> <see cref="Type"/> and <see cref="Model"/> are known.
+        /// </para>
+        /// <para>
+        /// This constructor may <c>throw</c> if <paramref name="model"/> is non-<c>null</c> and incompatible with
+        /// <paramref name="declaredModelType"/>.
+        /// </para>
+        /// </remarks>
+        protected ViewDataDictionary([NotNull] ViewDataDictionary source, object model, Type declaredModelType)
+            : this(source._metadataProvider,
                    new ModelStateDictionary(source.ModelState),
-                   new CopyOnWriteDictionary<string, object>(source, StringComparer.OrdinalIgnoreCase),
+                   declaredModelType,
-                   new TemplateInfo(source.TemplateInfo))
+                   data: new CopyOnWriteDictionary<string, object>(source, StringComparer.OrdinalIgnoreCase),
+                   templateInfo: new TemplateInfo(source.TemplateInfo))
         {
-            // Avoid copying information about the object type. To do so when model==null would confuse the
+            // This is the core constructor called when Model is known.
-            // ViewDataDictionary<TModel>.ModelMetadata getter.
+            var modelType = GetModelType(model);
-            if (source.ModelMetadata?.ModelType != typeof(object))
+            if (modelType == source.ModelMetadata.ModelType && model == source.ModelMetadata.Model)
             {
-                _modelMetadata = source.ModelMetadata;
+                // Preserve any customizations made to source.ModelMetadata if the Type that will be calculated in
+                // SetModel() and source.Model match new instance's values.
+                ModelMetadata = source.ModelMetadata;
+            }
+            else if (model == null)
+            {
+                // Ensure ModelMetadata is never null though SetModel() isn't called.
+                ModelMetadata = _metadataProvider.GetMetadataForType(() => null, _declaredModelType);
             }
 
-            // If we're constructing a derived ViewDataDictionary<TModel> where TModel is a non-Nullable value type,
+            // If we're constructing a ViewDataDictionary<TModel> where TModel is a non-Nullable value type,
-            // SetModel will throw if we try to call it with null. We should not throw in that case.
+            // SetModel() will throw if we try to call it with null. We should not throw in that case.
             if (model != null)
             {
                 SetModel(model);
             }
         }
 
-        private ViewDataDictionary(IModelMetadataProvider metadataProvider,
+        private ViewDataDictionary(
-                                   ModelStateDictionary modelState,
+            IModelMetadataProvider metadataProvider,
-                                   IDictionary<string, object> data,
+            ModelStateDictionary modelState,
-                                   TemplateInfo templateInfo)
+            Type declaredModelType,
+            IDictionary<string, object> data,
+            TemplateInfo templateInfo)
         {
             _metadataProvider = metadataProvider;
             ModelState = modelState;
+            _declaredModelType = declaredModelType;
             _data = data;
             TemplateInfo = templateInfo;
         }
 
         public object Model
         {
-            get { return _model; }
+            get
-            set { SetModel(value); }
+            {
+                return _model;
+            }
+            set
+            {
+                // Reset ModelMetadata to ensure Model and ModelMetadata.Model remain equal.
+                _modelMetadata = null;
+                SetModel(value);
+            }
         }
 
-        public ModelStateDictionary ModelState { get; private set; }
+        public ModelStateDictionary ModelState { get; }
 
-        public virtual ModelMetadata ModelMetadata
+        /// <summary>
+        /// <see cref="ModelMetadata"/> for the current <see cref="Model"/> value or the declared <see cref="Type"/> if
+        /// <see cref="Model"/> is <c>null</c>.
+        /// </summary>
+        /// <remarks>
+        /// Value is never <c>null</c> but may describe the <see cref="object"/> class in some cases. This may for
+        /// example occur in controllers if <see cref="Model"/> is <c>null</c>.
+        /// </remarks>
+        public ModelMetadata ModelMetadata
         {
             get
             {
@@ -93,19 +253,18 @@ namespace Microsoft.AspNet.Mvc
             }
             set
             {
+                if (value == null)
+                {
+                    throw new ArgumentNullException(Resources.FormatPropertyOfTypeCannotBeNull(
+                        nameof(ViewDataDictionary.ModelMetadata),
+                        nameof(ViewDataDictionary)));
+                }
+
                 _modelMetadata = value;
             }
         }
 
-        public TemplateInfo TemplateInfo { get; private set; }
+        public TemplateInfo TemplateInfo { get; }
-
-        /// <summary>
-        /// Provider for subclasses that need it to override <see cref="ModelMetadata"/>.
-        /// </summary>
-        protected IModelMetadataProvider MetadataProvider
-        {
-            get { return _metadataProvider; }
-        }
 
         #region IDictionary properties
         // Do not just pass through to _data: Indexer should not throw a KeyNotFoundException.
@@ -194,19 +353,46 @@ namespace Microsoft.AspNet.Mvc
         // enough so as not to depend on the "this" pointer referencing a fully constructed object.
         protected virtual void SetModel(object value)
         {
+            EnsureCompatible(value);
             _model = value;
-            if (value == null)
+
+            // Reset or override ModelMetadata based on runtime value type. Fall back to declared type if value is
+            // null. When called from the Model setter, ModelMetadata will (temporarily) be null. When called from
+            // a constructor, current ModelMetadata may already be set to preserve customizations made in parent scope.
+            var modelType = GetModelType(value);
+            if (ModelMetadata == null || ModelMetadata.ModelType != modelType)
             {
-                // Unable to determine model metadata.
+                ModelMetadata = _metadataProvider.GetMetadataForType(() => value, modelType);
-                _modelMetadata = null;
             }
-            else if (_modelMetadata == null || value.GetType() != ModelMetadata.ModelType)
+        }
+
+        // Throw if given value is incompatible with the declared Model Type.
+        private void EnsureCompatible(object value)
+        {
+            // IsCompatibleObject verifies if the value is either an instance of _declaredModelType or (if value is
+            // null) that _declaredModelType is a nullable type.
+            var castWillSucceed = _declaredModelType.IsCompatibleWith(value);
+            if (!castWillSucceed)
             {
-                // Reset or override model metadata based on new value type.
+                string message;
-                _modelMetadata = _metadataProvider.GetMetadataForType(() => value, value.GetType());
+                if (value == null)
+                {
+                    message = Resources.FormatViewData_ModelCannotBeNull(_declaredModelType);
+                }
+                else
+                {
+                    message = Resources.FormatViewData_WrongTModelType(value.GetType(), _declaredModelType);
+                }
+
+                throw new InvalidOperationException(message);
             }
         }
 
+        private Type GetModelType(object value)
+        {
+            return (value == null) ? _declaredModelType : value.GetType();
+        }
+
         #region IDictionary methods
         public void Add([NotNull] string key, object value)
         {

src/Microsoft.AspNet.Mvc.Core/ViewDataDictionaryOfT.cs

@@ -1,88 +1,89 @@
 // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
-using System;
-using Microsoft.AspNet.Mvc.Core;
 using Microsoft.AspNet.Mvc.ModelBinding;
 
 namespace Microsoft.AspNet.Mvc
 {
     public class ViewDataDictionary<TModel> : ViewDataDictionary
     {
-        // Fallback ModelMetadata based on TModel. Used when Model is null and base ViewDataDictionary class is unable
+        /// <inheritdoc />
-        // to determine the correct metadata.
-        private readonly ModelMetadata _defaultModelMetadata;
-
         public ViewDataDictionary([NotNull] IModelMetadataProvider metadataProvider)
-            : base(metadataProvider)
+            : base(metadataProvider, declaredModelType: typeof(TModel))
         {
-            _defaultModelMetadata = MetadataProvider.GetMetadataForType(null, typeof(TModel));
         }
 
-        public ViewDataDictionary([NotNull] IModelMetadataProvider metadataProvider,
+        // References may not show up due to ITypeActivator use in RazorPageActivator.
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ViewDataDictionary{TModel}"/> class.
+        /// </summary>
+        /// <remarks>
+        /// For use when creating a <see cref="ViewDataDictionary{TModel}"/> for a new top-level scope.
+        /// </remarks>
+        /// <inheritdoc />
+        public ViewDataDictionary(
+            [NotNull] IModelMetadataProvider metadataProvider,
             [NotNull] ModelStateDictionary modelState)
-            : base(metadataProvider, modelState)
+            : base(metadataProvider, modelState, declaredModelType: typeof(TModel))
         {
         }
 
+        // References may not show up due to ITypeActivator use in RazorPageActivator.
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ViewDataDictionary{TModel}"/> class based in part on an
+        /// existing <see cref="ViewDataDictionary"/> instance.
+        /// </summary>
+        /// <remarks>
+        /// <para>
+        /// For use when copying a <see cref="ViewDataDictionary"/> instance and <typeparamref name="TModel"/> is known
+        /// but <see cref="Model"/> should be copied from the existing instance e.g. when copying from a base
+        /// <see cref="ViewDataDictionary"/> instance to a <see cref="ViewDataDictionary{TModel}"/> instance.
+        /// </para>
+        /// <para>
+        /// This constructor may <c>throw</c> if <c>source.Model</c> is non-<c>null</c> and incompatible with
+        /// <typeparamref name="TModel"/>. Pass <c>model: null</c> to
+        /// <see cref="ViewDataDictionary{TModel}(ViewDataDictionary, object)"/> to ignore <c>source.Model</c>.
+        /// </para>
+        /// </remarks>
         /// <inheritdoc />
         public ViewDataDictionary([NotNull] ViewDataDictionary source)
-            : this(source, source.Model)
+            : base(source, declaredModelType: typeof(TModel))
         {
         }
 
+        // Model parameter type is object to allow "model: null" calls even when TModel is a value type. A TModel
+        // parameter would likely require IEquatable<TModel> type restrictions to pass expected null value to the base
+        // constructor.
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ViewDataDictionary{TModel}"/> class based in part on an
+        /// existing <see cref="ViewDataDictionary"/> instance. This constructor is careful to avoid exceptions
+        /// <see cref="ViewDataDictionary.SetModel"/> may throw when <paramref name="model"/> is <c>null</c>.
+        /// </summary>
+        /// <remarks>
+        /// <para>
+        /// For use when copying a <see cref="ViewDataDictionary"/> instance and <typeparamref name="TModel"/> and
+        /// <see cref="Model"/> are known.
+        /// </para>
+        /// <para>
+        /// This constructor may <c>throw</c> if <paramref name="model"/> is non-<c>null</c> and incompatible with
+        /// <typeparamref name="TModel"/>.
+        /// </para>
+        /// </remarks>
         /// <inheritdoc />
         public ViewDataDictionary([NotNull] ViewDataDictionary source, object model)
-            : base(source, model)
+            : base(source, model, declaredModelType: typeof(TModel))
         {
-            var original = source as ViewDataDictionary<TModel>;
-            if (original != null)
-            {
-                _defaultModelMetadata = original._defaultModelMetadata;
-            }
-            else
-            {
-                _defaultModelMetadata = MetadataProvider.GetMetadataForType(null, typeof(TModel));
-            }
         }
 
         public new TModel Model
-        {
-            get { return (TModel)base.Model; }
-            set { SetModel(value); }
-        }
-
-        public override ModelMetadata ModelMetadata
         {
             get
             {
-                return base.ModelMetadata ?? _defaultModelMetadata;
+                return (base.Model == null) ? default(TModel) : (TModel)base.Model;
             }
-        }
+            set
-
-        protected override void SetModel(object value)
-        {
-            // IsCompatibleObject verifies if the value is either an instance of TModel or (if value is null) that
-            // TModel is a nullable type.
-            var castWillSucceed = typeof(TModel).IsCompatibleWith(value);
-
-            if (castWillSucceed)
             {
-                base.SetModel(value);
+                base.Model = value;
-            }
-            else
-            {
-                string message;
-                if (value == null)
-                {
-                    message = Resources.FormatViewData_ModelCannotBeNull(typeof(TModel));
-                }
-                else
-                {
-                    message = Resources.FormatViewData_WrongTModelType(value.GetType(), typeof(TModel));
-                }
-
-                throw new InvalidOperationException(message);
             }
         }
     }