Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate C# DefineConstants input #9612

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Validate C# DefineConstants input #9612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b3df917
Add tests for KeyValuePairListEncoding, allow multiple separators. Co…
Dec 5, 2024
2548986
add more test cases, parameter allowing for empty key
Dec 6, 2024
d955d17
support notifying the property page control if a constant was removed…
Dec 7, 2024
f96abbe
Remove state, rename
Dec 12, 2024
298c98f
Merge branch 'main' into dev/adamint/fix-define-constants-validation
Dec 12, 2024
2330881
update mock type
Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions ...rosoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/Debug/KeyValuePairListEncoding.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
adamint marked this conversation as resolved.
Show resolved Hide resolved
/// <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))
Comment on lines +26 to +27
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right?

Logically, (A && B) || B is the same as just B.

Where A is allowsEmptyKey and B is !string.IsNullOrEmpty(decodedEntryValue).

If I'm not mistaken, allowsEmptyKey does nothing here.

I think a ! should be removed:

Suggested change
if ((allowsEmptyKey && !string.IsNullOrEmpty(decodedEntryValue))
|| !string.IsNullOrEmpty(decodedEntryKey) || !string.IsNullOrEmpty(decodedEntryValue))
if ((allowsEmptyKey && string.IsNullOrEmpty(decodedEntryValue))
|| !string.IsNullOrEmpty(decodedEntryKey) || !string.IsNullOrEmpty(decodedEntryValue))

But the fact that this passed the unit tests makes me wonder if the tests need improving or if the check can just be removed here altogether.

{
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;
Expand All @@ -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));
}
Expand All @@ -67,7 +74,7 @@ static IEnumerable<string> ReadEntries(string rawText)
}
}

return (string.Empty, string.Empty);
return (entry, string.Empty);
}

static string Decode(string value)
Expand All @@ -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.
Comment on lines 88 to 89
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment still correct? With the changes here, these implementations have diverged.


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)
{
Expand Down
54 changes: 26 additions & 28 deletions ...age/DefineConstantsCSharpValueProvider.cs → ...DefineConstantsCAndFSharpValueProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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;
Expand All @@ -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);
}
}
1 change: 1 addition & 0 deletions ...sualStudio.ProjectSystem.Managed/ProjectSystem/Rules/PropertyPages/BuildPropertyPage.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
5 changes: 4 additions & 1 deletion .../Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/Mocks/ConfiguredProjectFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@ 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 mock = new Mock<ITestConfiguredProjectImpl>();
mock.Setup(c => c.Capabilities).Returns(capabilities!);
mock.Setup(c => c.ProjectConfiguration).Returns(projectConfiguration!);
mock.Setup(c => c.Services).Returns(services!);
mock.Setup(c => c.EnsureProjectEvaluatedAsync()).Returns(Task.CompletedTask);
mock.SetupGet(c => c.UnconfiguredProject).Returns(unconfiguredProject ?? UnconfiguredProjectFactory.Create());
return mock.Object;
}
Expand All @@ -32,4 +33,6 @@ public static ConfiguredProject ImplementUnconfiguredProject(UnconfiguredProject

return mock.Object;
}

internal interface ITestConfiguredProjectImpl : ConfiguredProject, ConfiguredProject2;
Copy link
Member

@drewnoakes drewnoakes Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than introducing a new interface, we can use Moq's built-in support for this.

https://github.com/devlooped/moq/wiki/Quickstart#advanced-features

From memory I think it's something like (those docs aren't super clear):

var mock = new Mock<ConfiguredProject>();
mock.Setup(...);
...
var mock2 = mock.As<ConfiguredProject2>();
mock2.Setup(c => c.EnsureProjectEvaluatedAsync()).Returns(Task.CompletedTask);

return mock.Object;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though creating a new interface was a pretty clever way to achieve the same thing. Use whichever you prefer. I don't think there's much material difference.

}
68 changes: 68 additions & 0 deletions ...itTests/ProjectSystem/VS/Properties/CSharp/DefineConstantsCAndFSharpValueProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
});
}
}
59 changes: 59 additions & 0 deletions ...tSystem.Managed.VS.UnitTests/ProjectSystem/VS/Properties/KeyValuePairListEncodingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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")]
adamint marked this conversation as resolved.
Show resolved Hide resolved
[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);
}
}
}
}
Loading