Skip to content

Conversation

@eerhardt
Copy link
Member

Contributes to #2339

cc @jeffhandley @terrajobst

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@eerhardt
Copy link
Member Author

fyi - @HongGit @StephenMolloy - as the owners of this area.

@jozkee
Copy link
Member

jozkee commented Aug 28, 2020

Unrelated to this PR: There is a file that doesn't even compile in System.Private.DataContractSerialization and is a duplicate of the homonymous named file CollectionDataContractAttribute.cs in System.Runtime.Serialization.Primitives, I think that file can be removed (on a separate PR):

src/libraries/System.Private.DataContractSerialization/src/System/Runtime/Serialization/CollectionDataContractAttribute.cs

src/libraries/System.Runtime.Serialization.Primitives/src/System/Runtime/Serialization/CollectionDataContractAttribute.cs

}

internal virtual XmlDictionaryString TopLevelElementName
internal virtual XmlDictionaryString? TopLevelElementName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[DisallowNull]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does [DisallowNull] work with overrides? This one is a bit tricky because the base class has:

class DataContract
{
    XmlDictionaryString _name; // can't be null

    public XmlDictionaryString Name { get { } set{ _name = value; } }

    public virtual XmlDictionaryString? TopLevelElementName
    {
        get { return _name; } set{ _name = value; }
    }
}

but then a derived class does:

class DerivedDataContract : DataContract
{
    XmlDictionaryString? _topLevelName ; // can be null

    public override XmlDictionaryString? TopLevelElementName
    {
        get { return _topLevelName ; } set{ _topLevelName = value; }
    }
}

And when trying to clone the objects, now we have a problem:

clonedHelper.TopLevelElementName = this.TopLevelElementName;
clonedHelper.TopLevelElementNamespace = this.TopLevelElementNamespace;

The getter can be null, but the setter doesn't allow for it.

}

public override void WriteXmlnsAttribute(string prefix, string ns)
public override void WriteXmlnsAttribute(string? prefix, string ns)
Copy link
Member

@krwq krwq Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public override void WriteStartAttribute(string prefix, string localName) and overload might be worth filing a bug on NRE or at least TODO-NULLABLE

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WriteStartAttribute methods are internal and are never called with null for prefix, so I left them as non-nullable and didn't change the code. But WriteXmlnsAttribute could be called with null, so that's why I fixed it here.

Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after fixing/resolving comments

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise; public API annotations LGTM.

@eerhardt
Copy link
Member Author

I believe all current feedback has been addressed. I left a few conversations open with pending questions. Please let me know if you have any more feedback. I'd like to merge this by the EOD.

@krwq krwq merged commit 722d550 into dotnet:master Aug 28, 2020
@eerhardt eerhardt deleted the DataContractSerializationNullable branch August 28, 2020 22:56
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants