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

665

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(...).

668

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?

690

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.

1942

const?

1945

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

unittests/Format/FormatTestJS.cpp

56

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

56

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

665

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();
+  bool shouldInsertSemiBetween(FormatToken *Previous, FormatToken *Next);
+  bool isJavaScriptIdentifier(FormatToken *FormatTok);
   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);
 }
 
+// shouldInsertSemiBetween returns true if Automatic Semicolon Insertion must
+// happen between |Previous| and |Next|. 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.
+bool UnwrappedLineParser::shouldInsertSemiBetween(FormatToken *Previous,
+                                                  FormatToken *Next) {
+  bool IsOnSameLine =
+      CommentsBeforeNextToken.empty()
+          ? Next->NewlinesBefore == 0
+          : CommentsBeforeNextToken.front()->NewlinesBefore == 0;
+  if (IsOnSameLine)
+    return false;
+
+  bool PreviousMustBeValue =
+      isJavaScriptIdentifier(Previous) || Previous->Tok.isLiteral();
+  bool NextMustBeValue = isJavaScriptIdentifier(Next) || Next->Tok.isLiteral();
+  if (NextMustBeValue && PreviousMustBeValue)
+    return true;
+  if ((Previous->is(tok::r_square) || Previous->is(tok::r_paren)) &&
+      NextMustBeValue)
+    return true;
+  if (PreviousMustBeValue && Next->is(tok::exclaim))
+    return true;
+  if (Previous->isOneOf(tok::plusplus, tok::minusminus) &&
+      NextMustBeValue)
+    return true;
+  return false;
+}
+
+bool UnwrappedLineParser::isJavaScriptIdentifier(FormatToken *FormatTok) {
+  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));
+}
+
+
 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);
+  FormatToken *Previous = FormatTok;
   readToken();
+  if (Style.Language == FormatStyle::LK_JavaScript &&
+      shouldInsertSemiBetween(Previous, FormatTok)) {
+    addUnwrappedLine();
+  }
 }
 
 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

protected:
   }
 
   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;
   }
+
+  static void verifyFormatNoMessup(
+      llvm::StringRef Code,
+      const FormatStyle &Style = getGoogleStyle(FormatStyle::LK_JavaScript)) {
+    std::string result = format(Code, Style);
+    EXPECT_EQ(Code.str(), result) << "Formatted:\n" << result;
+  }
 };
 
 TEST_F(FormatTestJS, UnderstandsJavaScriptOperators) {
   verifyFormat("a == = b;");
   verifyFormat("a != = b;");
 
   verifyFormat("a === b;");
   verifyFormat("aaaaaaa ===\n    b;", getGoogleJSStyleWithColumns(10));

 
 TEST_F(FormatTestJS, ForLoops) {
   verifyFormat("for (var i in [2, 3]) {\n"
                "}");
   verifyFormat("for (var i of [2, 3]) {\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) {
+  verifyFormatNoMessup("a\n"
+                       "b;");
+  verifyFormatNoMessup("a()\n"
+                       "b;");
+  verifyFormatNoMessup("a[b]\n"
+                       "c;");
+  verifyFormatNoMessup("1\n"
+                       "a;");
+  verifyFormatNoMessup("a\n"
+                       "1;");
+  verifyFormatNoMessup("a\n"
+                       "'x';");
+  verifyFormatNoMessup("a++\n"
+                       "b;");
+  verifyFormatNoMessup("a\n"
+                       "!b && c;");
+  EXPECT_EQ("var a", format("var\n"
+                            "a"));
+  EXPECT_EQ("x instanceof String", format("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"