clang-format: [JS] Handle string literals spanning character classes.

[mprobst]

Summary

If a RegExp contains a character group with a quote (/["]/), the
trailing end of it is first tokenized as a string literal, which leads
to the merging code seeing an unbalanced bracket.

This change parses regex literals from the left hand side. That
simplifies the parsing code and also allows correctly handling escapes
and character classes, hopefully correctly parsing all regex literals.

[mprobst]

Simplified escape handling.

[djasper]

lib/Format/Format.cpp

755

I wonder if escape sequences are also relevant to template strings. If yes, we are probably missing a test ;-).

828

Maybe add a test to see how this goes wrong and specifically also ensure it doesn't crash.

830

Remove the ().

858

I'd probably put this into the other two conditions to reduce indentation. More specifically, I'd do:

if (PrecedesOperand(Prev)) // sink the nullptr check into the function.
  return;

if (Prev->isOneOf(...)) {
}

896

No braces.

[mprobst]

• add test for if (...) /re/;
• address minor review comments
• fix unary handling.

lib/Format/Format.cpp

858

Not having a Prev is actually part of the true case (no token/start of file precedes an operand), i.e. if nothing precedes the slash, it must be a regexp, too. Given that, both of the nested checks only run if (Prev), so I think the nesting makes sense.

However it turns out my unary check was slightly off and untested (whoops). Added a test and fixed the code.

[mprobst]

lib/Format/Format.cpp

755

You cannot actually escape in a template string - backslashes in them are literal backslashes. I.e. foo\bar` does not work.

However we do have a test for that that is presumably wrong (but still passes with this change):

EXPECT_EQ("var x = ` \\` a`;\n"
          "var y;",
          format("var x = ` \\` a`;\n"
                 "var y;"));

We can fix that in a later change, I think it's unrelated to this change.

[djasper]

lib/Format/Format.cpp

826

lowerCamelCase for function names in LLVM.

858

I understand and I actually find that hard to understand. I think a better solution might be to pull out a function for this logic. So that you just call:

if (!canPrecedeRegexLiteral(Prev))
  return;

and then

bool canPrecedeRegeLiteral(const FormatToken *Prev) {
  if (!Prev)
    return true;
  ..
}

unittests/Format/FormatTestJS.cpp

609

This case we could actually fix, right? Because we know that the ")" belongs to the if and if the following token isn't a comment or an open brace, it starts a new statement. Probably shouldn't be done in the same patch, but maybe add a FIXME.

[mprobst]

• add test
• review comments, fix unary handling.
• Extract canPrecedeRegexLiteral function.

lib/Format/Format.cpp

858

I see, that's indeed cleaner. Done.

unittests/Format/FormatTestJS.cpp

609

Not sure how we'd do that in the general case.

