Skip to content
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.

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

Implementation of implicit imports during evaluation instead of parsing #1492

Merged
merged 6 commits into from
Jan 10, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 11 additions & 1 deletion ref/net46/Microsoft.Build/Microsoft.Build.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ protected ElementLocation() { }
public override int GetHashCode() { throw null; }
public override string ToString() { throw null; }
}
public enum ImplicitImportLocation
{
Bottom = 2,
None = 0,
Top = 1,
}
[System.Diagnostics.DebuggerDisplayAttribute("ProjectChooseElement (#Children={Count} HasOtherwise={OtherwiseElement != null})")]
public partial class ProjectChooseElement : Microsoft.Build.Construction.ProjectElementContainer
{
Expand All @@ -39,7 +45,6 @@ internal ProjectElement() { }
public virtual string Condition { [System.Diagnostics.DebuggerStepThroughAttribute]get { throw null; } [System.Diagnostics.DebuggerStepThroughAttribute]set { } }
public virtual Microsoft.Build.Construction.ElementLocation ConditionLocation { get { throw null; } }
public Microsoft.Build.Construction.ProjectRootElement ContainingProject { get { throw null; } }
public bool IsImplicit { get { throw null; } }
public string Label { [System.Diagnostics.DebuggerStepThroughAttribute]get { throw null; } [System.Diagnostics.DebuggerStepThroughAttribute]set { } }
public Microsoft.Build.Construction.ElementLocation LabelLocation { get { throw null; } }
public Microsoft.Build.Construction.ElementLocation Location { get { throw null; } }
Expand Down Expand Up @@ -84,8 +89,11 @@ public override void CopyFrom(Microsoft.Build.Construction.ProjectElement elemen
public partial class ProjectImportElement : Microsoft.Build.Construction.ProjectElement
{
internal ProjectImportElement() { }
public Microsoft.Build.Construction.ImplicitImportLocation ImplicitImportLocation { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
public string Project { get { throw null; } set { } }
public Microsoft.Build.Construction.ElementLocation ProjectLocation { get { throw null; } }
public string Sdk { get { throw null; } set { } }
public Microsoft.Build.Construction.ElementLocation SdkLocation { get { throw null; } }
protected override Microsoft.Build.Construction.ProjectElement CreateNewInstance(Microsoft.Build.Construction.ProjectRootElement owner) { throw null; }
}
[System.Diagnostics.DebuggerDisplayAttribute("#Imports={Count} Condition={Condition} Label={Label}")]
Expand Down Expand Up @@ -253,6 +261,8 @@ internal ProjectRootElement() { }
public System.Collections.Generic.ICollection<Microsoft.Build.Construction.ProjectPropertyGroupElement> PropertyGroups { get { throw null; } }
public System.Collections.Generic.ICollection<Microsoft.Build.Construction.ProjectPropertyGroupElement> PropertyGroupsReversed { get { throw null; } }
public string RawXml { get { throw null; } }
public string Sdk { [System.Diagnostics.DebuggerStepThroughAttribute]get { throw null; } [System.Diagnostics.DebuggerStepThroughAttribute]set { } }
public Microsoft.Build.Construction.ElementLocation SdkLocation { get { throw null; } }
public System.Collections.Generic.ICollection<Microsoft.Build.Construction.ProjectTargetElement> Targets { get { throw null; } }
public System.DateTime TimeLastChanged { [System.Diagnostics.DebuggerStepThroughAttribute]get { throw null; } }
public string ToolsVersion { [System.Diagnostics.DebuggerStepThroughAttribute]get { throw null; } [System.Diagnostics.DebuggerStepThroughAttribute]set { } }
Expand Down
12 changes: 11 additions & 1 deletion ref/netstandard1.3/Microsoft.Build/Microsoft.Build.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ protected ElementLocation() { }
public override int GetHashCode() { throw null; }
public override string ToString() { throw null; }
}
public enum ImplicitImportLocation
{
Bottom = 2,
None = 0,
Top = 1,
}
[System.Diagnostics.DebuggerDisplayAttribute("ProjectChooseElement (#Children={Count} HasOtherwise={OtherwiseElement != null})")]
public partial class ProjectChooseElement : Microsoft.Build.Construction.ProjectElementContainer
{
Expand All @@ -39,7 +45,6 @@ internal ProjectElement() { }
public virtual string Condition { [System.Diagnostics.DebuggerStepThroughAttribute]get { throw null; } [System.Diagnostics.DebuggerStepThroughAttribute]set { } }
public virtual Microsoft.Build.Construction.ElementLocation ConditionLocation { get { throw null; } }
public Microsoft.Build.Construction.ProjectRootElement ContainingProject { get { throw null; } }
public bool IsImplicit { get { throw null; } }
public string Label { [System.Diagnostics.DebuggerStepThroughAttribute]get { throw null; } [System.Diagnostics.DebuggerStepThroughAttribute]set { } }
public Microsoft.Build.Construction.ElementLocation LabelLocation { get { throw null; } }
public Microsoft.Build.Construction.ElementLocation Location { get { throw null; } }
Expand Down Expand Up @@ -84,8 +89,11 @@ public override void CopyFrom(Microsoft.Build.Construction.ProjectElement elemen
public partial class ProjectImportElement : Microsoft.Build.Construction.ProjectElement
{
internal ProjectImportElement() { }
public Microsoft.Build.Construction.ImplicitImportLocation ImplicitImportLocation { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } }
public string Project { get { throw null; } set { } }
public Microsoft.Build.Construction.ElementLocation ProjectLocation { get { throw null; } }
public string Sdk { get { throw null; } set { } }
public Microsoft.Build.Construction.ElementLocation SdkLocation { get { throw null; } }
protected override Microsoft.Build.Construction.ProjectElement CreateNewInstance(Microsoft.Build.Construction.ProjectRootElement owner) { throw null; }
}
[System.Diagnostics.DebuggerDisplayAttribute("#Imports={Count} Condition={Condition} Label={Label}")]
Expand Down Expand Up @@ -253,6 +261,8 @@ internal ProjectRootElement() { }
public System.Collections.Generic.ICollection<Microsoft.Build.Construction.ProjectPropertyGroupElement> PropertyGroups { get { throw null; } }
public System.Collections.Generic.ICollection<Microsoft.Build.Construction.ProjectPropertyGroupElement> PropertyGroupsReversed { get { throw null; } }
public string RawXml { get { throw null; } }
public string Sdk { [System.Diagnostics.DebuggerStepThroughAttribute]get { throw null; } [System.Diagnostics.DebuggerStepThroughAttribute]set { } }
public Microsoft.Build.Construction.ElementLocation SdkLocation { get { throw null; } }
public System.Collections.Generic.ICollection<Microsoft.Build.Construction.ProjectTargetElement> Targets { get { throw null; } }
public System.DateTime TimeLastChanged { [System.Diagnostics.DebuggerStepThroughAttribute]get { throw null; } }
public string ToolsVersion { [System.Diagnostics.DebuggerStepThroughAttribute]get { throw null; } [System.Diagnostics.DebuggerStepThroughAttribute]set { } }
Expand Down
1 change: 0 additions & 1 deletion src/Shared/XMakeAttributes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ internal static class XMakeAttributes
internal const string taskName = "TaskName";
internal const string continueOnError = "ContinueOnError";
internal const string project = "Project";
internal const string @implicit = "_Implicit";
internal const string taskParameter = "TaskParameter";
internal const string itemName = "ItemName";
internal const string propertyName = "PropertyName";
Expand Down
24 changes: 24 additions & 0 deletions src/XMakeBuildEngine/Construction/ImplicitImportLocation.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

namespace Microsoft.Build.Construction
{
/// <summary>
/// Represents the location of an implicit import.
/// </summary>
public enum ImplicitImportLocation
{
/// <summary>
/// The import is not implicitly added and is explicitly added in a user-specified location.
/// </summary>
None,
/// <summary>
/// The import was implicitly added at the top of the project.
/// </summary>
Top,
/// <summary>
/// The import was implicitly added at the bottom of the project.
/// </summary>
Bottom
}
}
5 changes: 0 additions & 5 deletions src/XMakeBuildEngine/Construction/ProjectElement.cs
Original file line number Diff line number Diff line change
Expand Up @@ -292,11 +292,6 @@ public ElementLocation Location
get { return XmlElement.Location; }
}

/// <summary>
/// Gets the Implicit state of the element: true if the element was not in the read XML.
/// </summary>
public bool IsImplicit => XmlElement.HasAttribute(XMakeAttributes.@implicit);

/// <summary>
/// Gets the name of the associated element.
/// Useful for display in some circumstances.
Expand Down
17 changes: 2 additions & 15 deletions src/XMakeBuildEngine/Construction/ProjectElementContainer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -455,11 +455,6 @@ internal void UpdateElementValue(ProjectElement child)
/// </remarks>
internal void AddToXml(ProjectElement child)
{
if (child.IsImplicit)
{
return;
}

if (child.ExpressedAsAttribute)
{
// todo children represented as attributes need to be placed in order too
Expand All @@ -485,7 +480,7 @@ internal void AddToXml(ProjectElement child)
// If none is found, then the node being added is inserted as the only node of its kind

ProjectElement referenceSibling;
Predicate<ProjectElement> siblingIsExplicitElement = _ => _.ExpressedAsAttribute == false && _.IsImplicit == false;
Predicate<ProjectElement> siblingIsExplicitElement = _ => _.ExpressedAsAttribute == false;

if (TrySearchLeftSiblings(child.PreviousSibling, siblingIsExplicitElement, out referenceSibling))
{
Expand Down Expand Up @@ -567,11 +562,6 @@ private string GetElementIndentation(XmlElementWithLocation xmlElement)

internal void RemoveFromXml(ProjectElement child)
{
if (child.IsImplicit)
{
return;
}

if (child.ExpressedAsAttribute)
{
XmlElement.RemoveAttribute(child.XmlElement.Name);
Expand Down Expand Up @@ -630,10 +620,7 @@ private void AddInitialChild(ProjectElement child)

_count++;

if (!child.IsImplicit)
{
MarkDirty("Add child element named '{0}'", child.ElementName);
}
MarkDirty("Add child element named '{0}'", child.ElementName);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a change in behavior. Does it need to happen for Remove just above this? Or others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is removed because I'm reverting the original implementation where the stuff is added to the XML DOM. There is no IsImplicit property anymore so the if is removed.

The change above this is removed for the same reason.

}

/// <summary>
Expand Down
47 changes: 47 additions & 0 deletions src/XMakeBuildEngine/Construction/ProjectImportElement.cs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,38 @@ public string Project
/// </summary>
public ElementLocation ProjectLocation => XmlElement.GetAttributeLocation(XMakeAttributes.project);

/// <summary>
/// Gets or sets the SDK that contains the import.
/// </summary>
public string Sdk
{
get
{
return
FileUtilities.FixFilePath(ProjectXmlUtilities.GetAttributeValue(XmlElement, XMakeAttributes.sdk));
}

set
{
ErrorUtilities.VerifyThrowArgumentLength(value, XMakeAttributes.sdk);

ProjectXmlUtilities.SetOrRemoveAttribute(XmlElement, XMakeAttributes.sdk, value);
MarkDirty("Set Import Sdk {0}", value);
}
}

/// <summary>
/// Location of the Sdk attribute
/// </summary>
public ElementLocation SdkLocation => XmlElement.GetAttributeLocation(XMakeAttributes.sdk);

/// <summary>
/// Gets the <see cref="ImplicitImportLocation"/> of the import. This indicates if the import was implicitly
/// added because of the <see cref="ProjectRootElement.Sdk"/> attribute and the location where the project was
/// imported.
/// </summary>
public ImplicitImportLocation ImplicitImportLocation { get; internal set; } = ImplicitImportLocation.None;

/// <summary>
/// Creates an unparented ProjectImportElement, wrapping an unparented XmlElement.
/// Validates the project value.
Expand All @@ -77,6 +109,21 @@ internal static ProjectImportElement CreateDisconnected(string project, ProjectR
return import;
}

/// <summary>
/// Creates an implicit ProjectImportElement as if it was in the project.
/// </summary>
/// <returns></returns>
internal static ProjectImportElement CreateImplicit(string project, ProjectRootElement containingProject, ImplicitImportLocation implicitImportLocation, string sdkName)
{
ProjectImportElement import = CreateDisconnected(project, containingProject);

import.ImplicitImportLocation = implicitImportLocation;

import.Sdk = sdkName;

return import;
}

/// <summary>
/// Overridden to verify that the potential parent and siblings
/// are acceptable. Throws InvalidOperationException if they are not.
Expand Down
73 changes: 36 additions & 37 deletions src/XMakeBuildEngine/Construction/ProjectRootElement.cs
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,6 @@ public class ProjectRootElement : ProjectElementContainer
/// </summary>
private BuildEventContext _buildEventContext;

/// <summary>
/// Xpath expression that will find any element with the implicit attribute
/// </summary>
private static readonly string ImplicitAttributeXpath = $"//*[@{XMakeAttributes.@implicit}]";

/// <summary>
/// Initialize a ProjectRootElement instance from a XmlReader.
/// May throw InvalidProjectFileException.
Expand Down Expand Up @@ -654,6 +649,28 @@ public string InitialTargets
}
}

/// <summary>
/// Gets or sets a semicolon delimited list of software development kits (SDK) that the project uses.
/// If a value is specified, an Sdk.props is simplicity imported at the top of the project and an
/// Sdk.targets is simplicity imported at the bottom from the specified SDK.
/// If the value is null or empty, removes the attribute.
/// </summary>
public string Sdk
{
[DebuggerStepThrough]
get
{
return ProjectXmlUtilities.GetAttributeValue(XmlElement, XMakeAttributes.sdk);
Copy link
Contributor

Choose a reason for hiding this comment

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

As things get more involved with the SDKs, it might be worth creating a new SDKProjectElement which is represented as an attribute (reusing the way metadata as attributes represent themselves). This type would be a valid child for ProjectRootElements and ProjectImportElements. Then, the new type could gain responsibilities such as parsing the semicolon delimited string into a {name, version} object, doing data validation, etc

Copy link
Member

Choose a reason for hiding this comment

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

I'm in favor of putting duplicated logic in a central place, but here I don't think it's helpful to represent it as a child element expressed as an attribute. That just complicates the object model without any benefit that I see, and we can centralize the logic into utility methods or similar without changing the object model.

}

[DebuggerStepThrough]
set
{
ProjectXmlUtilities.SetOrRemoveAttribute(XmlElement, XMakeAttributes.sdk, value);
MarkDirty("Set project Sdk to '{0}'", value);
}
}

/// <summary>
/// Gets or sets the value of TreatAsLocalProperty. If there is no tag, returns empty string.
/// If the value being set is null or empty, removes the attribute.
Expand Down Expand Up @@ -710,10 +727,8 @@ public string RawXml
{
using (ProjectWriter projectWriter = new ProjectWriter(stringWriter))
{
var xmlWithNoImplicits = RemoveImplicits();

projectWriter.Initialize(xmlWithNoImplicits);
xmlWithNoImplicits.Save(projectWriter);
projectWriter.Initialize(XmlDocument);
XmlDocument.Save(projectWriter);
}

return stringWriter.ToString();
Expand Down Expand Up @@ -848,6 +863,14 @@ public ElementLocation InitialTargetsLocation
get { return XmlElement.GetAttributeLocation(XMakeAttributes.initialTargets); }
}

/// <summary>
/// Location of the Sdk attribute, if any
/// </summary>
public ElementLocation SdkLocation
{
get { return XmlElement.GetAttributeLocation(XMakeAttributes.sdk); }
}

/// <summary>
/// Location of the TreatAsLocalProperty attribute, if any
/// </summary>
Expand Down Expand Up @@ -1752,10 +1775,8 @@ public void Save(Encoding saveEncoding)
{
using (ProjectWriter projectWriter = new ProjectWriter(_projectFileLocation.File, saveEncoding))
{
var xmlWithNoImplicits = RemoveImplicits();

projectWriter.Initialize(xmlWithNoImplicits);
xmlWithNoImplicits.Save(projectWriter);
projectWriter.Initialize(XmlDocument);
XmlDocument.Save(projectWriter);
}

_encoding = saveEncoding;
Expand Down Expand Up @@ -1784,26 +1805,6 @@ public void Save(Encoding saveEncoding)
#endif
}

private XmlDocument RemoveImplicits()
{
if (XmlDocument.SelectSingleNode(ImplicitAttributeXpath) == null)
{
return XmlDocument;
}

var xmlWithNoImplicits = (XmlDocument) XmlDocument.CloneNode(deep: true);

var implicitElements =
xmlWithNoImplicits.SelectNodes(ImplicitAttributeXpath);

foreach (XmlNode implicitElement in implicitElements)
{
implicitElement.ParentNode.RemoveChild(implicitElement);
}

return xmlWithNoImplicits;
}

/// <summary>
/// Save the project to the file system, if dirty or the path is different.
/// Creates any necessary directories.
Expand Down Expand Up @@ -1837,10 +1838,8 @@ public void Save(TextWriter writer)
{
using (ProjectWriter projectWriter = new ProjectWriter(writer))
{
var xmlWithNoImplicits = RemoveImplicits();

projectWriter.Initialize(xmlWithNoImplicits);
xmlWithNoImplicits.Save(projectWriter);
projectWriter.Initialize(XmlDocument);
XmlDocument.Save(projectWriter);
}

_versionOnDisk = Version;
Expand Down
Loading