Don't run Command CanExecute on incorrect inherited binding context type (#572)

* Allow Command CanExecute to recover when run on inherited bindingcontext

* Make exception handler more generic

* Checking types in Command delegates to avoid exception in the first place

* Adding type chekc to other Command constructor

* Use nameof for ArgumentNullExceptions

* Add unit tests for null parameters, handle value types and Nullable<T>

4 files changed, 258 insertions, 14 deletions

diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla47971.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla47971.cs
new file mode 100644
index 00000000..351028d4
--- /dev/null
+++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla47971.cs
@@ -0,0 +1,109 @@
+using System;
+using System.Collections.Generic;
+using System.Collections.ObjectModel;
+using System.ComponentModel;
+using System.Runtime.CompilerServices;
+using Xamarin.Forms.CustomAttributes;
+using Xamarin.Forms.Internals;
+
+namespace Xamarin.Forms.Controls.Issues
+{
+	[Preserve(AllMembers = true)]
+	[Issue(IssueTracker.Bugzilla, 47971, "UWP doesn't display list items when binding a CommandParameter to BindingContext in a DataTemplate and including a CanExecute method", PlatformAffected.WinRT)]
+	public class Bugzilla47971 : TestContentPage
+	{
+		protected override void Init()
+		{
+			var viewModel = new _47971ViewModel();
+
+			var lv = new ListView { BindingContext = viewModel };
+
+			lv.SetBinding(ListView.ItemsSourceProperty, new Binding("Models"));
+			lv.SetBinding(ListView.SelectedItemProperty, new Binding("SelectedModel"));
+
+			lv.ItemTemplate = new DataTemplate(() =>
+			{
+				var tc = new TextCell();
+
+				tc.SetBinding(TextCell.TextProperty, new Binding("Name"));
+				tc.SetBinding(TextCell.CommandParameterProperty, new Binding("."));
+				tc.SetBinding(TextCell.CommandProperty, new Binding("BindingContext.ModelSelectedCommand", source: lv));
+
+				return tc;
+			});
+
+			var layout = new StackLayout { Spacing = 10 };
+			var instructions = new Label {Text = "The ListView below should display three items (Item1, Item2, and Item3). If it does not, this test has failed." };
+
+			layout.Children.Add(instructions);
+			layout.Children.Add(lv);
+
+			Content = layout;
+		}
+
+		[Preserve(AllMembers = true)]
+		internal class _47971ViewModel : INotifyPropertyChanged
+		{
+			_47971ItemModel _selectedModel;
+			Command<_47971ItemModel> _modelSelectedCommand;
+			ObservableCollection<_47971ItemModel> _models;
+
+			public ObservableCollection<_47971ItemModel> Models
+			{
+				get { return _models; }
+				set
+				{
+					_models = value;
+					OnPropertyChanged();
+				}
+			}
+
+			public _47971ItemModel SelectedModel
+			{
+				get { return _selectedModel; }
+				set
+				{
+					_selectedModel = value;
+					OnPropertyChanged();
+				}
+			}
+
+			public Command<_47971ItemModel> ModelSelectedCommand => _modelSelectedCommand ??
+				(_modelSelectedCommand = new Command<_47971ItemModel>(ModelSelectedCommandExecute, CanExecute));
+
+			bool CanExecute(_47971ItemModel itemModel)
+			{
+				return true;
+			}
+
+			void ModelSelectedCommandExecute(_47971ItemModel model)
+			{
+				System.Diagnostics.Debug.WriteLine(model.Name);
+			}
+
+			public _47971ViewModel()
+			{
+				_models = new ObservableCollection<_47971ItemModel>(
+					new List<_47971ItemModel>()
+					{
+						new _47971ItemModel() { Name = "Item 1"},
+						new _47971ItemModel() { Name = "Item 2"},
+						new _47971ItemModel() { Name = "Item 3"}
+					});
+			}
+
+			public event PropertyChangedEventHandler PropertyChanged;
+
+			protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null)
+			{
+				PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
+			}
+		}
+
+		[Preserve(AllMembers = true)]
+		internal class _47971ItemModel
+		{
+			public string Name { get; set; }
+		}
+	}
+}
\ No newline at end of file
diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems
index 3b728f4d..08c26ba1 100644
--- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems
+++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems
@@ -139,6 +139,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla46494.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla44476.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla46630.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Bugzilla47971.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)CarouselAsync.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla34561.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla34727.cs" />
diff --git a/Xamarin.Forms.Core.UnitTests/CommandTests.cs b/Xamarin.Forms.Core.UnitTests/CommandTests.cs
index 1182fe40..c653b976 100644
--- a/Xamarin.Forms.Core.UnitTests/CommandTests.cs
+++ b/Xamarin.Forms.Core.UnitTests/CommandTests.cs
@@ -1,7 +1,6 @@
 using System;
 using NUnit.Framework;
 
