From 25e999005627e455f6e7d0a86f698d1a3d6f5984 Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Tue, 3 Nov 2020 14:18:46 +0100 Subject: [PATCH] Fix/remove TODO-NULLABLE in System.Private.Xml (#44149) * Fix/remove TODO-NULLABLE in System.Private.Xml * update contract for System.Xml.ReaderWriter * follow up changes --- src/libraries/System.Data.Common/src/System/Data/xmlsaver.cs | 2 +- .../src/System/Runtime/Serialization/XmlSerializableReader.cs | 2 +- .../src/System/Xml/XmlDictionaryReader.cs | 2 +- .../src/System/Xml/Core/XmlAsyncCheckReader.cs | 2 +- .../System.Private.Xml/src/System/Xml/Core/XmlReader.cs | 2 +- .../System.Private.Xml/src/System/Xml/Core/XmlSubtreeReader.cs | 2 +- .../System.Private.Xml/src/System/Xml/Core/XmlTextReader.cs | 2 +- .../src/System/Xml/Core/XmlTextReaderImpl.Unix.cs | 4 +++- .../src/System/Xml/Core/XmlTextReaderImpl.cs | 10 +++++----- .../src/System/Xml/Core/XmlValidatingReader.cs | 2 +- .../src/System/Xml/Core/XmlValidatingReaderImpl.cs | 2 +- .../src/System/Xml/Core/XmlWrappingReader.cs | 2 +- .../System.Private.Xml/src/System/Xml/Core/XsdCachingReader.cs | 2 +- .../src/System/Xml/Core/XsdValidatingReader.cs | 2 +- .../src/System/Xml/Dom/DocumentXPathNavigator.cs | 7 +------ .../src/System/Xml/Serialization/XmlSerializationReader.cs | 2 +- .../src/System/Xml/Xsl/XPath/IXpathBuilder.cs | 6 ++---- .../System.Private.Xml/src/System/Xml/Xsl/XmlIlGenerator.cs | 1 - .../src/System/Xml/Xsl/XmlQueryTypeFactory.cs | 9 +++++---- .../src/System/Xml/Xsl/XsltOld/XsltCompileContext.cs | 7 +++++-- .../System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs | 6 +++--- 21 files changed, 37 insertions(+), 39 deletions(-) diff --git a/src/libraries/System.Data.Common/src/System/Data/xmlsaver.cs b/src/libraries/System.Data.Common/src/System/Data/xmlsaver.cs index 7307670..4f68992 100644 --- a/src/libraries/System.Data.Common/src/System/Data/xmlsaver.cs +++ b/src/libraries/System.Data.Common/src/System/Data/xmlsaver.cs @@ -3360,7 +3360,7 @@ namespace System.Data get { return _xmlreader.Depth; } } - public override string? BaseURI + public override string BaseURI { get { return _xmlreader.BaseURI; } } diff --git a/src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/XmlSerializableReader.cs b/src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/XmlSerializableReader.cs index 6980f99..d5faeff 100644 --- a/src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/XmlSerializableReader.cs +++ b/src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/XmlSerializableReader.cs @@ -76,7 +76,7 @@ namespace System.Runtime.Serialization public override bool HasValue { get { return InnerReader.HasValue; } } public override string Value { get { return InnerReader.Value; } } public override int Depth { get { return InnerReader.Depth; } } - public override string? BaseURI { get { return InnerReader.BaseURI; } } + public override string BaseURI { get { return InnerReader.BaseURI; } } public override bool IsEmptyElement { get { return InnerReader.IsEmptyElement; } } public override bool IsDefault { get { return InnerReader.IsDefault; } } public override XmlSpace XmlSpace { get { return InnerReader.XmlSpace; } } diff --git a/src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlDictionaryReader.cs b/src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlDictionaryReader.cs index 894aa04..160d6ff 100644 --- a/src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlDictionaryReader.cs +++ b/src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlDictionaryReader.cs @@ -1344,7 +1344,7 @@ namespace System.Xml } } - public override string? BaseURI + public override string BaseURI { get { diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlAsyncCheckReader.cs b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlAsyncCheckReader.cs index bbb0479..1569a36 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlAsyncCheckReader.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlAsyncCheckReader.cs @@ -151,7 +151,7 @@ namespace System.Xml } } - public override string? BaseURI + public override string BaseURI { get { diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlReader.cs b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlReader.cs index 310d9de..e27203c 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlReader.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlReader.cs @@ -140,7 +140,7 @@ namespace System.Xml public abstract int Depth { get; } // Gets the base URI of the current node. - public abstract string? BaseURI { get; } + public abstract string BaseURI { get; } // Gets a value indicating whether the current node is an empty element (for example, ). public abstract bool IsEmptyElement { get; } diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlSubtreeReader.cs b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlSubtreeReader.cs index 3325ddc..5e395fe 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlSubtreeReader.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlSubtreeReader.cs @@ -180,7 +180,7 @@ namespace System.Xml } } - public override string? BaseURI + public override string BaseURI { get { diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReader.cs b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReader.cs index 38e8705..95ef09c 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReader.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReader.cs @@ -148,7 +148,7 @@ namespace System.Xml get { return _impl.Depth; } } - public override string? BaseURI + public override string BaseURI { get { return _impl.BaseURI; } } diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.Unix.cs b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.Unix.cs index 74836c7..4321e78 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.Unix.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.Unix.cs @@ -1,11 +1,13 @@ // 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; + namespace System.Xml { internal partial class XmlTextReaderImpl { - static partial void ConvertAbsoluteUnixPathToAbsoluteUri(ref string? url, XmlResolver? resolver) + static partial void ConvertAbsoluteUnixPathToAbsoluteUri([NotNullIfNotNull("url")] ref string? url, XmlResolver? resolver) { // new Uri(uri, UriKind.RelativeOrAbsolute) returns a Relative Uri for absolute unix paths (e.g. /tmp). // We convert the native unix path to a 'file://' uri string to make it an Absolute Uri. diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.cs b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.cs index 042d5c4..6927a9e 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.cs @@ -210,7 +210,7 @@ namespace System.Xml private int _parsingStatesStackTop = -1; // current node base uri and encoding - private string? _reportedBaseUri; + private string _reportedBaseUri = string.Empty; private Encoding? _reportedEncoding; // DTD @@ -712,7 +712,7 @@ namespace System.Xml } } - _reportedBaseUri = baseUriStr; + _reportedBaseUri = baseUriStr ?? string.Empty; _closeInput = closeInput; _laterInitParam = new LaterInitParam(); @@ -764,7 +764,7 @@ namespace System.Xml // Initializes a new instance of the XmlTextReaderImpl class with the specified arguments. // This constructor is used when creating XmlTextReaderImpl via XmlReader.Create - internal XmlTextReaderImpl(TextReader input, XmlReaderSettings settings, string? baseUriStr, XmlParserContext? context) + internal XmlTextReaderImpl(TextReader input, XmlReaderSettings settings, string baseUriStr, XmlParserContext? context) : this(settings.GetXmlResolver(), settings, context) { ConvertAbsoluteUnixPathToAbsoluteUri(ref baseUriStr, settings.GetXmlResolver()); @@ -941,7 +941,7 @@ namespace System.Xml } // Returns the base URI of the current node. - public override string? BaseURI + public override string BaseURI { get { @@ -9912,6 +9912,6 @@ namespace System.Xml Buffer.BlockCopy(src, srcOffset, dst, dstOffset, count); } - static partial void ConvertAbsoluteUnixPathToAbsoluteUri(ref string? url, XmlResolver? resolver); + static partial void ConvertAbsoluteUnixPathToAbsoluteUri([NotNullIfNotNull("url")] ref string? url, XmlResolver? resolver); } } diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlValidatingReader.cs b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlValidatingReader.cs index 1daa6a6..e817e4a 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlValidatingReader.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlValidatingReader.cs @@ -91,7 +91,7 @@ namespace System.Xml get { return _impl.Depth; } } - public override string? BaseURI + public override string BaseURI { get { return _impl.BaseURI; } } diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlValidatingReaderImpl.cs b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlValidatingReaderImpl.cs index ab27891..5cf5a33 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlValidatingReaderImpl.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlValidatingReaderImpl.cs @@ -356,7 +356,7 @@ namespace System.Xml } // Returns the base URI of the current node. - public override string? BaseURI + public override string BaseURI { get { diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlWrappingReader.cs b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlWrappingReader.cs index 4d9060a..c0c4187 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlWrappingReader.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlWrappingReader.cs @@ -39,7 +39,7 @@ namespace System.Xml public override bool HasValue { get { return reader.HasValue; } } public override string Value { get { return reader.Value; } } public override int Depth { get { return reader.Depth; } } - public override string? BaseURI { get { return reader.BaseURI; } } + public override string BaseURI { get { return reader.BaseURI; } } public override bool IsEmptyElement { get { return reader.IsEmptyElement; } } public override bool IsDefault { get { return reader.IsDefault; } } public override XmlSpace XmlSpace { get { return reader.XmlSpace; } } diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Core/XsdCachingReader.cs b/src/libraries/System.Private.Xml/src/System/Xml/Core/XsdCachingReader.cs index 72a424a..a5a87a9 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Core/XsdCachingReader.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Core/XsdCachingReader.cs @@ -177,7 +177,7 @@ namespace System.Xml } // Gets the base URI of the current node. - public override string? BaseURI + public override string BaseURI { get { diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Core/XsdValidatingReader.cs b/src/libraries/System.Private.Xml/src/System/Xml/Core/XsdValidatingReader.cs index 620603a..cfdb657 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Core/XsdValidatingReader.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Core/XsdValidatingReader.cs @@ -344,7 +344,7 @@ namespace System.Xml } // Gets the base URI of the current node. - public override string? BaseURI + public override string BaseURI { get { diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Dom/DocumentXPathNavigator.cs b/src/libraries/System.Private.Xml/src/System/Xml/Dom/DocumentXPathNavigator.cs index 166c50f..125e828 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Dom/DocumentXPathNavigator.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Dom/DocumentXPathNavigator.cs @@ -180,10 +180,6 @@ namespace System.Xml return ValueText; default: Debug.Assert(_source.Value != null); - // TODO-NULLABLE: Consider switching this.Value to nullable even if that implies switching it in the base type. - // Also consider the following: - // * this.SetValue() does not accept null. - // * _source.Value is nullable. return _source.Value; } } @@ -223,8 +219,7 @@ namespace System.Xml && nextSibling.IsText); value = builder.ToString(); } - // TODO-NULLABLE: Consider making this nullable given that _source.Value is nullable, - // OR we could change this getter to return string.Empty if _source.Value == null. + Debug.Assert(value != null); return value; } diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReader.cs b/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReader.cs index 471c668..e29f7f8 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReader.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReader.cs @@ -171,7 +171,7 @@ namespace System.Xml.Serialization if (_d == null) { _d = new XmlDocument(_r.NameTable); - _d.SetBaseURI(_r.BaseURI!); // TODO-NULLABLE: string.Empty should be passed in case of null URI + _d.SetBaseURI(_r.BaseURI); } return _d; } diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XPath/IXpathBuilder.cs b/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XPath/IXpathBuilder.cs index e44b4dd..f4e6203 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XPath/IXpathBuilder.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XPath/IXpathBuilder.cs @@ -8,22 +8,20 @@ using System.Xml.XPath; namespace System.Xml.Xsl.XPath { - // TODO-NULLABLE: Replace [MaybeNull] with ? once https://github.com/dotnet/runtime/pull/40197 is in. internal interface IXPathBuilder { // Should be called once per build void StartBuild(); // Should be called after build for result tree post-processing - [return: MaybeNull] [return: NotNullIfNotNull("result")] - Node EndBuild([AllowNull] Node result); + Node? EndBuild(Node? result); Node String(string value); Node Number(double value); - Node Operator(XPathOperator op, [AllowNull] Node left, [AllowNull] Node right); + Node Operator(XPathOperator op, Node? left, Node? right); Node Axis(XPathAxis xpathAxis, XPathNodeType nodeType, string? prefix, string? name); diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XmlIlGenerator.cs b/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XmlIlGenerator.cs index 16d19e0..e5314b4 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XmlIlGenerator.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XmlIlGenerator.cs @@ -70,7 +70,6 @@ namespace System.Xml.Xsl // SxS Note: The way the trace file names are created (hardcoded) is NOT SxS safe. However the files are // created only for internal tracing purposes. In addition XmlILTrace class is not compiled into retail // builds. As a result it is fine to suppress the FxCop SxS warning. - // TODO-NULLABLE: missing [return: NotNullIfNull("typeBldr")] public XmlILCommand? Generate(QilExpression query, TypeBuilder? typeBldr) { _qil = query; diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XmlQueryTypeFactory.cs b/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XmlQueryTypeFactory.cs index ce571c9..0acdd26 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XmlQueryTypeFactory.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XmlQueryTypeFactory.cs @@ -855,10 +855,11 @@ namespace System.Xml.Xsl { get { - // TODO-NULLABLE: Per documentation of base class we should not be returning null here - // Currently we return null for some type codes (i.e. Item, Node) - // but doc says we should be returning AnyType - // which presumably means XmlSchemaComplexType.AnyType + // https://github.com/dotnet/runtime/issues/44148 + // Per documentation of base class we should not be returning null here + // Currently we return null for some type codes (i.e. Item, Node) + // but doc says we should be returning AnyType + // which presumably means XmlSchemaComplexType.AnyType return _schemaType!; } } diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XsltOld/XsltCompileContext.cs b/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XsltOld/XsltCompileContext.cs index e09e8f9..a0f3474 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XsltOld/XsltCompileContext.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Xsl/XsltOld/XsltCompileContext.cs @@ -603,13 +603,16 @@ namespace System.Xml.Xsl.XsltOld private int _minargs; private int _maxargs; private XPathResultType _returnType; - private XPathResultType[] _argTypes = null!; // TODO-NULLABLE: public constructor doesn't initialize it but also seem to have no references, consider removing + private XPathResultType[] _argTypes = null!; // Used by derived classes which initialize it public XsltFunctionImpl() { } + public XsltFunctionImpl(int minArgs, int maxArgs, XPathResultType returnType, XPathResultType[] argTypes) { - this.Init(minArgs, maxArgs, returnType, argTypes); + Init(minArgs, maxArgs, returnType, argTypes); } + + [MemberNotNull(nameof(_argTypes))] protected void Init(int minArgs, int maxArgs, XPathResultType returnType, XPathResultType[] argTypes) { _minargs = minArgs; diff --git a/src/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs b/src/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs index 65a81de..ef1f8e2 100644 --- a/src/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs +++ b/src/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs @@ -773,7 +773,7 @@ namespace System.Xml { protected XmlReader() { } public abstract int AttributeCount { get; } - public abstract string? BaseURI { get; } + public abstract string BaseURI { get; } public virtual bool CanReadBinaryContent { get { throw null; } } public virtual bool CanReadValueChunk { get { throw null; } } public virtual bool CanResolveEntity { get { throw null; } } @@ -1012,7 +1012,7 @@ namespace System.Xml public XmlTextReader(string xmlFragment, System.Xml.XmlNodeType fragType, System.Xml.XmlParserContext? context) { } protected XmlTextReader(System.Xml.XmlNameTable nt) { } public override int AttributeCount { get { throw null; } } - public override string? BaseURI { get { throw null; } } + public override string BaseURI { get { throw null; } } public override bool CanReadBinaryContent { get { throw null; } } public override bool CanReadValueChunk { get { throw null; } } public override bool CanResolveEntity { get { throw null; } } @@ -1151,7 +1151,7 @@ namespace System.Xml public XmlValidatingReader(string xmlFragment, System.Xml.XmlNodeType fragType, System.Xml.XmlParserContext context) { } public XmlValidatingReader(System.Xml.XmlReader reader) { } public override int AttributeCount { get { throw null; } } - public override string? BaseURI { get { throw null; } } + public override string BaseURI { get { throw null; } } public override bool CanReadBinaryContent { get { throw null; } } public override bool CanResolveEntity { get { throw null; } } public override int Depth { get { throw null; } } -- 2.7.4