Regular expression recognition needs to happen in the lexing phase (otherwise it'd greatly complicate everything later on). And in the lexing phase, it's rather complicated to track if the code is in an if statement, as of course the if condition can contain arbitrary code.

Normally the fix for this is having the parser call down into the lexer with the contextual information that at this location a slash introduces a regex token, but we cannot do that, as we are neither parsing here, nor do we have precise grammatical structure around.

[djasper]

Looks good.

[djasper]

Submitted as r250648.

lib/Format/Format.cpp

FormatTokenLexer(SourceManager &SourceMgr, FileID ID, FormatStyle &Style,
     std::sort(ForEachMacros.begin(), ForEachMacros.end());
   }
 
   ArrayRef<FormatToken *> lex() {
     assert(Tokens.empty());
     assert(FirstInLineIndex == 0);
     do {
       Tokens.push_back(getNextToken());
+      if (Style.Language == FormatStyle::LK_JavaScript)
+        tryParseJSRegexLiteral();
       tryMergePreviousTokens();
       if (Tokens.back()->NewlinesBefore > 0 || Tokens.back()->IsMultiline)
         FirstInLineIndex = Tokens.size() - 1;
     } while (Tokens.back()->Tok.isNot(tok::eof));
     return Tokens;
   }
 
   const AdditionalKeywords &getKeywords() { return Keywords; }
 
 private:
   void tryMergePreviousTokens() {
     if (tryMerge_TMacro())
       return;
     if (tryMergeConflictMarkers())
       return;
     if (tryMergeLessLess())
       return;
 
     if (Style.Language == FormatStyle::LK_JavaScript) {
-      if (tryMergeJSRegexLiteral())
-        return;
-      if (tryMergeEscapeSequence())
-        return;
       if (tryMergeTemplateString())
         return;
 
       static const tok::TokenKind JSIdentity[] = {tok::equalequal, tok::equal};
       static const tok::TokenKind JSNotIdentity[] = {tok::exclaimequal,
                                                      tok::equal};
       static const tok::TokenKind JSShiftEqual[] = {tok::greater, tok::greater,
                                                     tok::greaterequal};
bool tryMergeTokens(ArrayRef<tok::TokenKind> Kinds, TokenType NewType) {
     Tokens.resize(Tokens.size() - Kinds.size() + 1);
     First[0]->TokenText = StringRef(First[0]->TokenText.data(),
                                     First[0]->TokenText.size() + AddLength);
     First[0]->ColumnWidth += AddLength;
     First[0]->Type = NewType;
     return true;
   }
 
-  // Tries to merge an escape sequence, i.e. a "\\" and the following
-  // character. Use e.g. inside JavaScript regex literals.
-  bool tryMergeEscapeSequence() {
-    if (Tokens.size() < 2)
-      return false;
-    FormatToken *Previous = Tokens[Tokens.size() - 2];
-    if (Previous->isNot(tok::unknown) || Previous->TokenText != "\\")
-      return false;
-    ++Previous->ColumnWidth;
-    StringRef Text = Previous->TokenText;
-    Previous->TokenText = StringRef(Text.data(), Text.size() + 1);
-    resetLexer(SourceMgr.getFileOffset(Tokens.back()->Tok.getLocation()) + 1);
-    Tokens.resize(Tokens.size() - 1);
-    Column = Previous->OriginalColumn + Previous->ColumnWidth;
-    return true;
-  }
-
-  // Try to determine whether the current token ends a JavaScript regex literal.
-  // We heuristically assume that this is a regex literal if we find two
-  // unescaped slashes on a line and the token before the first slash is one of
-  // "(;,{}![:?", a binary operator or 'return', as those cannot be followed by
-  // a division.
-  bool tryMergeJSRegexLiteral() {
-    if (Tokens.size() < 2)
-      return false;
+  // Returns \c true if \p Tok can only be followed by an operand in JavaScript.
+  bool PrecedesOperand(FormatToken *Tok) {
+    // NB: This is not entirely correct, as an r_paren can introduce an operand
+    // location in e.g. `if (foo) /bar/.exec(...);`. That is a rare enough
+    // corner case to not matter in practice, though.
+    return (Tok->isOneOf(tok::period, tok::l_paren, tok::comma, tok::l_brace,
+                         tok::r_brace, tok::l_square, tok::semi, tok::exclaim,
+                         tok::colon, tok::question, tok::tilde) ||
+            Tok->isOneOf(tok::kw_return, tok::kw_do, tok::kw_case,
+                         tok::kw_throw, tok::kw_else, tok::kw_new,
+                         tok::kw_delete, tok::kw_void, tok::kw_typeof,
+                         Keywords.kw_instanceof, Keywords.kw_in) ||
+            Tok->isBinaryOperator());
+  }
+
+  // Tries to parse a JavaScript Regex literal starting at the current token,
+  // if that begins with a slash and is in a location where JavaScript allows
+  // regex literals. Changes the current token to a regex literal and updates
+  // its text if successful.
+  void tryParseJSRegexLiteral() {
+    FormatToken *RegexToken = Tokens.back();
+    if (!RegexToken->isOneOf(tok::slash, tok::slashequal))
+      return;
 
-    // If this is a string literal with a slash inside, compute the slash's
-    // offset and try to find the beginning of the regex literal.
-    // Also look at tok::unknown, as it can be an unterminated char literal.
-    size_t SlashInStringPos = StringRef::npos;
-    if (Tokens.back()->isOneOf(tok::string_literal, tok::char_constant,
-                               tok::unknown)) {
-      // Start search from position 1 as otherwise, this is an unknown token
-      // for an unterminated /*-comment which is handled elsewhere.
-      SlashInStringPos = Tokens.back()->TokenText.find('/', 1);
-      if (SlashInStringPos == StringRef::npos)
-        return false;
+    FormatToken *Prev = nullptr;
+    for (auto I = Tokens.rbegin() + 1, E = Tokens.rend(); I != E; ++I) {
+      // NB: Because previous pointers are not initialized yet, this cannot use
+      // Token.getPreviousNonComment.
+      if ((*I)->isNot(tok::comment)) {
+        Prev = *I;
+        break;
+      }
+    }
+    if (Prev) {
+      if (Prev->isOneOf(tok::plus, tok::plusplus, tok::minus,
+                        tok::minusminus)) {
+        // Regex literal can only follow after prefix unary operators, not after
+        // postfix unary operators.
+        bool PrefixUnary =
+            Tokens.size() < 3 || !PrecedesOperand(Tokens[Tokens.size() - 3]);
+        if (!PrefixUnary)
+          return;
+      } else if (!PrecedesOperand(Prev)) {
+        // The previous token must introduce an operand location where regex
+        // literals can occur.
+        return;
+      }
     }
 
-    // If a regex literal ends in "\//", this gets represented by an unknown
-    // token "\" and a comment.
-    bool MightEndWithEscapedSlash =
-        Tokens.back()->is(tok::comment) &&
-        Tokens.back()->TokenText.startswith("//") &&
-        Tokens[Tokens.size() - 2]->TokenText == "\\";
-    if (!MightEndWithEscapedSlash && SlashInStringPos == StringRef::npos &&
-        (Tokens.back()->isNot(tok::slash) ||
-         (Tokens[Tokens.size() - 2]->is(tok::unknown) &&
-          Tokens[Tokens.size() - 2]->TokenText == "\\")))
-      return false;
-
-    unsigned TokenCount = 0;
+    // 'Manually' lex ahead in the current file buffer.
+    const char *Offset = Lex->getBufferLocation();
+    const char *RegexBegin = Offset - RegexToken->TokenText.size();
+    StringRef Buffer = Lex->getBuffer();
     bool InCharacterClass = false;
-    for (auto I = Tokens.rbegin() + 1, E = Tokens.rend(); I != E; ++I) {
-      ++TokenCount;
-      auto Prev = I + 1;
-      while (Prev != E && Prev[0]->is(tok::comment))
-        ++Prev;
-      // Slashes in character classes (delimited by [ and ]) do not need
-      // escaping. Escaping of the squares themselves is already handled by
-      // \c tryMergeEscapeSequence(), a plain tok::r_square must be non-escaped.
-      if (I[0]->is(tok::r_square))
+    bool HaveClosingSlash = false;
+    for (; !HaveClosingSlash && Offset != Buffer.end(); ++Offset) {
+      // Regular expressions are terminated with a '/', which can only be
+      // escaped using '\' or a character class between '[' and ']'.
+      // See http://www.ecma-international.org/ecma-262/5.1/#sec-7.8.5.
+      switch (*Offset) {
+      case '\\':
+        // Skip the escaped character.
+        ++Offset;
+        break;
+      case '[':
         InCharacterClass = true;
-      if (I[0]->is(tok::l_square)) {
-        if (!InCharacterClass)
-          return false;
+        break;
+      case ']':
         InCharacterClass = false;
+        break;
+      case '/':
+        if (!InCharacterClass) {
+          HaveClosingSlash = true;
         }
-      if (!InCharacterClass && I[0]->isOneOf(tok::slash, tok::slashequal) &&
-          (Prev == E ||
-           ((Prev[0]->isOneOf(tok::l_paren, tok::semi, tok::l_brace,
-                              tok::r_brace, tok::exclaim, tok::l_square,
-                              tok::colon, tok::comma, tok::question,
-                              tok::kw_return) ||
-             Prev[0]->isBinaryOperator())))) {
-        unsigned LastColumn = Tokens.back()->OriginalColumn;
-        SourceLocation Loc = Tokens.back()->Tok.getLocation();
-        if (MightEndWithEscapedSlash) {
-          // This regex literal ends in '\//'. Skip past the '//' of the last
-          // token and re-start lexing from there.
-          resetLexer(SourceMgr.getFileOffset(Loc) + 2);
-        } else if (SlashInStringPos != StringRef::npos) {
-          // This regex literal ends in a string_literal with a slash inside.
-          // Calculate end column and reset lexer appropriately.
-          resetLexer(SourceMgr.getFileOffset(Loc) + SlashInStringPos + 1);
-          LastColumn += SlashInStringPos;
-        }
-        Tokens.resize(Tokens.size() - TokenCount);
-        Tokens.back()->Tok.setKind(tok::unknown);
-        Tokens.back()->Type = TT_RegexLiteral;
-        // Treat regex literals like other string_literals.
-        Tokens.back()->Tok.setKind(tok::string_literal);
-        Tokens.back()->ColumnWidth += LastColumn - I[0]->OriginalColumn;
-        return true;
-      }
+        break;
 
-      // There can't be a newline inside a regex literal.
-      if (I[0]->NewlinesBefore > 0)
-        return false;
-    }
-    return false;
+    RegexToken->Type = TT_RegexLiteral;
+    // Treat regex literals like other string_literals.
+    RegexToken->Tok.setKind(tok::string_literal);
+    RegexToken->TokenText = StringRef(RegexBegin, Offset - RegexBegin);
+    RegexToken->ColumnWidth = RegexToken->TokenText.size();
+
+    resetLexer(SourceMgr.getFileOffset(Lex->getSourceLocation(Offset)));
   }
 
   bool tryMergeTemplateString() {
     if (Tokens.size() < 2)
       return false;
 
     FormatToken *EndBacktick = Tokens.back();
     // Backticks get lexed as tok::unknown tokens. If a template string contains

unittests/Format/FormatTestJS.cpp

TEST_F(FormatTestJS, RegexLiteralClassification) {
   // Not regex literals.
   verifyFormat("var a = a / 2 + b / 3;");
 }
 
 TEST_F(FormatTestJS, RegexLiteralSpecialCharacters) {
   verifyFormat("var regex = /=/;");
   verifyFormat("var regex = /a*/;");
   verifyFormat("var regex = /a+/;");
   verifyFormat("var regex = /a?/;");
   verifyFormat("var regex = /.a./;");
   verifyFormat("var regex = /a\\*/;");
   verifyFormat("var regex = /^a$/;");
   verifyFormat("var regex = /\\/a/;");
   verifyFormat("var regex = /(?:x)/;");
   verifyFormat("var regex = /x(?=y)/;");
   verifyFormat("var regex = /x(?!y)/;");
   verifyFormat("var regex = /x|y/;");
   verifyFormat("var regex = /a{2}/;");
   verifyFormat("var regex = /a{1,3}/;");
 
   verifyFormat("var regex = /[abc]/;");
   verifyFormat("var regex = /[^abc]/;");
   verifyFormat("var regex = /[\\b]/;");
   verifyFormat("var regex = /[/]/;");
   verifyFormat("var regex = /[\\/]/;");
   verifyFormat("var regex = /\\[/;");
   verifyFormat("var regex = /\\\\[/]/;");
+  verifyFormat("var regex = /}[\"]/;");
+  verifyFormat("var regex = /}[/\"]/;");
+  verifyFormat("var regex = /}[\"/]/;");
 
   verifyFormat("var regex = /\\b/;");
   verifyFormat("var regex = /\\B/;");
   verifyFormat("var regex = /\\d/;");
   verifyFormat("var regex = /\\D/;");
   verifyFormat("var regex = /\\f/;");
   verifyFormat("var regex = /\\n/;");
   verifyFormat("var regex = /\\r/;");