Skip to content

Commit 7d91bf5

Browse files
stephentoubstephentoub
authored
Fix incorrect suffix search for alternation with RegexOptions.RightToLeft (#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
1 parent af52e82 commit 7d91bf5

File tree

4 files changed

+53
-49
lines changed

4 files changed

+53
-49
lines changed

‎src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexPrefixAnalyzer.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ static bool Process(RegexNode node, ref ValueStringBuilder vsb)
359359
}
360360

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

‎src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/Regex.CompileToAssembly.Tests.cs

+46-47
Original file line numberDiff line numberDiff line change
@@ -14,63 +14,62 @@ namespace System.Text.RegularExpressions.Tests
1414
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
1515
public class RegexCompileToAssemblyTests : FileCleanupTestBase
1616
{
17-
public static bool IsDebug => typeof(Regex).Assembly.GetCustomAttributes(false).OfType<DebuggableAttribute>().Any(da => da.IsJITTrackingEnabled);
18-
public static bool IsRelease => !IsDebug;
19-
public static bool IsDebugAndRemoteExecutorSupported => IsDebug && RemoteExecutor.IsSupported;
20-
21-
[ConditionalFact(nameof(IsRelease))]
22-
public void CompileToAssembly_PNSE()
17+
[Fact]
18+
public void CompileToAssembly_SimpleTest()
2319
{
24-
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(null, null));
25-
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(null, null, null));
26-
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(null, null, null, null));
20+
bool isDebug = typeof(Regex).Assembly.GetCustomAttributes(false).OfType<DebuggableAttribute>().Any(da => da.IsJITTrackingEnabled);
2721

28-
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(
29-
[new RegexCompilationInfo("abcd", RegexOptions.None, "abcd", "SomeNamespace", true)],
30-
new AssemblyName("abcd")));
22+
if (!isDebug)
23+
{
24+
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(null, null));
25+
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(null, null, null));
26+
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(null, null, null, null));
3127

32-
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(
33-
[new RegexCompilationInfo("abcd", RegexOptions.None, "abcd", "SomeNamespace", true)],
34-
new AssemblyName("abcd"),
35-
[new CustomAttributeBuilder(typeof(AssemblyCompanyAttribute).GetConstructor([typeof(string)]), new[] { "TestCompany" })]));
28+
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(
29+
[new RegexCompilationInfo("abcd", RegexOptions.None, "abcd", "SomeNamespace", true)],
30+
new AssemblyName("abcd")));
3631

37-
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(
38-
[new RegexCompilationInfo("abcd", RegexOptions.None, "abcd", "SomeNamespace", true)],
39-
new AssemblyName("abcd"),
40-
[new CustomAttributeBuilder(typeof(AssemblyCompanyAttribute).GetConstructor([typeof(string)]), new[] { "TestCompany" })],
41-
"resourceFile"));
42-
}
32+
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(
33+
[new RegexCompilationInfo("abcd", RegexOptions.None, "abcd", "SomeNamespace", true)],
34+
new AssemblyName("abcd"),
35+
[new CustomAttributeBuilder(typeof(AssemblyCompanyAttribute).GetConstructor([typeof(string)]), new[] { "TestCompany" })]));
4336

44-
[ConditionalFact(nameof(IsDebugAndRemoteExecutorSupported))]
45-
public void CompileToAssembly_SimpleUseInDebug()
46-
{
47-
RemoteExecutor.Invoke(() =>
37+
Assert.Throws<PlatformNotSupportedException>(() => Regex.CompileToAssembly(
38+
[new RegexCompilationInfo("abcd", RegexOptions.None, "abcd", "SomeNamespace", true)],
39+
new AssemblyName("abcd"),
40+
[new CustomAttributeBuilder(typeof(AssemblyCompanyAttribute).GetConstructor([typeof(string)]), new[] { "TestCompany" })],
41+
"resourceFile"));
42+
}
43+
else if (RemoteExecutor.IsSupported)
4844
{
49-
(RegexCompilationInfo rci, string validInput, string invalidInput)[] regexes =
50-
[
51-
(new RegexCompilationInfo("abcd", RegexOptions.None, "Type1", "Namespace1", ispublic: true), "123abcd123", "123abed123"),
52-
(new RegexCompilationInfo("(a|b|cde)+", RegexOptions.None, "Type2", "Namespace2.Sub", ispublic: true), "abcde", "cd"),
53-
];
45+
RemoteExecutor.Invoke(() =>
46+
{
47+
(RegexCompilationInfo rci, string validInput, string invalidInput)[] regexes =
48+
[
49+
(new RegexCompilationInfo("abcd", RegexOptions.None, "Type1", "Namespace1", ispublic: true), "123abcd123", "123abed123"),
50+
(new RegexCompilationInfo("(a|b|cde)+", RegexOptions.None, "Type2", "Namespace2.Sub", ispublic: true), "abcde", "cd"),
51+
];
5452

55-
string assemblyName = Path.GetRandomFileName();
53+
string assemblyName = Path.GetRandomFileName();
5654

57-
string cwd = Environment.CurrentDirectory;
58-
Environment.CurrentDirectory = TestDirectory;
59-
try
60-
{
61-
Regex.CompileToAssembly(regexes.Select(r => r.rci).ToArray(), new AssemblyName(assemblyName));
62-
}
63-
finally
64-
{
65-
Environment.CurrentDirectory = cwd;
66-
}
55+
string cwd = Environment.CurrentDirectory;
56+
Environment.CurrentDirectory = TestDirectory;
57+
try
58+
{
59+
Regex.CompileToAssembly(regexes.Select(r => r.rci).ToArray(), new AssemblyName(assemblyName));
60+
}
61+
finally
62+
{
63+
Environment.CurrentDirectory = cwd;
64+
}
6765

68-
string assemblyPath = Path.Combine(TestDirectory, assemblyName + ".dll");
69-
Assert.True(File.Exists(assemblyPath));
66+
string assemblyPath = Path.Combine(TestDirectory, assemblyName + ".dll");
67+
Assert.True(File.Exists(assemblyPath));
7068

71-
// Uncomment to save the assembly to the desktop for inspection:
72-
// File.Copy(assemblyPath, Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.DesktopDirectory), Path.GetFileName(assemblyPath)));
73-
}).Dispose();
69+
// Uncomment to save the assembly to the desktop for inspection:
70+
// File.Copy(assemblyPath, Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.DesktopDirectory), Path.GetFileName(assemblyPath)));
71+
}).Dispose();
72+
}
7473
}
7574
}
7675
}

