Skip to content

Commit

Permalink
Fix incorrect suffix search for alternation with RegexOptions.RightTo…
Browse files Browse the repository at this point in the history 
…Left (dotnet#101408)

* Fix incorrect suffix search for alternation with RegexOptions.RightToLeft

Our limited RegexOptions.RightToLeft support for prefix optimizations is mishandling alternations. For RTL, this actually needs to create a suffix, but for alternations, we were actually creating a prefix. As this option is rarely used and it's not worth significant investment in optimizing, the fix is just to disable the handling of alternations for RTL, as the support has been broken since it was introduced in .NET 7.

* Fix new tests

* Avoid annoying skip message
  • Loading branch information
@stephentoub @matouskozak
stephentoub authored and matouskozak committed Apr 30, 2024
1 parent 9de2e08 commit 2974608
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 49 deletions.
2 changes: 1 addition & 1 deletion .../System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexPrefixAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ static bool Process(RegexNode node, ref ValueStringBuilder vsb)
}

// Alternation: find a string that's a shared prefix of all branches
case RegexNodeKind.Alternate:
case RegexNodeKind.Alternate when !rtl: // for RTL we'd need to be matching the suffixes of the alternation cases
{
int childCount = node.ChildCount();

Expand Down
93 changes: 46 additions & 47 deletions ...ies/System.Text.RegularExpressions/tests/FunctionalTests/Regex.CompileToAssembly.Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,63 +14,62 @@ namespace System.Text.RegularExpressions.Tests
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
public class RegexCompileToAssemblyTests : FileCleanupTestBase
{
public static bool IsDebug => typeof(Regex).Assembly.GetCustomAttributes(false).OfType<DebuggableAttribute>().Any(da => da.IsJITTrackingEnabled);
public static bool IsRelease => !IsDebug;
public static bool IsDebugAndRemoteExecutorSupported => IsDebug && RemoteExecutor.IsSupported;

[ConditionalFact(nameof(IsRelease))]
public void CompileToAssembly_PNSE()
[Fact]
public void CompileToAssembly_SimpleTest()
{
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(null, null));
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(null, null, null));
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(null, null, null, null));
bool isDebug = typeof(Regex).Assembly.GetCustomAttributes(false).OfType<DebuggableAttribute>().Any(da => da.IsJITTrackingEnabled);

Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(
[new RegexCompilationInfo("abcd", RegexOptions.None, "abcd", "SomeNamespace", true)],
new AssemblyName("abcd")));
if (!isDebug)
{
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(null, null));
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(null, null, null));
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(null, null, null, null));

Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(
[new RegexCompilationInfo("abcd", RegexOptions.None, "abcd", "SomeNamespace", true)],
new AssemblyName("abcd"),
[new CustomAttributeBuilder(typeof(AssemblyCompanyAttribute).GetConstructor([typeof(string)]), new[] { "TestCompany" })]));
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(
[new RegexCompilationInfo("abcd", RegexOptions.None, "abcd", "SomeNamespace", true)],
new AssemblyName("abcd")));

Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(
[new RegexCompilationInfo("abcd", RegexOptions.None, "abcd", "SomeNamespace", true)],
new AssemblyName("abcd"),
[new CustomAttributeBuilder(typeof(AssemblyCompanyAttribute).GetConstructor([typeof(string)]), new[] { "TestCompany" })],
"resourceFile"));
}
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(
[new RegexCompilationInfo("abcd", RegexOptions.None, "abcd", "SomeNamespace", true)],
new AssemblyName("abcd"),
[new CustomAttributeBuilder(typeof(AssemblyCompanyAttribute).GetConstructor([typeof(string)]), new[] { "TestCompany" })]));

