Parse invalid property under target #8190

JaynieBai · 2022-11-25T06:49:03Z

Context

Put a property inside a target without an enclosing PropertyGroup and build. Error informing me that is an invalid child element of .

Changes Made

Add one condition when parse the element under target. If the element has inner text and no other child elements except text, then this should be a property and throw invalid child element of

Tests

ReadInvalidPropertyUnderTarget

Notes

JaynieBai · 2022-11-25T08:26:28Z

The current error after change is error MSB4067: The element beneath element is unrecognized
Not sure if we need to define a new error message to suggest author propertyName is an invalid child element of target.

Forgind

Two questions:
Should we add a change wave?
Should we have a custom message so we can suggest that they probably intended to wrap it in a PropertyGroup?

src/Build/Evaluation/ProjectParser.cs

format equals Co-authored-by: Forgind <12969783+Forgind@users.noreply.github.com>

JaynieBai · 2022-11-28T02:18:50Z

Should we add a change wave?
Should we have a custom message so we can suggest that they probably intended to wrap it in a PropertyGroup?

What's the change wave?
How to add a custom message? From build output message?

rainersigwald

Should we add a change wave?
Should we have a custom message so we can suggest that they probably intended to wrap it in a PropertyGroup?

What's the change wave? How to add a custom message? From build output message?

I like @Forgind's suggestions.

For change waves, please read https://github.com/dotnet/msbuild/blob/main/documentation/wiki/ChangeWaves-Dev.md and ask any follow-up questions--I'm sure that doc can be improved.

For the new error message, I commented in the PR.

I would also like to see a test added, probably like this one and in this file:

msbuild/src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs

Lines 160 to 177 in d797c48

    
                   /// <summary> 
        
                   /// Read a target with a missing name 
        
                   /// </summary> 
        
                   [Fact] 
        
                   public void ReadInvalidMissingName() 
        
                   { 
        
                       Assert.Throws<InvalidProjectFileException>(() => 
        
                       { 
        
                           string content = @" 
        
                               <Project> 
        
                                   <Target/> 
        
                               </Project> 
        
                           "; 
        
                           ProjectRootElement.Create(XmlReader.Create(new StringReader(content))); 
        
                       } 
        
                      ); 
        
                   }

src/Build/Evaluation/ProjectParser.cs

rainersigwald · 2022-11-28T15:22:17Z

src/Build/Evaluation/ProjectParser.cs

