From 1106e57a59fcdb605ede14c3dfa98cba6cb9a605 Mon Sep 17 00:00:00 2001
From: stakx <stakx@eml.cc>
Date: Thu, 6 Jun 2019 19:37:10 +0200
Subject: [PATCH] Prevent auto-stubbing failure due to inaccessible property
 accessors (#847)

1. Add regression tests for lazy `SetupAllProperties`

There are two groups of tests here:

 * Those that test `SetupAllProperties` (called either directly, or
   implicitly through `Mock.Of<T>`) followed by a property acces. This
   reflects `SetupAllProperties`' current implementation, which only
   sets up properties upon their first access (just-in-time).

 * Those that test `SetupAllProperties` itself, without any property
   access. Unsurprisingly, these currently pass, but the tests are
   still good to have in case we ever return `SetupAllProperties` to a
   ahead-of-time batch operation implementation.

2. Fix regression by checking accessor visibility

3. Update the changelog
---
 CHANGELOG.md                                  |  1 +
 src/Moq/Interception/InterceptionAspects.cs   | 30 ++++++++++----
 .../Regressions/IssueReportsFixture.cs        | 39 +++++++++++++++++++
 3 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7fdfb9187..860fa5b40 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -19,6 +19,7 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1
 #### Fixed
 
 * `mock.SetupAllProperties()` now setups write-only properties for strict mocks, so that accessing such properties will not throw anymore. (@vanashimko, #836)
+* Regression: `mock.SetupAllProperties()` and `Mock.Of<T>` fail due to inaccessible property accessors (@Mexe13, #845)
 
 
 ## 4.11.0 (2019-05-28)
diff --git a/src/Moq/Interception/InterceptionAspects.cs b/src/Moq/Interception/InterceptionAspects.cs
index e5fa1607b..c52f908fd 100644
--- a/src/Moq/Interception/InterceptionAspects.cs
+++ b/src/Moq/Interception/InterceptionAspects.cs
@@ -359,20 +359,36 @@ public static bool Handle(Invocation invocation, Mock mock)
 				if (property.CanRead)
 				{
 					var getter = property.GetGetMethod(true);
-					propertyValue = CreateInitialPropertyValue(mock, getter);
-					getterSetup = new AutoImplementedPropertyGetterSetup(expression, getter, () => propertyValue);
-					mock.Setups.Add(getterSetup);
+					if (ProxyFactory.Instance.IsMethodVisible(getter, out _))
+					{
+						propertyValue = CreateInitialPropertyValue(mock, getter);
+						getterSetup = new AutoImplementedPropertyGetterSetup(expression, getter, () => propertyValue);
+						mock.Setups.Add(getterSetup);
+					}
+
+					// If we wanted to optimise for speed, we'd probably be forgiven
+					// for removing the above `IsMethodVisible` guard, as it's rather
+					// unlikely to encounter non-public getters such as the following
+					// in real-world code:
+					//
+					//     public T Property { internal get; set; }
+					//
+					// Usually, it's only the setters that are made non-public. For
+					// now however, we prefer correctness.
 				}
 
 				Setup setterSetup = null;
 				if (property.CanWrite)
 				{
 					MethodInfo setter = property.GetSetMethod(nonPublic: true);
-					setterSetup = new AutoImplementedPropertySetterSetup(expression, setter, (newValue) =>
+					if (ProxyFactory.Instance.IsMethodVisible(setter, out _))
 					{
-						propertyValue = newValue;
-					});
-					mock.Setups.Add(setterSetup);
+						setterSetup = new AutoImplementedPropertySetterSetup(expression, setter, (newValue) =>
+						{
+							propertyValue = newValue;
+						});
+						mock.Setups.Add(setterSetup);
+					}
 				}
 
 				Setup setupToExecute = invocationMethod.IsPropertyGetter() ? getterSetup : setterSetup;
diff --git a/tests/Moq.Tests/Regressions/IssueReportsFixture.cs b/tests/Moq.Tests/Regressions/IssueReportsFixture.cs
index 3cfa67419..71ddb5caa 100644
--- a/tests/Moq.Tests/Regressions/IssueReportsFixture.cs
+++ b/tests/Moq.Tests/Regressions/IssueReportsFixture.cs
@@ -2571,6 +2571,45 @@ public void Setting_up_indexer_with_SetupProperty_throws_with_error_message_poin
 
 		#endregion
 
+		#region 845
+
+		public class Issue845
+		{
+			public class Foo
+			{
+				public virtual object Bar { get; internal set; }
+			}
+
+			[Fact]
+			public void Mock_Of_can_deal_with_internal_setter()
+			{
+				_ = Mock.Of<Foo>();
+			}
+
+			[Fact]
+			public void Mock_Of_plus_property_access_can_deal_with_internal_setter()
+			{
+				var mocked = Mock.Of<Foo>();
+				_ = mocked.Bar;
+			}
+
+			[Fact]
+			public void SetupAllProperties_can_deal_with_internal_setter()
+			{
+				var mock = new Mock<Foo>();
+				mock.SetupAllProperties();
+			}
+
+			[Fact]
+			public void SetupAllProperties_plus_property_access_can_deal_with_internal_setter()
+			{
+				var mock = new Mock<Foo>();
+				mock.SetupAllProperties();
+				_ = mock.Object.Bar;
+			}
+		}
+		#endregion
+
 		// Old @ Google Code
 
 		#region #47