[ConditionalFact(nameof(IsDebugAndRemoteExecutorSupported))]
public void CompileToAssembly_SimpleUseInDebug()
{
RemoteExecutor.Invoke(() =>
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(
[new RegexCompilationInfo("abcd", RegexOptions.None, "abcd", "SomeNamespace", true)],
new AssemblyName("abcd"),
[new CustomAttributeBuilder(typeof(AssemblyCompanyAttribute).GetConstructor([typeof(string)]), new[] { "TestCompany" })],
"resourceFile"));
}
else if (RemoteExecutor.IsSupported)
{
(RegexCompilationInfo rci, string validInput, string invalidInput)[] regexes =
[
(new RegexCompilationInfo("abcd", RegexOptions.None, "Type1", "Namespace1", ispublic: true), "123abcd123", "123abed123"),
(new RegexCompilationInfo("(a|b|cde)+", RegexOptions.None, "Type2", "Namespace2.Sub", ispublic: true), "abcde", "cd"),
];
RemoteExecutor.Invoke(() =>
{
(RegexCompilationInfo rci, string validInput, string invalidInput)[] regexes =
[
(new RegexCompilationInfo("abcd", RegexOptions.None, "Type1", "Namespace1", ispublic: true), "123abcd123", "123abed123"),
(new RegexCompilationInfo("(a|b|cde)+", RegexOptions.None, "Type2", "Namespace2.Sub", ispublic: true), "abcde", "cd"),
];
string assemblyName = Path.GetRandomFileName();
string assemblyName = Path.GetRandomFileName();
string cwd = Environment.CurrentDirectory;
Environment.CurrentDirectory = TestDirectory;
try
{
Regex.CompileToAssembly(regexes.Select(r => r.rci).ToArray(), new AssemblyName(assemblyName));
}
finally
{
Environment.CurrentDirectory = cwd;
}
string cwd = Environment.CurrentDirectory;
Environment.CurrentDirectory = TestDirectory;
try
{
Regex.CompileToAssembly(regexes.Select(r => r.rci).ToArray(), new AssemblyName(assemblyName));
}
finally
{
Environment.CurrentDirectory = cwd;
}
string assemblyPath = Path.Combine(TestDirectory, assemblyName + ".dll");
Assert.True(File.Exists(assemblyPath));
string assemblyPath = Path.Combine(TestDirectory, assemblyName + ".dll");
Assert.True(File.Exists(assemblyPath));
// Uncomment to save the assembly to the desktop for inspection:
// File.Copy(assemblyPath, Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.DesktopDirectory), Path.GetFileName(assemblyPath)));
}).Dispose();
// Uncomment to save the assembly to the desktop for inspection:
// File.Copy(assemblyPath, Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.DesktopDirectory), Path.GetFileName(assemblyPath)));
}).Dispose();
}
}
}
}
5 changes: 5 additions & 0 deletions src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/Regex.Match.Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -698,6 +698,11 @@ public static IEnumerable<object[]> Match_MemberData()
yield return (@"(...)(?(1)\w*|\s*)[a1 ]", "zabcaaaaaaa", RegexOptions.RightToLeft, 0, 11, true, "aaaa");
yield return (@"(...)(?(1)\w*|\s*)[a1 ]", "---- ", RegexOptions.RightToLeft, 0, 11, true, "--- ");
yield return (@"(aaa)(?(1)aaa|b?)*", "aaaaaa", RegexOptions.None, 0, 6, true, "aaaaaa");

yield return (@"AAB|AAC", "AABAACD", RegexOptions.RightToLeft, 0, 6, true, "AAC");
yield return (@"AAB|AA\d", "AABAACD", RegexOptions.RightToLeft, 0, 6, true, "AAB");
yield return (@"(AB){3,}", "1234ABABABAB5678", RegexOptions.RightToLeft, 0, 16, true, "ABABABAB");
yield return (@"(AB){1,3}", "1234ABABABAB5678", RegexOptions.RightToLeft, 0, 16, true, "ABABAB");
}

// Character Class Subtraction
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Text.RegularExpressions/tests/UnitTests/RegexFindOptimizationsTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ public void TrailingAnchor(string pattern, int options, int expectedMode, int ex
[InlineData(@"(ab){2,4}(de){4,}", 0, (int)FindNextStartingPositionMode.LeadingString_LeftToRight, "abab")]
[InlineData(@"(ab){2,4}(de){4,}", (int)RegexOptions.RightToLeft, (int)FindNextStartingPositionMode.LeadingString_RightToLeft, "de")]
[InlineData(@"ab|(abc)|(abcd)", 0, (int)FindNextStartingPositionMode.LeadingString_LeftToRight, "ab")]
[InlineData(@"ab|(abc)|(abcd)", (int)RegexOptions.RightToLeft, (int)FindNextStartingPositionMode.LeadingString_RightToLeft, "ab")]
[InlineData(@"ab(?=cd)", 0, (int)FindNextStartingPositionMode.LeadingString_LeftToRight, "ab")]
[InlineData(@"ab(?=cd)", (int)RegexOptions.RightToLeft, (int)FindNextStartingPositionMode.LeadingString_RightToLeft, "ab")]
[InlineData(@"\bab(?=\w)(?!=\d)c\b", 0, (int)FindNextStartingPositionMode.LeadingString_LeftToRight, "abc")]
Expand All @@ -110,6 +109,7 @@ public void LeadingPrefix(string pattern, int options, int expectedMode, string
[InlineData(@"a", (int)RegexOptions.IgnoreCase | (int)RegexOptions.RightToLeft, (int)FindNextStartingPositionMode.LeadingSet_RightToLeft, "Aa")]
[InlineData(@"ab|cd|ef|gh", (int)RegexOptions.RightToLeft, (int)FindNextStartingPositionMode.LeadingSet_RightToLeft, "bdfh")]
[InlineData(@"\bab(?=\w)(?!=\d)c\b", (int)(RegexOptions.IgnoreCase | RegexOptions.RightToLeft), (int)FindNextStartingPositionMode.LeadingSet_RightToLeft, "Cc")]
[InlineData(@"ab|(abc)|(abcd)", (int)RegexOptions.RightToLeft, (int)FindNextStartingPositionMode.LeadingSet_RightToLeft, "bcd")]
public void LeadingSet(string pattern, int options, int expectedMode, string expectedChars)
{
RegexFindOptimizations opts = ComputeOptimizations(pattern, (RegexOptions)options);
Expand Down

0 comments on commit 2974608

Please sign in to comment.