+                        if (childElement.ChildNodes.Count == 1 && childElement.FirstChild.NodeType == XmlNodeType.Text)
+                        {
+                            // If the element has inner text and no other child elements except text, then this should be a property and throw invalid child element of <Target>
+                            ProjectXmlUtilities.ThrowProjectInvalidChildElement(childElement.Name, childElement.ParentNode.Name, childElement.Location);


To get a new, custom error message for this case, change this to something like

Suggested change

ProjectXmlUtilities.ThrowProjectInvalidChildElement(childElement.Name, childElement.ParentNode.Name, childElement.Location);

ProjectErrorUtilities.ThrowInvalidProject(childElement.Location, "PropertyOutsidePropertyGroupInTarget", childElement.Name);

and add a new string near here:

msbuild/src/Build/Resources/Strings.resx

Lines 1270 to 1273 in d797c48

<data name="UnrecognizedChildElement" xml:space="preserve">

<value>MSB4067: The element <{0}> beneath element <{1}> is unrecognized.</value>

<comment>{StrBegin="MSB4067: "}</comment>

</data>

src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs

rainersigwald

This is starting to look great, just a few more things to wrap up!

documentation/wiki/ChangeWaves-Dev.md

src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs

src/Build/Resources/Strings.resx

Forgind

Looks pretty good to me! Just a couple little things.

src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs

src/Build/Resources/Strings.resx

Forgind · 2022-12-01T06:24:06Z

src/Build/Resources/Strings.resx

@@ -1271,6 +1271,10 @@
    <value>MSB4067: The element &lt;{0}&gt; beneath element &lt;{1}&gt; is unrecognized.</value>
    <comment>{StrBegin="MSB4067: "}</comment>
  </data>
+  <data name="PropertyOutsidePropertyGroupInTarget" xml:space="preserve" Condition="$([MSBuild]::AreFeaturesEnabled('17.6'))">
+    <value>MSB4070: The property &lt;{0}&gt; beneath target &lt;{1}&gt; is unrecognized. Properties must be inside a &lt;PropertyGroup&gt; element.</value>


Maybe:

Suggested change

<value>MSB4070: The property <{0}> beneath target <{1}> is unrecognized. Properties must be inside a <PropertyGroup> element.</value>

<value>MSB4070: The property <{0}> beneath target <{1}> is unrecognized. If you intended this to be a property, it must be inside a <PropertyGroup> element.</value>

? They might have been trying to do something else.

Ping on this point, which I think is the only outstanding issue.

Should we add hint "If you intended this to be a property" for the new MSB4067?

Yes please.

danmoseley · 2022-12-06T04:31:00Z

src/Build/Resources/Strings.resx

@@ -1271,6 +1271,10 @@
    <value>MSB4067: The element &lt;{0}&gt; beneath element &lt;{1}&gt; is unrecognized.</value>
    <comment>{StrBegin="MSB4067: "}</comment>
  </data>
+  <data name="PropertyOutsidePropertyGroupInTarget" xml:space="preserve" Condition="$([MSBuild]::AreFeaturesEnabled('17.6'))">
+    <value>MSB4067: The element &lt;{0}&gt; beneath element &lt;{1}&gt; is unrecognized. If you intended this to be a property, it must be inside a &lt;PropertyGroup&gt; element.</value>


Suggested change

<value>MSB4067: The element <{0}> beneath element <{1}> is unrecognized. If you intended this to be a property, it must be inside a <PropertyGroup> element.</value>

<value>MSB4067: The element <{0}> beneath element <{1}> is unrecognized. If you intended this to be a property, enclose it within a <PropertyGroup> element.</value>

Otherwise you might interpret this to mean properties can't be inside targets, when they can.

rainersigwald · 2022-12-06T15:35:23Z

src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs

+                    collection.LoadProject(file.Path).Build().ShouldBeTrue();
+                });
+
+                var expectedString = "If you intended this to be a property, enclose it within a <PropertyGroup> element";


Sorry, I think I missed this before. Please don't check against English strings in unit tests, because that will make them fail on non-English OSes (after we get a translation for this specific string).

There aren't any tests in this assembly that do the check the most-correct way (extracting the resource by name).

Can you instead check for the code (MSB4067) outside the if, and only if the new stuff is enabled additionally check that the message contains PropertyGroup? That part of the string shouldn't be localized so I think that'd be a pretty good test.

Update expected string in case the localization

parse invalid property under target

e531a10

JaynieBai changed the title ~~parse invalid property under target~~ Parse invalid property under target Nov 25, 2022

JaynieBai added 2 commits November 24, 2022 23:34

Remove wrong checks

bc87b69

Fix typo error

7539d3b

JaynieBai marked this pull request as ready for review November 25, 2022 08:20

JaynieBai requested review from Forgind and rokonec November 25, 2022 08:50

Forgind approved these changes Nov 27, 2022

View reviewed changes

src/Build/Evaluation/ProjectParser.cs Outdated Show resolved Hide resolved

Update src/Build/Evaluation/ProjectParser.cs

9c4d718

format equals Co-authored-by: Forgind <12969783+Forgind@users.noreply.github.com>

rainersigwald reviewed Nov 28, 2022

View reviewed changes

JaynieBai added 3 commits November 29, 2022 04:47

Add customer message and change wave

bf32ca3

Merge

19f6669

Upload xlf resources

0aa61ff

Forgind reviewed Nov 29, 2022

View reviewed changes

