clang-format: [JS] Handle certain cases of ASI.

[mprobst]

Summary

Automatic Semicolon Insertion can only be properly handled by parsing
source code. However conservatively catching just a few, common
situations prevents breaking code during development, which greatly
improves usability.

JS code should still use semicolons, and ASI code should be flagged by
a compiler or linter.

[mprobst]

• Handle unary !.

[djasper]

lib/Format/UnwrappedLineParser.cpp

667

Make the parameters const.

I'd also like this to have "JS" in the name somehow. And I am worried that this might be confusing because it says "shouldInsert", but we don't insert (at least not yet). Maybe rename this to wouldTriggerJavaScriptASI(...).

670

I think it is a bit confusing to hand in both Previous and Next but still rely on some state of the class. Maybe it is better to move the 4 lines from nextToken into this function?

692

I'd just hand in "Keywords" for now and make this a local (static) function. We should also refactor this whole thing to not have the class definition in the header probably.

Also, this is incorrect as it doesn't return true for C++ keywords that aren't JS keywords, e.g. try "struct". Leave a FIXME for me to sort this out.

1968

const?

1970

In LLVM-style, we generally don't use braces for single-statement ifs.

unittests/Format/FormatTestJS.cpp

64

I don't think it is useful to do this as there are always ways in which formatting can fail leaving the input untouched. We still want to verify that formatting takes place.

To do so, manually mess up the code in some way and use EXPECT_EQ. There should be various examples in FormatTest.cpp.

[mprobst]

• Handle unary !.
• Address review comments.

unittests/Format/FormatTestJS.cpp

64

Done - introduced a verifyFormat(str, str, style) to make it a less verbose.

[mprobst]

• Handle unary !.
• Address review comments.
• Support declarations and statements in ASI.

[djasper]

Looks good.

lib/Format/UnwrappedLineParser.cpp

667

We don't usually put usernames into FIXMEs

[djasper]

Submitted as r263470.

lib/Format/UnwrappedLineParser.h

private:
   void parseChildBlock();
   void parsePPDirective();
   void parsePPDefine();
   void parsePPIf(bool IfDef);
   void parsePPElIf();
   void parsePPElse();
   void parsePPEndIf();
   void parsePPUnknown();
+  void readTokenWithJavaScriptASI();
   void parseStructuralElement();
   bool tryToParseBracedList();
   bool parseBracedList(bool ContinueOnSemicolons = false);
   void parseParens();
   void parseSquare();
   void parseIfThenElse();
   void parseTryCatch();
   void parseForOrWhileLoop();

lib/Format/UnwrappedLineParser.cpp

return Tok.isNot(tok::semi) && Tok.isNot(tok::l_brace) &&
          // Colon is used in labels, base class lists, initializer lists,
          // range-based for loops, ternary operator, but should never be the
          // first token in an unwrapped line.
          Tok.isNot(tok::colon) &&
          // 'noexcept' is a trailing annotation.
          Tok.isNot(tok::kw_noexcept);
 }
 
