clang-format: [JS] re-quote single or double quoted strings.

[mprobst]

Summary

Automatically turns "foo" into 'foo' (or vice versa, depending on configuration).
This makes it more convenient to follow the Google JavaScript style guide:
https://google.github.io/styleguide/javascriptguide.xml?showone=Strings#Strings

This functionality is behind the option "Quotes", which can be:

• "leave" (no re-quoting)
• "single" (change to single quotes)
• or "double" (change to double quotes)

This also changes single quoted JavaScript string literals to be treated as
tok::string_literal, not tok::char_literal, which fixes two unrelated tests.

[mprobst]

Fixed typo.

[djasper]

lib/Format/Format.cpp

1579

What's the reason not to do this in FormatTokenLexer, ideally at the same time when increasing the length?

1585

LLVM comes with its own string stream implementation. Use that: raw_string_ostream.

[mprobst]

• Move code closer together by piping Replacements into the FormatTokenLexer.

[mprobst]

lib/Format/Format.cpp

1579

Done. I initially wanted to avoid having FormatTokenLexer generate Replacements, but this seems cleaner indeed.

[mprobst]

• Move code closer together by piping Replacements into the FormatTokenLexer.
• remove sstream import

[djasper]

Nice, the behavior now seems nicely isolated. Is this a purely stylistic kind of change or do you think every JS developer working on any kind of codebase will want it? Generally this feels like something we should carefully enable with a dedicated style option. Where can I find more information about it?

[mprobst]

Nice, the behavior now seems nicely isolated. Is this a purely stylistic
kind of change or do you think every JS developer working on any kind of
codebase will want it? Generally this feels like something we should
carefully enable with a dedicated style option. Where can I find more
information about it?

According to Google JavaScript style, you should always use single quotes
for string literals. Outside of that, people usually prefer one over the
other (I think), some people just use both indiscriminately. We could make
it an option, and only turn it on for a specific Google style.

Martin

[djasper]

Sounds like a good plan. Should also mention the corresponding style guide reference in the commit message:

https://google.github.io/styleguide/javascriptguide.xml?showone=Strings#Strings

[mprobst]

• Move code closer together by piping Replacements into the FormatTokenLexer.
• remove sstream import
• * Treat JavaScript single quoted string literals as tok::string_literal, not

[djasper]

include/clang/Format/Format.h

606 ↗