-
 namespace Xamarin.Forms.Core.UnitTests
 {
 	[TestFixture]
@@ -151,5 +150,104 @@ namespace Xamarin.Forms.Core.UnitTests
 			Assert.AreEqual (expected, cmd.CanExecute ("Foo"));
 			Assert.AreEqual ("Foo", result);
 		}
+
+		class FakeParentContext
+		{
+		}
+
+		// ReSharper disable once ClassNeverInstantiated.Local
+		class FakeChildContext
+		{
+		}
+
+		[Test]
+		public void CanExecuteReturnsFalseIfParameterIsWrongReferenceType()
+		{
+			var command = new Command<FakeChildContext>(context => { }, context => true);
+
+			Assert.IsFalse(command.CanExecute(new FakeParentContext()), "the parameter is of the wrong type");
+		}
+
+		[Test]
+		public void CanExecuteReturnsFalseIfParameterIsWrongValueType()
+		{
+			var command = new Command<int>(context => { }, context => true);
+
+			Assert.IsFalse(command.CanExecute(10.5), "the parameter is of the wrong type");
+		}
+
+		[Test]
+		public void CanExecuteUsesParameterIfReferenceTypeAndSetToNull()
+		{
+			var command = new Command<FakeChildContext>(context => { }, context => true);
+
+			Assert.IsTrue(command.CanExecute(null), "null is a valid value for a reference type");
+		}
+
+		[Test]
+		public void CanExecuteUsesParameterIfNullableAndSetToNull()
+		{
+			var command = new Command<int?>(context => { }, context => true);
+
+			Assert.IsTrue(command.CanExecute(null), "null is a valid value for a Nullable<int> type");
+		}
+
+		[Test]
+		public void CanExecuteIgnoresParameterIfValueTypeAndSetToNull()
+		{
+			var command = new Command<int>(context => { }, context => true);
+
+			Assert.IsFalse(command.CanExecute(null), "null is not a valid valid for int");
+		}
+
+		[Test]
+		public void ExecuteDoesNotRunIfParameterIsWrongReferenceType()
+		{
+			int executions = 0;
+			var command = new Command<FakeChildContext>(context => executions += 1);
+
+			Assert.DoesNotThrow(() => command.Execute(new FakeParentContext()), "the command should not execute, so no exception should be thrown");
+			Assert.IsTrue(executions == 0, "the command should not have executed");
+		}
+
+		[Test]
+		public void ExecuteDoesNotRunIfParameterIsWrongValueType()
+		{
+			int executions = 0;
+			var command = new Command<int>(context => executions += 1);
+
+			Assert.DoesNotThrow(() => command.Execute(10.5), "the command should not execute, so no exception should be thrown");
+			Assert.IsTrue(executions == 0, "the command should not have executed");
+		}
+
+		[Test]
+		public void ExecuteRunsIfReferenceTypeAndSetToNull()
+		{
+			int executions = 0;
+			var command = new Command<FakeChildContext>(context => executions += 1);
+
+			Assert.DoesNotThrow(() => command.Execute(null), "null is a valid value for a reference type");
+			Assert.IsTrue(executions == 1, "the command should have executed");
+		}
+
+		[Test]
+		public void ExecuteRunsIfNullableAndSetToNull()
+		{
+			int executions = 0;
+			var command = new Command<int?>(context => executions += 1);
+
+			Assert.DoesNotThrow(() => command.Execute(null), "null is a valid value for a Nullable<int> type");
+			Assert.IsTrue(executions == 1, "the command should have executed");
+		}
+
+		[Test]
+		public void ExecuteDoesNotRunIfValueTypeAndSetToNull()
+		{
+			int executions = 0;
+			var command = new Command<int>(context => executions += 1);
+
+			Assert.DoesNotThrow(() => command.Execute(null), "null is not a valid value for int");
+			Assert.IsTrue(executions == 0, "the command should not have executed");
+		}
 	}
 }
