Skip to content

Ensure metadata location when created from xml element with known location #8599

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

Merged
merged 8 commits into from
Apr 23, 2023
Merged

Ensure metadata location when created from xml element with known location #8599

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
226514e
Populate location of attributes/elements created from other node with…
JanKrivanek Mar 27, 2023
5ab53ea
Handle nulls gracefully
JanKrivanek Mar 27, 2023
9ad1805
Fix API breaking change
JanKrivanek Mar 27, 2023
438d48b
Replace setter with AsyncLocal
JanKrivanek Apr 17, 2023
60fdf2e
Merge remote-tracking branch 'upstream/main' into proto-metadata-loc
JanKrivanek Apr 17, 2023
c03fd0f
Remove temporary hack
JanKrivanek Apr 17, 2023
c912e28
Add missing comment
JanKrivanek Apr 17, 2023
d87a96f
Add unit tests
JanKrivanek Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Build/Construction/ProjectMetadataElement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,17 @@ public string Value
/// Creates an unparented ProjectMetadataElement, wrapping an unparented XmlElement.
/// Caller should then ensure the element is added to a parent.
/// </summary>
internal static ProjectMetadataElement CreateDisconnected(string name, ProjectRootElement containingProject)
internal static ProjectMetadataElement CreateDisconnected(string name, ProjectRootElement containingProject, ElementLocation location = null)
{
XmlUtilities.VerifyThrowArgumentValidElementName(name);
ErrorUtilities.VerifyThrowArgument(!FileUtilities.ItemSpecModifiers.IsItemSpecModifier(name), "ItemSpecModifierCannotBeCustomMetadata", name);
ErrorUtilities.VerifyThrowInvalidOperation(!XMakeElements.ReservedItemNames.Contains(name), "CannotModifyReservedItemMetadata", name);

XmlElementWithLocation element = containingProject.CreateElement(name);
if (location != null)
{
element.Location = location;
Copy link
Member

@ladipro ladipro Apr 5, 2023

Choose a reason for hiding this comment

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

nit: I wonder if there's a slightly more elegant way of setting location for newly created elements than adding a Location setter. If you passed the location to ProjectRootElement.CreateElement which in turn would pass it to a new overload of XmlDocumentWithLocation.CreateElement, you could set its _reader field for the duration of the base call, similar to what Load does. Note that _reader is of type IXmlLineInfo, so not necessarily an XML reader. Or you could introduce a new field (_currentLocation?) , which would then even avoid boxing.

Overall, I would find it nicer to leave the *WithLocation classes concerned with setting location without leaking the setter like this. What do you think?

Copy link
Member Author

@JanKrivanek JanKrivanek Apr 12, 2023

Choose a reason for hiding this comment

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

Thanks for looking into this! Great suggestion!

I was as well trying to see if I can just extend the creation and avoid subsequent mutation.
The problem that stopped me and let me to current code is that the XmlDocumentWithLocation extends XmlDocument and our overriden CreateElement is not being called directy, but via a XmlDocument function that inside calls internal helper function:

image

So we'd need to duplicate some of that internal XmlDocument functionality (and maintain it going forward).

Not sure which is the lesser evil (allowing mutations vs duplicating XmlDocument code) - I arbitrarily decided for openng for mutation.

What do you think about this?

Copy link
Member Author

@JanKrivanek JanKrivanek Apr 17, 2023

Choose a reason for hiding this comment

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

Great thanks for offline discussion and tips! - I ended up passing the info via AsyncLocal filed.

}

return new ProjectMetadataElement(element, containingProject);
}
Expand Down
20 changes: 19 additions & 1 deletion src/Build/Construction/ProjectRootElement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1327,13 +1327,22 @@ public ProjectMetadataElement CreateMetadataElement(string name)
/// Caller must add it to the location of choice in the project.
/// </summary>
public ProjectMetadataElement CreateMetadataElement(string name, string unevaluatedValue)
{
return this.CreateMetadataElement(name, unevaluatedValue, null);
}

/// <summary>
/// Creates a metadata node.
/// Caller must add it to the location of choice in the project.
/// </summary>
public ProjectMetadataElement CreateMetadataElement(string name, string unevaluatedValue, ElementLocation location)
{
if (Link != null)
{
return RootLink.CreateMetadataElement(name, unevaluatedValue);
}

ProjectMetadataElement metadatum = ProjectMetadataElement.CreateDisconnected(name, this);
ProjectMetadataElement metadatum = ProjectMetadataElement.CreateDisconnected(name, this, location);

metadatum.Value = unevaluatedValue;

Expand Down Expand Up @@ -1785,6 +1794,15 @@ internal static ProjectRootElement OpenProjectOrSolution(string fullPath, IDicti
return projectRootElement;
}

/// <summary>
/// Creates a metadata node.
/// Caller must add it to the location of choice in the project.
/// </summary>
internal ProjectMetadataElement CreateMetadataElement(XmlAttributeWithLocation attribute)
{
return CreateMetadataElement(attribute.Name, attribute.Value, attribute.Location);
}

/// <summary>
/// Creates a XmlElement with the specified name in the document
/// containing this project.
Expand Down
5 changes: 5 additions & 0 deletions src/Build/ElementLocation/XmlElementWithLocation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ internal ElementLocation Location

return _elementLocation;
}

set
{
_elementLocation = value;
}
}

/// <summary>
Expand Down
4 changes: 2 additions & 2 deletions src/Build/Evaluation/ProjectParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ private ProjectItemElement ParseProjectItemElement(XmlElementWithLocation elemen
}
else if (isValidMetadataNameInAttribute)
{
ProjectMetadataElement metadatum = _project.CreateMetadataElement(attribute.Name, attribute.Value);
ProjectMetadataElement metadatum = _project.CreateMetadataElement(attribute);
metadatum.ExpressedAsAttribute = true;
metadatum.Parent = item;

Expand Down Expand Up @@ -744,7 +744,7 @@ private ProjectItemDefinitionElement ParseProjectItemDefinitionXml(XmlElementWit
}
else if (isValidMetadataNameInAttribute)
{
ProjectMetadataElement metadatum = _project.CreateMetadataElement(attribute.Name, attribute.Value);
ProjectMetadataElement metadatum = _project.CreateMetadataElement(attribute);
metadatum.ExpressedAsAttribute = true;
metadatum.Parent = itemDefinition;

Expand Down
5 changes: 5 additions & 0 deletions src/Shared/XmlUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ internal static XmlElementWithLocation RenameXmlElement(XmlElementWithLocation o
? (XmlElementWithLocation)oldElement.OwnerDocument.CreateElement(newElementName)
: (XmlElementWithLocation)oldElement.OwnerDocument.CreateElement(newElementName, xmlNamespace);

if (oldElement.Location != null)
{
newElement.Location = oldElement.Location;
}

// Copy over all the attributes.
foreach (XmlAttribute oldAttribute in oldElement.Attributes)
{
Expand Down