From c871e237b00d3f07f76b370c194ae9430137a5d7 Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Mon, 9 Nov 2020 13:07:15 +0100 Subject: [PATCH] Fix/remove TODO-NULLABLEs (#44300) * Fix/remove TODO-NULLABLEs * remove redundant ! * apply Jozkee's feedback * address feedback --- .../ComponentModel/DataAnnotations/CompareAttribute.cs | 2 -- .../ComponentModel/DataAnnotations/ValidationAttribute.cs | 2 -- .../src/System/ComponentModel/DataAnnotations/Validator.cs | 1 - .../src/System/Data/Odbc/OdbcConnectionOpen.cs | 2 +- src/libraries/System.Data.OleDb/src/ColumnBinding.cs | 3 +-- src/libraries/System.Data.OleDb/src/OleDbCommand.cs | 1 - .../System.Data.OleDb/src/OleDbConnectionFactory.cs | 3 +-- .../System.Data.OleDb/src/OleDbConnectionInternal.cs | 2 +- src/libraries/System.Data.OleDb/src/OleDbDataReader.cs | 14 ++++++++------ src/libraries/System.Data.OleDb/src/UnsafeNativeMethods.cs | 2 +- .../System.Linq/src/System/Linq/Select.SpeedOpt.cs | 2 +- .../src/System/Xml/Linq/Extensions.cs | 6 ++---- .../src/System/Xml/Linq/XContainer.cs | 3 +-- .../src/System/Xml/Linq/XDocumentType.cs | 14 ++++++-------- .../src/System/Xml/Linq/XNodeBuilder.cs | 1 - .../src/System/Xml/Linq/XNodeDocumentOrderComparer.cs | 2 +- .../src/System/Xml/Linq/XNodeReader.cs | 4 +++- .../src/System/Xml/XPath/XNodeNavigator.cs | 4 +++- .../System.Xml.XDocument/ref/System.Xml.XDocument.cs | 7 ++++--- 19 files changed, 34 insertions(+), 41 deletions(-) diff --git a/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/CompareAttribute.cs b/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/CompareAttribute.cs index dab56c6..ec7caa4f 100644 --- a/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/CompareAttribute.cs +++ b/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/CompareAttribute.cs @@ -60,8 +60,6 @@ namespace System.ComponentModel.DataAnnotations var display = attributes.OfType().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(); } diff --git a/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationAttribute.cs b/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationAttribute.cs index 01ec012..eedd0de 100644 --- a/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationAttribute.cs +++ b/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/ValidationAttribute.cs @@ -273,8 +273,6 @@ namespace System.ComponentModel.DataAnnotations _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)!; } diff --git a/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/Validator.cs b/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/Validator.cs index b6251f5..1363b58 100644 --- a/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/Validator.cs +++ b/src/libraries/System.ComponentModel.Annotations/src/System/ComponentModel/DataAnnotations/Validator.cs @@ -137,7 +137,6 @@ namespace System.ComponentModel.DataAnnotations 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)); diff --git a/src/libraries/System.Data.Odbc/src/System/Data/Odbc/OdbcConnectionOpen.cs b/src/libraries/System.Data.Odbc/src/System/Data/Odbc/OdbcConnectionOpen.cs index 008facb..4b5b73b 100644 --- a/src/libraries/System.Data.Odbc/src/System/Data/Odbc/OdbcConnectionOpen.cs +++ b/src/libraries/System.Data.Odbc/src/System/Data/Odbc/OdbcConnectionOpen.cs @@ -32,7 +32,7 @@ namespace System.Data.Odbc { 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()!; } diff --git a/src/libraries/System.Data.OleDb/src/ColumnBinding.cs b/src/libraries/System.Data.OleDb/src/ColumnBinding.cs index 3433415..8eca4d6 100644 --- a/src/libraries/System.Data.OleDb/src/ColumnBinding.cs +++ b/src/libraries/System.Data.OleDb/src/ColumnBinding.cs @@ -948,8 +948,7 @@ namespace System.Data.OleDb 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() diff --git a/src/libraries/System.Data.OleDb/src/OleDbCommand.cs b/src/libraries/System.Data.OleDb/src/OleDbCommand.cs index a47d9cd..a6b9503 100644 --- a/src/libraries/System.Data.OleDb/src/OleDbCommand.cs +++ b/src/libraries/System.Data.OleDb/src/OleDbCommand.cs @@ -818,7 +818,6 @@ namespace System.Data.OleDb 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(); diff --git a/src/libraries/System.Data.OleDb/src/OleDbConnectionFactory.cs b/src/libraries/System.Data.OleDb/src/OleDbConnectionFactory.cs index 9b1a7cd..2893c01 100644 --- a/src/libraries/System.Data.OleDb/src/OleDbConnectionFactory.cs +++ b/src/libraries/System.Data.OleDb/src/OleDbConnectionFactory.cs @@ -33,8 +33,7 @@ namespace System.Data.OleDb 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; } diff --git a/src/libraries/System.Data.OleDb/src/OleDbConnectionInternal.cs b/src/libraries/System.Data.OleDb/src/OleDbConnectionInternal.cs index 09ab698..f2b98ef 100644 --- a/src/libraries/System.Data.OleDb/src/OleDbConnectionInternal.cs +++ b/src/libraries/System.Data.OleDb/src/OleDbConnectionInternal.cs @@ -385,7 +385,7 @@ namespace System.Data.OleDb 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; diff --git a/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs b/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs index 98846ea..f3b6a0e 100644 --- a/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs +++ b/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs @@ -951,12 +951,12 @@ namespace System.Data.OleDb 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; @@ -964,9 +964,10 @@ namespace System.Data.OleDb 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 @@ namespace System.Data.OleDb _connection.AddWeakReference(reader, OleDbReferenceCollection.DataReaderTag); } } + return reader; } @@ -1022,8 +1024,9 @@ namespace System.Data.OleDb { 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 @@ namespace System.Data.OleDb 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); diff --git a/src/libraries/System.Data.OleDb/src/UnsafeNativeMethods.cs b/src/libraries/System.Data.OleDb/src/UnsafeNativeMethods.cs index 4f67170..b7be994 100644 --- a/src/libraries/System.Data.OleDb/src/UnsafeNativeMethods.cs +++ b/src/libraries/System.Data.OleDb/src/UnsafeNativeMethods.cs @@ -723,7 +723,7 @@ namespace System.Data.Common 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*/); diff --git a/src/libraries/System.Linq/src/System/Linq/Select.SpeedOpt.cs b/src/libraries/System.Linq/src/System/Linq/Select.SpeedOpt.cs index e23cbd4..f93b7c0 100644 --- a/src/libraries/System.Linq/src/System/Linq/Select.SpeedOpt.cs +++ b/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( - Func selector, IPartition partition, ref IEnumerable? result) // TODO-NULLABLE: https://github.com/dotnet/roslyn/issues/38327 + Func selector, IPartition partition, ref IEnumerable? result) { result = partition is EmptyPartition ? EmptyPartition.Instance : diff --git a/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/Extensions.cs b/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/Extensions.cs index 7be6300..024823a 100644 --- a/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/Extensions.cs +++ b/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/Extensions.cs @@ -271,15 +271,13 @@ namespace System.Xml.Linq /// for each in this of . /// in document order /// - public static IEnumerable InDocumentOrder(this IEnumerable source) where T : XNode + public static IEnumerable InDocumentOrder(this IEnumerable source) where T : XNode? { if (source == null) throw new ArgumentNullException(nameof(source)); return DocumentOrderIterator(source); } - // TODO-NULLABLE: Consider changing to T? instead. - // If we do it, we will also need to change XNodeDocumentOrderComparer to implement IComparer instead. - private static IEnumerable DocumentOrderIterator(IEnumerable source) where T : XNode + private static IEnumerable DocumentOrderIterator(IEnumerable source) where T : XNode? { int count; T[] items = EnumerableHelpers.ToArray(source, out count); diff --git a/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XContainer.cs b/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XContainer.cs index 1a0d849..fbc3509 100644 --- a/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XContainer.cs +++ b/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XContainer.cs @@ -1041,8 +1041,7 @@ namespace System.Xml.Linq 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) { diff --git a/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XDocumentType.cs b/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XDocumentType.cs index 7ba36d1..8bf4c83 100644 --- a/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XDocumentType.cs +++ b/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 @@ namespace System.Xml.Linq /// /// Initializes an empty instance of the class. /// - 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; } /// @@ -53,20 +54,17 @@ namespace System.Xml.Linq /// /// Gets or sets the internal subset for this Document Type Definition (DTD). /// + [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 @@ namespace System.Xml.Linq return _name.GetHashCode() ^ (_publicId != null ? _publicId.GetHashCode() : 0) ^ (_systemId != null ? _systemId.GetHashCode() : 0) ^ - (_internalSubset != null ? _internalSubset.GetHashCode() : 0); + _internalSubset.GetHashCode(); } } } diff --git a/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeBuilder.cs b/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeBuilder.cs index 5269b2f..027eafd 100644 --- a/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeBuilder.cs +++ b/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeBuilder.cs @@ -83,7 +83,6 @@ namespace System.Xml.Linq public override void WriteDocType(string name, string? pubid, string? sysid, string? subset) { - Debug.Assert(subset != null); AddNode(new XDocumentType(name, pubid, sysid, subset)); } diff --git a/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeDocumentOrderComparer.cs b/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeDocumentOrderComparer.cs index 6abc99f..461021f 100644 --- a/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeDocumentOrderComparer.cs +++ b/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeDocumentOrderComparer.cs @@ -13,7 +13,7 @@ namespace System.Xml.Linq /// public sealed class XNodeDocumentOrderComparer : IComparer, - IComparer + IComparer { /// /// Compares two nodes to determine their relative XML document order. diff --git a/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeReader.cs b/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeReader.cs index 96b9d9f..b1a4e58 100644 --- a/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeReader.cs +++ b/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeReader.cs @@ -552,9 +552,11 @@ namespace System.Xml.Linq 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!; diff --git a/src/libraries/System.Private.Xml.Linq/src/System/Xml/XPath/XNodeNavigator.cs b/src/libraries/System.Private.Xml.Linq/src/System/Xml/XPath/XNodeNavigator.cs index c301348..2455ff8 100644 --- a/src/libraries/System.Private.Xml.Linq/src/System/Xml/XPath/XNodeNavigator.cs +++ b/src/libraries/System.Private.Xml.Linq/src/System/Xml/XPath/XNodeNavigator.cs @@ -462,11 +462,12 @@ namespace System.Xml.XPath { 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 @@ namespace System.Xml.XPath } a = GetNextNamespaceDeclarationGlobal(a); } + if (localName == "xml") { _source = GetXmlNamespaceDeclaration(); diff --git a/src/libraries/System.Xml.XDocument/ref/System.Xml.XDocument.cs b/src/libraries/System.Xml.XDocument/ref/System.Xml.XDocument.cs index 5849d80..f43da1b 100644 --- a/src/libraries/System.Xml.XDocument/ref/System.Xml.XDocument.cs +++ b/src/libraries/System.Xml.XDocument/ref/System.Xml.XDocument.cs @@ -22,7 +22,7 @@ namespace System.Xml.Linq public static System.Collections.Generic.IEnumerable Descendants(this System.Collections.Generic.IEnumerable source, System.Xml.Linq.XName? name) where T : System.Xml.Linq.XContainer { throw null; } public static System.Collections.Generic.IEnumerable Elements(this System.Collections.Generic.IEnumerable source) where T : System.Xml.Linq.XContainer { throw null; } public static System.Collections.Generic.IEnumerable Elements(this System.Collections.Generic.IEnumerable source, System.Xml.Linq.XName? name) where T : System.Xml.Linq.XContainer { throw null; } - public static System.Collections.Generic.IEnumerable InDocumentOrder(this System.Collections.Generic.IEnumerable source) where T : System.Xml.Linq.XNode { throw null; } + public static System.Collections.Generic.IEnumerable InDocumentOrder(this System.Collections.Generic.IEnumerable source) where T : System.Xml.Linq.XNode? { throw null; } public static System.Collections.Generic.IEnumerable Nodes(this System.Collections.Generic.IEnumerable source) where T : System.Xml.Linq.XContainer { throw null; } public static void Remove(this System.Collections.Generic.IEnumerable source) { } public static void Remove(this System.Collections.Generic.IEnumerable source) where T : System.Xml.Linq.XNode { } @@ -212,8 +212,9 @@ namespace System.Xml.Linq } 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 @@ namespace System.Xml.Linq 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.Collections.IComparer + public sealed partial class XNodeDocumentOrderComparer : System.Collections.Generic.IComparer, System.Collections.IComparer { public XNodeDocumentOrderComparer() { } public int Compare(System.Xml.Linq.XNode? x, System.Xml.Linq.XNode? y) { throw null; } -- 2.7.4