[clang-format] PR49960 clang-format doesn't handle ASI after "return" on JavaScript

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=49960

clang-format can mutate legal javascript code due to missing semicolons, but clang format pulls the next line into the first causing incorrect grammar.

function t() {
  if (true) return
  const v = 42
}

clang-format result:

function t() {
  if (true)
    return const v = 42
}

This was already somewhat addressed by the work of @mprobst in https://reviews.llvm.org/rG1dcbbcfc5cf06d2eacc68fbe9b6fc1fb12168d6f back in 2016, but this revision aims to identify other possible places so clang-format wouldn't be quite so destructive as people develop.

[HazardyKnusperkeks]

It's a strange language :)

[mprobst]

clang/lib/Format/UnwrappedLineParser.cpp

1015

can you add comments explaining what syntax is being detected here?

1019

parsePPEndIf uses a different method to peek - any opinion on why you're doing it differently here?

[MyDeveloperDay]

Move Peek next token into a function, add a comment for clarity

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

1019

I'm not entirely convinced parsePPEndIf IS looking at the next token

shouldn't

unsigned TokenPosition = Tokens->getPosition();
FormatToken *PeekNext = AllTokens[TokenPosition];

be

unsigned TokenPosition = Tokens->getPosition();
FormatToken *PeekNext = AllTokens[TokenPosition+1];

I followed the pattern in parseStructuralElement() but I think I should avoid that repitition by adding a function

also from elsewhere we see this:

FormatToken *Previous = AllTokens[Tokens->getPosition() - 1];

and so by definition, I would think it was.

FormatToken *CurrentToken= AllTokens[Tokens->getPosition()];
FormatToken *NextToken= AllTokens[Tokens->getPosition() + 1];

[mprobst]

clang/lib/Format/UnwrappedLineParser.cpp

1001

nit: just return AllTokens[TokenPosition]?

1014

do you still need this, given peeknext protects against running past the doc end?

clang/lib/Format/UnwrappedLineParser.h

188 ↗

(On Diff #355817)

cam you document what this does to the "current token" pointer?

[MyDeveloperDay]

Updating based on revision comments, but found that the indentation can be incorrect. (will put to planned changes)

clang/lib/Format/UnwrappedLineParser.cpp

return FormatTok->isOneOf(
       // import/export
       Keywords.kw_import, tok::kw_export);
 }
 
 // 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;
 
+  if (Previous->isOneOf(tok::kw_return, tok::kw_break, tok::kw_continue,
+                        Keywords.kw_yield)) {
+    if (Next->isOneOf(Keywords.kw_let, tok::kw_const, Keywords.kw_var))
+      return addUnwrappedLine();
+    // Don't go past the end of the file.
+    if (!eof()) {
+      if (Next->is(tok::identifier)) {
+        // Peek the next token.
+        unsigned StoredPosition = Tokens->getPosition();
+        FormatToken *NextNext = Tokens->getNextToken();
+        Tokens->setPosition(StoredPosition);
+        if (NextNext && NextNext->is(tok::equal))
+          return addUnwrappedLine();
+      }
+    }
+  }
+
   bool IsOnSameLine =
       CommentsBeforeNextToken.empty()
           ? Next->NewlinesBefore == 0
           : CommentsBeforeNextToken.front()->NewlinesBefore == 0;
   if (IsOnSameLine)
     return;
 
   bool PreviousMustBeValue = mustBeJSIdentOrValue(Keywords, Previous);

clang/unittests/Format/FormatTestJS.cpp

verifyFormat(
       "}\n",
       "interface Assertable<State extends {}> {\n"
       "  assert<ExportedState extends {}, DependencyState extends State = "
       "State>(callback: Callback<ExportedState, DependencyState>): asserts "
       "this is ExtendedState<DependencyState&ExportedState>;\n"
       "}\n");
 }
 
+TEST_F(FormatTestJS, ASIAfterReturn) {
+  verifyFormat("if (true) return\n"
+               "const v = 42");
+  verifyFormat("if (true) return\n"
+               "let v = 42");
+  verifyFormat("if (true) return\n"
+               "var v = 42");
+  verifyFormat("if (true) break\n"
+               "let v = 42");
+  verifyFormat("if (true) break\n"
+               "const v = 42");
+  verifyFormat("if (true) break\n"
+               "v = 42");
+  verifyFormat("if (true) continue\n"
+               "let v = 42");
+  verifyFormat("if (true) continue\n"
+               "var v = 42");
+  verifyFormat("if (true) continue\n"
+               "const v = 42");
+  verifyFormat("if (true) yield\n"
+               "let v = 42");
+  verifyFormat("if (true) yield\n"
+               "var v = 42");
+  verifyFormat("if (true) yield\n"
+               "const v = 42");
+
+  verifyFormat("if (true) break\n"
+               "var v = 42");
+  verifyFormat("if (true) return\n"
+               "v = 42");
+  verifyFormat("if (true) continue\n"
+               "v = 42");
+  verifyFormat("if (true) yield\n"
+               "v = 42");
+}
+
 } // namespace format
 } // end namespace clang