[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

1021

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

1025

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

1025

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]?

1020

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

clang/lib/Format/UnwrappedLineParser.h

188

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.h

private:
   // not be taken in this round.
   void conditionalCompilationCondition(bool Unreachable);
   void conditionalCompilationStart(bool Unreachable);
   void conditionalCompilationAlternative();
   void conditionalCompilationEnd();
 
   bool isOnNewLine(const FormatToken &FormatTok);
 
+  // Peek to the next token without impacting on the current token
+  // position. Returns null if no more tokens.
+  FormatToken *peekNextToken() const;
+
   // Compute hash of the current preprocessor branch.
   // This is used to identify the different branches, and thus track if block
   // open and close in the same branch.
   size_t computePPHash() const;
 
   // FIXME: We are constantly running into bugs where Line.Level is incorrectly
   // subtracted from beyond 0. Introduce a method to subtract from Line.Level
   // and use that everywhere in the Parser.

clang/lib/Format/UnwrappedLineParser.cpp

return FormatTok->isOneOf(
       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_async, Keywords.kw_function,
       // import/export
       Keywords.kw_import, tok::kw_export);
 }
 
+FormatToken *UnwrappedLineParser::peekNextToken() const {
+  unsigned TokenPosition = Tokens->getPosition();
+  return (++TokenPosition < AllTokens.size()) ? AllTokens[TokenPosition]
+                                              : nullptr;
+}
+
 // 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();
+    }
+    // Look for  return xxx = val; but not return xxx();
+    if (Next->is(tok::identifier)) {
+      FormatToken *PeekNext = peekNextToken();
+      if (PeekNext && PeekNext->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");
+
+  verifyFormat("void foo() {\n"
+               "  if (true) return\n"
+               "  v = 42\n"
+               "}");
+  verifyFormat("void foo() {\n"
+               "  if (true) return\n"
+               "  const v = 42\n"
+               "}");
+  verifyFormat("void foo() {\n"
+               "  if (true) return\n"
+               "  let v = 42\n"
+               "}");
+  verifyFormat("void foo() {\n"
+               "  if (true) return\n"
+               "  var v = 42\n"
+               "}");
+}
+
 } // namespace format
 } // end namespace clang