‎src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/Regex.Match.Tests.cs

+5
Original file line numberDiff line numberDiff line change
@@ -698,6 +698,11 @@ public static IEnumerable<object[]> Match_MemberData()
698698
yield return (@"(...)(?(1)\w*|\s*)[a1 ]", "zabcaaaaaaa", RegexOptions.RightToLeft, 0, 11, true, "aaaa");
699699
yield return (@"(...)(?(1)\w*|\s*)[a1 ]", "---- ", RegexOptions.RightToLeft, 0, 11, true, "--- ");
700700
yield return (@"(aaa)(?(1)aaa|b?)*", "aaaaaa", RegexOptions.None, 0, 6, true, "aaaaaa");
701+
702+
yield return (@"AAB|AAC", "AABAACD", RegexOptions.RightToLeft, 0, 6, true, "AAC");
703+
yield return (@"AAB|AA\d", "AABAACD", RegexOptions.RightToLeft, 0, 6, true, "AAB");
704+
yield return (@"(AB){3,}", "1234ABABABAB5678", RegexOptions.RightToLeft, 0, 16, true, "ABABABAB");
705+
yield return (@"(AB){1,3}", "1234ABABABAB5678", RegexOptions.RightToLeft, 0, 16, true, "ABABAB");
701706
}
702707

703708
// Character Class Subtraction

‎src/libraries/System.Text.RegularExpressions/tests/UnitTests/RegexFindOptimizationsTests.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,6 @@ public void TrailingAnchor(string pattern, int options, int expectedMode, int ex
8888
[InlineData(@"(ab){2,4}(de){4,}", 0, (int)FindNextStartingPositionMode.LeadingString_LeftToRight, "abab")]
8989
[InlineData(@"(ab){2,4}(de){4,}", (int)RegexOptions.RightToLeft, (int)FindNextStartingPositionMode.LeadingString_RightToLeft, "de")]
9090
[InlineData(@"ab|(abc)|(abcd)", 0, (int)FindNextStartingPositionMode.LeadingString_LeftToRight, "ab")]
91-
[InlineData(@"ab|(abc)|(abcd)", (int)RegexOptions.RightToLeft, (int)FindNextStartingPositionMode.LeadingString_RightToLeft, "ab")]
9291
[InlineData(@"ab(?=cd)", 0, (int)FindNextStartingPositionMode.LeadingString_LeftToRight, "ab")]
9392
[InlineData(@"ab(?=cd)", (int)RegexOptions.RightToLeft, (int)FindNextStartingPositionMode.LeadingString_RightToLeft, "ab")]
9493
[InlineData(@"\bab(?=\w)(?!=\d)c\b", 0, (int)FindNextStartingPositionMode.LeadingString_LeftToRight, "abc")]
@@ -110,6 +109,7 @@ public void LeadingPrefix(string pattern, int options, int expectedMode, string
110109
[InlineData(@"a", (int)RegexOptions.IgnoreCase | (int)RegexOptions.RightToLeft, (int)FindNextStartingPositionMode.LeadingSet_RightToLeft, "Aa")]
111110
[InlineData(@"ab|cd|ef|gh", (int)RegexOptions.RightToLeft, (int)FindNextStartingPositionMode.LeadingSet_RightToLeft, "bdfh")]
112111
[InlineData(@"\bab(?=\w)(?!=\d)c\b", (int)(RegexOptions.IgnoreCase | RegexOptions.RightToLeft), (int)FindNextStartingPositionMode.LeadingSet_RightToLeft, "Cc")]
112+
[InlineData(@"ab|(abc)|(abcd)", (int)RegexOptions.RightToLeft, (int)FindNextStartingPositionMode.LeadingSet_RightToLeft, "bcd")]
113113
public void LeadingSet(string pattern, int options, int expectedMode, string expectedChars)
114114
{
115115
RegexFindOptimizations opts = ComputeOptimizations(pattern, (RegexOptions)options);

0 commit comments

Comments
 (0)