Merge pull request #1373 from dotnet/dev/bartde/rx_nullable_last

Final wave of #nullable in Rx.

Rx.NET/Source/src/System.Reactive/Disposables/CompositeDisposable.cs

@@ -2,8 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT License.
 // See the LICENSE file in the project root for more information. 
 
-#nullable disable
-
 using System.Collections;
 using System.Collections.Generic;
 using System.Threading;
@@ -18,7 +16,7 @@ namespace System.Reactive.Disposables
     {
         private readonly object _gate = new object();
         private bool _disposed;
-        private List<IDisposable> _disposables;
+        private List<IDisposable?> _disposables;
         private int _count;
         private const int ShrinkThreshold = 64;
 
@@ -31,7 +29,7 @@ namespace System.Reactive.Disposables
         /// </summary>
         public CompositeDisposable()
         {
-            _disposables = new List<IDisposable>();
+            _disposables = new List<IDisposable?>();
         }
 
         /// <summary>
@@ -46,7 +44,7 @@ namespace System.Reactive.Disposables
                 throw new ArgumentOutOfRangeException(nameof(capacity));
             }
 
-            _disposables = new List<IDisposable>(capacity);
+            _disposables = new List<IDisposable?>(capacity);
         }
 
         /// <summary>
@@ -62,7 +60,11 @@ namespace System.Reactive.Disposables
                 throw new ArgumentNullException(nameof(disposables));
             }
 
-            Init(disposables, disposables.Length);
+            _disposables = ToList(disposables);
+
+            // _count can be read by other threads and thus should be properly visible
+            // also releases the _disposables contents so it becomes thread-safe
+            Volatile.Write(ref _count, _disposables.Count);
         }
 
         /// <summary>
@@ -78,27 +80,23 @@ namespace System.Reactive.Disposables
                 throw new ArgumentNullException(nameof(disposables));
             }
 
-            // If the disposables is a collection, get its size
-            // and use it as a capacity hint for the copy.
-            if (disposables is ICollection<IDisposable> c)
-            {
-                Init(disposables, c.Count);
-            }
-            else
-            {
-                // Unknown sized disposables, use the default capacity hint
-                Init(disposables, DefaultCapacity);
-            }
+            _disposables = ToList(disposables);
+
+            // _count can be read by other threads and thus should be properly visible
+            // also releases the _disposables contents so it becomes thread-safe
+            Volatile.Write(ref _count, _disposables.Count);
         }
 
-        /// <summary>
-        /// Initialize the inner disposable list and count fields.
-        /// </summary>
-        /// <param name="disposables">The enumerable sequence of disposables.</param>
-        /// <param name="capacityHint">The number of items expected from <paramref name="disposables"/></param>
-        private void Init(IEnumerable<IDisposable> disposables, int capacityHint)
+        private static List<IDisposable?> ToList(IEnumerable<IDisposable> disposables)
         {
-            var list = new List<IDisposable>(capacityHint);
+            var capacity = disposables switch
+            {
+                IDisposable[] a => a.Length,
+                ICollection<IDisposable> c => c.Count,
+                _ => DefaultCapacity
+            };
+
+            var list = new List<IDisposable?>(capacity);
 
             // do the copy and null-check in one step to avoid a
             // second loop for just checking for null items
@@ -112,11 +110,7 @@ namespace System.Reactive.Disposables
                 list.Add(d);
             }
 
-            _disposables = list;
-
-            // _count can be read by other threads and thus should be properly visible
-            // also releases the _disposables contents so it becomes thread-safe
-            Volatile.Write(ref _count, _disposables.Count);
+            return list;
         }
 
         /// <summary>
