[Parse] Ban trailing space for /.../ regex literals

Ban space and tab as the last character of a
`/.../` regex literal, unless escaped with a
backslash. This matches the banning of space and
tab as the first character, and helps avoid breaking
source in even more cases.

Author: hamishknight

include/swift/AST/DiagnosticsParse.def

@@ -143,6 +143,9 @@ ERROR(lex_invalid_closing_delimiter,none,
 ERROR(lex_regex_literal_invalid_starting_char,none,
       "regex literal may not start with %0; add backslash to escape",
       (StringRef))
+ERROR(lex_regex_literal_invalid_ending_char,none,
+      "regex literal may not end with %0; use extended literal instead",
+      (StringRef))
 ERROR(lex_regex_literal_unterminated,none,
       "unterminated regex literal", ())
 

lib/Parse/Lexer.cpp

@@ -1984,6 +1984,17 @@ bool Lexer::tryLexRegexLiteral(const char *TokStart) {
   if (!LangOpts.EnableExperimentalStringProcessing || !regexLiteralLexingFn)
     return false;
 
+  auto spaceOrTabDescription = [](char c) -> StringRef {
+    switch (c) {
+    case ' ':
+      return "space";
+    case '\t':
+      return "tab";
+    default:
+      llvm_unreachable("Unhandled case");
+    }
+  };
+
   bool MustBeRegex = true;
   bool IsForwardSlash = (*TokStart == '/');
 
@@ -2012,31 +2023,14 @@ bool Lexer::tryLexRegexLiteral(const char *TokStart) {
     // TODO: This heuristic should be sunk into the Swift library once we have a
     // way of doing fix-its from there.
     auto *RegexContentStart = TokStart + 1;
-    switch (*RegexContentStart) {
-    case ' ':
-    case '\t': {
+    if (*RegexContentStart == ' ' || *RegexContentStart == '\t') {
       if (!MustBeRegex)
         return false;
 
       // We must have a regex, so emit an error for space and tab.
-      StringRef DiagChar;
-      switch (*RegexContentStart) {
-      case ' ':
-        DiagChar = "space";
-        break;
-      case '\t':
-        DiagChar = "tab";
-        break;
-      default:
-        llvm_unreachable("Unhandled case");
-      }
       diagnose(RegexContentStart, diag::lex_regex_literal_invalid_starting_char,
-               DiagChar)
-          .fixItInsert(getSourceLoc(RegexContentStart), "\\");
-      break;
-    }
-    default:
-      break;
+               spaceOrTabDescription(*RegexContentStart))
+      .fixItInsert(getSourceLoc(RegexContentStart), "\\");
     }
   }
 
@@ -2054,58 +2048,77 @@ bool Lexer::tryLexRegexLiteral(const char *TokStart) {
   if (Ptr == TokStart)
     return false;
 
-  // If we're lexing `/.../`, error if we ended on the opening of a comment.
-  // We prefer to lex the comment as it's more likely than not that is what
-  // the user is expecting.
-  // TODO: This should be sunk into the Swift library.
-  if (IsForwardSlash && Ptr[-1] == '/' && (*Ptr == '*' || *Ptr == '/')) {
-    if (!MustBeRegex)
-      return false;
+  // Perform some additional heuristics to see if we can lex `/.../`.
+  // TODO: These should all be sunk into the Swift library.
+  if (IsForwardSlash) {
+    // If we're lexing `/.../`, error if we ended on the opening of a comment.
+    // We prefer to lex the comment as it's more likely than not that is what
+    // the user is expecting.
+    if (Ptr[-1] == '/' && (*Ptr == '*' || *Ptr == '/')) {
+      if (!MustBeRegex)
+        return false;
 
-    diagnose(TokStart, diag::lex_regex_literal_unterminated);
-
-    // Move the pointer back to the '/' of the comment.
-    Ptr--;
-  }
-
-  // If we're tentatively lexing `/.../`, scan to make sure we don't have any
-  // unbalanced ')'s. This helps avoid ambiguity with unapplied operator
-  // references e.g `reduce(1, /)` and `foo(/, 0) / 2`. This would be invalid
-  // regex syntax anyways. This ensures users can surround their operator ref
-  // in parens `(/)` to fix the issue. This also applies to prefix operators
-  // that can be disambiguated as e.g `(/S.foo)`. Note we need to track whether
-  // or not we're in a custom character class `[...]`, as parens are literal
-  // there.
-  // TODO: This should be sunk into the Swift library.
-  if (IsForwardSlash && !MustBeRegex) {
-    unsigned CharClassDepth = 0;
-    unsigned GroupDepth = 0;
-    for (auto *Cursor = TokStart + 1; Cursor < Ptr - 1; Cursor++) {
-      switch (*Cursor) {
-      case '\\':
-        // Skip over the next character of an escape.
-        Cursor++;
-        break;
-      case '(':
-        if (CharClassDepth == 0)
-          GroupDepth += 1;
-        break;
-      case ')':
-        if (CharClassDepth != 0)
+      diagnose(TokStart, diag::lex_regex_literal_unterminated);
+
+      // Move the pointer back to the '/' of the comment.
+      Ptr--;
+    }
+    auto *TokEnd = Ptr - 1;
+    auto *ContentEnd = TokEnd - 1;
+
+    // We also ban unescaped space and tab at the end of a `/.../` literal.
+    if (*TokEnd == '/' && (TokEnd - TokStart > 2) && ContentEnd[-1] != '\\' &&
+        (*ContentEnd == ' ' || *ContentEnd == '\t')) {
+      if (!MustBeRegex)
+        return false;
+
+      // Diagnose and suggest using a `#/.../#` literal instead. We could
+      // suggest escaping, but that would be wrong if the user has written (?x).
+      // TODO: Should we suggest this for space-as-first character too?
+      diagnose(ContentEnd, diag::lex_regex_literal_invalid_ending_char,
+               spaceOrTabDescription(*ContentEnd))
+      .fixItInsert(getSourceLoc(TokStart), "#")
+      .fixItInsert(getSourceLoc(Ptr), "#");
+    }
+
+    // If we're tentatively lexing `/.../`, scan to make sure we don't have any
+    // unbalanced ')'s. This helps avoid ambiguity with unapplied operator
+    // references e.g `reduce(1, /)` and `foo(/, 0) / 2`. This would be invalid
+    // regex syntax anyways. This ensures users can surround their operator ref
+    // in parens `(/)` to fix the issue. This also applies to prefix operators
+    // that can be disambiguated as e.g `(/S.foo)`. Note we need to track whether
+    // or not we're in a custom character class `[...]`, as parens are literal
+    // there.
+    if (!MustBeRegex) {
+      unsigned CharClassDepth = 0;
+      unsigned GroupDepth = 0;
+      for (auto *Cursor = TokStart + 1; Cursor < TokEnd; Cursor++) {
+        switch (*Cursor) {
+        case '\\':
+          // Skip over the next character of an escape.
+          Cursor++;
           break;
+        case '(':
+          if (CharClassDepth == 0)
+            GroupDepth += 1;
+          break;
+        case ')':
+          if (CharClassDepth != 0)
+            break;
 
-        // Invalid, so bail.
-        if (GroupDepth == 0)
-          return false;
+          // Invalid, so bail.
+          if (GroupDepth == 0)
+            return false;
 
-        GroupDepth -= 1;
-        break;
-      case '[':
-        CharClassDepth += 1;
-        break;
-      case ']':
-        if (CharClassDepth != 0)
-          CharClassDepth -= 1;
+          GroupDepth -= 1;
+          break;
+        case '[':
+          CharClassDepth += 1;
+          break;
+        case ']':
+          if (CharClassDepth != 0)
+            CharClassDepth -= 1;
+        }
       }
     }
   }