Address review feedback

- Make `\h` and `\H` supported for now
- Check character class ranges
- Diagnose unquantifiable escape sequences

Author: hamishknight

Sources/_RegexParser/Regex/AST/Atom.swift

@@ -668,6 +668,23 @@ extension AST.Atom.EscapedBuiltin {
       return nil
     }
   }
+
+  public var isQuantifiable: Bool {
+    switch self {
+    case .alarm, .escape, .formfeed, .newline, .carriageReturn, .tab,
+        .singleDataUnit, .decimalDigit, .notDecimalDigit, .horizontalWhitespace,
+        .notHorizontalWhitespace, .notNewline, .newlineSequence, .whitespace,
+        .notWhitespace, .verticalTab, .notVerticalTab, .wordCharacter,
+        .notWordCharacter, .backspace, .graphemeCluster, .trueAnychar:
+      return true
+
+    case .wordBoundary, .notWordBoundary, .startOfSubject,
+        .endOfSubjectBeforeNewline, .endOfSubject,
+        .firstMatchingPositionInSubject, .resetStartOfMatch, .textSegment,
+        .notTextSegment:
+      return false
+    }
+  }
 }
 
 extension AST.Atom {
@@ -749,6 +766,8 @@ extension AST.Atom {
     case .changeMatchingOptions:
       return false
     // TODO: Are callouts quantifiable?
+    case .escaped(let esc):
+      return esc.isQuantifiable
     default:
       return true
     }

Sources/_RegexParser/Regex/Parse/Diagnostics.swift

@@ -45,7 +45,6 @@ enum ParseError: Error, Hashable {
 
   case cannotReferToWholePattern
 
-  case notQuantifiable
   case quantifierRequiresOperand(String)
 
   case backtrackingDirectiveMustHaveName(String)
@@ -83,6 +82,8 @@ enum ParseError: Error, Hashable {
   case duplicateNamedCapture(String)
   case invalidCharacterClassRangeOperand
   case invalidQuantifierRange(Int, Int)
+  case invalidCharacterRange(from: Character, to: Character)
+  case notQuantifiable
 }
 
 extension IdentifierKind {
@@ -125,8 +126,6 @@ extension ParseError: CustomStringConvertible {
       return "invalid escape sequence '\\\(c)'"
     case .cannotReferToWholePattern:
       return "cannot refer to whole pattern here"
-    case .notQuantifiable:
-      return "expression is not quantifiable"
     case .quantifierRequiresOperand(let q):
       return "quantifier '\(q)' must appear after expression"
     case .backtrackingDirectiveMustHaveName(let b):
@@ -191,6 +190,10 @@ extension ParseError: CustomStringConvertible {
       return "group named '\(str)' already exists"
     case let .invalidQuantifierRange(lhs, rhs):
       return "range lower bound '\(lhs)' must be less than or equal to upper bound '\(rhs)'"
+    case let .invalidCharacterRange(from: lhs, to: rhs):
+      return "character '\(lhs)' must compare less than or equal to '\(rhs)'"
+    case .notQuantifiable:
+      return "expression is not quantifiable"
     }
   }
 }

Sources/_RegexParser/Regex/Parse/Parse.swift

@@ -227,9 +227,6 @@ extension Parser {
         if let (amt, kind, trivia) =
             try source.lexQuantifier(context: context) {
           let location = loc(_start)
-          guard operand.isQuantifiable else {
-            throw Source.LocatedError(ParseError.notQuantifiable, location)
-          }
           result.append(.quantification(
             .init(amt, kind, operand, location, trivia: trivia)))
         } else {

Sources/_RegexParser/Regex/Parse/Sema.swift

@@ -182,17 +182,15 @@ extension RegexValidator {
     _ esc: AST.Atom.EscapedBuiltin, at loc: SourceLocation
   ) throws {
     switch esc {
-    case .resetStartOfMatch, .singleDataUnit, .horizontalWhitespace,
-        .notHorizontalWhitespace, .verticalTab, .notVerticalTab,
+    case .resetStartOfMatch, .singleDataUnit, .verticalTab, .notVerticalTab,
         // '\N' needs to be emitted using 'emitAny'.
         .notNewline:
       throw error(.unsupported("'\\\(esc.character)'"), at: loc)
 
     // Character classes.
     case .decimalDigit, .notDecimalDigit, .whitespace, .notWhitespace,
-        .wordCharacter, .notWordCharacter, .graphemeCluster, .trueAnychar:
-      // TODO: What about scalar matching mode for .graphemeCluster? We
-      // currently crash at runtime.
+        .wordCharacter, .notWordCharacter, .graphemeCluster, .trueAnychar,
+        .horizontalWhitespace, .notHorizontalWhitespace:
       break
 
     case .newlineSequence:
@@ -271,18 +269,20 @@ extension RegexValidator {
       throw error(.invalidCharacterClassRangeOperand, at: rhs.location)
     }
 
-    guard lhs.literalCharacterValue != nil else {
+    guard let lhsChar = lhs.literalCharacterValue else {
       throw error(
         .unsupported("character class range operand"), at: lhs.location)
     }
 
-    guard rhs.literalCharacterValue != nil else {
+    guard let rhsChar = rhs.literalCharacterValue else {
       throw error(
         .unsupported("character class range operand"), at: rhs.location)
     }
 
-    // TODO: Validate lhs <= rhs? That may require knowledge of case
-    // insensitivity though.
+    guard lhsChar <= rhsChar else {
+      throw error(
+        .invalidCharacterRange(from: lhsChar, to: rhsChar), at: range.dashLoc)
+    }
   }
 
   func validateCharacterClassMember(
@@ -341,6 +341,9 @@ extension RegexValidator {
 
   func validateQuantification(_ quant: AST.Quantification) throws {
     try validateNode(quant.child)
+    guard quant.child.isQuantifiable else {
+      throw error(.notQuantifiable, at: quant.child.location)
+    }
     switch quant.amount.value {
     case .range(let lhs, let rhs):
       guard lhs.value <= rhs.value else {

Tests/RegexTests/ParseTests.swift

@@ -503,6 +503,8 @@ extension RegexTests {
     parseTest("[-a-]", charClass("-", "a", "-"))
 
     parseTest("[a-z]", charClass(range_m("a", "z")))
+    parseTest("[a-a]", charClass(range_m("a", "a")))
+    parseTest("[B-a]", charClass(range_m("B", "a")))
 
     // FIXME: AST builder helpers for custom char class types
     parseTest("[a-d--a-c]", charClass(
@@ -2442,6 +2444,11 @@ extension RegexTests {
 
     diagnosticTest(#"|([\d-c])?"#, .invalidCharacterClassRangeOperand)
 
+    diagnosticTest(#"[_-A]"#, .invalidCharacterRange(from: "_", to: "A"))
+    diagnosticTest(#"(?i)[_-A]"#, .invalidCharacterRange(from: "_", to: "A"))
+    diagnosticTest(#"[c-b]"#, .invalidCharacterRange(from: "c", to: "b"))
+    diagnosticTest(#"[\u{66}-\u{65}]"#, .invalidCharacterRange(from: "\u{66}", to: "\u{65}"))
+
     // MARK: Bad escapes
 
     diagnosticTest("\\", .expectedEscape)
@@ -2555,6 +2562,17 @@ extension RegexTests {
     diagnosticTest("{1,3}", .quantifierRequiresOperand("{1,3}"))
     diagnosticTest("a{3,2}", .invalidQuantifierRange(3, 2))
 
+    // These are not quantifiable.
+    diagnosticTest(#"\b?"#, .notQuantifiable)
+    diagnosticTest(#"\B*"#, .notQuantifiable)
+    diagnosticTest(#"\A+"#, .notQuantifiable)
+    diagnosticTest(#"\Z??"#, .notQuantifiable)
+    diagnosticTest(#"\G*?"#, .notQuantifiable)
+    diagnosticTest(#"\z+?"#, .notQuantifiable)
+    diagnosticTest(#"\K{1}"#, .unsupported(#"'\K'"#))
+    diagnosticTest(#"\y{2,5}"#, .notQuantifiable)
+    diagnosticTest(#"\Y{3,}"#, .notQuantifiable)
+
     // MARK: Unicode scalars
 
     diagnosticTest(#"\u{G}"#, .expectedNumber("G", kind: .hex))
@@ -2641,13 +2659,13 @@ extension RegexTests {
 
     diagnosticTest("(*MARK)", .backtrackingDirectiveMustHaveName("MARK"))
     diagnosticTest("(*:)", .expectedNonEmptyContents)
-    diagnosticTest("(*MARK:a)?", .notQuantifiable)
-    diagnosticTest("(*FAIL)+", .notQuantifiable)
-    diagnosticTest("(*COMMIT:b)*", .notQuantifiable)
-    diagnosticTest("(*PRUNE:a)??", .notQuantifiable)
-    diagnosticTest("(*SKIP:a)*?", .notQuantifiable)
-    diagnosticTest("(*F)+?", .notQuantifiable)
-    diagnosticTest("(*:a){2}", .notQuantifiable)
+    diagnosticTest("(*MARK:a)?", .unsupported("backtracking directive"))
+    diagnosticTest("(*FAIL)+", .unsupported("backtracking directive"))
+    diagnosticTest("(*COMMIT:b)*", .unsupported("backtracking directive"))
+    diagnosticTest("(*PRUNE:a)??", .unsupported("backtracking directive"))
+    diagnosticTest("(*SKIP:a)*?", .unsupported("backtracking directive"))
+    diagnosticTest("(*F)+?", .unsupported("backtracking directive"))
+    diagnosticTest("(*:a){2}", .unsupported("backtracking directive"))
 
     // MARK: Oniguruma absent functions