Remove syncroot fields from collections (dotnet#34198)

* Syncroot

* Fix typo in build-native

* Specialized

* Remove noise

* Revert non generic SortedList

* Test fixups

* CLR test fixes

* Revert "CLR test fixes"

This reverts commit 8db135c.

* Disable syncroot tests for NETFX

* Update QueueTests.cs

* Test fixes

* Typo

Author: danmoseley

build.cmd

@@ -1,4 +1,3 @@
 @echo off
 powershell -ExecutionPolicy ByPass -NoProfile %~dp0eng\build.ps1 %*
-echo Build.cmd ErrorLevel=%ERRORLEVEL%
 exit /b %ERRORLEVEL%

src/Common/tests/System/Collections/ICollection.NonGeneric.Tests.cs

@@ -83,13 +83,6 @@ protected virtual ICollection NonGenericICollectionFactory(int count)
         /// </summary>
         protected virtual bool ICollection_NonGeneric_HasNullSyncRoot => false;
 
-        /// <summary>
-        /// Used for the ICollection_NonGeneric_SyncRootType_MatchesExcepted test. Most SyncRoots are created
-        /// using System.Threading.Interlocked.CompareExchange(ref _syncRoot, new Object(), null)
-        /// so we should test that the SyncRoot is the type we expect.
-        /// </summary>
-        protected virtual Type ICollection_NonGeneric_SyncRootType => typeof(object);
-
         /// <summary>
         /// Used for the ICollection_NonGeneric_CopyTo_IndexLargerThanArrayCount_ThrowsArgumentException tests. Some
         /// implementations throw a different exception type (e.g. ArgumentOutOfRangeException).
@@ -147,22 +140,6 @@ public void ICollection_NonGeneric_SyncRoot(int count)
             {
                 Assert.Equal(ICollection_NonGeneric_HasNullSyncRoot, collection.SyncRoot == null);
                 Assert.Same(collection.SyncRoot, collection.SyncRoot);
-
-                if (!ICollection_NonGeneric_HasNullSyncRoot)
-                {
-                    Assert.IsType(ICollection_NonGeneric_SyncRootType, collection.SyncRoot);
-
-                    if (ICollection_NonGeneric_SyncRootType == collection.GetType())
-                    {
-                        // If we expect the SyncRoot to be the same type as the collection, 
-                        // the SyncRoot should be the same as the collection (e.g. HybridDictionary)
-                        Assert.Same(collection, collection.SyncRoot);
-                    }
-                    else
-                    {
-                        Assert.NotSame(collection, collection.SyncRoot);
-                    }
-                }
             }
             else
             {

src/Native/build-native.cmd

@@ -68,8 +68,7 @@ call "%_VSCOMNTOOLS%\VsDevCmd.bat"
 :RunVCVars
 if "%VisualStudioVersion%"=="16.0" (
     goto :VS2019
-)
-else if "%VisualStudioVersion%"=="15.0" (
+) else if "%VisualStudioVersion%"=="15.0" (
     goto :VS2017
 ) else if "%VisualStudioVersion%"=="14.0" (
     goto :VS2015

src/System.Collections.NonGeneric/src/System/Collections/Queue.cs

@@ -29,8 +29,6 @@ public class Queue : ICollection, ICloneable
         private int _size; // Number of elements. Do not rename (binary serialization)
         private int _growFactor; // 100 == 1.0, 130 == 1.3, 200 == 2.0. Do not rename (binary serialization)
         private int _version; // Do not rename (binary serialization)
-        [NonSerialized]
-        private object _syncRoot;
 
         private const int _MinimumGrow = 4;
         private const int _ShrinkThreshold = 32;
@@ -107,17 +105,7 @@ public virtual bool IsSynchronized
             get { return false; }
         }
 
-        public virtual object SyncRoot
-        {
-            get
-            {
-                if (_syncRoot == null)
-                {
-                    System.Threading.Interlocked.CompareExchange(ref _syncRoot, new object(), null);
-                }
-                return _syncRoot;
-            }
-        }
+        public virtual object SyncRoot => this;
 
         // Removes all Objects from the queue.
         public virtual void Clear()

src/System.Collections.NonGeneric/src/System/Collections/SortedList.cs

@@ -71,7 +71,7 @@ public class SortedList : IDictionary, ICloneable
         private KeyList keyList; // Do not rename (binary serialization)
         private ValueList valueList; // Do not rename (binary serialization)
         [NonSerialized]
-        private object _syncRoot;
+        private object _syncRoot; // Serialized indirectly in SyncSortedList
 
         private const int _defaultCapacity = 16;
 

src/System.Collections.NonGeneric/src/System/Collections/Stack.cs

@@ -27,8 +27,6 @@ public class Stack : ICollection, ICloneable
         private object[] _array; // Storage for stack elements. Do not rename (binary serialization)
         private int _size; // Number of items in the stack. Do not rename (binary serialization)
         private int _version; // Used to keep enumerator in sync w/ collection. Do not rename (binary serialization)
-        [NonSerialized]
-        private object _syncRoot;
 
         private const int _defaultCapacity = 10;
 
@@ -79,17 +77,7 @@ public virtual bool IsSynchronized
             get { return false; }
         }
 
-        public virtual object SyncRoot
-        {
-            get
-            {
-                if (_syncRoot == null)
-                {
-                    System.Threading.Interlocked.CompareExchange<Object>(ref _syncRoot, new object(), null);
-                }
-                return _syncRoot;
-            }
-        }
+        public virtual object SyncRoot => this;
 
         // Removes all Objects from the Stack.
         public virtual void Clear()

src/System.Collections.NonGeneric/tests/ArrayListTests.cs

@@ -2548,6 +2548,7 @@ public DerivedArrayList(ICollection c) : base(c) { }
         }
     }
 
