Validate C# DefineConstants input

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

@@ -4,32 +4,38 @@ 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, 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 decodedEntryKey = Decode(entryKey);
             var decodedEntryValue = Decode(entryValue);
-
+            
             if (!string.IsNullOrEmpty(decodedEntryKey))
             {
                 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;
@@ -67,7 +73,7 @@ static IEnumerable<string> ReadEntries(string rawText)
                 }
             }
 
-            return (string.Empty, string.Empty);
+            return (entry, string.Empty);
         }
 
         static string Decode(string value)
@@ -76,12 +82,13 @@ 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 → src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/Properties/InterceptedProjectProperties/BuildPropertyPage/DefineConstantsCAndFSharpValueProvider.cs

@@ -7,25 +7,18 @@ namespace Microsoft.VisualStudio.ProjectSystem.Properties;
 
 [ExportInterceptingPropertyValueProvider(ConfiguredBrowseObject.DefineConstantsProperty, ExportInterceptingPropertyValueProviderFile.ProjectFile)]
 [AppliesTo(ProjectCapability.CSharpOrFSharp)]
-internal class DefineConstantsValueProvider : InterceptingPropertyValueProviderBase
+[method: ImportingConstructor]
+internal class DefineConstantsCAndFSharpValueProvider(IProjectAccessor projectAccessor, ConfiguredProject project) : InterceptingPropertyValueProviderBase
 {
-    private readonly IProjectAccessor _projectAccessor;
-    private readonly ConfiguredProject _project;
+    private const string DefineConstantsRecursivePrefix = "$(DefineConstants)";
 
-    internal const string DefineConstantsRecursivePrefix = "$(DefineConstants)";
-
-    [ImportingConstructor]
-    public DefineConstantsValueProvider(IProjectAccessor projectAccessor, ConfiguredProject project)
+    private static IEnumerable<string> ParseDefinedConstantsFromUnevaluatedValue(string unevaluatedValue)
     {
-        _projectAccessor = projectAccessor;
-        _project = project;
-    }
+        string substring = unevaluatedValue.Length <= DefineConstantsRecursivePrefix.Length  || !unevaluatedValue.StartsWith(DefineConstantsRecursivePrefix)
+            ? unevaluatedValue
+            : unevaluatedValue.Substring(DefineConstantsRecursivePrefix.Length);
 
-    internal 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);
+        return substring.Split(';').Where(x => x.Length > 0);
     }
 
     public override async Task<string> OnGetUnevaluatedPropertyValueAsync(string propertyName, string unevaluatedPropertyValue, IProjectProperties defaultProperties)
@@ -38,10 +31,11 @@ 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, bool.FalseString)).ToList();
+
+        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,11 +45,12 @@ 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();
-        return await _projectAccessor.OpenProjectForReadAsync(_project, project =>
+        await ((ConfiguredProject2)project).EnsureProjectEvaluatedAsync();
+
+        return await projectAccessor.OpenProjectForReadAsync(project, msbuildProject =>
         {
-            project.ReevaluateIfNecessary();
-            ProjectProperty defineConstantsProperty = project.GetProperty(ConfiguredBrowseObject.DefineConstantsProperty);
+            msbuildProject.ReevaluateIfNecessary();
+            ProjectProperty defineConstantsProperty = msbuildProject.GetProperty(ConfiguredBrowseObject.DefineConstantsProperty);
             while (defineConstantsProperty.IsImported && defineConstantsProperty.Predecessor is not null)
             {
                 defineConstantsProperty = defineConstantsProperty.Predecessor;
@@ -76,18 +71,21 @@ 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 foundConstants = KeyValuePairListEncoding.Parse(unevaluatedPropertyValue, 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())
+        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);
     }
 }

src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/Rules/PropertyPages/BuildPropertyPage.xaml

@@ -61,6 +61,7 @@
           <NameValuePair Name="AllowsCustomStrings" Value="True" />
           <NameValuePair Name="ShouldDisplayEvaluatedPreview" Value="True" />
           <NameValuePair Name="SingleValueConfigurationCommandEnabled" Value="False" />
+          <NameValuePair Name="MultiValueSplitRegex" Value=";" />
         </ValueEditor.Metadata>
       </ValueEditor>
     </StringProperty.ValueEditors>

tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/Mocks/ConfiguredProjectFactory.cs

@@ -6,11 +6,15 @@ internal static class ConfiguredProjectFactory
 {
     public static ConfiguredProject Create(IProjectCapabilitiesScope? capabilities = null, ProjectConfiguration? projectConfiguration = null, ConfiguredProjectServices? services = null, UnconfiguredProject? unconfiguredProject = null)
     {
-        var mock = new Mock<ConfiguredProject>();
+        var mock2 = new Mock<ConfiguredProject2>();
+        mock2.Setup(c => c.EnsureProjectEvaluatedAsync()).Returns(Task.CompletedTask);
+
+        var mock = mock2.As<ConfiguredProject>();
         mock.Setup(c => c.Capabilities).Returns(capabilities!);
         mock.Setup(c => c.ProjectConfiguration).Returns(projectConfiguration!);
         mock.Setup(c => c.Services).Returns(services!);
         mock.SetupGet(c => c.UnconfiguredProject).Returns(unconfiguredProject ?? UnconfiguredProjectFactory.Create());
+
         return mock.Object;
     }
 

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

@@ -0,0 +1,68 @@
+// 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 DefineConstantsCAndFSharpValueProviderTests
+{
+    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")]
+    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 DefineConstantsCAndFSharpValueProvider 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 DefineConstantsCAndFSharpValueProvider(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", new[] { "key1", "value1", "key2", "value2" })]
+    [InlineData("key1=value1;;key2=value2", new[] { "key1", "value1", "key2", "value2" })]
+    [InlineData("key1=value1;;;key2=value2", new[] { "key1", "value1", "key2", "value2" })]
+    [InlineData("key1=value1;key2=value2;key3=value3", new[] { "key1", "value1", "key2", "value2", "key3", "value3" })]
+    [InlineData("key1;key2=value2", new[] { "key1", "", "key2", "value2" })]
+    [InlineData("key1;key2;key3=value3", new[] { "key1", "", "key2", "", "key3", "value3" })]
+    [InlineData("key1;;;key3;;", new[] { "key1", "", "key3", "" })]
+    [InlineData("", new string[0])]
+    [InlineData(" ", new string[0])]
+    [InlineData("=", new string[0])]
+    [InlineData("/=", new[] { "=", "" })]
+    [InlineData("key1=value1;/=value2=", new[] { "key1", "value1", "=value2", "" })]
+    [InlineData("key1=value1;=value2", new[] { "key1", "value1" })]
+    [InlineData("==", new string[0])]
+    [InlineData("=/=", new string[0])]
+    [InlineData("/==", new[] { "=", "" })]
+    [InlineData(";", new string[0])]
+    public void Parse_ValidInput_ReturnsExpectedPairs(string input, string[] expectedPairs)
+    {
+        var result = KeyValuePairListEncoding.Parse(input, ';').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=", "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);
+            }
+        }
+    }
+}