Fix/remove TODO-NULLABLEs (#44300)

* Fix/remove TODO-NULLABLEs

* remove redundant !

* apply Jozkee's feedback

* address feedback

src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/CompareAttribute.cs

@@ -60,8 +60,6 @@ public CompareAttribute(string otherProperty) : base(SR.CompareAttribute_MustMat
             var display = attributes.OfType<DisplayAttribute>().FirstOrDefault();
             if (display != null)
             {
-                // TODO-NULLABLE: This will return null if [DisplayName] is specified but no Name has been defined - probably a bug.
-                // Should fall back to OtherProperty in this case instead.
                 return display.GetName();
             }
 

src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationAttribute.cs

@@ -273,8 +273,6 @@ private void SetResourceAccessorByPropertyLookup()
                                                     _errorMessageResourceType.FullName));
             }
 
-
-            // TODO-NULLABLE: If the user-provided resource returns null, an ArgumentNullException is thrown - should probably throw a better exception
             _errorMessageResourceAccessor = () => (string)property.GetValue(null, null)!;
         }
 

src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/Validator.cs

@@ -137,7 +137,6 @@ public static class Validator
                 throw new ArgumentNullException(nameof(instance));
             }
 
-            // TODO-NULLABLE: null validationContext isn't supported (GetObjectValidationErrors will throw), remove that check
             if (validationContext != null && instance != validationContext.ObjectInstance)
             {
                 throw new ArgumentException(SR.Validator_InstanceMustMatchValidationContextInstance, nameof(instance));

src/libraries/System.Data.Odbc/src/System/Data/Odbc/OdbcConnectionOpen.cs

@@ -32,7 +32,7 @@ public override string ServerVersion
         {
             get
             {
-                // TODO-NULLABLE: This seems like it returns null if the connection is open, whereas the docs say it should throw
+                // https://github.com/dotnet/runtime/issues/44289: This seems like it returns null if the connection is open, whereas the docs say it should throw
                 // InvalidOperationException
                 return OuterConnection.Open_GetServerVersion()!;
             }

src/libraries/System.Data.OleDb/src/ColumnBinding.cs

@@ -948,8 +948,7 @@ internal OleDbDataReader Value_HCHAPTER()
             Debug.Assert(NativeDBType.HCHAPTER == DbType, "Value_HCHAPTER");
             Debug.Assert(DBStatus.S_OK == StatusValue(), "Value_HCHAPTER");
 
-            // TODO-NULLABLE: This shouldn't return null
-            return DataReader().ResetChapter(IndexForAccessor, IndexWithinAccessor, RowBinding, ValueOffset)!;
+            return DataReader().ResetChapter(IndexForAccessor, IndexWithinAccessor, RowBinding, ValueOffset);
         }
 
         private sbyte Value_I1()

src/libraries/System.Data.OleDb/src/OleDbCommand.cs

@@ -818,7 +818,6 @@ private int ExecuteCommandText(out object executeResult)
             RuntimeHelpers.PrepareConstrainedRegions();
             try
             {
-                // TODO-NULLABLE: Code below seems to assume that bindings will always be non-null
                 if (null != bindings)
                 { // parameters may be suppressed
                     rowbinding = bindings.RowBinding();

src/libraries/System.Data.OleDb/src/OleDbConnectionFactory.cs

@@ -33,8 +33,7 @@ public override DbProviderFactory ProviderFactory
 
         protected override DbConnectionInternal CreateConnection(DbConnectionOptions options, DbConnectionPoolKey poolKey, object poolGroupProviderInfo, DbConnectionPool? pool, DbConnection? owningObject)
         {
-            // TODO-NULLABLE: owningObject may actually be null (see DbConnectionPool.CreateObject), in which case this will throw...
-            DbConnectionInternal result = new OleDbConnectionInternal((OleDbConnectionString)options, (OleDbConnection)owningObject!);
+            DbConnectionInternal result = new OleDbConnectionInternal((OleDbConnectionString)options, (OleDbConnection?)owningObject);
             return result;
         }
 

src/libraries/System.Data.OleDb/src/OleDbConnectionInternal.cs

@@ -385,7 +385,7 @@ internal void EnlistTransactionInternal(SysTx.Transaction? transaction)
             using (IDBInfoWrapper wrapper = IDBInfo())
             {
                 UnsafeNativeMethods.IDBInfo dbInfo = wrapper.Value;
-                // TODO-NULLABLE: check may not be necessary (and thus method may return non-nullable)
+                // https://github.com/dotnet/runtime/issues/44288: check may not be necessary (and thus method may return non-nullable)
                 if (null == dbInfo)
                 {
                     return null;

src/libraries/System.Data.OleDb/src/OleDbDataReader.cs

@@ -951,22 +951,23 @@ protected override DbDataReader GetDbDataReader(int ordinal)
             return GetData(ordinal);
         }
 
-        internal OleDbDataReader? ResetChapter(int bindingIndex, int index, RowBinding rowbinding, int valueOffset)
+        internal OleDbDataReader ResetChapter(int bindingIndex, int index, RowBinding rowbinding, int valueOffset)
         {
             return GetDataForReader(_metadata![bindingIndex + index].ordinal, rowbinding, valueOffset);
         }
 
-        private OleDbDataReader? GetDataForReader(IntPtr ordinal, RowBinding rowbinding, int valueOffset)
+        private OleDbDataReader GetDataForReader(IntPtr ordinal, RowBinding rowbinding, int valueOffset)
         {
             UnsafeNativeMethods.IRowsetInfo rowsetInfo = IRowsetInfo();
             UnsafeNativeMethods.IRowset? result;
             OleDbHResult hr;
             hr = rowsetInfo.GetReferencedRowset((IntPtr)ordinal, ref ODB.IID_IRowset, out result);
 
             ProcessResults(hr);
+            // Per docs result can be null only when hr is DB_E_NOTAREFERENCECOLUMN which in most of the cases will cause the exception in ProcessResult
 
             OleDbDataReader? reader = null;
-            // TODO: Not sure if GetReferenceRowset above actually returns null, calling code seems to assume it doesn't
+
             if (null != result)
             {
                 // only when the first datareader is closed will the connection close
@@ -983,6 +984,7 @@ protected override DbDataReader GetDbDataReader(int ordinal)
                     _connection.AddWeakReference(reader, OleDbReferenceCollection.DataReaderTag);
                 }
             }
+
             return reader;
         }
 
@@ -1022,8 +1024,9 @@ public override Type GetFieldType(int index)
         {
             if (null != _metadata)
             {
-                // TODO-NULLABLE: Should throw if null (empty), though it probably doesn't happen
-                return _metadata[index].type.dataType!;
+                Type? fieldType = _metadata[index].type.dataType;
+                Debug.Assert(fieldType != null);
+                return fieldType;
             }
             throw ADP.DataReaderNoData();
         }
@@ -2273,7 +2276,6 @@ internal void DumpToSchemaTable(UnsafeNativeMethods.IRowset? rowset)
             using (OleDbDataReader dataReader = new OleDbDataReader(_connection, _command, int.MinValue, 0))
             {
                 dataReader.InitializeIRowset(rowset, ChapterHandle.DB_NULL_HCHAPTER, IntPtr.Zero);
-                // TODO-NULLABLE: BuildSchemaTableInfo asserts that rowset isn't null, but doesn't do anything with it
                 dataReader.BuildSchemaTableInfo(rowset!, true, false);
 
                 hiddenColumns = GetPropertyValue(ODB.DBPROP_HIDDENCOLUMNS);

src/libraries/System.Data.OleDb/src/UnsafeNativeMethods.cs

@@ -723,7 +723,7 @@ internal interface IRowsetInfo
             System.Data.OleDb.OleDbHResult GetReferencedRowset(
                 [In] IntPtr iOrdinal,
                 [In] ref Guid riid,
-                [Out, MarshalAs(UnmanagedType.Interface)] out IRowset ppRowset);
+                [Out, MarshalAs(UnmanagedType.Interface)] out IRowset? ppRowset);
 
             //[PreserveSig]
             //int GetSpecification(/*deleted parameter signature*/);

src/libraries/System.Linq/src/System/Linq/Select.SpeedOpt.cs

@@ -11,7 +11,7 @@ namespace System.Linq
     public static partial class Enumerable
     {
         static partial void CreateSelectIPartitionIterator<TResult, TSource>(
-            Func<TSource, TResult> selector, IPartition<TSource> partition, ref IEnumerable<TResult>? result) // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/38327
+            Func<TSource, TResult> selector, IPartition<TSource> partition, ref IEnumerable<TResult>? result)
         {
             result = partition is EmptyPartition<TSource> ?
                 EmptyPartition<TResult>.Instance :

src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/Extensions.cs

@@ -271,15 +271,13 @@ public static IEnumerable<XElement> DescendantsAndSelf(this IEnumerable<XElement
         /// for each <see cref="XElement"/> in this <see cref="IEnumerable"/> of <see cref="XElement"/>.
         /// in document order
         /// </returns>
-        public static IEnumerable<T> InDocumentOrder<T>(this IEnumerable<T> source) where T : XNode
+        public static IEnumerable<T> InDocumentOrder<T>(this IEnumerable<T> source) where T : XNode?
         {
             if (source == null) throw new ArgumentNullException(nameof(source));
             return DocumentOrderIterator<T>(source);
         }
 
-        // TODO-NULLABLE: Consider changing to T? instead.
-        // If we do it, we will also need to change XNodeDocumentOrderComparer to implement IComparer<XNode?> instead.
-        private static IEnumerable<T> DocumentOrderIterator<T>(IEnumerable<T> source) where T : XNode
+        private static IEnumerable<T> DocumentOrderIterator<T>(IEnumerable<T> source) where T : XNode?
         {
             int count;
             T[] items = EnumerableHelpers.ToArray(source, out count);

src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XContainer.cs

@@ -1041,8 +1041,7 @@ public async ValueTask<bool> ReadContentFromAsync(XContainer rootContainer, XmlR
             public bool ReadContentFrom(XContainer rootContainer, XmlReader r, LoadOptions o)
             {
                 XNode? newNode = null;
-                // TODO-NULLABLE: Consider changing XmlReader.BaseURI to non-nullable.
-                string baseUri = r.BaseURI!;
+                string baseUri = r.BaseURI;
 
                 switch (r.NodeType)
                 {

src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XDocumentType.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Diagnostics.CodeAnalysis;
 using System.Threading;
 using System.Threading.Tasks;
 
@@ -19,12 +20,12 @@ public class XDocumentType : XNode
         /// <summary>
         /// Initializes an empty instance of the <see cref="XDocumentType"/> class.
         /// </summary>
-        public XDocumentType(string name, string? publicId, string? systemId, string internalSubset)
+        public XDocumentType(string name, string? publicId, string? systemId, string? internalSubset)
         {
             _name = XmlConvert.VerifyName(name);
             _publicId = publicId;
             _systemId = systemId;
-            _internalSubset = internalSubset;
+            _internalSubset = internalSubset ?? string.Empty;
         }
 
         /// <summary>
@@ -53,20 +54,17 @@ internal XDocumentType(XmlReader r)
         /// <summary>
         /// Gets or sets the internal subset for this Document Type Definition (DTD).
         /// </summary>
+        [AllowNull]
         public string InternalSubset
         {
             get
             {
-                // TODO-NULLABLE: As per documentation, this should return string.Empty.
-                // Should we check for null here?
-                // This is also referenced by XNodeReader.Value which overrides XmlReader.Value, which is non-nullable.
-                // There is one case that passes a nullable parameter (XNodeBuilder.WriteDocType), currently we are just asserting that the nullable parameter does not receive null.
                 return _internalSubset;
             }
             set
             {
                 bool notify = NotifyChanging(this, XObjectChangeEventArgs.Value);
-                _internalSubset = value;
+                _internalSubset = value ?? string.Empty;
                 if (notify) NotifyChanged(this, XObjectChangeEventArgs.Value);
             }
         }
@@ -184,7 +182,7 @@ internal override int GetDeepHashCode()
             return _name.GetHashCode() ^
                 (_publicId != null ? _publicId.GetHashCode() : 0) ^
                 (_systemId != null ? _systemId.GetHashCode() : 0) ^
-                (_internalSubset != null ? _internalSubset.GetHashCode() : 0);
+                _internalSubset.GetHashCode();
         }
     }
 }

src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeBuilder.cs

@@ -83,7 +83,6 @@ public override void WriteComment(string? text)
 
         public override void WriteDocType(string name, string? pubid, string? sysid, string? subset)
         {
-            Debug.Assert(subset != null);
             AddNode(new XDocumentType(name, pubid, sysid, subset));
         }
 

src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeDocumentOrderComparer.cs

@@ -13,7 +13,7 @@ namespace System.Xml.Linq
     /// </summary>
     public sealed class XNodeDocumentOrderComparer :
         IComparer,
-        IComparer<XNode>
+        IComparer<XNode?>
     {
         /// <summary>
         /// Compares two nodes to determine their relative XML document order.

src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeReader.cs

@@ -552,9 +552,11 @@ public override void Close()
             return null;
         }
 
-        // TODO-NULLABLE: decide if base signature should be switched to return string?
         public override string GetAttribute(int index)
         {
+            // https://github.com/dotnet/runtime/issues/44287
+            //     We should replace returning null with ArgumentOutOfRangeException
+            //     In case of not interactive state likely we should throw InvalidOperationException
             if (!IsInteractive)
             {
                 return null!;

src/libraries/System.Private.Xml.Linq/src/System/Xml/XPath/XNodeNavigator.cs

@@ -462,11 +462,12 @@ public override bool MoveToNamespace(string localName)
                 {
                     return false; // backcompat
                 }
-                // TODO-NULLABLE: Unnecessary null check?
+
                 if (localName != null && localName.Length == 0)
                 {
                     localName = "xmlns"; // backcompat
                 }
+
                 XAttribute? a = GetFirstNamespaceDeclarationGlobal(e);
                 while (a != null)
                 {
@@ -478,6 +479,7 @@ public override bool MoveToNamespace(string localName)
                     }
                     a = GetNextNamespaceDeclarationGlobal(a);
                 }
+
                 if (localName == "xml")
                 {
                     _source = GetXmlNamespaceDeclaration();

src/libraries/System.Xml.XDocument/ref/System.Xml.XDocument.cs

@@ -22,7 +22,7 @@ public static partial class Extensions
         public static System.Collections.Generic.IEnumerable<System.Xml.Linq.XElement> Descendants<T>(this System.Collections.Generic.IEnumerable<T?> source, System.Xml.Linq.XName? name) where T : System.Xml.Linq.XContainer { throw null; }
         public static System.Collections.Generic.IEnumerable<System.Xml.Linq.XElement> Elements<T>(this System.Collections.Generic.IEnumerable<T?> source) where T : System.Xml.Linq.XContainer { throw null; }
         public static System.Collections.Generic.IEnumerable<System.Xml.Linq.XElement> Elements<T>(this System.Collections.Generic.IEnumerable<T?> source, System.Xml.Linq.XName? name) where T : System.Xml.Linq.XContainer { throw null; }
-        public static System.Collections.Generic.IEnumerable<T> InDocumentOrder<T>(this System.Collections.Generic.IEnumerable<T> source) where T : System.Xml.Linq.XNode { throw null; }
+        public static System.Collections.Generic.IEnumerable<T> InDocumentOrder<T>(this System.Collections.Generic.IEnumerable<T> source) where T : System.Xml.Linq.XNode? { throw null; }
         public static System.Collections.Generic.IEnumerable<System.Xml.Linq.XNode> Nodes<T>(this System.Collections.Generic.IEnumerable<T?> source) where T : System.Xml.Linq.XContainer { throw null; }
         public static void Remove(this System.Collections.Generic.IEnumerable<System.Xml.Linq.XAttribute?> source) { }
         public static void Remove<T>(this System.Collections.Generic.IEnumerable<T?> source) where T : System.Xml.Linq.XNode { }
@@ -212,8 +212,9 @@ public partial class XDocument : System.Xml.Linq.XContainer
     }
     public partial class XDocumentType : System.Xml.Linq.XNode
     {
-        public XDocumentType(string name, string? publicId, string? systemId, string internalSubset) { }
+        public XDocumentType(string name, string? publicId, string? systemId, string? internalSubset) { }
         public XDocumentType(System.Xml.Linq.XDocumentType other) { }
+        [System.Diagnostics.CodeAnalysis.AllowNull]
         public string InternalSubset { get { throw null; } set { } }
         public string Name { get { throw null; } set { } }
         public override System.Xml.XmlNodeType NodeType { get { throw null; } }
@@ -425,7 +426,7 @@ public abstract partial class XNode : System.Xml.Linq.XObject
         public abstract void WriteTo(System.Xml.XmlWriter writer);
         public abstract System.Threading.Tasks.Task WriteToAsync(System.Xml.XmlWriter writer, System.Threading.CancellationToken cancellationToken);
     }
-    public sealed partial class XNodeDocumentOrderComparer : System.Collections.Generic.IComparer<System.Xml.Linq.XNode>, System.Collections.IComparer
+    public sealed partial class XNodeDocumentOrderComparer : System.Collections.Generic.IComparer<System.Xml.Linq.XNode?>, System.Collections.IComparer
     {
         public XNodeDocumentOrderComparer() { }
         public int Compare(System.Xml.Linq.XNode? x, System.Xml.Linq.XNode? y) { throw null; }