Skip to content

Commit

Permalink
Don't compile globbing regexes on .NET Framework (#6632)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ladipro
ladipro committed Jul 9, 2021
1 parent 86368d3 commit d150e93
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 47 deletions.
2 changes: 2 additions & 0 deletions documentation/wiki/ChangeWaves.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ The opt-out comes in the form of setting the environment variable `MSBuildDisabl
- [Allow users that have certain special characters in their username to build successfully when using exec](https://github.com/dotnet/msbuild/pull/6223)
- [Fail restore operations when an SDK is unresolveable](https://github.com/dotnet/msbuild/pull/6430)
### 17.0
- [Scheduler should honor BuildParameters.DisableInprocNode](https://github.com/dotnet/msbuild/pull/6400)
- [Don't compile globbing regexes on .NET Framework](https://github.com/dotnet/msbuild/pull/6632)
- [Default to transitively copying content items](https://github.com/dotnet/msbuild/pull/6622)

## Change Waves No Longer In Rotation
27 changes: 23 additions & 4 deletions src/Build.UnitTests/Globbing/MSBuildGlob_Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,20 +326,39 @@ public void GlobMatchingShouldWorkWithComplexRelativeLiterals()
[InlineData(
@"a/b\c",
@"d/e\f/**\a.cs",
@"d\e/f\g/h\i/a.cs")]
@"d\e/f\g/h\i/a.cs",
@"d\e/f\", @"g/h\i/", @"a.cs")]
[InlineData(
@"a/b\c",
@"d/e\f/*b*\*.cs",
@"d\e/f\abc/a.cs")]
@"d\e/f\abc/a.cs",
@"d\e/f\", @"abc/", @"a.cs")]
[InlineData(
@"a/b/\c",
@"d/e\/*b*/\*.cs",
@"d\e\\abc/\a.cs")]
public void GlobMatchingIgnoresSlashOrientationAndRepetitions(string globRoot, string fileSpec, string stringToMatch)
@"d\e\\abc/\a.cs",
@"d\e\\", @"abc\\", @"a.cs")]
public void GlobMatchingIgnoresSlashOrientationAndRepetitions(string globRoot, string fileSpec, string stringToMatch,
string fixedDirectoryPart, string wildcardDirectoryPart, string filenamePart)
{
var glob = MSBuildGlob.Parse(globRoot, fileSpec);

Assert.True(glob.IsMatch(stringToMatch));

MSBuildGlob.MatchInfoResult result = glob.MatchInfo(stringToMatch);
Assert.True(result.IsMatch);

string NormalizeSlashes(string path)
{
string normalizedPath = path.Replace(Path.DirectorySeparatorChar == '/' ? '\\' : '/', Path.DirectorySeparatorChar);
return NativeMethodsShared.IsWindows ? normalizedPath.Replace("\\\\", "\\") : normalizedPath;
}

var rootedFixedDirectoryPart = Path.Combine(FileUtilities.NormalizePath(globRoot), fixedDirectoryPart);

Assert.Equal(FileUtilities.GetFullPathNoThrow(rootedFixedDirectoryPart), result.FixedDirectoryPartMatchGroup);
Assert.Equal(NormalizeSlashes(wildcardDirectoryPart), result.WildcardDirectoryPartMatchGroup);
Assert.Equal(NormalizeSlashes(filenamePart), result.FilenamePartMatchGroup);
}
}
}
20 changes: 18 additions & 2 deletions src/Build/Globbing/MSBuildGlob.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Text.RegularExpressions;
using Microsoft.Build.Collections;
using Microsoft.Build.Shared;
using Microsoft.Build.Utilities;
using Microsoft.NET.StringTools;

namespace Microsoft.Build.Globbing
Expand Down Expand Up @@ -126,10 +127,13 @@ public MatchInfoResult MatchInfo(string stringToMatch)
normalizedInput,
_state.Value.Regex,
out bool isMatch,
out string fixedDirectoryPart,
out string wildcardDirectoryPart,
out string filenamePart);

// We don't capture the fixed directory part in the regex but we can infer it from the other two.
int fixedDirectoryPartLength = normalizedInput.Length - wildcardDirectoryPart.Length - filenamePart.Length;
string fixedDirectoryPart = normalizedInput.Substring(0, fixedDirectoryPartLength);

return new MatchInfoResult(isMatch, fixedDirectoryPart, wildcardDirectoryPart, filenamePart);
}

Expand Down Expand Up @@ -202,8 +206,20 @@ public static MSBuildGlob Parse(string globRoot, string fileSpec)
if (regex == null)
{
RegexOptions regexOptions = FileMatcher.DefaultRegexOptions;
// compile the regex since it's expected to be used multiple times
Regex newRegex = new Regex(matchFileExpression, FileMatcher.DefaultRegexOptions | RegexOptions.Compiled);
// For the kind of regexes used here, compilation on .NET Framework tends to be expensive and not worth the small
// run-time boost so it's enabled only on .NET Core by default.
#if RUNTIME_TYPE_NETCORE
bool compileRegex = true;
#else
bool compileRegex = !ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_0);
#endif
if (compileRegex)
{
regexOptions |= RegexOptions.Compiled;
}
Regex newRegex = new Regex(matchFileExpression, regexOptions);
lock (s_regexCache)
{
if (!s_regexCache.TryGetValue(matchFileExpression, out regex))
Expand Down
50 changes: 21 additions & 29 deletions src/Shared/FileMatcher.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ internal class FileMatcher

private static class FileSpecRegexParts
{
internal const string FixedDirGroupStart = "^(?<FIXEDDIR>";
internal const string BeginningOfLine = "^";
internal const string WildcardGroupStart = "(?<WILDCARDDIR>";
internal const string FilenameGroupStart = "(?<FILENAME>";
internal const string GroupEnd = ")";
Expand All @@ -71,10 +71,10 @@ private static class FileSpecRegexParts
}

/*
* MAX_PATH + FileSpecRegexParts.BeginningOfLine.Length + FileSpecRegexParts.FixedDirWildcardDirSeparator.Length
+ FileSpecRegexParts.WildcardDirFilenameSeparator.Length + FileSpecRegexParts.EndOfLine.Length;
* FileSpecRegexParts.BeginningOfLine.Length + FileSpecRegexParts.WildcardGroupStart.Length + FileSpecRegexParts.GroupEnd.Length
+ FileSpecRegexParts.FilenameGroupStart.Length + FileSpecRegexParts.GroupEnd.Length + FileSpecRegexParts.EndOfLine.Length;
*/
private const int FileSpecRegexMinLength = 44;
private const int FileSpecRegexMinLength = 31;

/// <summary>
/// The Default FileMatcher does not cache directory enumeration.
Expand Down Expand Up @@ -501,7 +501,7 @@ GetFileSystemEntries getFileSystemEntries
else
{
// Relative
pathRoot = String.Empty;
pathRoot = string.Empty;
startingElement = 0;
}
}
Expand All @@ -516,7 +516,7 @@ GetFileSystemEntries getFileSystemEntries
// If there is a zero-length part, then that means there was an extra slash.
if (parts[i].Length == 0)
{
longParts[i - startingElement] = String.Empty;
longParts[i - startingElement] = string.Empty;
}
else
{
Expand Down Expand Up @@ -556,7 +556,7 @@ GetFileSystemEntries getFileSystemEntries
}
}

return pathRoot + String.Join(s_directorySeparator, longParts);
return pathRoot + string.Join(s_directorySeparator, longParts);
}

/// <summary>
Expand Down Expand Up @@ -630,8 +630,8 @@ private static void PreprocessFileSpecForSplitting
*
* **
*/
fixedDirectoryPart = String.Empty;
wildcardDirectoryPart = String.Empty;
fixedDirectoryPart = string.Empty;
wildcardDirectoryPart = string.Empty;
filenamePart = filespec;
return;
}
Expand Down Expand Up @@ -661,7 +661,7 @@ private static void PreprocessFileSpecForSplitting

// We know the fixed director part now.
fixedDirectoryPart = filespec.Substring(0, indexOfLastDirectorySeparator + 1);
wildcardDirectoryPart = String.Empty;
wildcardDirectoryPart = string.Empty;
filenamePart = filespec.Substring(indexOfLastDirectorySeparator + 1);
return;
}
Expand All @@ -682,7 +682,7 @@ private static void PreprocessFileSpecForSplitting
*
* dir?\**
*/
fixedDirectoryPart = String.Empty;
fixedDirectoryPart = string.Empty;
wildcardDirectoryPart = filespec.Substring(0, indexOfLastDirectorySeparator + 1);
filenamePart = filespec.Substring(indexOfLastDirectorySeparator + 1);
return;
Expand Down Expand Up @@ -1207,10 +1207,10 @@ string filenamePart
{
#if DEBUG
ErrorUtilities.VerifyThrow(
FileSpecRegexMinLength == FileSpecRegexParts.FixedDirGroupStart.Length
FileSpecRegexMinLength == FileSpecRegexParts.BeginningOfLine.Length
+ FileSpecRegexParts.WildcardGroupStart.Length
+ FileSpecRegexParts.FilenameGroupStart.Length
+ (FileSpecRegexParts.GroupEnd.Length * 3)
+ (FileSpecRegexParts.GroupEnd.Length * 2)
+ FileSpecRegexParts.EndOfLine.Length,
"Checked-in length of known regex components differs from computed length. Update checked-in constant."
);
Expand Down Expand Up @@ -1278,7 +1278,7 @@ private static bool HasMisplacedRecursiveOperator(string str)
/// </summary>
private static void AppendRegularExpressionFromFixedDirectory(ReuseableStringBuilder regex, string fixedDir)
{
regex.Append(FileSpecRegexParts.FixedDirGroupStart);
regex.Append(FileSpecRegexParts.BeginningOfLine);

bool isUncPath = NativeMethodsShared.IsWindows && fixedDir.Length > 1
&& fixedDir[0] == '\\' && fixedDir[1] == '\\';
Expand All @@ -1292,8 +1292,6 @@ private static void AppendRegularExpressionFromFixedDirectory(ReuseableStringBui
{
AppendRegularExpressionFromChar(regex, fixedDir[i]);
}

regex.Append(FileSpecRegexParts.GroupEnd);
}

/// <summary>
Expand Down Expand Up @@ -1569,9 +1567,9 @@ private static int LastIndexOfDirectoryOrRecursiveSequence(string str, int start
FixupParts fixupParts = null)
{
needsRecursion = false;
fixedDirectoryPart = String.Empty;
wildcardDirectoryPart = String.Empty;
filenamePart = String.Empty;
fixedDirectoryPart = string.Empty;
wildcardDirectoryPart = string.Empty;
filenamePart = string.Empty;

if (!RawFileSpecIsValid(filespec))
{
Expand Down Expand Up @@ -1663,9 +1661,7 @@ internal Result()
internal bool isLegalFileSpec; // initially false
internal bool isMatch; // initially false
internal bool isFileSpecRecursive; // initially false
internal string fixedDirectoryPart = String.Empty;
internal string wildcardDirectoryPart = String.Empty;
internal string filenamePart = String.Empty;
internal string wildcardDirectoryPart = string.Empty;
}

/// <summary>
Expand Down Expand Up @@ -1857,9 +1853,8 @@ out matchResult.isLegalFileSpec
fileToMatch,
regexFileMatch,
out matchResult.isMatch,
out matchResult.fixedDirectoryPart,
out matchResult.wildcardDirectoryPart,
out matchResult.filenamePart);
out _);
}

return matchResult;
Expand All @@ -1869,20 +1864,17 @@ out matchResult.isLegalFileSpec
string fileToMatch,
Regex fileSpecRegex,
out bool isMatch,
out string fixedDirectoryPart,
out string wildcardDirectoryPart,
out string filenamePart)
{
Match match = fileSpecRegex.Match(fileToMatch);

isMatch = match.Success;
fixedDirectoryPart = string.Empty;
wildcardDirectoryPart = String.Empty;
wildcardDirectoryPart = string.Empty;
filenamePart = string.Empty;

if (isMatch)
{
fixedDirectoryPart = match.Groups["FIXEDDIR"].Value;
wildcardDirectoryPart = match.Groups["WILDCARDDIR"].Value;
filenamePart = match.Groups["FILENAME"].Value;
}
Expand Down Expand Up @@ -2093,7 +2085,7 @@ enum SearchAction
return SearchAction.ReturnEmptyList;
}

stripProjectDirectory = !String.Equals(fixedDirectoryPart, oldFixedDirectoryPart, StringComparison.OrdinalIgnoreCase);
stripProjectDirectory = !string.Equals(fixedDirectoryPart, oldFixedDirectoryPart, StringComparison.OrdinalIgnoreCase);
}
else
{
Expand Down
24 changes: 12 additions & 12 deletions src/Shared/UnitTests/FileMatcher_Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1653,7 +1653,7 @@ public void ExcludeComplexPattern(string include, string[] exclude, string[] mat
"",
"",
"",
"^(?<FIXEDDIR>)(?<WILDCARDDIR>)(?<FILENAME>)$",
"^(?<WILDCARDDIR>)(?<FILENAME>)$",
false,
true
)]
Expand Down Expand Up @@ -1723,7 +1723,7 @@ public void ExcludeComplexPattern(string include, string[] exclude, string[] mat
"",
@"*fo?ba?\",
"*fo?ba?",
@"^(?<FIXEDDIR>)(?<WILDCARDDIR>[^/\\]*fo.ba.[/\\]+)(?<FILENAME>[^/\\]*fo.ba.)$",
@"^(?<WILDCARDDIR>[^/\\]*fo.ba.[/\\]+)(?<FILENAME>[^/\\]*fo.ba.)$",
true,
true
)]
Expand All @@ -1733,7 +1733,7 @@ public void ExcludeComplexPattern(string include, string[] exclude, string[] mat
"",
"",
"?oo*.",
@"^(?<FIXEDDIR>)(?<WILDCARDDIR>)(?<FILENAME>[^\.].oo[^\.]*)$",
@"^(?<WILDCARDDIR>)(?<FILENAME>[^\.].oo[^\.]*)$",
false,
true
)]
Expand All @@ -1743,7 +1743,7 @@ public void ExcludeComplexPattern(string include, string[] exclude, string[] mat
"",
"",
"*.*foo*.*",
@"^(?<FIXEDDIR>)(?<WILDCARDDIR>)(?<FILENAME>[^/\\]*foo[^/\\]*)$",
@"^(?<WILDCARDDIR>)(?<FILENAME>[^/\\]*foo[^/\\]*)$",
false,
true
)]
Expand All @@ -1753,7 +1753,7 @@ public void ExcludeComplexPattern(string include, string[] exclude, string[] mat
@"\foo///bar\\\",
@"?foo///bar\\\",
"foo",
@"^(?<FIXEDDIR>[/\\]+foo[/\\]+bar[/\\]+)(?<WILDCARDDIR>.foo[/\\]+bar[/\\]+)(?<FILENAME>foo)$",
@"^[/\\]+foo[/\\]+bar[/\\]+(?<WILDCARDDIR>.foo[/\\]+bar[/\\]+)(?<FILENAME>foo)$",
true,
true
)]
Expand All @@ -1763,7 +1763,7 @@ public void ExcludeComplexPattern(string include, string[] exclude, string[] mat
@"\./.\foo/.\./bar\./.\",
@"?foo/.\./bar\./.\",
"foo",
@"^(?<FIXEDDIR>[/\\]+foo[/\\]+bar[/\\]+)(?<WILDCARDDIR>.foo[/\\]+bar[/\\]+)(?<FILENAME>foo)$",
@"^[/\\]+foo[/\\]+bar[/\\]+(?<WILDCARDDIR>.foo[/\\]+bar[/\\]+)(?<FILENAME>foo)$",
true,
true
)]
Expand All @@ -1773,7 +1773,7 @@ public void ExcludeComplexPattern(string include, string[] exclude, string[] mat
@"foo\",
@"**/**\bar/**\**/foo\**/**\",
"bar",
@"^(?<FIXEDDIR>foo[/\\]+)(?<WILDCARDDIR>((.*/)|(.*\\)|())bar((/)|(\\)|(/.*/)|(/.*\\)|(\\.*\\)|(\\.*/))foo((/)|(\\)|(/.*/)|(/.*\\)|(\\.*\\)|(\\.*/)))(?<FILENAME>bar)$",
@"^foo[/\\]+(?<WILDCARDDIR>((.*/)|(.*\\)|())bar((/)|(\\)|(/.*/)|(/.*\\)|(\\.*\\)|(\\.*/))foo((/)|(\\)|(/.*/)|(/.*\\)|(\\.*\\)|(\\.*/)))(?<FILENAME>bar)$",
true,
true
)]
Expand All @@ -1783,7 +1783,7 @@ public void ExcludeComplexPattern(string include, string[] exclude, string[] mat
@"foo\\\.///",
@"**\\\.///**\\\.///bar\\\.///**\\\.///**\\\.///foo\\\.///**\\\.///**\\\.///",
"bar",
@"^(?<FIXEDDIR>foo[/\\]+)(?<WILDCARDDIR>((.*/)|(.*\\)|())bar((/)|(\\)|(/.*/)|(/.*\\)|(\\.*\\)|(\\.*/))foo((/)|(\\)|(/.*/)|(/.*\\)|(\\.*\\)|(\\.*/)))(?<FILENAME>bar)$",
@"^foo[/\\]+(?<WILDCARDDIR>((.*/)|(.*\\)|())bar((/)|(\\)|(/.*/)|(/.*\\)|(\\.*\\)|(\\.*/))foo((/)|(\\)|(/.*/)|(/.*\\)|(\\.*\\)|(\\.*/)))(?<FILENAME>bar)$",
true,
true
)]
Expand Down Expand Up @@ -1821,7 +1821,7 @@ bool expectedIsLegalFileSpec
@"$()+.[^{\",
@"?$()+.[^{\",
"$()+.[^{",
@"^(?<FIXEDDIR>\$\(\)\+\.\[\^\{[/\\]+)(?<WILDCARDDIR>.\$\(\)\+\.\[\^\{[/\\]+)(?<FILENAME>\$\(\)\+\.\[\^\{)$",
@"^\$\(\)\+\.\[\^\{[/\\]+(?<WILDCARDDIR>.\$\(\)\+\.\[\^\{[/\\]+)(?<FILENAME>\$\(\)\+\.\[\^\{)$",
true,
true
)]
Expand All @@ -1831,7 +1831,7 @@ bool expectedIsLegalFileSpec
@"\\\.\foo/",
"",
"bar",
@"^(?<FIXEDDIR>\\\\foo[/\\]+)(?<WILDCARDDIR>)(?<FILENAME>bar)$",
@"^\\\\foo[/\\]+(?<WILDCARDDIR>)(?<FILENAME>bar)$",
false,
true
)]
Expand Down Expand Up @@ -1864,7 +1864,7 @@ bool expectedIsLegalFileSpec
@"$()+.[^{|/",
@"?$()+.[^{|/",
"$()+.[^{|",
@"^(?<FIXEDDIR>\$\(\)\+\.\[\^\{\|[/\\]+)(?<WILDCARDDIR>.\$\(\)\+\.\[\^\{\|[/\\]+)(?<FILENAME>\$\(\)\+\.\[\^\{\|)$",
@"^\$\(\)\+\.\[\^\{\|[/\\]+(?<WILDCARDDIR>.\$\(\)\+\.\[\^\{\|[/\\]+)(?<FILENAME>\$\(\)\+\.\[\^\{\|)$",
true,
true
)]
Expand All @@ -1874,7 +1874,7 @@ bool expectedIsLegalFileSpec
@"///./foo/",
"",
"bar",
@"^(?<FIXEDDIR>[/\\]+foo[/\\]+)(?<WILDCARDDIR>)(?<FILENAME>bar)$",
@"^[/\\]+foo[/\\]+(?<WILDCARDDIR>)(?<FILENAME>bar)$",
false,
true
)]
Expand Down

0 comments on commit d150e93

Please sign in to comment.