clang-format: [JS] simplify template string wrapping.

[mprobst]

Summary

Previously, clang-format would try to wrap template string substitutions
by indenting relative to the openening ${. This helped with
indenting structured strings, such as strings containing HTML, as the
substitutions would be aligned according to the structure of the string.

However it turns out that the overwhelming majority of template string +
substitution usages are for substitutions into non-structured strings,
e.g. URLs or just plain messages. For these situations, clang-format
would often produce very ugly indents, in particular for strings
containing no line breaks:

return `<a href='http://google3/${file}?l=${row}'>${file}</a>(${
                                                                row
                                                              },${
                                                                  col
                                                                }): `;

This change makes clang-format indent template string substitutions as
if they were string concatenation operations. It wraps +4 on overlong
lines and keeps all operands on the same line:

return `<a href='http://google3/${file}?l=${row}'>${file}</a>(${
    row},${col}): `;

While this breaks some lexical continuity between the ${ and row}
here, the overall effects are still a huge improvement, and users can
still manually break the string using + if desired.

[djasper]

Yay for *removing* complexity for a change :).
Let me know how it goes in practice.

[mprobst]

• clang-format: [JS] wrap calls w/ literals in template strings.

[djasper]

lib/Format/ContinuationIndenter.cpp

1139 ↗

