Skip to content

Switch Markup property to XmlNode?[]? to reflect it can contain null elements #41488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jozkee merged 2 commits into from
Aug 28, 2020
Merged

Switch Markup property to XmlNode?[]? to reflect it can contain null elements #41488

jozkee merged 2 commits into from
Aug 28, 2020

Conversation

@jozkee
Copy link
Member

@jozkee jozkee commented Aug 28, 2020

Switches Markup property from XmlNode[]? to XmlNode?[]? in XmlSchemaAppInfo and XmlSchemaDocumentation.

@jozkee jozkee added this to the 5.0.0 milestone Aug 28, 2020
@jozkee jozkee self-assigned this Aug 28, 2020
@ghost
Copy link

ghost commented Aug 28, 2020

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

jeffhandley
jeffhandley approved these changes Aug 28, 2020
View reviewed changes
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

LGTM but please get another review. 😃

safern
safern reviewed Aug 28, 2020
View reviewed changes
src/libraries/System.Private.Xml/src/System/Xml/Serialization/SchemaObjectWriter.cs
for (int ia = 0; ia < a.Length; ia++)
{
XmlNode ai = (XmlNode)a[ia];
XmlNode ai = (XmlNode)a[ia]!;
Copy link
Member

@safern safern Aug 28, 2020

Choose a reason for hiding this comment

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

How do we know this can't be null?

Copy link
Member Author

@jozkee jozkee Aug 28, 2020

Choose a reason for hiding this comment

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

I am not 100% sure but if ai happens to be null that would cause a NRE in line 552. So product code already assumes that ai can't be null.

src/libraries/System.Private.Xml/src/System/Xml/Schema/XsdBuilder.cs
}

internal override void ProcessMarkup(XmlNode[] markup)
internal override void ProcessMarkup(XmlNode?[] markup)
Copy link
Member

@safern safern Aug 28, 2020

Choose a reason for hiding this comment

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

Why can't markup be null here?

Copy link
Member Author

@jozkee jozkee Aug 28, 2020

Choose a reason for hiding this comment

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

markup is always initialized in the only call site.

runtime/src/libraries/System.Private.Xml/src/System/Xml/Schema/Parser.cs

Lines 246 to 252 in 454d66d

XmlNode?[] markup = new XmlNode[list.Count];
for (int i = 0; i < list.Count; i++)
{
markup[i] = list[i];
}
_builder!.ProcessMarkup(markup);

src/libraries/System.Private.Xml/src/System/Xml/Schema/XdrBuilder.cs
}

internal override void ProcessMarkup(XmlNode[] markup)
internal override void ProcessMarkup(XmlNode?[] markup)
Copy link
Member

@safern safern Aug 28, 2020

Choose a reason for hiding this comment

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

Why can't markup be null?

Copy link
Member Author

@jozkee jozkee Aug 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Same answer as previous comment #41488 (comment).

eerhardt
eerhardt approved these changes Aug 28, 2020
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

:shipit:

@jozkee jozkee merged commit 7408f1b into dotnet:master Aug 28, 2020
@carlossanlop carlossanlop mentioned this pull request Aug 28, 2020
Merged
14 tasks
@jozkee jozkee deleted the nullability_markup branch August 28, 2020 06:58
@jaredpar jaredpar mentioned this pull request Aug 28, 2020
Closed
@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.

Reviewers

@krwq krwq krwq approved these changes

@jeffhandley jeffhandley jeffhandley approved these changes

@eerhardt eerhardt eerhardt approved these changes

@safern safern safern approved these changes

@carlossanlop carlossanlop Awaiting requested review from carlossanlop

@pgovind pgovind Awaiting requested review from pgovind

Assignees

@jozkee jozkee

Labels

area-System.Xml

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jozkee @krwq @jeffhandley @eerhardt @safern