Change matching option scoping behavior to match PCRE

Previously we would always parse a
"change matching option" sequence as a group, and
for the isolated syntax e.g `(?x)`, we would have
it implicitly wrap everything after in the same
group by having it do a recursive parse.

This matched the Oniguruma behavior for such
isolated groups, and was easy to implement, but
its behavior is quite unintuitive when it comes
to alternations, as e.g `a(?x)b|c` becomes
`a(?x:b|c)`, which may not be expected.

Instead, let's follow PCRE's behavior by having
such isolated cases affect the syntax options for
the remainder of the current group, including
across alternation branches. This is done by
lexing such cases as atoms (as they aren't really
group-like anymore), and having them change the
syntax options when we encounter them. The existing
scoping rules around groups take care of resetting
the options when we exit the scope.

Author: hamishknight

Sources/_RegexParser/Regex/AST/Atom.swift

@@ -72,6 +72,9 @@ extension AST {
 
       // (*ACCEPT), (*FAIL), ...
       case backtrackingDirective(BacktrackingDirective)
+
+      // (?i), (?i-m), ...
+      case changeMatchingOptions(MatchingOptionSequence)
     }
   }
 }
@@ -91,6 +94,7 @@ extension AST.Atom {
     case .subpattern(let v):            return v
     case .callout(let v):               return v
     case .backtrackingDirective(let v): return v
+    case .changeMatchingOptions(let v): return v
     case .any:                          return nil
     case .startOfLine:                  return nil
     case .endOfLine:                    return nil
@@ -691,7 +695,7 @@ extension AST.Atom {
       return nil
 
     case .property, .any, .startOfLine, .endOfLine, .backreference, .subpattern,
-        .callout, .backtrackingDirective:
+        .callout, .backtrackingDirective, .changeMatchingOptions:
       return nil
     }
   }
@@ -731,7 +735,7 @@ extension AST.Atom {
 
     case .property, .escaped, .any, .startOfLine, .endOfLine,
         .backreference, .subpattern, .namedCharacter, .callout,
-        .backtrackingDirective:
+        .backtrackingDirective, .changeMatchingOptions:
       return nil
     }
   }