+static bool mustBeJSIdentOrValue(const AdditionalKeywords &Keywords,
+                                 const FormatToken *FormatTok) {
+  if (FormatTok->Tok.isLiteral())
+    return true;
+  // FIXME(martinprobst): This returns true for C/C++ keywords like 'struct'.
+  return FormatTok->is(tok::identifier) &&
+         (FormatTok->Tok.getIdentifierInfo() == nullptr ||
+          !FormatTok->isOneOf(Keywords.kw_in, Keywords.kw_of,
+                              Keywords.kw_finally, Keywords.kw_function,
+                              Keywords.kw_import, Keywords.kw_is,
+                              Keywords.kw_let, Keywords.kw_var,
+                              Keywords.kw_abstract, Keywords.kw_extends,
+                              Keywords.kw_implements, Keywords.kw_instanceof,
+                              Keywords.kw_interface, Keywords.kw_throws));
+}
+
+// isJSDeclOrStmt returns true if |FormatTok| starts a declaration or statement
+// when encountered after a value (see mustBeJSIdentOrValue).
+static bool isJSDeclOrStmt(const AdditionalKeywords &Keywords,
+                           const FormatToken *FormatTok) {
+  return FormatTok->isOneOf(
+      tok::kw_return,
+      // conditionals
+      tok::kw_if, tok::kw_else,
+      // loops
+      tok::kw_for, tok::kw_while, tok::kw_do, tok::kw_continue, tok::kw_break,
+      // switch/case
+      tok::kw_switch, tok::kw_case,
+      // exceptions
+      tok::kw_throw, tok::kw_try, tok::kw_catch, Keywords.kw_finally,
+      // declaration
+      tok::kw_const, tok::kw_class, Keywords.kw_var, Keywords.kw_let,
+      Keywords.kw_function);
+}
+
+// readTokenWithJavaScriptASI reads the next token and terminates the current
+// line if JavaScript Automatic Semicolon Insertion must
+// happen between the current token and the next token.
+//
+// This method is conservative - it cannot cover all edge cases of JavaScript,
+// but only aims to correctly handle certain well known cases. It *must not*
+// return true in speculative cases.
+void UnwrappedLineParser::readTokenWithJavaScriptASI() {
+  FormatToken *Previous = FormatTok;
+  readToken();
+  FormatToken *Next = FormatTok;
+
+  bool IsOnSameLine =
+      CommentsBeforeNextToken.empty()
+          ? Next->NewlinesBefore == 0
+          : CommentsBeforeNextToken.front()->NewlinesBefore == 0;
+  if (IsOnSameLine)
+    return;
+
+  bool PreviousMustBeValue = mustBeJSIdentOrValue(Keywords, Previous);
+  if (Next->is(tok::exclaim) && PreviousMustBeValue)
+    addUnwrappedLine();
+  bool NextMustBeValue = mustBeJSIdentOrValue(Keywords, Next);
+  if (NextMustBeValue && (PreviousMustBeValue ||
+                          Previous->isOneOf(tok::r_square, tok::r_paren,
+                                            tok::plusplus, tok::minusminus)))
+    addUnwrappedLine();
+  if (PreviousMustBeValue && isJSDeclOrStmt(Keywords, Next))
+    addUnwrappedLine();
+}
+
 void UnwrappedLineParser::parseStructuralElement() {
   assert(!FormatTok->is(tok::l_brace));
   if (Style.Language == FormatStyle::LK_TableGen &&
       FormatTok->is(tok::pp_include)) {
     nextToken();
     if (FormatTok->is(tok::string_literal))
       nextToken();
     addUnwrappedLine();
case tok::identifier: {
       if ((Style.Language == FormatStyle::LK_JavaScript ||
            Style.Language == FormatStyle::LK_Java) &&
           FormatTok->is(Keywords.kw_interface)) {
         parseRecord();
         addUnwrappedLine();
         return;
       }
 
+      // See if the following token should start a new unwrapped line.
       StringRef Text = FormatTok->TokenText;
       nextToken();
       if (Line->Tokens.size() == 1 &&
           // JS doesn't have macros, and within classes colons indicate fields,
           // not labels.
           Style.Language != FormatStyle::LK_JavaScript) {
         if (FormatTok->Tok.is(tok::colon) && !Line->MustBeDeclaration) {
           parseLabel();
void UnwrappedLineParser::flushComments(bool NewlineBeforeNext) {
   CommentsBeforeNextToken.clear();
 }
 
 void UnwrappedLineParser::nextToken() {
   if (eof())
     return;
   flushComments(isOnNewLine(*FormatTok));
   pushToken(FormatTok);
+  if (Style.Language != FormatStyle::LK_JavaScript)
     readToken();
+  else
+    readTokenWithJavaScriptASI();
 }
 
 const FormatToken *UnwrappedLineParser::getPreviousToken() {
   // FIXME: This is a dirty way to access the previous token. Find a better
   // solution.
   if (!Line || Line->Tokens.empty())
     return nullptr;
   return Line->Tokens.back().Tok;

unittests/Format/FormatTestJS.cpp

static FormatStyle getGoogleJSStyleWithColumns(unsigned ColumnLimit) {
     FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
     Style.ColumnLimit = ColumnLimit;
     return Style;
   }
 
   static void verifyFormat(
       llvm::StringRef Code,
       const FormatStyle &Style = getGoogleStyle(FormatStyle::LK_JavaScript)) {
-    std::string result = format(test::messUp(Code), Style);
-    EXPECT_EQ(Code.str(), result) << "Formatted:\n" << result;
+    std::string Result = format(test::messUp(Code), Style);
+    EXPECT_EQ(Code.str(), Result) << "Formatted:\n" << Result;
+  }
+
+  static void verifyFormat(
+      llvm::StringRef Expected,
+      llvm::StringRef Code,
+      const FormatStyle &Style = getGoogleStyle(FormatStyle::LK_JavaScript)) {
+    std::string Result = format(Code, Style);
+    EXPECT_EQ(Expected.str(), Result) << "Formatted:\n" << Result;
   }
 };
 
 TEST_F(FormatTestJS, UnderstandsJavaScriptOperators) {
   verifyFormat("a == = b;");
   verifyFormat("a != = b;");
 
   verifyFormat("a === b;");
   verifyFormat("aaaaaaa ===\n    b;", getGoogleJSStyleWithColumns(10));
   verifyFormat("a !== b;");
   verifyFormat("aaaaaaa !==\n    b;", getGoogleJSStyleWithColumns(10));
TEST_F(FormatTestJS, ForLoops) {
   verifyFormat("for (var i of [2, 3]) {\n"
                "}");
   verifyFormat("for (let {a, b} of x) {\n"
                "}");
   verifyFormat("for (let {a, b} in x) {\n"
                "}");
 }
 
-TEST_F(FormatTestJS, AutomaticSemicolonInsertion) {
+TEST_F(FormatTestJS, WrapRespectsAutomaticSemicolonInsertion) {
   // The following statements must not wrap, as otherwise the program meaning
   // would change due to automatic semicolon insertion.
   // See http://www.ecma-international.org/ecma-262/5.1/#sec-7.9.1.
   verifyFormat("return aaaaa;", getGoogleJSStyleWithColumns(10));
   verifyFormat("continue aaaaa;", getGoogleJSStyleWithColumns(10));
   verifyFormat("break aaaaa;", getGoogleJSStyleWithColumns(10));
   verifyFormat("throw aaaaa;", getGoogleJSStyleWithColumns(10));
   verifyFormat("aaaaaaaaa++;", getGoogleJSStyleWithColumns(10));
   verifyFormat("aaaaaaaaa--;", getGoogleJSStyleWithColumns(10));
   verifyFormat("return [\n"
                "  aaa\n"
                "];",
                getGoogleJSStyleWithColumns(12));
 }
 
+TEST_F(FormatTestJS, AutomaticSemicolonInsertionHeuristic) {
+  verifyFormat("a\n"
+               "b;",
+               " a \n"
+               " b ;");
+  verifyFormat("a()\n"
+               "b;",
+               " a ()\n"
+               " b ;");
+  verifyFormat("a[b]\n"
+               "c;",
+               "a [b]\n"
+               "c ;");
+  verifyFormat("1\n"
+               "a;",
+               "1 \n"
+               "a ;");
+  verifyFormat("a\n"
+               "1;",
+               "a \n"
+               "1 ;");
+  verifyFormat("a\n"
+               "'x';",
+               "a \n"
+               " 'x';");
+  verifyFormat("a++\n"
+               "b;",
+               "a ++\n"
+               "b ;");
+  verifyFormat("a\n"
+               "!b && c;",
+               "a \n"
+               " ! b && c;");
+  verifyFormat("a\n"
+               "if (1) f();",
+               " a\n"
+               " if (1) f();");
+  verifyFormat("a\n"
+               "class X {}",
+               " a\n"
+               " class X {}");
+  verifyFormat("var a", "var\n"
+                        "a");
+  verifyFormat("x instanceof String", "x\n"
+                                      "instanceof\n"
+                                      "String");
+}
+
 TEST_F(FormatTestJS, ClosureStyleCasts) {
   verifyFormat("var x = /** @type {foo} */ (bar);");
 }
 
 TEST_F(FormatTestJS, TryCatch) {
   verifyFormat("try {\n"
                "  f();\n"
                "} catch (e) {\n"
TEST_F(FormatTestJS, RegexLiteralSpecialCharacters) {
   verifyFormat("var regex = /\\W/;");
   verifyFormat("var regex = /a(a)\\1/;");
   verifyFormat("var regex = /\\0/;");
   verifyFormat("var regex = /\\\\/g;");
   verifyFormat("var regex = /\\a\\\\/g;");
   verifyFormat("var regex = /\a\\//g;");
   verifyFormat("var regex = /a\\//;\n"
                "var x = 0;");
-  EXPECT_EQ("var regex = /'/g;", format("var regex = /'/g ;"));
-  EXPECT_EQ("var regex = /'/g;  //'", format("var regex = /'/g ; //'"));
-  EXPECT_EQ("var regex = /\\/*/;\n"
+  verifyFormat("var regex = /'/g;", "var regex = /'/g ;");
+  verifyFormat("var regex = /'/g;  //'", "var regex = /'/g ; //'");
+  verifyFormat("var regex = /\\/*/;\n"
                "var x = 0;",
-            format("var regex = /\\/*/;\n"
-                   "var x=0;"));
-  EXPECT_EQ("var x = /a\\//;", format("var x = /a\\//  \n;"));
+               "var regex = /\\/*/;\n"
+               "var x=0;");
+  verifyFormat("var x = /a\\//;", "var x = /a\\//  \n;");
   verifyFormat("var regex = /\"/;", getGoogleJSStyleWithColumns(16));
   verifyFormat("var regex =\n"
                "    /\"/;",
                getGoogleJSStyleWithColumns(15));
   verifyFormat("var regex =  //\n"
                "    /a/;");
   verifyFormat("var regexs = [\n"
                "  /d/,   //\n"
TEST_F(FormatTestJS, Modules) {
   verifyFormat("export interface Foo { foo: number; }\n"
                "export class Bar {\n"
                "  blah(): string { return this.blah; };\n"
                "}");
 }
 
 TEST_F(FormatTestJS, TemplateStrings) {
   // Keeps any whitespace/indentation within the template string.
-  EXPECT_EQ("var x = `hello\n"
+  verifyFormat("var x = `hello\n"
             "     ${  name    }\n"
             "  !`;",
-            format("var x    =    `hello\n"
+            "var x    =    `hello\n"
                    "     ${  name    }\n"
-                   "  !`;"));
+                   "  !`;");
 
   verifyFormat("var x =\n"
                "    `hello ${world}` >= some();",
                getGoogleJSStyleWithColumns(34)); // Barely doesn't fit.
   verifyFormat("var x = `hello ${world}` >= some();",
                getGoogleJSStyleWithColumns(35)); // Barely fits.
-  EXPECT_EQ("var x = `hello\n"
+  verifyFormat("var x = `hello\n"
             "  ${world}` >=\n"
             "    some();",
-            format("var x =\n"
+            "var x =\n"
                    "    `hello\n"
                    "  ${world}` >= some();",
-                   getGoogleJSStyleWithColumns(21))); // Barely doesn't fit.
-  EXPECT_EQ("var x = `hello\n"
+                   getGoogleJSStyleWithColumns(21)); // Barely doesn't fit.
+  verifyFormat("var x = `hello\n"
             "  ${world}` >= some();",
-            format("var x =\n"
+            "var x =\n"
                    "    `hello\n"
                    "  ${world}` >= some();",
-                   getGoogleJSStyleWithColumns(22))); // Barely fits.
+                   getGoogleJSStyleWithColumns(22)); // Barely fits.
 
   verifyFormat("var x =\n"
                "    `h`;",
                getGoogleJSStyleWithColumns(11));
-  EXPECT_EQ(
-      "var x =\n    `multi\n  line`;",
-      format("var x = `multi\n  line`;", getGoogleJSStyleWithColumns(13)));
+  verifyFormat("var x =\n    `multi\n  line`;", "var x = `multi\n  line`;",
+               getGoogleJSStyleWithColumns(13));
   verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
                "    `aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa`);");
 
   // Make sure template strings get a proper ColumnWidth assigned, even if they
   // are first token in line.
   verifyFormat(
       "var a = aaaaaaaaaaaaaaaaaaaaaaaaaaaa ||\n"
       "    `aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa`;");
 
   // Two template strings.
   verifyFormat("var x = `hello` == `hello`;");
 
   // Comments in template strings.
-  EXPECT_EQ("var x = `//a`;\n"
+  verifyFormat("var x = `//a`;\n"
             "var y;",
-            format("var x =\n `//a`;\n"
-                   "var y  ;"));
-  EXPECT_EQ("var x = `/*a`;\n"
+            "var x =\n `//a`;\n"
+                   "var y  ;");
+  verifyFormat("var x = `/*a`;\n"
                "var y;",
-            format("var x =\n `/*a`;\n"
-                   "var y;"));
+               "var x =\n `/*a`;\n"
+               "var y;");
   // Unterminated string literals in a template string.
   verifyFormat("var x = `'`;  // comment with matching quote '\n"
                "var y;");
   verifyFormat("var x = `\"`;  // comment with matching quote \"\n"
                "var y;");
-  EXPECT_EQ("it(`'aaaaaaaaaaaaaaa   `, aaaaaaaaa);",
-            format("it(`'aaaaaaaaaaaaaaa   `,   aaaaaaaaa) ;",
-                   getGoogleJSStyleWithColumns(40)));
+  verifyFormat("it(`'aaaaaaaaaaaaaaa   `, aaaaaaaaa);",
+               "it(`'aaaaaaaaaaaaaaa   `,   aaaaaaaaa) ;",
+               getGoogleJSStyleWithColumns(40));
   // Backticks in a comment - not a template string.
-  EXPECT_EQ("var x = 1  // `/*a`;\n"
+  verifyFormat("var x = 1  // `/*a`;\n"
                "    ;",
-            format("var x =\n 1  // `/*a`;\n"
-                   "    ;"));
-  EXPECT_EQ("/* ` */ var x = 1; /* ` */",
-            format("/* ` */ var x\n= 1; /* ` */"));
+               "var x =\n 1  // `/*a`;\n"
+               "    ;");
+  verifyFormat("/* ` */ var x = 1; /* ` */", "/* ` */ var x\n= 1; /* ` */");
   // Comment spans multiple template strings.
-  EXPECT_EQ("var x = `/*a`;\n"
+  verifyFormat("var x = `/*a`;\n"
                "var y = ` */ `;",
-            format("var x =\n `/*a`;\n"
-                   "var y =\n ` */ `;"));
+               "var x =\n `/*a`;\n"
+               "var y =\n ` */ `;");
   // Escaped backtick.
-  EXPECT_EQ("var x = ` \\` a`;\n"
+  verifyFormat("var x = ` \\` a`;\n"
                "var y;",
-            format("var x = ` \\` a`;\n"
-                   "var y;"));
+               "var x = ` \\` a`;\n"
+               "var y;");
 }
 
 TEST_F(FormatTestJS, CastSyntax) { verifyFormat("var x = <type>foo;"); }
 
 TEST_F(FormatTestJS, TypeArguments) {
   verifyFormat("class X<Y> {}");
   verifyFormat("new X<Y>();");
   verifyFormat("foo<Y>(a);");
verifyFormat("xxxxxxxxxxx(\n"
                "      y();  //\n"
                "    });",
                getGoogleJSStyleWithColumns(40));
   verifyFormat("while (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n"
                "       bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb) {\n}");
 }
 
 TEST_F(FormatTestJS, JSDocAnnotations) {
-  EXPECT_EQ("/**\n"
+  verifyFormat("/**\n"
                " * @export {this.is.a.long.path.to.a.Type}\n"
                " */",
-            format("/**\n"
-                   " * @export {this.is.a.long.path.to.a.Type}\n"
-                   " */",
-                   getGoogleJSStyleWithColumns(20)));
+               "/**\n"
+               getGoogleJSStyleWithColumns(20));
 }
 
 TEST_F(FormatTestJS, RequoteStringsSingle) {
-  EXPECT_EQ("var x = 'foo';", format("var x = \"foo\";"));
-  EXPECT_EQ("var x = 'fo\\'o\\'';", format("var x = \"fo'o'\";"));
-  EXPECT_EQ("var x = 'fo\\'o\\'';", format("var x = \"fo\\'o'\";"));
-  EXPECT_EQ("var x =\n"
-            "    'foo\\'';",
-            // Code below is 15 chars wide, doesn't fit into the line with the
-            // \ escape added.
-            format("var x = \"foo'\";", getGoogleJSStyleWithColumns(15)));
+  verifyFormat("var x = 'foo';", "var x = \"foo\";");
+  verifyFormat("var x = 'fo\\'o\\'';", "var x = \"fo'o'\";");
+  verifyFormat("var x = 'fo\\'o\\'';", "var x = \"fo\\'o'\";");
+  verifyFormat(
+      "var x =\n"
+      "var x = \"foo'\";", getGoogleJSStyleWithColumns(15));
   // Removes no-longer needed \ escape from ".
-  EXPECT_EQ("var x = 'fo\"o';", format("var x = \"fo\\\"o\";"));
+  verifyFormat("var x = 'fo\"o';", "var x = \"fo\\\"o\";");
   // Code below fits into 15 chars *after* removing the \ escape.
-  EXPECT_EQ("var x = 'fo\"o';",
-            format("var x = \"fo\\\"o\";", getGoogleJSStyleWithColumns(15)));
+  verifyFormat("var x = 'fo\"o';", "var x = \"fo\\\"o\";",
+               getGoogleJSStyleWithColumns(15));
 }
 
 TEST_F(FormatTestJS, RequoteStringsDouble) {
   FormatStyle DoubleQuotes = getGoogleStyle(FormatStyle::LK_JavaScript);
   DoubleQuotes.JavaScriptQuotes = FormatStyle::JSQS_Double;
   verifyFormat("var x = \"foo\";", DoubleQuotes);
-  EXPECT_EQ("var x = \"foo\";", format("var x = 'foo';", DoubleQuotes));
-  EXPECT_EQ("var x = \"fo'o\";", format("var x = 'fo\\'o';", DoubleQuotes));
+  verifyFormat("var x = \"foo\";", "var x = 'foo';", DoubleQuotes);
+  verifyFormat("var x = \"fo'o\";", "var x = 'fo\\'o';", DoubleQuotes);
 }
 
 TEST_F(FormatTestJS, RequoteStringsLeave) {
   FormatStyle LeaveQuotes = getGoogleStyle(FormatStyle::LK_JavaScript);
   LeaveQuotes.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   verifyFormat("var x = \"foo\";", LeaveQuotes);
   verifyFormat("var x = 'foo';", LeaveQuotes);
 }
 
 } // end namespace tooling
 } // end namespace clang