+    [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] // Changed behavior
     public class ArrayList_SyncRootTests
     {
         private ArrayList _arrDaughter;
@@ -2576,12 +2577,12 @@ public void GetSyncRoot()
                 _arrGrandDaughter = ArrayList.Synchronized(arrMother2);
                 _arrDaughter = ArrayList.Synchronized(arrMother2);
 
-                Assert.False(arrMother2.SyncRoot is ArrayList);
-                Assert.False(arrSon1.SyncRoot is ArrayList);
-                Assert.False(arrSon2.SyncRoot is ArrayList);
-                Assert.False(_arrDaughter.SyncRoot is ArrayList);
+                Assert.True(arrMother2.SyncRoot is ArrayList);
+                Assert.True(arrSon1.SyncRoot is ArrayList);
+                Assert.True(arrSon2.SyncRoot is ArrayList);
+                Assert.True(_arrDaughter.SyncRoot is ArrayList);
                 Assert.Equal(arrSon1.SyncRoot, arrMother2.SyncRoot);
-                Assert.False(_arrGrandDaughter.SyncRoot is ArrayList);
+                Assert.True(_arrGrandDaughter.SyncRoot is ArrayList);
 
                 arrMother2 = new ArrayList();
                 for (int i = 0; i < NumberOfElements; i++)

src/System.Collections.NonGeneric/tests/CollectionBaseTests.cs

@@ -299,8 +299,8 @@ public static void SyncRoot()
             // SyncRoot should be the reference to the underlying collection, not to MyCollection
             var collBase = new MyCollection();
             object syncRoot = collBase.SyncRoot;
-            Assert.NotEqual(syncRoot, collBase);
-            Assert.Equal(collBase.SyncRoot, collBase.SyncRoot);
+            Assert.NotNull(syncRoot);
+            Assert.Same(collBase.SyncRoot, collBase.SyncRoot);
         }
 
         [Fact]

src/System.Collections.NonGeneric/tests/HashtableTests.cs

@@ -1194,6 +1194,7 @@ private void RemoveElements(string strName)
         }
     }
 
+    [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] // Changed behavior
     public class Hashtable_SyncRootTests
     {
         private Hashtable _hashDaughter;

src/System.Collections.NonGeneric/tests/QueueTests.cs

@@ -834,6 +834,7 @@ public Foo(int intValue)
         }
     }
 
+    [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] // Changed behavior
     public class Queue_SyncRootTests
     {
         private const int NumberOfElements = 1000;
@@ -850,7 +851,7 @@ public void SyncRoot()
             {
                 queueMother.Enqueue(i);
             }
-            Assert.Equal(queueMother.SyncRoot.GetType(), typeof(object));
+            Assert.IsType<Queue>(queueMother.SyncRoot);
 
             var queueSon = Queue.Synchronized(queueMother);
             _queueGrandDaughter = Queue.Synchronized(queueSon);