@@ -740,6 +744,8 @@ extension AST.Atom {
     switch kind {
     case .backtrackingDirective(let b):
       return b.isQuantifiable
+    case .changeMatchingOptions:
+      return false
     // TODO: Are callouts quantifiable?
     default:
       return true

Sources/_RegexParser/Regex/AST/Group.swift

@@ -68,9 +68,7 @@ extension AST {
       case atomicScriptRun
 
       // (?iJmnsUxxxDPSWy{..}-iJmnsUxxxDPSW:)
-      // Isolated options are written as e.g (?i), and implicitly form a group
-      // containing all the following elements of the current group.
-      case changeMatchingOptions(MatchingOptionSequence, isIsolated: Bool)
+      case changeMatchingOptions(MatchingOptionSequence)
 
       // NOTE: Comments appear to be groups, but are not parsed
       // the same. They parse more like quotes, so are not
@@ -87,21 +85,6 @@ extension AST.Group.Kind {
     }
   }
 
-  /// Whether this is a group with an implicit scope, e.g isolated matching
-  /// options implicitly become parent groups for the rest of the elements in
-  /// the current group:
-  ///
-  ///      (a(?i)bc)de -> (a(?i:bc))de
-  ///
-  public var hasImplicitScope: Bool {
-    switch self {
-    case .changeMatchingOptions(_, let isIsolated):
-      return isIsolated
-    default:
-      return false
-    }
-  }
-
   /// If this is a named group, its name, `nil` otherwise.
   public var name: String? {
     switch self {

Sources/_RegexParser/Regex/Parse/LexicalAnalysis.swift

@@ -712,6 +712,22 @@ extension Source {
     return .init(caretLoc: nil, adding: adding, minusLoc: nil, removing: [])
   }
 
+  /// A matching option changing atom.
+  ///
+  ///     '(?' MatchingOptionSeq ')'
+  ///
+  mutating func lexChangeMatchingOptionAtom(
+    context: ParsingContext
+  ) throws -> AST.MatchingOptionSequence? {
+    try tryEating { src in
+      guard src.tryEat(sequence: "(?"),
+            let seq = try src.lexMatchingOptionSequence(context: context)
+      else { return nil }
+      try src.expect(")")
+      return seq
+    }
+  }
+
   /// Try to consume explicitly spelled-out PCRE2 group syntax.
   mutating func lexExplicitPCRE2GroupStart() -> AST.Group.Kind? {
     tryEating { src in
@@ -852,7 +868,7 @@ extension Source {
         // otherwise a matching option specifier. Conversely, '(?P' can be the
         // start of a matching option sequence, or a reference if it is followed
         // by '=' or '<'.
-        guard !src.shouldLexGroupLikeAtom() else { return nil }
+        guard !src.shouldLexGroupLikeAtom(context: context) else { return nil }
 
         guard src.tryEat("(") else { return nil }
         if src.tryEat("?") {
@@ -877,22 +893,13 @@ extension Source {
 
           // Matching option changing group (?iJmnsUxxxDPSWy{..}-iJmnsUxxxDPSW:).
           if let seq = try src.lexMatchingOptionSequence(context: context) {
-            if src.tryEat(":") {
-              return .changeMatchingOptions(seq, isIsolated: false)
-            }
-            // If this isn't start of an explicit group, we should have an
-            // implicit group that covers the remaining elements of the current
-            // group.
-            // TODO: This implicit scoping behavior matches Oniguruma, but PCRE
-            // also does it across alternations, which will require additional
-            // handling.
-            guard src.tryEat(")") else {
+            guard src.tryEat(":") else {
               if let next = src.peek() {
                 throw ParseError.invalidMatchingOption(next)
               }
               throw ParseError.expected(")")
             }
-            return .changeMatchingOptions(seq, isIsolated: true)
+            return .changeMatchingOptions(seq)
           }
 
           guard let next = src.peek() else {
@@ -1041,18 +1048,8 @@ extension Source {
     context: ParsingContext
   ) throws -> Located<AST.Group.Kind>? {
     try tryEating { src in
-      guard src.tryEat(sequence: "(?"),
-            let group = try src.lexGroupStart(context: context)
-      else { return nil }
-
-      // Implicitly scoped groups are not supported here.
-      guard !group.value.hasImplicitScope else {
-        throw LocatedError(
-          ParseError.unsupportedCondition("implicitly scoped group"),
-          group.location
-        )
-      }
-      return group
+      guard src.tryEat(sequence: "(?") else { return nil }
+      return try src.lexGroupStart(context: context)
     }
   }
 
@@ -1239,17 +1236,19 @@ extension Source {
     allowWholePatternRef: Bool = false, allowRecursionLevel: Bool = false
   ) throws -> AST.Reference? {
     let kind = try recordLoc { src -> AST.Reference.Kind? in
-      // Note this logic should match canLexNumberedReference.
-      if src.tryEat("+") {
-        return .relative(try src.expectNumber().value)
-      }
-      if src.tryEat("-") {
-        return .relative(try -src.expectNumber().value)
-      }
-      if let num = try src.lexNumber() {
-        return .absolute(num.value)
+      try src.tryEating { src in
+        // Note this logic should match canLexNumberedReference.
+        if src.tryEat("+"), let num = try src.lexNumber() {
+          return .relative(num.value)
+        }
+        if src.tryEat("-"), let num = try src.lexNumber() {
+          return .relative(-num.value)
+        }
+        if let num = try src.lexNumber() {
+          return .absolute(num.value)
+        }
+        return nil
       }
-      return nil
     }
     guard let kind = kind else { return nil }
     guard allowWholePatternRef || kind.value != .recurseWholePattern else {
@@ -1478,8 +1477,21 @@ extension Source {
     return src.canLexNumberedReference()
   }
 
+  private func canLexMatchingOptionsAsAtom(context: ParsingContext) -> Bool {
+    var src = self
+
+    // See if we can lex a matching option sequence that terminates in ')'. Such
+    // a sequence is an atom. If an error is thrown, there are invalid elements
+    // of the matching option sequence. In such a case, we can lex as a group
+    // and diagnose the invalid group kind.
+    guard (try? src.lexMatchingOptionSequence(context: context)) != nil else {
+      return false
+    }
+    return src.tryEat(")")
+  }
+
   /// Whether a group specifier should be lexed as an atom instead of a group.
-  private func shouldLexGroupLikeAtom() -> Bool {
+  private func shouldLexGroupLikeAtom(context: ParsingContext) -> Bool {
     var src = self
     guard src.tryEat("(") else { return false }
 
@@ -1493,6 +1505,9 @@ extension Source {
       // The start of an Oniguruma 'of-contents' callout.
       if src.tryEat("{") { return true }
 
+      // A matching option atom (?x), (?i), ...
+      if src.canLexMatchingOptionsAsAtom(context: context) { return true }
+
       return false
     }
     // The start of a backreference directive or Oniguruma named callout.
@@ -1753,13 +1768,20 @@ extension Source {
   ///
   ///     GroupLikeAtom -> GroupLikeReference | Callout | BacktrackingDirective
   ///
-  mutating func expectGroupLikeAtom() throws -> AST.Atom.Kind {
+  mutating func expectGroupLikeAtom(
+    context: ParsingContext
+  ) throws -> AST.Atom.Kind {
     try recordLoc { src in
       // References that look like groups, e.g (?R), (?1), ...
       if let ref = try src.lexGroupLikeReference() {
         return ref.value
       }
 
+      // Change matching options atom (?i), (?x-i), ...
+      if let seq = try src.lexChangeMatchingOptionAtom(context: context) {
+        return .changeMatchingOptions(seq)
+      }
+
       // (*ACCEPT), (*FAIL), (*MARK), ...
       if let b = try src.lexBacktrackingDirective() {
         return .backtrackingDirective(b)
@@ -1828,8 +1850,8 @@ extension Source {
 
       // If we have group syntax that was skipped over in lexGroupStart, we
       // need to handle it as an atom, or throw an error.
-      if !customCC && src.shouldLexGroupLikeAtom() {
-        return try src.expectGroupLikeAtom()
+      if !customCC && src.shouldLexGroupLikeAtom(context: context) {
+        return try src.expectGroupLikeAtom(context: context)
       }
 
       // A quantifier here is invalid.

Sources/_RegexParser/Regex/Parse/Parse.swift

@@ -282,42 +282,53 @@ extension Parser {
       loc(start)))
   }
 
+  /// Apply the syntax options of a given matching option sequence to the
+  /// current set of options.
+  private mutating func applySyntaxOptions(
+    of opts: AST.MatchingOptionSequence
+  ) {
+    // We skip this for multi-line, as extended syntax is always enabled there.
+    if context.syntax.contains(.multilineExtendedSyntax) { return }
+
+    // Check if we're introducing or removing extended syntax.
+    // TODO: PCRE differentiates between (?x) and (?xx) where only the latter
+    // handles non-semantic whitespace in a custom character class. Other
+    // engines such as Oniguruma, Java, and ICU do this under (?x). Therefore,
+    // treat (?x) and (?xx) as the same option here. If we ever get a strict
+    // PCRE mode, we will need to change this to handle that.
+    if opts.resetsCurrentOptions {
+      context.syntax.remove(.extendedSyntax)
+    }
+    if opts.adding.contains(where: \.isAnyExtended) {
+      context.syntax.insert(.extendedSyntax)
+    }
+    if opts.removing.contains(where: \.isAnyExtended) {
+      context.syntax.remove(.extendedSyntax)
+    }
+  }
+
+  /// Apply the syntax options of a matching option changing group to the
+  /// current set of options.
+  private mutating func applySyntaxOptions(of group: AST.Group.Kind) {
+    if case .changeMatchingOptions(let seq) = group {
+      applySyntaxOptions(of: seq)
+    }
+  }
+
   /// Perform a recursive parse for the body of a group.
   mutating func parseGroupBody(
     start: Source.Position, _ kind: AST.Located<AST.Group.Kind>
   ) throws -> AST.Group {
     context.recordGroup(kind.value)
 
-    // Check if we're introducing or removing extended syntax. We skip this for
-    // multi-line, as extended syntax is always enabled there.
-    // TODO: PCRE differentiates between (?x) and (?xx) where only the latter
-    // handles non-semantic whitespace in a custom character class. Other
-    // engines such as Oniguruma, Java, and ICU do this under (?x). Therefore,
-    // treat (?x) and (?xx) as the same option here. If we ever get a strict
-    // PCRE mode, we will need to change this to handle that.
     let currentSyntax = context.syntax
-    if !context.syntax.contains(.multilineExtendedSyntax) {
-      if case .changeMatchingOptions(let c, isIsolated: _) = kind.value {
-        if c.resetsCurrentOptions {
-          context.syntax.remove(.extendedSyntax)
-        }
-        if c.adding.contains(where: \.isAnyExtended) {
-          context.syntax.insert(.extendedSyntax)
-        }
-        if c.removing.contains(where: \.isAnyExtended) {
-          context.syntax.remove(.extendedSyntax)
-        }
-      }
-    }
+    applySyntaxOptions(of: kind.value)
     defer {
       context.syntax = currentSyntax
     }
 
     let child = try parseNode()
-    // An implicit scoped group has already consumed its closing paren.
-    if !kind.value.hasImplicitScope {
-      try source.expect(")")
-    }
+    try source.expect(")")
     return .init(kind, child, loc(start))
   }
 
@@ -409,6 +420,11 @@ extension Parser {
     }
 
     if let atom = try source.lexAtom(context: context) {
+      // If we have a change matching options atom, apply the syntax options. We
+      // already take care of scoping syntax options within a group.
+      if case .changeMatchingOptions(let opts) = atom.kind {
+        applySyntaxOptions(of: opts)
+      }
       // TODO: track source locations
       return .atom(atom)
     }

Sources/_RegexParser/Regex/Printing/DumpAST.swift

@@ -156,6 +156,9 @@ extension AST.Atom {
 
     case .backtrackingDirective(let d): return "\(d)"
 
+    case .changeMatchingOptions(let opts):
+      return "changeMatchingOptions<\(opts)>"
+
     case .char, .scalar:
       fatalError("Unreachable")
     }
@@ -225,22 +228,21 @@ extension AST.Reference: _ASTPrintable {
 extension AST.Group.Kind: _ASTPrintable {
   public var _dumpBase: String {
     switch self {
-    case .capture:                return "capture"
-    case .namedCapture(let s):    return "capture<\(s.value)>"
-    case .balancedCapture(let b): return "balanced capture \(b)"
-    case .nonCapture:             return "nonCapture"
-    case .nonCaptureReset:        return "nonCaptureReset"
-    case .atomicNonCapturing:     return "atomicNonCapturing"
-    case .lookahead:              return "lookahead"
-    case .negativeLookahead:      return "negativeLookahead"
-    case .nonAtomicLookahead:     return "nonAtomicLookahead"
-    case .lookbehind:             return "lookbehind"
-    case .negativeLookbehind:     return "negativeLookbehind"
-    case .nonAtomicLookbehind:    return "nonAtomicLookbehind"
-    case .scriptRun:              return "scriptRun"
-    case .atomicScriptRun:        return "atomicScriptRun"
-    case .changeMatchingOptions(let seq, let isIsolated):
-      return "changeMatchingOptions<\(seq), \(isIsolated)>"
+    case .capture:                        return "capture"
+    case .namedCapture(let s):            return "capture<\(s.value)>"
+    case .balancedCapture(let b):         return "balanced capture \(b)"
+    case .nonCapture:                     return "nonCapture"
+    case .nonCaptureReset:                return "nonCaptureReset"
+    case .atomicNonCapturing:             return "atomicNonCapturing"
+    case .lookahead:                      return "lookahead"
+    case .negativeLookahead:              return "negativeLookahead"
+    case .nonAtomicLookahead:             return "nonAtomicLookahead"
+    case .lookbehind:                     return "lookbehind"
+    case .negativeLookbehind:             return "negativeLookbehind"
+    case .nonAtomicLookbehind:            return "nonAtomicLookbehind"
+    case .scriptRun:                      return "scriptRun"
+    case .atomicScriptRun:                return "atomicScriptRun"
+    case .changeMatchingOptions(let seq): return "changeMatchingOptions<\(seq)>"
     }
   }
 }