Validate C# DefineConstants input

src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/Debug/KeyValuePairListEncoding.cs

@@ -4,32 +4,39 @@ namespace Microsoft.VisualStudio.ProjectSystem.Debug;
 
 internal static class KeyValuePairListEncoding
 {
-    public static IEnumerable<(string Name, string Value)> Parse(string input)
+    /// <summary>
+    /// Parses the input string into a collection of key-value pairs with the given separator.
+    /// </summary>
+    /// <param name="input">The input string to parse.</param>
+    /// <param name="allowsEmptyKey">Indicates whether empty keys are allowed. If this is true, a pair will be returned if an empty key has a non-empty value. ie, =4</param>
+    /// <param name="separator">The character used to separate entries in the input string.</param>
+    public static IEnumerable<(string Name, string Value)> Parse(string input, bool allowsEmptyKey = false, char separator = ',')
     {
         if (string.IsNullOrWhiteSpace(input))
         {
             yield break;
         }
 
-        foreach (var entry in ReadEntries(input))
+        foreach (var entry in ReadEntries(input, separator))
         {
-            var (entryKey, entryValue) = SplitEntry(entry);
+            var (entryKey, entryValue) = SplitEntry(entry, allowsEmptyKey);
             var decodedEntryKey = Decode(entryKey);
             var decodedEntryValue = Decode(entryValue);
-
-            if (!string.IsNullOrEmpty(decodedEntryKey))
+
+            if ((allowsEmptyKey && !string.IsNullOrEmpty(decodedEntryValue))
+                || !string.IsNullOrEmpty(decodedEntryKey) || !string.IsNullOrEmpty(decodedEntryValue))
             {
                 yield return (decodedEntryKey, decodedEntryValue);
             }
         }
 
-        static IEnumerable<string> ReadEntries(string rawText)
+        static IEnumerable<string> ReadEntries(string rawText, char separator)
         {
             bool escaped = false;
             int entryStart = 0;
             for (int i = 0; i < rawText.Length; i++)
             {
-                if (rawText[i] == ',' && !escaped)
+                if (rawText[i] == separator && !escaped)
                 {
                     yield return rawText.Substring(entryStart, i - entryStart);
                     entryStart = i + 1;
@@ -48,12 +55,12 @@ static IEnumerable<string> ReadEntries(string rawText)
             yield return rawText.Substring(entryStart);
         }
 
-        static (string EncodedKey, string EncodedValue) SplitEntry(string entry)
+        static (string EncodedKey, string EncodedValue) SplitEntry(string entry, bool allowsEmptyKey)
         {
             bool escaped = false;
             for (int i = 0; i < entry.Length; i++)
             {
-                if (entry[i] == '=' && !escaped)
+                if (entry[i] == '=' && !escaped && (allowsEmptyKey || i > 0))
                 {
                     return (entry.Substring(0, i), entry.Substring(i + 1));
                 }
@@ -67,7 +74,7 @@ static IEnumerable<string> ReadEntries(string rawText)
                 }
             }
 
-            return (string.Empty, string.Empty);
+            return (entry, string.Empty);
         }
 
         static string Decode(string value)
@@ -76,12 +83,15 @@ static string Decode(string value)
         }
     }
 
-    public static string Format(IEnumerable<(string Name, string Value)> pairs)
+    public static string Format(IEnumerable<(string Name, string Value)> pairs, char separator = ',')
     {
         // Copied from ActiveLaunchProfileEnvironmentVariableValueProvider in the .NET Project System.
         // In future, EnvironmentVariablesNameValueListEncoding should be exported from that code base and imported here.
-
-        return string.Join(",", pairs.Select(kvp => $"{Encode(kvp.Name)}={Encode(kvp.Value)}"));
+        return string.Join(
+            separator.ToString(),
+            pairs.Select(kvp => string.IsNullOrEmpty(kvp.Value) 
+                ? Encode(kvp.Name) 
+                : $"{Encode(kvp.Name)}={Encode(kvp.Value)}"));
 
         static string Encode(string value)
         {

src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/Properties/InterceptedProjectProperties/BuildPropertyPage/DefineConstantsCSharpValueProvider.cs

@@ -7,25 +7,28 @@ namespace Microsoft.VisualStudio.ProjectSystem.Properties;
 
 [ExportInterceptingPropertyValueProvider(ConfiguredBrowseObject.DefineConstantsProperty, ExportInterceptingPropertyValueProviderFile.ProjectFile)]
 [AppliesTo(ProjectCapability.CSharpOrFSharp)]
-internal class DefineConstantsValueProvider : InterceptingPropertyValueProviderBase
+internal class DefineConstantsCSharpValueProvider : InterceptingPropertyValueProviderBase
 {
+    private const string DefineConstantsRecursivePrefix = "$(DefineConstants)";
+
     private readonly IProjectAccessor _projectAccessor;
     private readonly ConfiguredProject _project;
-
-    internal const string DefineConstantsRecursivePrefix = "$(DefineConstants)";
-
+    private HashSet<string> _removedValues = [];
+
     [ImportingConstructor]
-    public DefineConstantsValueProvider(IProjectAccessor projectAccessor, ConfiguredProject project)
+    public DefineConstantsCSharpValueProvider(IProjectAccessor projectAccessor, ConfiguredProject project)
     {
         _projectAccessor = projectAccessor;
         _project = project;
     }
 
-    internal static IEnumerable<string> ParseDefinedConstantsFromUnevaluatedValue(string unevaluatedValue)
+    private static IEnumerable<string> ParseDefinedConstantsFromUnevaluatedValue(string unevaluatedValue)
     {
-        return unevaluatedValue.Length <= DefineConstantsRecursivePrefix.Length  || !unevaluatedValue.StartsWith(DefineConstantsRecursivePrefix)
-            ? Array.Empty<string>() 
-            : unevaluatedValue.Substring(DefineConstantsRecursivePrefix.Length).Split(';').Where(x => x.Length > 0);
+        string substring = unevaluatedValue.Length <= DefineConstantsRecursivePrefix.Length  || !unevaluatedValue.StartsWith(DefineConstantsRecursivePrefix)
+            ? unevaluatedValue
+            : unevaluatedValue.Substring(DefineConstantsRecursivePrefix.Length);
+
+        return substring.Split(';').Where(x => x.Length > 0);
     }
 
     public override async Task<string> OnGetUnevaluatedPropertyValueAsync(string propertyName, string unevaluatedPropertyValue, IProjectProperties defaultProperties)
@@ -38,10 +41,12 @@ public override async Task<string> OnGetUnevaluatedPropertyValueAsync(string pro
             return string.Empty;
         }
 
-        return KeyValuePairListEncoding.Format(
-            ParseDefinedConstantsFromUnevaluatedValue(unevaluatedDefineConstantsValue)
-                .Select(symbol => (Key: symbol, Value: bool.FalseString))
-        );
+        var pairs = KeyValuePairListEncoding.Parse(unevaluatedDefineConstantsValue, separator: ';').Select(pair => pair.Name)
+            .Where(symbol => !string.IsNullOrEmpty(symbol))
+            .Select(symbol => (symbol, _removedValues.Contains(symbol) ? "null" : bool.FalseString)).ToList();
+        pairs.AddRange(_removedValues.Select(value => (value, "null")));
+
+        return KeyValuePairListEncoding.Format(pairs, separator: ',');
     }
 
     // We cannot rely on the unevaluated property value as obtained through Project.GetProperty.UnevaluatedValue - the reason is that for a recursively-defined
@@ -51,7 +56,11 @@ public override async Task<string> OnGetUnevaluatedPropertyValueAsync(string pro
     // 2. to override IsValueDefinedInContextAsync, as this will always return false
     private async Task<string?> GetUnevaluatedDefineConstantsPropertyValueAsync()
     {
-        await ((ConfiguredProject2)_project).EnsureProjectEvaluatedAsync();
+        if (_project is ConfiguredProject2 configuredProject2)
+        {
+            await configuredProject2.EnsureProjectEvaluatedAsync();
+        }
+
         return await _projectAccessor.OpenProjectForReadAsync(_project, project =>
         {
             project.ReevaluateIfNecessary();
@@ -76,18 +85,33 @@ public override async Task<bool> IsValueDefinedInContextAsync(string propertyNam
         IEnumerable<string> innerConstants =
             ParseDefinedConstantsFromUnevaluatedValue(await defaultProperties.GetUnevaluatedPropertyValueAsync(ConfiguredBrowseObject.DefineConstantsProperty) ?? string.Empty);
 
-        IEnumerable<string> constantsToWrite = KeyValuePairListEncoding.Parse(unevaluatedPropertyValue)
+        var separatedPairs = KeyValuePairListEncoding.Parse(unevaluatedPropertyValue, separator: ',').ToList();
+
+        var foundConstants = separatedPairs 
+            // we receive a comma-separated list, and each item may have multiple values separated by semicolons
+            // so we should also parse each value using ; as the separator
+            // ie "A,B;C" -> [A, B;C] -> [A, B, C]
+            .SelectMany(pair => KeyValuePairListEncoding.Parse(pair.Name, allowsEmptyKey: true, separator: ';'))
             .Select(pair => pair.Name)
-            .Where(x => !innerConstants.Contains(x))
+            .Where(pair => !string.IsNullOrEmpty(pair))
+            .Select(constant => constant.Trim(';')) // trim any leading or trailing semicolons, because we will add our own separating semicolons
+            .Where(constant => !string.IsNullOrEmpty(constant)) // you aren't allowed to add a semicolon as a constant
             .Distinct()
             .ToList();
 
-        if (!constantsToWrite.Any())
+        // if a value included multiple constants (such as A;B), we should remove the value in the UI (since it's been replaced) 
+        _removedValues = separatedPairs
+            .Select(pair => pair.Name)
+            .Where(name => !foundConstants.Contains(name) && !string.IsNullOrWhiteSpace(name.Replace(";", "")))
+            .ToHashSet();
+
+        var writeableConstants = foundConstants.Where(constant => !innerConstants.Contains(constant)).ToList();
+        if (writeableConstants.Count == 0)
         {
             await defaultProperties.DeletePropertyAsync(propertyName, dimensionalConditions);
             return null;
         }
-
-        return $"{DefineConstantsRecursivePrefix};" + string.Join(";", constantsToWrite);
+        
+        return string.Join(";", writeableConstants);
     }
 }

tests/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/ProjectSystem/VS/Properties/CSharp/DefineConstantsCSharpValueProviderTests.cs

@@ -0,0 +1,69 @@
+// Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. See the LICENSE.md file in the project root for more information.
+
+using Microsoft.Build.Construction;
+using Microsoft.VisualStudio.ProjectSystem.Properties;
+
+namespace Microsoft.VisualStudio.ProjectSystem.VS.Properties;
+
+public class DefineConstantsCSharpValueProviderTests
+{
+    private const string PropertyName = "DefineConstants";
+
+    [Theory]
+    [InlineData("DEBUG;TRACE", "DEBUG=False,TRACE=False")]
+    [InlineData("", "")]
+    public async Task GetExistingUnevaluatedValue(string? defineConstantsValue, string expectedFormattedValue)
+    {
+        var provider = CreateInstance(defineConstantsValue, out _, out _);
+
+        var actualPropertyValue = await provider.OnGetUnevaluatedPropertyValueAsync(string.Empty, string.Empty, null!);
+        Assert.Equal(expectedFormattedValue, actualPropertyValue);
+    }
+
+    [Theory]
+    [InlineData("DEBUG,TRACE", null, "DEBUG;TRACE", "DEBUG=False,TRACE=False")]
+    [InlineData("$(DefineConstants),DEBUG,TRACE", "PROP1;PROP2", "$(DefineConstants);DEBUG;TRACE", "$(DefineConstants)=False,DEBUG=False,TRACE=False")]
+    [InlineData("Constant0,$(DefineConstants),Constant1;;Constant2;Constant3;,Constant4", "Constant4", "Constant0;$(DefineConstants);Constant1;Constant2;Constant3", "Constant0=False,$(DefineConstants)=False,Constant1=False,Constant2=False,Constant3=False,Constant1;;Constant2;Constant3;=null")]
+    public async Task SetUnevaluatedValue(string unevaluatedValueToSet, string? defineConstantsValue, string? expectedSetUnevaluatedValue, string expectedFormattedValue)
+    {
+        var provider = CreateInstance(null, out var projectAccessor, out var project);
+        Mock<IProjectProperties> mockProjectProperties = new Mock<IProjectProperties>();
+        mockProjectProperties
+            .Setup(p => p.GetUnevaluatedPropertyValueAsync(ConfiguredBrowseObject.DefineConstantsProperty))
+            .ReturnsAsync(defineConstantsValue);
+
+        var setPropertyValue = await provider.OnSetPropertyValueAsync(PropertyName, unevaluatedValueToSet, mockProjectProperties.Object);
+        Assert.Equal(expectedSetUnevaluatedValue, setPropertyValue);
+
+        await SetDefineConstantsPropertyAsync(projectAccessor, project, setPropertyValue);
+
+        var actualPropertyFormattedValue = await provider.OnGetUnevaluatedPropertyValueAsync(string.Empty, string.Empty, null!);
+        Assert.Equal(expectedFormattedValue, actualPropertyFormattedValue);
+    }
+
+    private static DefineConstantsCSharpValueProvider CreateInstance(string? defineConstantsValue, out IProjectAccessor projectAccessor, out ConfiguredProject project)
+    {
+        var projectXml = defineConstantsValue is not null
+            ? $"""
+               <Project>
+                   <PropertyGroup>
+                       <{ConfiguredBrowseObject.DefineConstantsProperty}>{defineConstantsValue}</{ConfiguredBrowseObject.DefineConstantsProperty}>
+                   </PropertyGroup>
+               </Project>
+               """
+            : "<Project></Project>";
+
+        projectAccessor = IProjectAccessorFactory.Create(ProjectRootElementFactory.Create(projectXml));
+        project = ConfiguredProjectFactory.Create();
+
+        return new DefineConstantsCSharpValueProvider(projectAccessor, project);
+    }
+
+    private static async Task SetDefineConstantsPropertyAsync(IProjectAccessor projectAccessor, ConfiguredProject project, string? setPropertyValue)
+    {
+        await projectAccessor.OpenProjectXmlForWriteAsync(project.UnconfiguredProject, projectXml =>
+        {
+            projectXml.AddProperty(PropertyName, setPropertyValue);
+        });
+    }
+}

tests/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/ProjectSystem/VS/Properties/KeyValuePairListEncodingTests.cs

@@ -0,0 +1,59 @@
+// Licensed to the .NET Foundation under one or more agreements. The .NET Foundation licenses this file to you under the MIT license. See the LICENSE.md file in the project root for more information.
+
+using Microsoft.VisualStudio.ProjectSystem.Debug;
+
+namespace Microsoft.VisualStudio.ProjectSystem.VS.Properties;
+
+public class KeyValuePairListEncodingTests
+{
+    [Theory]
+    [InlineData("key1=value1;key2=value2", true, new[] { "key1", "value1", "key2", "value2" })]
+    [InlineData("key1=value1;;key2=value2", true, new[] { "key1", "value1", "key2", "value2" })]
+    [InlineData("key1=value1;;;key2=value2", true, new[] { "key1", "value1", "key2", "value2" })]
+    [InlineData("key1=value1;key2=value2;key3=value3", true, new[] { "key1", "value1", "key2", "value2", "key3", "value3" })]
+    [InlineData("key1;key2=value2", true, new[] { "key1", "", "key2", "value2" })]
+    [InlineData("key1;key2;key3=value3", true, new[] { "key1", "", "key2", "", "key3", "value3" })]
+    [InlineData("key1;;;key3;;", true, new[] { "key1", "", "key3", "" })]
+    [InlineData("", true, new string[0])]
+    [InlineData(" ", true, new string[0])]
+    [InlineData("=", true, new string[0])]
+    [InlineData("", false, new string[0])]
+    [InlineData(" ", false, new string[0])]
+    [InlineData("=", false, new[] { "=", "" })] // = can count as part of the key here
+    [InlineData("key1=value1;=value2=", true, new[] { "key1", "value1", "", "value2=" })]
+    [InlineData("key1=value1;=value2=", false, new[] { "key1", "value1", "=value2", "" })]
+    [InlineData("key1=value1;=value2", false, new[] { "key1", "value1", "=value2", "" })]
+    [InlineData("==", true, new[] { "", "=" })]
+    [InlineData(";", true, new string[0])]
+    public void Parse_ValidInput_ReturnsExpectedPairs(string input, bool allowsEmptyKey, string[] expectedPairs)
+    {
+        var result = KeyValuePairListEncoding.Parse(input, allowsEmptyKey, ';').SelectMany(pair => new[] { pair.Name, pair.Value }).ToArray();
+        Assert.Equal(expectedPairs, result);
+    }
+
+    [Theory]
+    [InlineData(new[] { "key1", "value1", "key2", "value2" }, "key1=value1;key2=value2")]
+    [InlineData(new[] { "key1", "value1", "key2", "value2", "key3", "value3" }, "key1=value1;key2=value2;key3=value3")]
+    [InlineData(new[] { "key1", "", "key2", "value2" }, "key1;key2=value2")]
+    [InlineData(new[] { "key1", "", "key2", "", "key3", "value3" }, "key1;key2;key3=value3")]
+    [InlineData(new string[0], "")]
+    public void Format_ValidPairs_ReturnsExpectedString(string[] pairs, string expectedString)
+    {
+        var nameValuePairs = ToNameValues(pairs);
+        var result = KeyValuePairListEncoding.Format(nameValuePairs, ';');
+        Assert.Equal(expectedString, result);
+        return;
+
+        static IEnumerable<(string Name, string Value)> ToNameValues(IEnumerable<string> pairs)
+        {
+            using var e = pairs.GetEnumerator();
+            while (e.MoveNext())
+            {
+                var name = e.Current;
+                Assert.True(e.MoveNext());
+                var value = e.Current;
+                yield return (name, value);
+            }
+        }
+    }
+}