src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs Outdated Show resolved Hide resolved

Fix test with exact match

9f03ac7

rainersigwald reviewed Nov 30, 2022

View reviewed changes

Refactor the test case

f7c548b

Forgind approved these changes Dec 1, 2022

View reviewed changes

src/Build.OM.UnitTests/Construction/ProjectTargetElement_Tests.cs Show resolved Hide resolved

src/Build/Resources/Strings.resx Outdated Show resolved Hide resolved

Forgind reviewed Dec 1, 2022

View reviewed changes

JaynieBai added 5 commits November 30, 2022 23:01

Change the error to msb4073

036c991

fix error comment

8b2427b

Keep MSB4067 for PropertyOutsidePropertyGroupInTarget

2214bc6

Update resource string

5ab93db

Update test expected string

b985551

danmoseley reviewed Dec 6, 2022

View reviewed changes

Update the resource string

529e6ea

rainersigwald reviewed Dec 6, 2022

View reviewed changes

Update ProjectTargetElement_Tests.cs

ae8d7e8

Update expected string in case the localization

JaynieBai added 2 commits December 7, 2022 19:02

Update test expected string and code

bc3ebcf

Update test expected code and string

ad1142b

rainersigwald approved these changes Dec 8, 2022

View reviewed changes

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Dec 12, 2022

JaynieBai merged commit fd7cd72 into main Dec 13, 2022

JaynieBai deleted the jennybai/issue5773 branch December 13, 2022 02:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse invalid property under target #8190

Parse invalid property under target #8190

JaynieBai commented Nov 25, 2022 •

edited

JaynieBai commented Nov 25, 2022 •

edited

Forgind left a comment

JaynieBai commented Nov 28, 2022

rainersigwald left a comment

rainersigwald Nov 28, 2022

rainersigwald left a comment

Forgind left a comment

Forgind Dec 1, 2022

rainersigwald Dec 2, 2022

JaynieBai Dec 5, 2022

rainersigwald Dec 5, 2022

danmoseley Dec 6, 2022 •

edited

JaynieBai Dec 6, 2022

rainersigwald Dec 6, 2022

JaynieBai Dec 8, 2022

	/// <summary>
	/// Read a target with a missing name
	/// </summary>
	[Fact]
	public void ReadInvalidMissingName()
	{
	Assert.Throws<InvalidProjectFileException>(() =>
	{
	string content = @"
	<Project>
	<Target/>
	</Project>
	";

	ProjectRootElement.Create(XmlReader.Create(new StringReader(content)));
	}
	);
	}

	ProjectXmlUtilities.ThrowProjectInvalidChildElement(childElement.Name, childElement.ParentNode.Name, childElement.Location);
	ProjectErrorUtilities.ThrowInvalidProject(childElement.Location, "PropertyOutsidePropertyGroupInTarget", childElement.Name);

	<data name="UnrecognizedChildElement" xml:space="preserve">
	<value>MSB4067: The element <{0}> beneath element <{1}> is unrecognized.</value>
	<comment>{StrBegin="MSB4067: "}</comment>
	</data>

	<value>MSB4070: The property <{0}> beneath target <{1}> is unrecognized. Properties must be inside a <PropertyGroup> element.</value>
	<value>MSB4070: The property <{0}> beneath target <{1}> is unrecognized. If you intended this to be a property, it must be inside a <PropertyGroup> element.</value>

Parse invalid property under target #8190

Parse invalid property under target #8190

Conversation

JaynieBai commented Nov 25, 2022 • edited

Context

Changes Made

Tests

Notes

JaynieBai commented Nov 25, 2022 • edited

Forgind left a comment

Choose a reason for hiding this comment

JaynieBai commented Nov 28, 2022

rainersigwald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

Forgind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley Dec 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaynieBai commented Nov 25, 2022 •

edited

JaynieBai commented Nov 25, 2022 •

edited

danmoseley Dec 6, 2022 •

edited