Log item self-expansion #8581
Merged
Log item self-expansion #8581
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JanKrivanek
changed the title
Forgind
reviewed
Mar 31, 2023
There was a problem hiding this comment.
I think this should work. I wasn't actually aware of how confusing the self-referential behavior is before you brought it up, so I appreciate it!
For the cases when people are using this in the wild, were they using it intentionally, or were they broken and just confused?
src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs
Outdated
Show resolved
Hide resolved
| @@ -26,7 +26,7 @@ namespace Microsoft.Build.Evaluation | |||
| /// ProjectMetadataElement, and these can be added, removed, and modified. | |||
| /// </remarks> | |||
| [DebuggerDisplay("{_itemType} #Metadata={MetadataCount}")] | |||
| public class ProjectItemDefinition : IKeyed, IMetadataTable, IItemDefinition<ProjectMetadata>, IProjectMetadataParent | |||
| public class ProjectItemDefinition : IKeyed, IMetadataTable, IItemDefinition<ProjectMetadata>, IProjectMetadataParent, IItemMetadata | |||
There was a problem hiding this comment.
It seems weird for a "ProjectItemDefinition" to be an "IItemMetadata"...it's possible some restructruing would help clarify that?
There was a problem hiding this comment.
Sorry if I misunderstood here. I renamed the interface for clarity (IItemMetadata -> IItemTypeDefinition) - hope this is adressing the concern.
src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs
Show resolved
Hide resolved
JanKrivanek
added
the
Feature: Warning Waves
Co-authored-by: Forgind <[email protected]>
JanKrivanek
changed the title
JanKrivanek
added
the
merge-when-branch-open
JaynieBai
pushed a commit
that referenced
this pull request
Apr 23, 2023
…ation (#8599) Fixes #8579 Context MetadataElement of an ItemElement doesn't have line&col of Location set despite those are created from known xml. This way errors referencing the Location from metadata would end up pointing to location (0, 0) in the file. Reason This happens only to XmlElementWithLocation (as opposed to XmlAttributeWithLocation) during creating XmlElementWithLocation from scratch (during parsing phase that doesn't have the reader with locations already available) from an existing XmlAttributeWithLocation (that has the proper location intitialized from Load phase). Due to the need to keep the constructors contract (we override existing XmlElement), we cannot pass the additional info in the constructor. Changes Made Making the Location property of XmlElementWithLocation writable - so that it can be properly rewritten if constructed from an existing node with available location info. Testing Hand testing plus applying the changfe to the other PR: #8581, solved the issue with (0, 0) warnings location Note This or #8581 PR (whichever is merged later) should remove the workaround https://github.com/dotnet/msbuild/pull/8581/files#diff-e289ba4ce7fa0e72cf63049cce595eafcad1e7b2034ccb3305cd0f06c2f822b8R196-R197
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8527
Context
Self referencing item metadata in an item definition within the target leads to possible unintended expansion (and cross-applying of pre-existing item instances).
This behavior is not a bug - it's the implication of the target batching feature - but might be confusing. So detection and warning is added here
Changes Made
A self reference (qualified or unqualified) of metadata within the item defeiniton (which is within a target) is detected and high importance message is issued.
Testing
Tests added for a warning case and for a non-warning case (self-referencing metadata outside of target context)
Doc
https://github.com/MicrosoftDocs/visualstudio-docs-pr/pull/11034
Possible impact
It's unfortunately hard to obtain data on prevalence of this pattern due to limitations of available code searches (github search doesn't support '@' sign and no escaping option; SourceGraph, grep.app doesn't support regex lookaheads). So I cannot quantify (I'll try again later on).
But there are some sparse evidence of usage:
all of those seem to be doing something else then intended (the variable part of path is empty, so it gets just the base directory), but they do not break.
tl;dr;:
unless we are able to better quantify, we might resort to demote the warning to a messageDemoted to Message severity