Nullable: System.Xml, part 7 (Serialization)

[krwq]

This almost completes System.Xml with 1 more tiny PR left which will enable annotations globally and add annotations to 2 leftover files and disable annotations where we've decided not to add them (XSLT runtime and tt template files).

[ghost]

Tagging subscribers to this area: @buyaa-n, @krwq
See info in area-owners.md if you want to be subscribed.

[krwq]

Marked as draft, need to figure out what's wrong with the tests since I don't recall any product changes. I'll run IL diff tomorrow on before & after to make sure there is no incidental product changes

[krwq]

The failures I was seeing were related to dotnet/csharplang#3393 - I've submitted the workaround

[jozkee]

Submitting my thoughts so far.
I am currently at XmlSerializationReader, will continue reviewing tomorrow.

[jozkee]

Why is this non-nullable? Based on the following snippet:

class MyXmlReader : XmlSerializationReader
{
    protected override void InitCallbacks()
    {
        throw new NotImplementedException();
    }

    protected override void InitIDs()
    {
        throw new NotImplementedException();
    }
}

You can access the Reader property on a MyXmlReader instance and see that is null.
Even though documentation says that you should not derive from it, I think is a good thing to consider https://docs.microsoft.com/dotnet/api/system.xml.serialization.xmlserializationreader?view=net-5.0

[krwq]

The reason this is non-null is because code is not expecting it to be null. You will get NRE on many methods. for derived classes you're supposed to call the Init method which initializes it with non-null but as you said it is not meant to be derived by users.

[jozkee]

You will get NRE on many methods. for derived classes you're supposed to call the Init method which initializes it with non-null

I think is fine for this to be nullable if it will be null until Init() is called, that can make users aware of needing to call the method to avoid NullReferenceExceptions.

[krwq]

at this point this will only cause lots of ! in the code for no much benefit. it's private so derived classes won't see the annotation

[jozkee]

It's private but it is exposed through the Reader property which is protected.
OK, since it's easier to handle as not-null it the code and this is not meant to be used by real world users, you can keep it as non-nullable.

[pgovind]

If you're going to make the return type nullable, you'd also have to remove the ! here I think

[krwq]

See my comment #41261 (comment)

[pgovind]

The setter doesn't have an explicit null check here right? Is the DisallowNull attribute still correct?

[krwq]

yes, if you assign null here then _locals second generic arg will be nullable which will cause chain reaction everywhere as nothing expects that to be null... This is an internal class so not a good tradeoff 😄

[jozkee]

on line 714: can events be nullable? cc @safern

[krwq]

not sure, I think Handler += null would be a bit weird though 😄

[jozkee]

The concern is not so much about the add and remove methods but because the underlying XmlNodeEventHandler can be null, but I think that _events.OnUnknownNode nullability doesn't have to reflect on the public event so this is fine as is.

[pgovind]

I think mapping should not be nullable here? Otherwise https://github.com/dotnet/runtime/pull/41261/files#diff-81667efb15f99a82497ac6bfe0ae7ea5R492 will be a NRE?

[jozkee]

Hmm that link didn't lead me anywhere.

[pgovind]

Ah, bad link. Try this instead: https://github.com/dotnet/runtime/pull/41261/files?file-filters%5B%5D=#diff-81667efb15f99a82497ac6bfe0ae7ea5R492. Although, now when I see it again, it is not a bug, so this change is good.

Edit: That link is also no good. It's line 492 in Mappings.cs.

[pgovind]

LGTM!

[krwq]

Thank you for review!