Open-XML-SDK

Open-XML-SDK copied to clipboard

Description

The Document property of the MainDocumentPart is defined as follows:

        /// <summary>
        /// Gets or sets the root element of this part.
        /// </summary>
        public DocumentFormat.OpenXml.Wordprocessing.Document Document
        {
            get
            {
                if (_rootElement is null)
                {
                    LoadDomTree<DocumentFormat.OpenXml.Wordprocessing.Document>();
                }

                return _rootElement!;
            }

            set
            {
                if (value is null)
                {
                    throw new ArgumentNullException(nameof(value));
                }

                SetDomTree(value);
            }
        }

Other subclasses of OpenXmlPart (e.g., WorkbookPart) define their respective root elements (e.g., Workbook) in the same way.

Firstly, this is inconsistent with the more generic RootElement property of OpenXmlPart, which correctly flags nullability:

        /// <summary>
        /// Gets the root element of the current part.
        /// Returns null when the current part is empty or is not an XML content type.
        /// </summary>
        public OpenXmlPartRootElement? RootElement
        {
            get
            {
                return PartRootElement;
            }
        }

Secondly, when nullability is enabled, the definition suggests that Document, for example, will not be null. However, the following unit test demonstrates that this is not the case:

    public class MainDocumentPartTests
    {
        [Fact]
        public void Document_NewMainDocumentPart_DocumentIsNull()
        {
            using var stream = new MemoryStream();
            using var wordDocument = WordprocessingDocument.Create(stream, WordprocessingDocumentType.Document);
            MainDocumentPart mainDocumentPart = wordDocument.AddMainDocumentPart();

            Assert.NotNull(mainDocumentPart);
            Assert.Null(mainDocumentPart.Document);

            // ReSharper disable HeuristicUnreachableCode
            Assert.Throws<NullReferenceException>(() => mainDocumentPart.Document.Body);
        }
    }

You will also see the comment telling ReSharper to disable the HeuristicUnreachableCode check after asserting that mainDocumentPart.Document is null. Owing to the incorrect nullability, ReSharper believes the assertion will throw and, thus, the next assertion will never be reached. This is not correct, of course.

Information

• .NET Target: all
• DocumentFormat.OpenXml Version: latest

Repro

See the unit test above.

Observed

The nullability of root elements is stated as non-nullable.

Expected

The nullability of root elements should be stated as nullable.

Good catch. Happy to accept a PR on this!

@twsouthwick, this is generated code. Not sure whether anybody outside of Microsoft can help in this case.

@twsouthwick, are you addressing this? As I said above, this is generated code, so the code generator will have to be fixed.

The change needed:

• Annotate property with [DisallowNull]
• Mark the property as nullable

We'll try to get that in for 2.14