From 36023d9f145c679aaeb90061b3323153d8f1260e Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Thu, 5 Nov 2020 15:11:00 +0100 Subject: [PATCH 1/4] Fix/remove TODO-NULLABLEs --- .../DataAnnotations/CompareAttribute.cs | 2 -- .../DataAnnotations/ValidationAttribute.cs | 2 -- .../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 +- .../System.Data.OleDb/src/OleDbDataReader.cs | 12 ++++++------ .../System.Linq/src/System/Linq/Select.SpeedOpt.cs | 2 +- .../src/System/Xml/Linq/Extensions.cs | 2 -- .../src/System/Xml/Linq/XContainer.cs | 3 +-- .../src/System/Xml/Linq/XDocumentType.cs | 10 +++------- .../src/System/Xml/Linq/XNodeBuilder.cs | 1 - .../src/System/Xml/Linq/XNodeReader.cs | 6 ++++-- .../src/System/Xml/XPath/XNodeNavigator.cs | 4 +++- .../System.Xml.XDocument/ref/System.Xml.XDocument.cs | 4 ++-- 17 files changed, 24 insertions(+), 36 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 dab56c6018e6e..ec7caa4fce68a 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 @@ public override string FormatErrorMessage(string name) => 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 01ec012f20de9..eedd0de4bf1a4 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 @@ 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)!; } 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 b6251f53451f3..1363b58d1540c 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 @@ public static bool TryValidateObject(object instance, ValidationContext validati 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 008facb51311e..4b5b73bfa6428 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 @@ 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()!; } diff --git a/src/libraries/System.Data.OleDb/src/ColumnBinding.cs b/src/libraries/System.Data.OleDb/src/ColumnBinding.cs index 34334156e3e1f..8eca4d6280676 100644 --- a/src/libraries/System.Data.OleDb/src/ColumnBinding.cs +++ b/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() diff --git a/src/libraries/System.Data.OleDb/src/OleDbCommand.cs b/src/libraries/System.Data.OleDb/src/OleDbCommand.cs index a47d9cd6fa444..a6b9503e0abf6 100644 --- a/src/libraries/System.Data.OleDb/src/OleDbCommand.cs +++ b/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(); diff --git a/src/libraries/System.Data.OleDb/src/OleDbConnectionFactory.cs b/src/libraries/System.Data.OleDb/src/OleDbConnectionFactory.cs index 9b1a7cd366424..2893c0173ee53 100644 --- a/src/libraries/System.Data.OleDb/src/OleDbConnectionFactory.cs +++ b/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; } diff --git a/src/libraries/System.Data.OleDb/src/OleDbConnectionInternal.cs b/src/libraries/System.Data.OleDb/src/OleDbConnectionInternal.cs index 09ab6982105b5..f2b98efd4e4cb 100644 --- a/src/libraries/System.Data.OleDb/src/OleDbConnectionInternal.cs +++ b/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; diff --git a/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs b/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs index 98846eab53ae1..4078b7d1c361c 100644 --- a/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs +++ b/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs @@ -951,15 +951,15 @@ 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; + UnsafeNativeMethods.IRowset result; OleDbHResult hr; hr = rowsetInfo.GetReferencedRowset((IntPtr)ordinal, ref ODB.IID_IRowset, out result); @@ -1022,8 +1022,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 +2274,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); 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 e23cbd49f3223..f93b7c082f38d 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 7be6300c1a1ab..5625b89c17f80 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 @@ -277,8 +277,6 @@ public static IEnumerable InDocumentOrder(this IEnumerable source) wher 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 { int 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 1a0d8492fd2fe..fbc3509c3d643 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 @@ public async ValueTask 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) { 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 7ba36d110aa0d..ea555c2f3f99b 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 @@ -14,12 +14,12 @@ public class XDocumentType : XNode private string _name; private string? _publicId; private string? _systemId; - private string _internalSubset; + private string? _internalSubset; /// /// 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; @@ -53,14 +53,10 @@ internal XDocumentType(XmlReader r) /// /// Gets or sets the internal subset for this Document Type Definition (DTD). /// - public string InternalSubset + 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 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 5269b2fc077a3..027eafd2ba5e4 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 @@ 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)); } 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 96b9d9f5d6807..05bf4a0612fb5 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 @@ -378,7 +378,7 @@ public override string Value case XmlNodeType.ProcessingInstruction: return ((XProcessingInstruction)o).Data; case XmlNodeType.DocumentType: - return ((XDocumentType)o).InternalSubset; + return ((XDocumentType)o).InternalSubset ?? string.Empty; default: return string.Empty; } @@ -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!; 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 c301348cda7cb..2455ff886c3cc 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 @@ 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(); 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 5849d80eb2d5b..1d067e1e7ff67 100644 --- a/src/libraries/System.Xml.XDocument/ref/System.Xml.XDocument.cs +++ b/src/libraries/System.Xml.XDocument/ref/System.Xml.XDocument.cs @@ -212,9 +212,9 @@ public override void WriteTo(System.Xml.XmlWriter writer) { } } 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) { } - public string InternalSubset { get { throw null; } set { } } + public string? InternalSubset { get { throw null; } set { } } public string Name { get { throw null; } set { } } public override System.Xml.XmlNodeType NodeType { get { throw null; } } public string? PublicId { get { throw null; } set { } } From 8d16edadb626c0c379e4e131f98d4b887d88fb5a Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Thu, 5 Nov 2020 15:46:10 +0100 Subject: [PATCH 2/4] remove redundant ! --- src/libraries/System.Data.OleDb/src/OleDbDataReader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs b/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs index 4078b7d1c361c..ed43c3ff58f99 100644 --- a/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs +++ b/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs @@ -1022,7 +1022,7 @@ public override Type GetFieldType(int index) { if (null != _metadata) { - Type? fieldType = _metadata[index].type.dataType!; + Type? fieldType = _metadata[index].type.dataType; Debug.Assert(fieldType != null); return fieldType; } From 04532ed411e32081aa60ca64933f606759b5b63b Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Fri, 6 Nov 2020 11:05:55 +0100 Subject: [PATCH 3/4] apply Jozkee's feedback --- .../src/System/Xml/Linq/Extensions.cs | 4 ++-- .../src/System/Xml/Linq/XDocumentType.cs | 10 ++++++---- .../src/System/Xml/Linq/XNodeDocumentOrderComparer.cs | 2 +- .../src/System/Xml/Linq/XNodeReader.cs | 2 +- .../System.Xml.XDocument/ref/System.Xml.XDocument.cs | 7 ++++--- 5 files changed, 14 insertions(+), 11 deletions(-) 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 5625b89c17f80..024823a914136 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,13 +271,13 @@ public static IEnumerable Elements(this IEnumerable source, XNa /// 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); } - 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/XDocumentType.cs b/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XDocumentType.cs index ea555c2f3f99b..e714df8e835b5 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; @@ -14,7 +15,7 @@ public class XDocumentType : XNode private string _name; private string? _publicId; private string? _systemId; - private string? _internalSubset; + private string _internalSubset; /// /// Initializes an empty instance of the class. @@ -24,7 +25,7 @@ public XDocumentType(string name, string? publicId, string? systemId, string? in _name = XmlConvert.VerifyName(name); _publicId = publicId; _systemId = systemId; - _internalSubset = internalSubset; + _internalSubset = internalSubset ?? string.Empty; } /// @@ -53,7 +54,8 @@ internal XDocumentType(XmlReader r) /// /// Gets or sets the internal subset for this Document Type Definition (DTD). /// - public string? InternalSubset + [AllowNull] + public string InternalSubset { get { @@ -62,7 +64,7 @@ public string? InternalSubset set { bool notify = NotifyChanging(this, XObjectChangeEventArgs.Value); - _internalSubset = value; + _internalSubset = value ?? string.Empty; if (notify) NotifyChanged(this, XObjectChangeEventArgs.Value); } } 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 6abc99f4dc025..461021f2a5da0 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 05bf4a0612fb5..b1a4e58c1dbf7 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 @@ -378,7 +378,7 @@ public override string Value case XmlNodeType.ProcessingInstruction: return ((XProcessingInstruction)o).Data; case XmlNodeType.DocumentType: - return ((XDocumentType)o).InternalSubset ?? string.Empty; + return ((XDocumentType)o).InternalSubset; default: return string.Empty; } 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 1d067e1e7ff67..f43da1b826971 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 @@ public static partial class Extensions 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 { } @@ -214,7 +214,8 @@ public partial class XDocumentType : System.Xml.Linq.XNode { public XDocumentType(string name, string? publicId, string? systemId, string? internalSubset) { } public XDocumentType(System.Xml.Linq.XDocumentType other) { } - public string? InternalSubset { get { throw null; } set { } } + [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; } } public string? PublicId { get { throw null; } set { } } @@ -425,7 +426,7 @@ public void ReplaceWith(params object?[] content) { } 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; } From 5d7e0e28b208d84351c24fb1d0d6c788e2ecdae8 Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Mon, 9 Nov 2020 10:22:02 +0100 Subject: [PATCH 4/4] address feedback --- src/libraries/System.Data.OleDb/src/OleDbDataReader.cs | 6 ++++-- src/libraries/System.Data.OleDb/src/UnsafeNativeMethods.cs | 2 +- .../src/System/Xml/Linq/XDocumentType.cs | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs b/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs index ed43c3ff58f99..f3b6a0e011710 100644 --- a/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs +++ b/src/libraries/System.Data.OleDb/src/OleDbDataReader.cs @@ -959,14 +959,15 @@ internal OleDbDataReader ResetChapter(int bindingIndex, int index, RowBinding ro private OleDbDataReader GetDataForReader(IntPtr ordinal, RowBinding rowbinding, int valueOffset) { UnsafeNativeMethods.IRowsetInfo rowsetInfo = IRowsetInfo(); - UnsafeNativeMethods.IRowset result; + 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 @@ private OleDbDataReader GetDataForReader(IntPtr ordinal, RowBinding rowbinding, _connection.AddWeakReference(reader, OleDbReferenceCollection.DataReaderTag); } } + return reader; } diff --git a/src/libraries/System.Data.OleDb/src/UnsafeNativeMethods.cs b/src/libraries/System.Data.OleDb/src/UnsafeNativeMethods.cs index 4f6717007d3e3..b7be99491c17a 100644 --- a/src/libraries/System.Data.OleDb/src/UnsafeNativeMethods.cs +++ b/src/libraries/System.Data.OleDb/src/UnsafeNativeMethods.cs @@ -723,7 +723,7 @@ System.Data.OleDb.OleDbHResult GetProperties( 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.Private.Xml.Linq/src/System/Xml/Linq/XDocumentType.cs b/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XDocumentType.cs index e714df8e835b5..8bf4c83aa3f15 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 @@ -182,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(); } } }