clang-format: [JavaScript] Do requoting in a separate pass

[djasper]

Summary

The attempt to fix requoting behavior in r280487 after changes to tooling::Replacements are incomplete. We essentially need to add to replacements at the same position, one to insert a line break and one to change the quoting and that's incompatible with the new tooling::Replacement API, which does not allow for order-dependent Replacements. To make the order clear, Replacements::merge() has to be used, but that requires the merged Replacement to actually refer to the changed text, which is hard to reproduce for the requoting.

This change fixes the behavior by moving the requoting to a completely separate pass. The added benefit is that no weird ColumnWidth calculations are necessary anymore and this should just work even if we implement string literal splitting in the future.

[mprobst]

lib/Format/Format.cpp

1707

I guess we don't worry about the performance consequences here?

[djasper]

lib/Format/Format.cpp

1707

I don't think so. Lexing is very fast and we do the same thing to sort includes. We only create a new virtual environment if there actually are quotes to change which should be rare and also creating this environment should be fast.

All in all, I'd ignore performance here until this actually shows up in a profile (or we trace a slow formatting back to it).

[djasper]

Submitted as r280874.

lib/Format/Format.cpp

std::string configurationAsText(const FormatStyle &Style) {
   // reference here.
   FormatStyle NonConstStyle = expandPresets(Style);
   Output << NonConstStyle;
   return Stream.str();
 }
 
 namespace {
 
-class Formatter : public TokenAnalyzer {
+class JavaScriptRequoter : public TokenAnalyzer {
 public:
-  Formatter(const Environment &Env, const FormatStyle &Style,
-            bool *IncompleteFormat)
-      : TokenAnalyzer(Env, Style), IncompleteFormat(IncompleteFormat) {}
+  JavaScriptRequoter(const Environment &Env, const FormatStyle &Style)
+      : TokenAnalyzer(Env, Style) {}
 
   tooling::Replacements
   analyze(TokenAnnotator &Annotator,
           SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
           FormatTokenLexer &Tokens) override {
-    tooling::Replacements RequoteReplaces;
-    deriveLocalStyle(AnnotatedLines);
     AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(),
                                           AnnotatedLines.end());
-
-    if (Style.Language == FormatStyle::LK_JavaScript &&
-        Style.JavaScriptQuotes != FormatStyle::JSQS_Leave)
-      requoteJSStringLiteral(AnnotatedLines, RequoteReplaces);
-
-    for (unsigned i = 0, e = AnnotatedLines.size(); i != e; ++i) {
-      Annotator.calculateFormattingInformation(*AnnotatedLines[i]);
-    }
-
-    Annotator.setCommentLineLevels(AnnotatedLines);
-
-    WhitespaceManager Whitespaces(
-        Env.getSourceManager(), Style,
-        inputUsesCRLF(Env.getSourceManager().getBufferData(Env.getFileID())));
-    ContinuationIndenter Indenter(Style, Tokens.getKeywords(),
-                                  Env.getSourceManager(), Whitespaces, Encoding,
-                                  BinPackInconclusiveFunctions);
-    UnwrappedLineFormatter(&Indenter, &Whitespaces, Style, Tokens.getKeywords(),
-                           IncompleteFormat)
-        .format(AnnotatedLines);
-    return RequoteReplaces.merge(Whitespaces.generateReplacements());
+    tooling::Replacements Result;
+    requoteJSStringLiteral(AnnotatedLines, Result);
+    return Result;
   }
 
 private:
-  // If the last token is a double/single-quoted string literal, generates a
-  // replacement with a single/double quoted string literal, re-escaping the
-  // contents in the process.
+  // Replaces double/single-quoted string literal as appropriate, re-escaping
+  // the contents in the process.
   void requoteJSStringLiteral(SmallVectorImpl<AnnotatedLine *> &Lines,
                               tooling::Replacements &Result) {
     for (AnnotatedLine *Line : Lines) {
       requoteJSStringLiteral(Line->Children, Result);
       if (!Line->Affected)
         continue;
       for (FormatToken *FormatTok = Line->First; FormatTok;
            FormatTok = FormatTok->Next) {
         StringRef Input = FormatTok->TokenText;
         if (FormatTok->Finalized || !FormatTok->isStringLiteral() ||
             // NB: testing for not starting with a double quote to avoid
-            // breaking
-            // `template strings`.
+            // breaking `template strings`.
             (Style.JavaScriptQuotes == FormatStyle::JSQS_Single &&
              !Input.startswith("\"")) ||
             (Style.JavaScriptQuotes == FormatStyle::JSQS_Double &&
              !Input.startswith("\'")))
           continue;
 
         // Change start and end quote.
         bool IsSingle = Style.JavaScriptQuotes == FormatStyle::JSQS_Single;
         SourceLocation Start = FormatTok->Tok.getLocation();
         auto Replace = [&](SourceLocation Start, unsigned Length,
                            StringRef ReplacementText) {
           auto Err = Result.add(tooling::Replacement(
               Env.getSourceManager(), Start, Length, ReplacementText));
           // FIXME: handle error. For now, print error message and skip the
           // replacement for release version.
           if (Err)
             llvm::errs() << llvm::toString(std::move(Err)) << "\n";
           assert(!Err);
         };
         Replace(Start, 1, IsSingle ? "'" : "\"");
         Replace(FormatTok->Tok.getEndLoc().getLocWithOffset(-1), 1,
                 IsSingle ? "'" : "\"");
 
         // Escape internal quotes.
-        size_t ColumnWidth = FormatTok->TokenText.size();
         bool Escaped = false;
         for (size_t i = 1; i < Input.size() - 1; i++) {
           switch (Input[i]) {
           case '\\':
             if (!Escaped && i + 1 < Input.size() &&
                 ((IsSingle && Input[i + 1] == '"') ||
                  (!IsSingle && Input[i + 1] == '\''))) {
               // Remove this \, it's escaping a " or ' that no longer needs
               // escaping
-              ColumnWidth--;
               Replace(Start.getLocWithOffset(i), 1, "");
               continue;
             }
             Escaped = !Escaped;
             break;
           case '\"':
           case '\'':
             if (!Escaped && IsSingle == (Input[i] == '\'')) {
               // Escape the quote.
               Replace(Start.getLocWithOffset(i), 0, "\\");
-              ColumnWidth++;
             }
             Escaped = false;
             break;
           default:
             Escaped = false;
             break;
           }
         }
-
-        // For formatting, count the number of non-escaped single quotes in them
-        // and adjust ColumnWidth to take the added escapes into account.
-        // FIXME(martinprobst): this might conflict with code breaking a long
-        // string literal (which clang-format doesn't do, yet). For that to
-        // work, this code would have to modify TokenText directly.
-        FormatTok->ColumnWidth = ColumnWidth;
       }
     }
   }
+};
+
+class Formatter : public TokenAnalyzer {
+public:
+  Formatter(const Environment &Env, const FormatStyle &Style,
+            bool *IncompleteFormat)
+      : TokenAnalyzer(Env, Style), IncompleteFormat(IncompleteFormat) {}
+
+  tooling::Replacements
+  analyze(TokenAnnotator &Annotator,
+          SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
+          FormatTokenLexer &Tokens) override {
+    tooling::Replacements Result;
+    deriveLocalStyle(AnnotatedLines);
+    AffectedRangeMgr.computeAffectedLines(AnnotatedLines.begin(),
+                                          AnnotatedLines.end());
+    for (unsigned i = 0, e = AnnotatedLines.size(); i != e; ++i) {
+      Annotator.calculateFormattingInformation(*AnnotatedLines[i]);
+    }
+    Annotator.setCommentLineLevels(AnnotatedLines);
+
+    WhitespaceManager Whitespaces(
+        Env.getSourceManager(), Style,
+        inputUsesCRLF(Env.getSourceManager().getBufferData(Env.getFileID())));
+    ContinuationIndenter Indenter(Style, Tokens.getKeywords(),
+                                  Env.getSourceManager(), Whitespaces, Encoding,
+                                  BinPackInconclusiveFunctions);
+    UnwrappedLineFormatter(&Indenter, &Whitespaces, Style, Tokens.getKeywords(),
+                           IncompleteFormat)
+        .format(AnnotatedLines);
+    for (const auto &R : Whitespaces.generateReplacements())
+      if (Result.add(R))
+        return Result;
+    return Result;
+  }
+
+private:
 
   static bool inputUsesCRLF(StringRef Text) {
     return Text.count('\r') * 2 > Text.count('\n');
   }
 
   bool
   hasCpp03IncompatibleFormat(const SmallVectorImpl<AnnotatedLine *> &Lines) {
     for (const AnnotatedLine *Line : Lines) {

 
 tooling::Replacements reformat(const FormatStyle &Style, StringRef Code,
                                ArrayRef<tooling::Range> Ranges,
                                StringRef FileName, bool *IncompleteFormat) {
   FormatStyle Expanded = expandPresets(Style);
   if (Expanded.DisableFormat)
     return tooling::Replacements();
 
-  std::unique_ptr<Environment> Env =
-      Environment::CreateVirtualEnvironment(Code, FileName, Ranges);
+  auto Env = Environment::CreateVirtualEnvironment(Code, FileName, Ranges);
+
+  if (Style.Language == FormatStyle::LK_JavaScript &&
+      Style.JavaScriptQuotes != FormatStyle::JSQS_Leave) {
+    JavaScriptRequoter Requoter(*Env, Expanded);
+    tooling::Replacements Requotes = Requoter.process();
+    if (!Requotes.empty()) {
+      auto NewCode = applyAllReplacements(Code, Requotes);
+      if (NewCode) {
+        auto NewEnv = Environment::CreateVirtualEnvironment(
+            *NewCode, FileName,
+            tooling::calculateRangesAfterReplacements(Requotes, Ranges));
+        Formatter Format(*NewEnv, Expanded, IncompleteFormat);
+        return Requotes.merge(Format.process());
+      }
+    }
+  }
+
   Formatter Format(*Env, Expanded, IncompleteFormat);
   return Format.process();
 }
 
 tooling::Replacements cleanup(const FormatStyle &Style, SourceManager &SM,
                               FileID ID, ArrayRef<CharSourceRange> Ranges) {
   Environment Env(SM, ID, Ranges);
   Cleaner Clean(Env, Style);

unittests/Format/FormatTestJS.cpp

verifyFormat("// clang-format off\n"
                "let x = \"single\";\n");
 }
 
 TEST_F(FormatTestJS, RequoteAndIndent) {
   verifyFormat("let x = someVeryLongFunctionThatGoesOnAndOn(\n"
                "    'double quoted string that needs wrapping');",
                "let x = someVeryLongFunctionThatGoesOnAndOn("
                "\"double quoted string that needs wrapping\");");
+
+  verifyFormat("let x =\n"
+               "    'foo\\'oo';\n"
+               "let x =\n"
+               "    'foo\\'oo';",
+               "let x=\"foo'oo\";\n"
+               "let x=\"foo'oo\";",
+               getGoogleJSStyleWithColumns(15));
 }
 
 TEST_F(FormatTestJS, RequoteStringsDouble) {
   FormatStyle DoubleQuotes = getGoogleStyle(FormatStyle::LK_JavaScript);
   DoubleQuotes.JavaScriptQuotes = FormatStyle::JSQS_Double;
   verifyFormat("var x = \"foo\";", DoubleQuotes);
   verifyFormat("var x = \"foo\";", "var x = 'foo';", DoubleQuotes);
   verifyFormat("var x = \"fo'o\";", "var x = 'fo\\'o';", DoubleQuotes);