Fixes for NonBacktracking with some combinations of loops and anchors (#80880)

* Add tests for loop plus anchors bug

* Fixes to loop derivative and pruning

* Add a test for high-priority nullable loop bodies

This is a simplified test case for a breaking test in the Rust patterns.

* Fix for high-priority nullable loop bodies

Author: olsaarik

src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/SymbolicRegexNode.cs

@@ -1111,26 +1111,32 @@ private SymbolicRegexNode<TSet> PruneLowerPriorityThanNullability(SymbolicRegexB
 
                 case SymbolicRegexNodeKind.Concat:
                     Debug.Assert(_left is not null && _right is not null);
-                    //in a concatenation (X|Y)Z priority is given to XZ when X is nullable
-                    //observe that (X|Y)Z is equivalent to (XZ|YZ) and the branch YZ must be ignored
-                    //when X is nullable (observe that XZ is nullable because this=(X|Y)Z is nullable)
-                    //if X is not nullable then XZ is maintaned as is, and YZ is pruned
-                    //e.g. (a{0,5}?|abc|b+)c* reduces to c*
-                    //---
-                    //in a concatenation XZ where X is not an alternation, both X and Z are pruned
-                    //e.g. a{0,5}?b{0,5}? reduces to ()
-                    prunedNode = _left._kind == SymbolicRegexNodeKind.Alternate ?
-                        (_left._left!.IsNullableFor(context) ?
+                    prunedNode = _left._kind switch
+                    {
+                        // If the left side is a concatenation, then we bring it to right associative form to expose
+                        // the first element of the concatenation for the cases below.
+                        SymbolicRegexNodeKind.Concat => CreateConcat(builder, _left._left!, CreateConcat(builder, _left._right!, _right))
+                            .PruneLowerPriorityThanNullability(builder, context),
+                        // In a concatenation (X|Y)Z priority is given to XZ when X is nullable.
+                        // Observe that (X|Y)Z is equivalent to (XZ|YZ) and the branch YZ must be ignored
+                        // when X is nullable (observe that XZ is nullable because this=(X|Y)Z is nullable).
+                        // If X is not nullable then XZ is maintaned as is, and YZ is pruned. For example,
+                        // (a{0,5}?|abc|b+)c* reduces to c*.
+                        SymbolicRegexNodeKind.Alternate => (_left._left!.IsNullableFor(context) ?
                             CreateConcat(builder, _left._left, _right).PruneLowerPriorityThanNullability(builder, context) :
-                            CreateAlternate(builder, CreateConcat(builder, _left._left, _right), CreateConcat(builder, _left._right!, _right).PruneLowerPriorityThanNullability(builder, context))) :
-                        CreateConcat(builder, _left.PruneLowerPriorityThanNullability(builder, context), _right.PruneLowerPriorityThanNullability(builder, context));
+                            CreateAlternate(builder, CreateConcat(builder, _left._left, _right),
+                                CreateConcat(builder, _left._right!, _right).PruneLowerPriorityThanNullability(builder, context), deduplicated: true)),
+                        // Loops have various cases that are handled uniformly for concatenations and standalone loops.
+                        SymbolicRegexNodeKind.Loop => PruneLoop(builder, context, _left, _right),
+                        // The previous cases handle all the ways that the left side of the concatenation could
+                        // contain branching. Thus if we get here it is safe to only prune the right side.
+                        _ => CreateConcat(builder, _left, _right.PruneLowerPriorityThanNullability(builder, context)),
+                    };
                     break;
 
                 case SymbolicRegexNodeKind.Loop:
-                    Debug.Assert(_left is not null);
-                    // Lazy nullable loop reduces to (), i.e., the loop body is just forgotten
-                    prunedNode = _info.IsLazyLoop && _lower == 0 ? builder.Epsilon :
-                        CreateLoop(builder, _left.PruneLowerPriorityThanNullability(builder, context), _lower, _upper, _info.IsLazyLoop);
+                    // Loops have various cases that are handled uniformly for standalone loops and concatenations.
+                    prunedNode = PruneLoop(builder, context, this, builder.Epsilon);
                     break;
 
                 case SymbolicRegexNodeKind.Effect:
@@ -1147,6 +1153,73 @@ private SymbolicRegexNode<TSet> PruneLowerPriorityThanNullability(SymbolicRegexB
 
             builder._pruneLowerPriorityThanNullabilityCache[key] = prunedNode;
             return prunedNode;
+
+            static SymbolicRegexNode<TSet> PruneLoop(SymbolicRegexBuilder<TSet> builder, uint context, SymbolicRegexNode<TSet> loop, SymbolicRegexNode<TSet> tail)
+            {
+                Debug.Assert(loop.Kind == SymbolicRegexNodeKind.Loop && loop._left is not null);
+
+                if (loop._lower == 0)
+                {
+                    // Loop is nullable at least due to a zero lower bound and the cases below handle the different
+                    // priorities of checking that lower bound.
+                    if (loop.IsLazy)
+                    {
+                        // In a lazy loop nullability from the zero lower bound has highest priority, so the loop is skipped completely
+                        return tail.PruneLowerPriorityThanNullability(builder, context);
+                    }
+                    else if (!loop._left.IsNullableFor(context))
+                    {
+                        // For an eager loop with a non-nullable body, the nullability from the zero lower bound has lowest priority.
+                        // Handle by case splitting into (1) doing at least one iteration in the loop and (2) skipping the loop and
+                        // continuing directly into the tail, which then must be pruned in the current context.
+                        return CreateAlternate(builder,
+                            CreateConcat(builder, CreateLoop(builder, loop._left, 1, loop._upper, loop.IsLazy), tail),
+                            tail.PruneLowerPriorityThanNullability(builder, context));
+                    }
+                    else if (loop._upper == int.MaxValue)
+                    {
+                        // The special case of a R*S loop with a nullable body is handled separately, as the general case handler could cause
+                        // infinite recursion. The paths that backtracking would explore before stopping here are (1) consuming a character in the
+                        // first iteration of the loop and (2) skipping the loop and continuing with anything higher priority than nullability in S.
+                        // For example, a pattern (a|b??)*c? would prune into essentially a?(a|b??)*c?|c?. Note that pruning only one peeled-out
+                        // iteration of the loop is necessary, since anchors could change the priority of nullability in other locations in the input.
+                        // Additionally, a high-priority nullable body will always be skipped, in which case only option (2) is included. Finally, in
+                        // all cases the loop body must not be dropped, as doing so could affect whether some capture groups match zero characters or
+                        // don't match at all.
+                        SymbolicRegexNode<TSet> skipLoopCase = CreateConcat(builder, loop._left.PruneLowerPriorityThanNullability(builder, context),
+                                tail.PruneLowerPriorityThanNullability(builder, context));
+                        return loop._left.IsHighPriorityNullableFor(context) ? skipLoopCase : CreateAlternate(builder,
+                            CreateConcat(builder, loop._left.PruneLowerPriorityThanNullability(builder, context), CreateConcat(builder, loop, tail)),
+                            skipLoopCase);
+                    }
+                    // For an eager loop with finite upper bound and nullable body fall back to the general case handler
+                }
+
+                Debug.Assert(loop._left.IsNullableFor(context));
+                // The general case handler peels one iteration out of the loop and prunes the resulting concatenation
+                return CreateConcat(builder, loop._left, CreateConcat(builder, loop.CreateLoopContinuation(builder), tail))
+                    .PruneLowerPriorityThanNullability(builder, context);
+            }
+        }
+
+        /// <summary>
+        /// Creates the remainder of a loop when one iteration is peeled out of it. In other words, for a loop R with
+        /// a body B returns a new regex S such that R is equivalent to BS.
+        /// </summary>
+        /// <param name="builder">the builder that owns this node</param>
+        /// <returns>the loop continuation regex</returns>
+        private SymbolicRegexNode<TSet> CreateLoopContinuation(SymbolicRegexBuilder<TSet> builder)
+        {
+            Debug.Assert(_kind == SymbolicRegexNodeKind.Loop && _left is not null);
+
+            // Note that the upper bound is guaranteed to be greater than zero, since otherwise the loop would have
+            // been simplified to nothing, and int.MaxValue is treated as infinity.
+            int newupper = _upper == int.MaxValue ? int.MaxValue : _upper - 1;
+            // Do not decrement the lower bound if it equals int.MaxValue
+            int newlower = _lower == 0 || _lower == int.MaxValue ? _lower : _lower - 1;
+
+            // The continued loop becomes epsilon when newlower == newupper == 0
+            return builder.CreateLoop(_left, IsLazy, newlower, newupper);
         }
 
         /// <summary>
@@ -1244,22 +1317,18 @@ private SymbolicRegexNode<TSet> CreateDerivative(SymbolicRegexBuilder<TSet> buil
                         Debug.Assert(_left is not null);
                         Debug.Assert(_upper > 0);
 
-                        SymbolicRegexNode<TSet> bodyDerivative = _left.CreateDerivative(builder, elem, context);
-                        if (bodyDerivative.IsNothing(builder._solver))
+                        if (_lower == 0 || _left.IsNullable || !_left.IsNullableFor(context))
                         {
-                            derivative = builder._nothing;
+                            // In these special cases the derivative concatenates the body's derivative with the decremented loop, so
+                            // d(R{m,n}) = d(R)R{max(0,m-1),n-1}.
+                            derivative = builder.CreateConcat(_left.CreateDerivative(builder, elem, context), CreateLoopContinuation(builder));
                         }
                         else
                         {
-                            // The loop derivative peels out one iteration and concatenates the body's derivative with the decremented loop,
-                            // so d(R{m,n}) = d(R)R{max(0,m-1),n-1}. Note that n is guaranteed to be greater than zero, since otherwise the
-                            // loop would have been simplified to nothing, and int.MaxValue is treated as infinity.
-                            int newupper = _upper == int.MaxValue ? int.MaxValue : _upper - 1;
-                            // do not decrement the lower bound if it equals int.MaxValue
-                            int newlower = _lower == 0 || _lower == int.MaxValue ? _lower : _lower - 1;
-                            // the continued loop becomes epsilon when newlower == newupper == 0
-                            // in which case the returned concatenation will be just bodyDerivative
-                            derivative = builder.CreateConcat(bodyDerivative, builder.CreateLoop(_left, IsLazy, newlower, newupper));
+                            // In the general case the concatenation must be created first and the derivative taken on that. For example,
+                            // the first derivative for (a|\b){2} with an input "ac" is (a|\b)|epsilon, but the rule above would produce
+                            // just (a|\b).
+                            derivative = builder.CreateConcat(_left, CreateLoopContinuation(builder)).CreateDerivative(builder, elem, context);
                         }
                         break;
                     }

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

@@ -960,6 +960,12 @@ public static IEnumerable<object[]> Match_MemberData()
 
             // Test for a bug in NonBacktracking's subsumption rule for XY subsuming X??Y, which didn't check that X is nullable
             yield return (@"XY|X??Y", "Y", RegexOptions.None, 0, 1, true, "Y");
+
+            // Tests for bugs in NonBacktracking, which didn't properly handle some combinations of loops and anchors
+            yield return (@"(a|\b){2}", "ac", RegexOptions.None, 0, 2, true, "a");
+            yield return (@"a?(\b|c)", "ac", RegexOptions.None, 0, 2, true, "ac");
+            yield return (@"(a|())*(\b|c)", "ac", RegexOptions.None, 0, 2, true, "ac");
+            yield return (@"(\b|a)*", "a", RegexOptions.None, 0, 1, true, "");
         }
 
         [OuterLoop("Takes several seconds to run")]