Fix source generated regex compilation failure due to mismatched notion of atomic...

During and post-parsing, we apply various optimizations to the regex node tree, in particular trying to annotate as much as possible as atomic in order to eliminate unnecessary backtracking.  Then later when RegexCompiler and the source generator view the final tree, they also compute for every node whether a child may backtrack, as doing so enables avoiding unnecessary backtracking-related code generation if the child is known to not backtrack (e.g. because it's now marked as atomic).  However, things can go awry if the compiler / source generator's view of what's atomic differs from what's actually generated.  Because of how optimizations are applied to the node tree, it's possible for a late optimization to make a transformation that then would enable a node to be made atomic, but we don't run that phase of the optimizer again, and thus the node is left non-atomic.  Then the source generator comes along, does its analysis, and sees that the node should be treated as atomic.  That leads to problems, because the node itself will have unnecessary backtracking code generated but the parent will rightly assume there wasn't anyway and won't generate the code necessary to compensate for it, or alternatively will generate code that causes problems (e.g. the source generator uses this information to determine whether it can output scopes).

Our outer loop tests that source gen our full regex corpus caught a case where this was happening.  A couple fixes, either of which on their own is sufficient to address this particular case, but each of which also brings other benefits:
1. When rendering a single-char loop, it consults the computed atomicity table to determine whether the rest of the source generation views it as atomic.  If it does, it instead does an atomic rendering.
2. When we do our ending backtracking elimination pass (i.e. walking down the right-hand side of atomic nodes to make anything that ends them also be atomic), we should also recur into lookarounds.

This also removes some duplicated code for reducing lookarounds, and renames some stale method names.

diff --git a/src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs b/src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs
index 19aba3c..053cbcf 100644 (file)
--- a/src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs
+++ b/src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs
@@ -2385,6 +2385,13 @@ namespace System.Text.RegularExpressions.Generator
             {
                 Debug.Assert(node.Kind is RegexNodeKind.Oneloop or RegexNodeKind.Notoneloop or RegexNodeKind.Setloop, $"Unexpected type: {node.Kind}");
 
+                // If this is actually atomic based on its parent, emit it as atomic instead; no backtracking necessary.
+                if (analysis.IsAtomicByAncestor(node))
+                {
+                    EmitSingleCharAtomicLoop(node);
+                    return;
+                }
+
                 // If this is actually a repeater, emit that instead; no backtracking necessary.
                 if (node.M == node.N)
                 {

diff --git a/src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexCompiler.cs b/src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexCompiler.cs
index b5669ed..765be58 100644 (file)
--- a/src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexCompiler.cs
+++ b/src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexCompiler.cs
@@ -2543,6 +2543,13 @@ namespace System.Text.RegularExpressions
             {
                 Debug.Assert(node.Kind is RegexNodeKind.Oneloop or RegexNodeKind.Notoneloop or RegexNodeKind.Setloop, $"Unexpected type: {node.Kind}");
 
+                // If this is actually atomic based on its parent, emit it as atomic instead; no backtracking necessary.
+                if (analysis.IsAtomicByAncestor(node))
+                {
+                    EmitSingleCharAtomicLoop(node);
+                    return;
+                }
+
                 // If this is actually a repeater, emit that instead; no backtracking necessary.
                 if (node.M == node.N)
                 {

diff --git a/src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexNode.cs b/src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexNode.cs
index e2bb2f4..70d3650 100644 (file)
--- a/src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexNode.cs
+++ b/src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexNode.cs
@@ -414,8 +414,10 @@ namespace System.Text.RegularExpressions
                         break;
 
                     // Just because a particular node is atomic doesn't mean all its descendants are.
-                    // Process them as well.
+                    // Process them as well. Lookarounds are implicitly atomic.
                     case RegexNodeKind.Atomic:
+                    case RegexNodeKind.PositiveLookaround:
+                    case RegexNodeKind.NegativeLookaround:
                         node = node.Child(0);
                         continue;
 
@@ -451,7 +453,7 @@ namespace System.Text.RegularExpressions
                                 node.Child(i).EliminateEndingBacktracking();
                             }
 
-                            if (node.Kind != RegexNodeKind.ExpressionConditional) // ReduceTestgroup will have already applied ending backtracking removal
+                            if (node.Kind != RegexNodeKind.ExpressionConditional) // ReduceExpressionConditional will have already applied ending backtracking removal
                             {
                                 node = node.Child(0);
                                 continue;
@@ -526,11 +528,10 @@ namespace System.Text.RegularExpressions
                 RegexNodeKind.Concatenate => ReduceConcatenation(),
                 RegexNodeKind.Group => ReduceGroup(),
                 RegexNodeKind.Loop or RegexNodeKind.Lazyloop => ReduceLoops(),
-                RegexNodeKind.NegativeLookaround => ReducePrevent(),
-                RegexNodeKind.PositiveLookaround => ReduceRequire(),
+                RegexNodeKind.PositiveLookaround or RegexNodeKind.NegativeLookaround => ReduceLookaround(),
                 RegexNodeKind.Set or RegexNodeKind.Setloop or RegexNodeKind.Setloopatomic or RegexNodeKind.Setlazy => ReduceSet(),
-                RegexNodeKind.ExpressionConditional => ReduceTestgroup(),
-                RegexNodeKind.BackreferenceConditional => ReduceTestref(),
+                RegexNodeKind.ExpressionConditional => ReduceExpressionConditional(),
+                RegexNodeKind.BackreferenceConditional => ReduceBackreferenceConditional(),
                 _ => this,
             };
         }
@@ -1800,7 +1801,7 @@ namespace System.Text.RegularExpressions
                             // can be made atomic.  Then if we do end up backtracking into the alternation,
                             // we at least won't need to backtrack into that loop.  The same is true for
                             // conditionals, though we don't want to process the condition expression
-                            // itself, as it's already considered atomic and handled as part of ReduceTestgroup.
+                            // itself, as it's already considered atomic and handled as part of ReduceExpressionConditional.
                             {
                                 int alternateBranches = node.ChildCount();
                                 for (int b = node.Kind == RegexNodeKind.ExpressionConditional ? 1 : 0; b < alternateBranches; b++)
@@ -1854,41 +1855,28 @@ namespace System.Text.RegularExpressions
             return null;
         }
 
-        /// <summary>Optimizations for positive lookaheads/behinds.</summary>
-        private RegexNode ReduceRequire()
+        /// <summary>Optimizations for positive and negative lookaheads/behinds.</summary>
+        private RegexNode ReduceLookaround()
         {
-            Debug.Assert(Kind == RegexNodeKind.PositiveLookaround);
+            Debug.Assert(Kind is RegexNodeKind.PositiveLookaround or RegexNodeKind.NegativeLookaround);
             Debug.Assert(ChildCount() == 1);
 
-            // A positive lookaround is a zero-width atomic assertion.
+            // A lookaround is a zero-width atomic assertion.
             // As it's atomic, nothing will backtrack into it, and we can
             // eliminate any ending backtracking from it.
             EliminateEndingBacktracking();
 
-            // A positive lookaround wrapped around an empty is a nop, and can just
-            // be made into an empty.  A developer typically doesn't write this, but
-            // rather it evolves due to optimizations resulting in empty.
-            if (Child(0).Kind == RegexNodeKind.Empty)
-            {
-                Kind = RegexNodeKind.Empty;
-                Children = null;
-            }
-
-            return this;
-        }
-
-        /// <summary>Optimizations for negative lookaheads/behinds.</summary>
-        private RegexNode ReducePrevent()
-        {
-            Debug.Assert(Kind == RegexNodeKind.NegativeLookaround);
-            Debug.Assert(ChildCount() == 1);
+            // A positive lookaround wrapped around an empty is a nop, and we can reduce it
+            // to simply Empty.  A developer typically doesn't write this, but rather it evolves
+            // due to optimizations resulting in empty.
 
             // A negative lookaround wrapped around an empty child, i.e. (?!), is
-            // sometimes used as a way to insert a guaranteed no-match into the expression.
-            // We can reduce it to simply Nothing.
+            // sometimes used as a way to insert a guaranteed no-match into the expression,
+            // often as part of a conditional. We can reduce it to simply Nothing.
+
             if (Child(0).Kind == RegexNodeKind.Empty)
             {
-                Kind = RegexNodeKind.Nothing;
+                Kind = Kind == RegexNodeKind.PositiveLookaround ? RegexNodeKind.Empty : RegexNodeKind.Nothing;
                 Children = null;
             }
 
@@ -1896,13 +1884,13 @@ namespace System.Text.RegularExpressions
         }
 
         /// <summary>Optimizations for backreference conditionals.</summary>
-        private RegexNode ReduceTestref()
+        private RegexNode ReduceBackreferenceConditional()
         {
             Debug.Assert(Kind == RegexNodeKind.BackreferenceConditional);
             Debug.Assert(ChildCount() is 1 or 2);
 
-            // This isn't so much an optimization as it is changing the tree for consistency.
-            // We want all engines to be able to trust that every Testref will have two children,
+            // This isn't so much an optimization as it is changing the tree for consistency. We want
+            // all engines to be able to trust that every backreference conditional will have two children,
             // even though it's optional in the syntax.  If it's missing a "not matched" branch,
             // we add one that will match empty.
             if (ChildCount() == 1)
@@ -1914,13 +1902,13 @@ namespace System.Text.RegularExpressions
         }
 
         /// <summary>Optimizations for expression conditionals.</summary>
-        private RegexNode ReduceTestgroup()
+        private RegexNode ReduceExpressionConditional()
         {
             Debug.Assert(Kind == RegexNodeKind.ExpressionConditional);
             Debug.Assert(ChildCount() is 2 or 3);
 
-            // This isn't so much an optimization as it is changing the tree for consistency.
-            // We want all engines to be able to trust that every Testgroup will have three children,
+            // This isn't so much an optimization as it is changing the tree for consistency. We want
+            // all engines to be able to trust that every expression conditional will have three children,
             // even though it's optional in the syntax.  If it's missing a "not matched" branch,
             // we add one that will match empty.
             if (ChildCount() == 2)