Fix TaskParser probably has a bug in switch de-duplication logic. (#5115

)

* handle duplicate property name

* use string resource, assign next error code

* assert exception message

* use Shouldly as suggested in code review

* split huge line of xml in test

* correct use of Should.Throw

* eliminate _switchesAdded variable

- no significant effect on performance because item count is always low

* assert only error code as suggested in code review

src/Tasks.UnitTests/XamlTaskFactory_Tests.cs

@@ -17,6 +17,7 @@
 using Microsoft.Build.Tasks.Xaml;
 using System.Xaml;
 using Xunit;
+using Shouldly;
 
 namespace Microsoft.Build.UnitTests.XamlTaskFactory_Tests
 {
@@ -191,6 +192,25 @@ public void TestBasicReversibleBooleanSwitches()
             Assert.Equal(PropertyType.Boolean, properties.First.Value.Type);
         }
 
+        [Fact]
+        public void TestParseIncorrect_PropertyNamesMustBeUnique()
+        {
+            string incorrectXmlContents = @"<ProjectSchemaDefinitions
+                                       xmlns=`clr-namespace:Microsoft.Build.Framework.XamlTypes;assembly=Microsoft.Build.Framework`
+                                       xmlns:x=`http://schemas.microsoft.com/winfx/2006/xaml`
+                                       xmlns:sys=`clr-namespace:System;assembly=mscorlib`
+                                       xmlns:impl=`clr-namespace:Microsoft.VisualStudio.Project.Contracts.Implementation;assembly=Microsoft.VisualStudio.Project.Contracts.Implementation`>
+                                     <Rule Name=`CL`>
+                                       <BoolProperty Name=`SameName` Switch=`Og` ReverseSwitch=`Og-` />
+                                       <BoolProperty Name=`SameName` Switch=`Og` ReverseSwitch=`Og-` />
+                                     </Rule>
+                                   </ProjectSchemaDefinitions>";
+
+            Should
+                .Throw<XamlParseException>(() => XamlTestHelpers.LoadAndParse(incorrectXmlContents, "CL"))
+                .Message.ShouldStartWith("MSB3724");
+        }
+
         /// <summary>
         /// Tests a basic non-reversible booleans switch
         /// </summary>

src/Tasks/Resources/Strings.resx

@@ -2221,6 +2221,10 @@
     <value>MSB3723: The parameter "{0}" requires missing parameter "{1}" to be set.</value>
     <comment>{StrBegin="MSB3723: "}</comment>
   </data>
+  <data name="Xaml.DuplicatePropertyName" xml:space="preserve">
+      <value>MSB3724: Unable to create Xaml task.  Duplicate property name '{0}'.</value>
+    <comment>{StrBegin="MSB3724: "}</comment>
+  </data>
 
   <!--
         The XslTransformation message bucket is: MSB3701-3710.

src/Tasks/XamlTaskFactory/TaskParser.cs

@@ -18,11 +18,6 @@ namespace Microsoft.Build.Tasks.Xaml
     /// </summary>
     internal class TaskParser
     {
-        /// <summary>
-        /// The set of switches added so far.
-        /// </summary>
-        private readonly HashSet<string> _switchesAdded = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
-
         /// <summary>
         /// The ordered list of how the switches get emitted.
         /// </summary>
@@ -305,10 +300,14 @@ private bool ParseParameter(XamlTypes.BaseProperty baseProperty, LinkedList<Prop
             }
 
             // generate the list of parameters in order
-            if (!_switchesAdded.Contains(propertyToAdd.Name))
+            if (!_switchOrderList.Contains(propertyToAdd.Name))
             {
                 _switchOrderList.Add(propertyToAdd.Name);
             }
+            else
+            {
+                throw new XamlParseException(ResourceUtilities.FormatResourceStringIgnoreCodeAndKeyword("Xaml.DuplicatePropertyName", propertyToAdd.Name));
+            }
 
             // Inherit the Prefix from the Tool
             if (String.IsNullOrEmpty(propertyToAdd.Prefix))