(On Diff #49214)

As this only affects JavaScript and doesn't make any sense for Java or C++, I'd like to have JS/JavaScript in the option and enum name.

lib/Format/Format.cpp

77

For almost all the other enums we use upper case.

1075

Are there more than the two options? Otherwise, I would find:

Style.Quotes == FormatStyle::QS_Single && FormatTok->TokenText.startswith("\'")

more intuitive.

1082

Move this up so you can use it in the if-statement?

unittests/Format/FormatTestJS.cpp

639

Why change this test? I think this was meant to test that we always add a line break between two consecutive string literals as otherwise the user could just have written this without the "' + '".

877

I don't think this is intended.

[mprobst]

• Move code closer together by piping Replacements into the FormatTokenLexer.
• remove sstream import
• * Treat JavaScript single quoted string literals as tok::string_literal, not
• Address review comments, mostly renaming things.

lib/Format/Format.cpp

1075

I would too, but think about template strings. Added a comment. I think it's safer to specifically check for the thing that we do handle/fix below, instead of just accepting everything we think is not correct.

unittests/Format/FormatTestJS.cpp

639

This was exposed by the char_literal/string_literal test, it aligns the behaviour with what would always happen for a double quoted string. It fits the no-alignment rule.

I've changed the test to make sure it actually wraps on two lines.

877

It is. This was incorrectly handled before, the line is longer than 20 chars. It's because I switched single quoted strings from char_literal to string_literal.

[mprobst]

• Move code closer together by piping Replacements into the FormatTokenLexer.
• remove sstream import
• * Treat JavaScript single quoted string literals as tok::string_literal, not
• Address review comments, mostly renaming things.
• Fix mustBreakBefore to handle tok::string_literal

[djasper]

lib/Format/Format.cpp

1119

Maybe add a FIXME saying that we also need to modify the TokenText if we want to be able to correctly split a long string literal.

1122

Thinking about this some more, I think it would be better to have replacements just changing each of the replaced quotes. Otherwise, this can get really tricky once we want to start to automatically split up long string literals. Should not make a difference performance-wise.

[mprobst]

• Move code closer together by piping Replacements into the FormatTokenLexer.
• remove sstream import
• * Treat JavaScript single quoted string literals as tok::string_literal, not
• Address review comments, mostly renaming things.
• Fix mustBreakBefore to handle tok::string_literal
• Insert individual replacements for each character change.

[mprobst]

unittests/Format/FormatTestJS.cpp

639–640

Reverted after some more discussion, this is the "must break before otherwise pointless string wrapping" feature.

[djasper]

Looks good.

lib/Format/Format.cpp

1128

I think this function doesn't carry its own weight.

[djasper]

Submitted as r262534.

lib/Format/Format.cpp

 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/YAMLTraits.h"
 #include <queue>
+#include <sstream>
 #include <string>
 
 #define DEBUG_TYPE "format-formatter"
 
 using clang::format::FormatStyle;
 
 LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(std::string)
 LLVM_YAML_IS_SEQUENCE_VECTOR(clang::format::FormatStyle::IncludeCategory)
static void enumeration(IO &IO, FormatStyle::UseTabStyle &Value) {
     IO.enumCase(Value, "Always", FormatStyle::UT_Always);
     IO.enumCase(Value, "true", FormatStyle::UT_Always);
     IO.enumCase(Value, "ForIndentation", FormatStyle::UT_ForIndentation);
   }
 };
 
 template <> struct ScalarEnumerationTraits<FormatStyle::ShortFunctionStyle> {
   static void enumeration(IO &IO, FormatStyle::ShortFunctionStyle &Value) {
     IO.enumCase(Value, "None", FormatStyle::SFS_None);
     IO.enumCase(Value, "false", FormatStyle::SFS_None);
     IO.enumCase(Value, "All", FormatStyle::SFS_All);
     IO.enumCase(Value, "true", FormatStyle::SFS_All);
     IO.enumCase(Value, "Inline", FormatStyle::SFS_Inline);
     IO.enumCase(Value, "Empty", FormatStyle::SFS_Empty);
   }
 };
 