(On Diff #112858)

This is not the right way to implement this:

• This is a static computation that we could do ahead of time. Doing it inside the combinatorial exploration of solutions is a waste.
• You are doing this always, even in code that doesn't have template strings or isn't even JavaScript.
• This can lead to unexpected behavior if the template string is in a completely unrelated part of the statement. E.g.

someFunction(`test`, { ... });

will be formatted differently from

someFunction('test', { ... });

[mprobst]

lib/Format/ContinuationIndenter.cpp

1139 ↗

(On Diff #112858)

Ack. Do you have suggestions?

I could introduce a bool ParenState::NoLineBreakMustPropagate; that controls this behaviour.

[djasper]

lib/Format/ContinuationIndenter.cpp

1139 ↗

(On Diff #112858)

That, too, would create a significant memory and runtime overhead to everyone just to fix this rather minor formatting detail.
If we were to set MatchingParen correctly to match up "${" to "}", we could measure the respective length and at a very early state (mustBreak()) decide that we need a line break between "${" and the corresponding "}". Now setting that correctly might be hard. Maybe it is ok, to linearly scan in that case as we would only do that if we actually find a template string ending in "${" and the distance to the "}" is usually short.

[mprobst]

lib/Format/ContinuationIndenter.cpp

1139 ↗

(On Diff #112858)

I cannot decide whether I must break in mustBreak - I need to just force wrapping after ${ if I need to wrap somewhere below, but if the code fits on the line, I don't want to wrap. So when I'm visiting ${ I cannot decide that, and when I'm at the object literal expression, the decision for ${ has already been made - right? Or am I missing something?

[mprobst]

• drop function name wrapping hack

[mprobst]

lib/Format/ContinuationIndenter.cpp

1139 ↗

(On Diff #112858)

I've dropped the function wrapping fix and will land like this, apparently we'll need to think about this a bit more.

cfe/trunk/lib/Format/ContinuationIndenter.cpp

if (PreviousNonComment &&
           TT_LeadingJavaAnnotation) &&
       Current.isNot(TT_BinaryOperator) && !PreviousNonComment->opensScope())
     State.Stack.back().BreakBeforeParameter = true;
 
   // If we break after { or the [ of an array initializer, we should also break
   // before the corresponding } or ].
   if (PreviousNonComment &&
       (PreviousNonComment->isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
-       opensProtoMessageField(*PreviousNonComment, Style) ||
-       (PreviousNonComment->is(TT_TemplateString) &&
-        PreviousNonComment->opensScope())))
+       opensProtoMessageField(*PreviousNonComment, Style)))
     State.Stack.back().BreakBeforeClosingBrace = true;
 
   if (State.Stack.back().AvoidBinPacking) {
     // If we are breaking after '(', '{', '<', this is not bin packing
     // unless AllowAllParametersOfDeclarationOnNextLine is false or this is a
     // dict/object literal.
     if (!Previous.isOneOf(tok::l_paren, tok::l_brace, TT_BinaryOperator) ||
         (!Style.AllowAllParametersOfDeclarationOnNextLine &&
if (Previous && (Previous->isOneOf(tok::l_paren, tok::comma, tok::colon) ||
       !Previous->isOneOf(TT_DictLiteral, TT_ObjCMethodExpr)) {
     State.Stack.back().NestedBlockInlined =
         !Newline &&
         (Previous->isNot(tok::l_paren) || Previous->ParameterCount > 1);
   }
 
   moveStatePastFakeLParens(State, Newline);
   moveStatePastScopeCloser(State);
-  if (Current.is(TT_TemplateString) && Current.opensScope())
-    State.Stack.back().LastSpace =
-        (Current.IsMultiline ? Current.LastLineColumnWidth
-                             : State.Column + Current.ColumnWidth) -
-        strlen("${");
   bool CanBreakProtrudingToken = !State.Stack.back().NoLineBreak &&
                                  !State.Stack.back().NoLineBreakInOperand;
   moveStatePastScopeOpener(State, Newline);
   moveStatePastFakeRParens(State);
 
   if (Current.is(TT_ObjCStringLiteral) && State.StartOfStringLiteral == 0)
     State.StartOfStringLiteral = State.Column + 1;
   else if (Current.isStringLiteral() && State.StartOfStringLiteral == 0)
if (Current.isOneOf(tok::l_brace, TT_ArrayInitializerLSquare) ||
     //                       int> v);
     // FIXME: We likely want to do this for more combinations of brackets.
     // Verify that it is wanted for ObjC, too.
     if (Current.is(tok::less) && Current.ParentBracket == tok::l_paren) {
       NewIndent = std::max(NewIndent, State.Stack.back().Indent);
       LastSpace = std::max(LastSpace, State.Stack.back().Indent);
     }
 
-    // JavaScript template strings are special as we always want to indent
-    // nested expressions relative to the ${}. Otherwise, this can create quite
-    // a mess.
-    if (Current.is(TT_TemplateString)) {
-      unsigned Column = Current.IsMultiline
-                            ? Current.LastLineColumnWidth
-                            : State.Column + Current.ColumnWidth;
-      NewIndent = Column;
-      LastSpace = Column;
-      NestedBlockIndent = Column;
-    }
-
     bool EndsInComma =
         Current.MatchingParen &&
         Current.MatchingParen->getPreviousNonComment() &&
         Current.MatchingParen->getPreviousNonComment()->is(tok::comma);
 
     AvoidBinPacking =
         (Style.Language == FormatStyle::LK_JavaScript && EndsInComma) ||
         (State.Line->MustBeDeclaration && !Style.BinPackParameters) ||

cfe/trunk/unittests/Format/FormatTestJS.cpp

TEST_F(FormatTestJS, TemplateStrings) {
   // template strings.
   verifyFormat("var f = `aaaaaaaaaaaaa:${aaaaaaa.aaaaa} aaaaaaaa\n"
                "         aaaaaaaaaaaaa:${aaaaaaa.aaaaa} aaaaaaaa`;",
                "var f = `aaaaaaaaaaaaa:${aaaaaaa.  aaaaa} aaaaaaaa\n"
                "         aaaaaaaaaaaaa:${  aaaaaaa. aaaaa} aaaaaaaa`;");
   verifyFormat("var x = someFunction(`${})`)  //\n"
                "            .oooooooooooooooooon();");
   verifyFormat("var x = someFunction(`${aaaa}${\n"
-               "                               aaaaa(  //\n"
-               "                                   aaaaa)\n"
-               "                             })`);");
+               "    aaaaa(  //\n"
+               "        aaaaa)})`);");
 }
 
 TEST_F(FormatTestJS, TemplateStringMultiLineExpression) {
   verifyFormat("var f = `aaaaaaaaaaaaaaaaaa: ${\n"
-               "                               aaaaa +  //\n"
-               "                               bbbb\n"
-               "                             }`;",
+               "    aaaaa +  //\n"
+               "    bbbb}`;",
                "var f = `aaaaaaaaaaaaaaaaaa: ${aaaaa +  //\n"
                "                               bbbb}`;");
   verifyFormat("var f = `\n"
                "  aaaaaaaaaaaaaaaaaa: ${\n"
-               "                        aaaaa +  //\n"
-               "                        bbbb\n"
-               "                      }`;",
+               "    aaaaa +  //\n"
+               "    bbbb}`;",
                "var f  =  `\n"
                "  aaaaaaaaaaaaaaaaaa: ${   aaaaa  +  //\n"
                "                        bbbb }`;");
   verifyFormat("var f = `\n"
                "  aaaaaaaaaaaaaaaaaa: ${\n"
-               "                        someFunction(\n"
-               "                            aaaaa +  //\n"
-               "                            bbbb)\n"
-               "                      }`;",
+               "    someFunction(\n"
+               "        aaaaa +  //\n"
+               "        bbbb)}`;",
                "var f  =  `\n"
                "  aaaaaaaaaaaaaaaaaa: ${someFunction (\n"
                "                            aaaaa  +   //\n"
                "                            bbbb)}`;");
 
   // It might be preferable to wrap before "someFunction".
   verifyFormat("var f = `\n"
                "  aaaaaaaaaaaaaaaaaa: ${someFunction({\n"
-               "                          aaaa: aaaaa,\n"
-               "                          bbbb: bbbbb,\n"
-               "                        })}`;",
+               "  aaaa: aaaaa,\n"
+               "  bbbb: bbbbb,\n"
+               "})}`;",
                "var f  =  `\n"
                "  aaaaaaaaaaaaaaaaaa: ${someFunction ({\n"
                "                          aaaa:  aaaaa,\n"
                "                          bbbb:  bbbbb,\n"
                "                        })}`;");
 }
 
 TEST_F(FormatTestJS, TemplateStringASI) {