diff --git a/Xamarin.Forms.Core/Command.cs b/Xamarin.Forms.Core/Command.cs
index 73ae1b08..d15574cf 100644
--- a/Xamarin.Forms.Core/Command.cs
+++ b/Xamarin.Forms.Core/Command.cs
@@ -1,22 +1,59 @@
 using System;
+using System.Reflection;
 using System.Windows.Input;
 
 namespace Xamarin.Forms
 {
-	public sealed class Command<T> : Command
+	public sealed class Command<T> : Command 
 	{
-		public Command(Action<T> execute) : base(o => execute((T)o))
+		public Command(Action<T> execute) 
+			: base(o =>
+			{
+				if (IsValidParameter(o))
+				{
+					execute((T)o);
+				}
+			})
 		{
 			if (execute == null)
-				throw new ArgumentNullException("execute");
+			{
+				throw new ArgumentNullException(nameof(execute));
+			}
 		}
 
-		public Command(Action<T> execute, Func<T, bool> canExecute) : base(o => execute((T)o), o => canExecute((T)o))
+		public Command(Action<T> execute, Func<T, bool> canExecute) 
+			: base(o =>
+			{
+				if (IsValidParameter(o))
+				{
+					execute((T)o);
+				}
+			}, o => IsValidParameter(o) && canExecute((T)o))
 		{
 			if (execute == null)
-				throw new ArgumentNullException("execute");
+				throw new ArgumentNullException(nameof(execute));
 			if (canExecute == null)
-				throw new ArgumentNullException("canExecute");
+				throw new ArgumentNullException(nameof(canExecute));
+		}
+
+		static bool IsValidParameter(object o)
+		{
+			if (o != null)
+			{
+				// The parameter isn't null, so we don't have to worry whether null is a valid option
+				return o is T;
+			}
+
+			var t = typeof(T);
+
+			// The parameter is null. Is T Nullable?
+			if (Nullable.GetUnderlyingType(t) != null)
+			{
+				return true;
+			}
+
+			// Not a Nullable, if it's a value type then null is not valid
+			return !t.GetTypeInfo().IsValueType;
 		}
 	}
 
@@ -28,7 +65,7 @@ namespace Xamarin.Forms
 		public Command(Action<object> execute)
 		{
 			if (execute == null)
-				throw new ArgumentNullException("execute");
+				throw new ArgumentNullException(nameof(execute));
 
 			_execute = execute;
 		}
@@ -36,13 +73,13 @@ namespace Xamarin.Forms
 		public Command(Action execute) : this(o => execute())
 		{
 			if (execute == null)
-				throw new ArgumentNullException("execute");
+				throw new ArgumentNullException(nameof(execute));
 		}
 
 		public Command(Action<object> execute, Func<object, bool> canExecute) : this(execute)
 		{
 			if (canExecute == null)
-				throw new ArgumentNullException("canExecute");
+				throw new ArgumentNullException(nameof(canExecute));
 
 			_canExecute = canExecute;
 		}
@@ -50,9 +87,9 @@ namespace Xamarin.Forms
 		public Command(Action execute, Func<bool> canExecute) : this(o => execute(), o => canExecute())
 		{
 			if (execute == null)
-				throw new ArgumentNullException("execute");
+				throw new ArgumentNullException(nameof(execute));
 			if (canExecute == null)
-				throw new ArgumentNullException("canExecute");
+				throw new ArgumentNullException(nameof(canExecute));
 		}
 
 		public bool CanExecute(object parameter)
@@ -73,8 +110,7 @@ namespace Xamarin.Forms
 		public void ChangeCanExecute()
 		{
 			EventHandler changed = CanExecuteChanged;
-			if (changed != null)
-				changed(this, EventArgs.Empty);
+			changed?.Invoke(this, EventArgs.Empty);
 		}
 	}
 }
\ No newline at end of file