@@ -197,7 +191,7 @@ namespace System.Reactive.Disposables
 
                 if (current.Capacity > ShrinkThreshold && _count < current.Capacity / 2)
                 {
-                    var fresh = new List<IDisposable>(current.Capacity / 2);
+                    var fresh = new List<IDisposable?>(current.Capacity / 2);
 
                     foreach (var d in current)
                     {
@@ -227,7 +221,7 @@ namespace System.Reactive.Disposables
         /// </summary>
         public void Dispose()
         {
-            List<IDisposable> currentDisposables = null;
+            List<IDisposable?>? currentDisposables = null;
 
             lock (_gate)
             {
@@ -238,7 +232,7 @@ namespace System.Reactive.Disposables
                     // nulling out the reference is faster no risk to
                     // future Add/Remove because _disposed will be true
                     // and thus _disposables won't be touched again.
-                    _disposables = null;
+                    _disposables = null!; // NB: All accesses are guarded by _disposed checks.
 
                     Volatile.Write(ref _count, 0);
                     Volatile.Write(ref _disposed, true);
@@ -259,7 +253,7 @@ namespace System.Reactive.Disposables
         /// </summary>
         public void Clear()
         {
-            IDisposable[] previousDisposables;
+            IDisposable?[] previousDisposables;
 
             lock (_gate)
             {
@@ -393,25 +387,25 @@ namespace System.Reactive.Disposables
         /// method to avoid allocation on disposed or empty composites.
         /// </summary>
         private static readonly CompositeEnumerator EmptyEnumerator =
-            new CompositeEnumerator(Array.Empty<IDisposable>());
+            new CompositeEnumerator(Array.Empty<IDisposable?>());
 
         /// <summary>
         /// An enumerator for an array of disposables.
         /// </summary>
         private sealed class CompositeEnumerator : IEnumerator<IDisposable>
         {
-            private readonly IDisposable[] _disposables;
+            private readonly IDisposable?[] _disposables;
             private int _index;
 
-            public CompositeEnumerator(IDisposable[] disposables)
+            public CompositeEnumerator(IDisposable?[] disposables)
             {
                 _disposables = disposables;
                 _index = -1;
             }
 
-            public IDisposable Current => _disposables[_index];
+            public IDisposable Current => _disposables[_index]!; // NB: _index is only advanced to non-null positions.
 
-            object IEnumerator.Current => _disposables[_index];
+            object IEnumerator.Current => _disposables[_index]!;
 
             public void Dispose()
             {

Rx.NET/Source/src/System.Reactive/Disposables/StableCompositeDisposable.cs

@@ -2,8 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT License.
 // See the LICENSE file in the project root for more information. 
 
-#nullable disable
-
 using System.Collections.Generic;
 using System.Threading;
 
@@ -92,8 +90,8 @@ namespace System.Reactive.Disposables
 
         private sealed class Binary : StableCompositeDisposable
         {
-            private IDisposable _disposable1;
-            private IDisposable _disposable2;
+            private IDisposable? _disposable1;
+            private IDisposable? _disposable2;
 
             public Binary(IDisposable disposable1, IDisposable disposable2)
             {
@@ -112,7 +110,7 @@ namespace System.Reactive.Disposables
 
         private sealed class NAryEnumerable : StableCompositeDisposable
         {
-            private volatile List<IDisposable> _disposables;
+            private volatile List<IDisposable>? _disposables;
 
             public NAryEnumerable(IEnumerable<IDisposable> disposables)
             {
@@ -121,7 +119,7 @@ namespace System.Reactive.Disposables
                 //
                 // Doing this on the list to avoid duplicate enumeration of disposables.
                 //
-                if (_disposables.Contains(null))
+                if (_disposables.Contains(null!))
                 {
                     throw new ArgumentException(Strings_Core.DISPOSABLES_CANT_CONTAIN_NULL, nameof(disposables));
                 }
@@ -144,19 +142,20 @@ namespace System.Reactive.Disposables
 
         private sealed class NAryArray : StableCompositeDisposable
         {
-            private IDisposable[] _disposables;
+            private IDisposable[]? _disposables;
 
             public NAryArray(IDisposable[] disposables)
             {
-                var n = disposables.Length;
-                var ds = new IDisposable[n];
-                // These are likely already vectorized in the framework
-                // At least they are faster than loop-copying
-                Array.Copy(disposables, 0, ds, 0, n);
-                if (Array.IndexOf(ds, null) != -1)
+                if (Array.IndexOf(disposables, null!) != -1)
                 {
                     throw new ArgumentException(Strings_Core.DISPOSABLES_CANT_CONTAIN_NULL, nameof(disposables));
                 }
+
+                var n = disposables.Length;
+                var ds = new IDisposable[n];
+
+                Array.Copy(disposables, 0, ds, 0, n);
+
                 Volatile.Write(ref _disposables, ds);
             }
 
@@ -181,7 +180,7 @@ namespace System.Reactive.Disposables
         /// </summary>
         private sealed class NAryTrustedArray : StableCompositeDisposable
         {
-            private IDisposable[] _disposables;
+            private IDisposable[]? _disposables;
 
             public NAryTrustedArray(IDisposable[] disposables)
             {

Rx.NET/Source/src/System.Reactive/Internal/ReflectionUtils.cs

@@ -2,8 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT License.
 // See the LICENSE file in the project root for more information. 
 
-#nullable disable
-
 using System.Globalization;
 using System.Reflection;
 
@@ -25,9 +23,9 @@ namespace System.Reactive
             return method.CreateDelegate(delegateType, o);
         }
 
-        public static void GetEventMethods<TSender, TEventArgs>(Type targetType, object target, string eventName, out MethodInfo addMethod, out MethodInfo removeMethod, out Type delegateType, out bool isWinRT)
+        public static void GetEventMethods<TSender, TEventArgs>(Type targetType, object? target, string eventName, out MethodInfo addMethod, out MethodInfo removeMethod, out Type delegateType, out bool isWinRT)
         {
-            EventInfo e;
+            EventInfo? e;
 
             if (target == null)
             {
@@ -46,19 +44,24 @@ namespace System.Reactive
                 }
             }
 
-            addMethod = e.GetAddMethod();
-            removeMethod = e.GetRemoveMethod();
+            var add = e.GetAddMethod();
 
-            if (addMethod == null)
+            if (add == null)
             {
                 throw new InvalidOperationException(Strings_Linq.EVENT_MISSING_ADD_METHOD);
             }
 
-            if (removeMethod == null)
+            addMethod = add;
+
+            var remove = e.GetRemoveMethod();
+
+            if (remove == null)
             {
                 throw new InvalidOperationException(Strings_Linq.EVENT_MISSING_REMOVE_METHOD);
             }
 
+            removeMethod = remove;
+
             var psa = addMethod.GetParameters();
             if (psa.Length != 1)
             {
@@ -88,7 +91,7 @@ namespace System.Reactive
 
             delegateType = psa[0].ParameterType;
 
-            var invokeMethod = delegateType.GetMethod("Invoke");
+            var invokeMethod = delegateType.GetMethod("Invoke")!; // NB: Delegates always have an Invoke method.
 
             var parameters = invokeMethod.GetParameters();
 

Rx.NET/Source/src/System.Reactive/Linq/Observable/Using.cs

@@ -2,8 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT License.
 // See the LICENSE file in the project root for more information. 
 
-#nullable disable
-
 using System.Reactive.Disposables;
 
 namespace System.Reactive.Linq.ObservableImpl
@@ -31,7 +29,7 @@ namespace System.Reactive.Linq.ObservableImpl
             {
             }
 
-            private IDisposable _disposable;
+            private IDisposable? _disposable;
 
             public void Run(Using<TSource, TResource> parent)
             {
@@ -45,7 +43,16 @@ namespace System.Reactive.Linq.ObservableImpl
                         disposable = resource;
                     }
 
-                    source = parent._observableFactory(resource);
+                    //
+                    // NB: We do allow the factory to return `null`, similar to the `using` statement in C#. However, we don't want to bother
+                    //     users with a TResource? parameter and cause a breaking change to their code, even if their factory returns non-null.
+                    //     Right now, we can't track non-null state across the invocation of resourceFactory into observableFactory. If we'd
+                    //     be able to do that, it would make sense to warn users about a possible null. In the absence of this, we'd end up
+                    //     with a lot of false positives (in fact, most code would cause a warning), and force users to pollute their code with
+                    //     the "damn-it" ! operator.
+                    //
+
+                    source = parent._observableFactory(resource!);
                 }
                 catch (Exception exception)
                 {