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
@@ -683,7 +687,7 @@ extension AST.Atom {
       return nil
 
     case .property, .any, .startOfLine, .endOfLine, .backreference, .subpattern,
-        .callout, .backtrackingDirective:
+        .callout, .backtrackingDirective, .changeMatchingOptions:
       return nil
     }
   }
@@ -723,7 +727,7 @@ extension AST.Atom {
 
     case .property, .escaped, .any, .startOfLine, .endOfLine,
         .backreference, .subpattern, .namedCharacter, .callout,
-        .backtrackingDirective:
+        .backtrackingDirective, .changeMatchingOptions:
       return nil
     }
   }
@@ -732,6 +736,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

@@ -706,6 +706,21 @@ extension Source {
     return .init(caretLoc: nil, adding: adding, minusLoc: nil, removing: [])
   }
 
+  /// A matching option changing atom.
+  ///
+  ///     '(?' MatchingOptionSeq ')'
+  ///
+  mutating func lexChangeMatchingOptionAtom(
+  ) throws -> AST.MatchingOptionSequence? {
+    try tryEating { src in
+      guard src.tryEat(sequence: "(?"),
+            let seq = try src.lexMatchingOptionSequence()
+      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
@@ -871,22 +886,13 @@ extension Source {
 
           // Matching option changing group (?iJmnsUxxxDPSWy{..}-iJmnsUxxxDPSW:).
           if let seq = try src.lexMatchingOptionSequence() {
-            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 {
@@ -1035,18 +1041,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)
     }
   }
 
@@ -1177,17 +1173,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 {
@@ -1431,6 +1429,12 @@ extension Source {
       // The start of an Oniguruma 'of-contents' callout.
       if src.tryEat("{") { return true }
 
+      // See if we can lex a change-matching-option sequence that terminates in
+      // ')'. Such a sequence is an atom.
+      if (try? src.lexMatchingOptionSequence()) != nil {
+        return src.tryEat(")")
+      }
+
       return false
     }
     // The start of a backreference directive or Oniguruma named callout.
@@ -1698,6 +1702,11 @@ extension Source {
         return ref.value
       }
 
+      // Change matching options atom (?i), (?x-i), ...
+      if let seq = try src.lexChangeMatchingOptionAtom() {
+        return .changeMatchingOptions(seq)
+      }
+
       // (*ACCEPT), (*FAIL), (*MARK), ...
       if let b = try src.lexBacktrackingDirective() {
         return .backtrackingDirective(b)

Sources/_RegexParser/Regex/Parse/Parse.swift

@@ -269,39 +269,50 @@ extension Parser {
       loc(start)))
   }
 
-  /// 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)
-
+  /// Apply the syntax options of a given matching option sequence to the
+  /// current set of options.
+  private mutating func applySyntaxOptions(
+    of opts: AST.MatchingOptionSequence
+  ) {
     // 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.
-    let currentSyntax = context.syntax
-    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)
-      }
+    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)
+
+    let currentSyntax = context.syntax
+    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))
   }
 
@@ -393,6 +404,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)>"
     }
   }
 }

Sources/_StringProcessing/ByteCodeGen.swift

@@ -34,6 +34,9 @@ extension Compiler.ByteCodeGen {
     case let .symbolicReference(id):
       builder.buildUnresolvedReference(id: id)
 
+    case let .changeMatchingOptions(optionSequence):
+      options.apply(optionSequence)
+
     case let .unconverted(astAtom):
       if let consumer = try astAtom.generateConsumer(options) {
         builder.buildConsume(by: consumer)
@@ -336,7 +339,7 @@ extension Compiler.ByteCodeGen {
     case .capture, .namedCapture, .balancedCapture:
       throw Unreachable("These should produce a capture node")
 
-    case .changeMatchingOptions(let optionSequence, _):
+    case .changeMatchingOptions(let optionSequence):
       options.apply(optionSequence)
       try emitNode(child)
 
@@ -562,6 +565,9 @@ extension Compiler.ByteCodeGen {
       }
 
     case let .capture(_, refId, child):
+      options.beginScope()
+      defer { options.endScope() }
+
       let cap = builder.makeCapture(id: refId)
       switch child {
       case let .matcher(_, m):

Sources/_StringProcessing/ConsumerInterface.swift

@@ -100,6 +100,10 @@ extension DSLTree.Atom {
       // TODO: Should we handle?
       return nil
 
+    case .changeMatchingOptions:
+      // TODO: Should we handle?
+      return nil
+
     case let .unconverted(a):
       return try a.generateConsumer(opts)
     }
@@ -178,7 +182,8 @@ extension AST.Atom {
       return nil
 
     case .escaped, .keyboardControl, .keyboardMeta, .keyboardMetaControl,
-      .backreference, .subpattern, .callout, .backtrackingDirective:
+        .backreference, .subpattern, .callout, .backtrackingDirective,
+        .changeMatchingOptions:
       // FIXME: implement
       return nil
     }