if (Tokens.size() < 4)
       return false;
     FormatToken *Last = Tokens.back();
     if (!Last->is(tok::r_paren))
       return false;
 
     FormatToken *String = Tokens[Tokens.size() - 2];
     if (!String->is(tok::string_literal) || String->IsMultiline)
       return false;
 
     if (!Tokens[Tokens.size() - 3]->is(tok::l_paren))
       return false;
 
     FormatToken *Macro = Tokens[Tokens.size() - 4];
     if (Macro->TokenText != "_T")
       return false;
 
     const char *Start = Macro->TokenText.data();
     const char *End = Last->TokenText.data() + Last->TokenText.size();
     String->TokenText = StringRef(Start, End - Start);
     String->IsFirst = Macro->IsFirst;
     String->LastNewlineOffset = Macro->LastNewlineOffset;
     String->WhitespaceRange = Macro->WhitespaceRange;
     String->OriginalColumn = Macro->OriginalColumn;
     String->ColumnWidth = encoding::columnWidthWithTabs(
bool tryMergeConflictMarkers() {
     // We merge all tokens in a line that starts with a conflict marker
     // into a single token with a special token type that the unwrapped line
     // parser will use to correctly rebuild the underlying code.
 
     FileID ID;
     // Get the position of the first token in the line.
     unsigned FirstInLineOffset;
     std::tie(ID, FirstInLineOffset) = SourceMgr.getDecomposedLoc(
         Tokens[FirstInLineIndex]->getStartOfNonWhitespace());
     StringRef Buffer = SourceMgr.getBuffer(ID)->getBuffer();
     // Calculate the offset of the start of the current line.
     auto LineOffset = Buffer.rfind('\n', FirstInLineOffset);
     if (LineOffset == StringRef::npos) {
       LineOffset = 0;
     } else {
       ++LineOffset;
     }
 
     auto FirstSpace = Buffer.find_first_of(" \n", LineOffset);
     StringRef LineStart;
     if (FirstSpace == StringRef::npos) {
       LineStart = Buffer.substr(LineOffset);
     } else {
       LineStart = Buffer.substr(LineOffset, FirstSpace - LineOffset);
     }
 
if (FirstNewlinePos == StringRef::npos) {
       // The last line of the token always starts in column 0.
       // Thus, the length can be precomputed even in the presence of tabs.
       FormatTok->LastLineColumnWidth = encoding::columnWidthWithTabs(
           Text.substr(Text.find_last_of('\n') + 1), 0, Style.TabWidth,
           Encoding);
       Column = FormatTok->LastLineColumnWidth;
     }
 
+    if (Style.Language == FormatStyle::LK_JavaScript &&
+        FormatTok->isStringLiteral() && FormatTok->TokenText.startswith("\"")) {
+      // Double quoted JavaScript strings get requoted as single quoted below.
+      // For formatting, count the number of non-escaped single quotes in them
+      // and adjust ColumnWidth to take the later added escapes into account.
+
+      StringRef Input = FormatTok->TokenText;
+      bool Escaped = false;
+      size_t ColumnWidth = FormatTok->TokenText.size();
+      for (size_t i = 0; i != Input.size(); ++i) {
+        switch (Input[i]) {
+        case '\\':
+          Escaped = !Escaped;
+          break;
+        case '\'':
+          if (!Escaped) {
+            // Will later need to insert a \ to escape the double quote.
+            ColumnWidth++;
+          }
+          Escaped = false;
+          break;
+        default:
+          Escaped = false;
+          break;
+        }
+      }
+      FormatTok->ColumnWidth = ColumnWidth;
+    }
+
     if (Style.Language == FormatStyle::LK_Cpp) {
       if (!(Tokens.size() > 0 && Tokens.back()->Tok.getIdentifierInfo() &&
             Tokens.back()->Tok.getIdentifierInfo()->getPPKeywordID() ==
                 tok::pp_define) &&
           std::find(ForEachMacros.begin(), ForEachMacros.end(),
                     FormatTok->Tok.getIdentifierInfo()) != ForEachMacros.end()) {
         FormatTok->Type = TT_ForEachMacro;
       } else if (FormatTok->is(tok::identifier)) {
tooling::Replacements format(SmallVectorImpl<AnnotatedLine *> &AnnotatedLines,
 
     Annotator.setCommentLineLevels(AnnotatedLines);
     ContinuationIndenter Indenter(Style, Tokens.getKeywords(), SourceMgr,
                                   Whitespaces, Encoding,
                                   BinPackInconclusiveFunctions);
     UnwrappedLineFormatter(&Indenter, &Whitespaces, Style, Tokens.getKeywords(),
                            IncompleteFormat)
         .format(AnnotatedLines);
-    return Whitespaces.generateReplacements();
+    tooling::Replacements Replaces = Whitespaces.generateReplacements();
+    if (Style.Language == FormatStyle::LK_JavaScript) {
+      FormatToken *Tok = (*AnnotatedLines.begin())->First;
+      requoteJSStringLiterals(Replaces, Tok);
+    }
+    return Replaces;
   }
 
 private:
   // Determines which lines are affected by the SourceRanges given as input.
   // Returns \c true if at least one line between I and E or one of their
   // children is affected.
   bool computeAffectedLines(SmallVectorImpl<AnnotatedLine *>::iterator I,
                             SmallVectorImpl<AnnotatedLine *>::iterator E) {
while (I != E) {
         SomeLineAffected = true;
 
       PreviousLine = Line;
       ++I;
     }
     return SomeLineAffected;
   }
 
+  // Finds all double-quoted string literals, and replaces them with single
+  // quoted string literals, escaping the contents in the process.
+  // The width of the extra \ escapes is taken into account in getNextToken.
+  void requoteJSStringLiterals(tooling::Replacements &Replaces,
+                               FormatToken *Current) {
+    for (; Current != nullptr; Current = Current->Next) {
+      if (!Current->isStringLiteral() || !Current->TokenText.startswith("\""))
+        continue;
+      StringRef Input = Current->TokenText;
+      std::stringstream Res;
+      Res << '\'';
+      bool escaped = false;
+      for (size_t i = 1; i < Input.size() - 1; i++) {
+        switch (Input[i]) {
+        case '\\':
+          escaped = !escaped;
+          break;
+        case '\'':
+          if (!escaped) {
+            Res << '\\';
+          }
+          escaped = false;
+          break;
+        default:
+          escaped = false;
+          break;
+        }
+        Res << Input[i];
+      }
+      Res << '\'';
+      SourceRange Range(Current->Tok.getLocation(), Current->Tok.getEndLoc());
+      Replaces.insert(tooling::Replacement(
+          SourceMgr, CharSourceRange::getCharRange(Range), Res.str()));
+    }
+  }
+
   // Determines whether 'Line' is affected by the SourceRanges given as input.
   // Returns \c true if line or one if its children is affected.
   bool nonPPLineAffected(AnnotatedLine *Line,
                          const AnnotatedLine *PreviousLine) {
     bool SomeLineAffected = false;
     Line->ChildrenAffected =
         computeAffectedLines(Line->Children.begin(), Line->Children.end());
     if (Line->ChildrenAffected)

unittests/Format/FormatTestJS.cpp

verifyFormat("var object_literal_with_long_name = {\n"
                "  b: 'bbbbbbbbbbbbbbbbbb'\n"
                "};");
 
   verifyFormat("f({a: 1, b: 2, c: 3});",
                getChromiumStyle(FormatStyle::LK_JavaScript));
   verifyFormat("f({'a': [{}]});");
 }
 
-TEST_F(FormatTestJS, SingleQuoteStrings) {
+TEST_F(FormatTestJS, SingleQuotedStrings) {
   verifyFormat("this.function('', true);");
 }
 
 TEST_F(FormatTestJS, GoogScopes) {
   verifyFormat("goog.scope(function() {\n"
                "var x = a.b;\n"
                "var y = c.d;\n"
                "});  // goog.scope");
TEST_F(FormatTestJS, TryCatch) {
   // But, of course, "catch" is a perfectly fine function name in JavaScript.
   verifyFormat("someObject.catch();");
   verifyFormat("someObject.new();");
   verifyFormat("someObject.delete();");
 }
 
 TEST_F(FormatTestJS, StringLiteralConcatenation) {
   verifyFormat("var literal = 'hello ' +\n"
                "    'world';");
 }
 
 TEST_F(FormatTestJS, RegexLiteralClassification) {
   // Regex literals.
   verifyFormat("var regex = /abc/;");
   verifyFormat("f(/abc/);");
   verifyFormat("f(abc, /abc/);");
   verifyFormat("some_map[/abc/];");
   verifyFormat("var x = a ? /abc/ : /abc/;");
verifyFormat("import {\n"
                "} from 'some/module.js';");
   verifyFormat("import {\n"
                "  X,\n"
                "  Y,\n"
                "} from 'some/module.js';");
   verifyFormat("import {\n"
                "  X,\n"
                "  Y,\n"
                "} from 'some/long/module.js';",
                getGoogleJSStyleWithColumns(20));
   verifyFormat("import {X as myLocalX, Y as myLocalY} from 'some/module.js';");
   verifyFormat("import * as lib from 'some/module.js';");
   verifyFormat("var x = {import: 1};\nx.import = 2;");
 
   verifyFormat("export function fn() {\n"
                "  return 'fn';\n"
                "}");
EXPECT_EQ("/**\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)));
 }
 
+TEST_F(FormatTestJS, RequoteDoubleQuotedStrings) {
+  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
+            // escaped added.
+            format("var x = \"foo'\";", getGoogleJSStyleWithColumns(15)));
+}
+
 } // end namespace tooling
 } // end namespace clang