clang-format: [JS] options for arrow functions.

[mprobst]

Summary

clang-format currently always wraps the body of non-empty arrow
functions:

const x = () => {
  z();
};

This change implements support for the AllowShortLambdasOnASingleLine
style options, and also sets the default Google style to SLS_All, i.e.
to indent arrow function bodies that have one or fewer statements on a
single line:

const x = () => { z(); };

Multi-statement arrow functions continue to be wrapped. Function
expressions (a = function() {}) and function/method declarations are
unaffected as well.

[mprobst]

FYI I've started a conversation on whether SLS_All should be the default Google style. But the option I think is useful in either case.

[krasimir]

FYI I've started a conversation on whether SLS_All should be the default Google style. But the option I think is useful in either case.

If that's the case, consider adding the option to the style in a follow-up patch:

GoogleStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;

that would have an advantage of making clear which test case diffs are result of the implementation and which are result of the style flip.

[mprobst]

If that's the case, consider adding the option to the style in a follow-up patch:

GoogleStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;

that would have an advantage of making clear which test case diffs are result of the implementation and which are result of the style flip.

Done!

[mprobst]

• Disable arrow functions on single lines by default.

[krasimir]

clang/unittests/Format/FormatTestJS.cpp

1793

we probably want to keep the empty body test case.

[RKSimon]

clang/lib/Format/TokenAnnotator.cpp

3165

@mprobst - this is breaking buildbots: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-android/builds/27196/steps/bootstrap%20clang/logs/stdio

Please can you remove the default case and add a suitable llvm_unreachable() after the switch statement ?

[mprobst]

clang/lib/Format/TokenAnnotator.cpp

3165

Sorry for the breakage, and thanks to Jinsong Ji for fixing while I was OOO.

clang/lib/Format/Format.cpp

if (Language == FormatStyle::LK_Java) {
     GoogleStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
     GoogleStyle.ColumnLimit = 100;
     GoogleStyle.SpaceAfterCStyleCast = true;
     GoogleStyle.SpacesBeforeTrailingComments = 1;
   } else if (Language == FormatStyle::LK_JavaScript) {
     GoogleStyle.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
     GoogleStyle.AlignOperands = false;
     GoogleStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty;
+    // TODO: still under discussion whether to switch to SLS_All.
+    GoogleStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_Empty;
     GoogleStyle.AlwaysBreakBeforeMultilineStrings = false;
     GoogleStyle.BreakBeforeTernaryOperators = false;
     // taze:, triple slash directives (`/// <...`), tslint:, and @see, which is
     // commonly followed by overlong URLs.
     GoogleStyle.CommentPragmas = "(taze:|^/[ \t]*<|tslint:|@see)";
     GoogleStyle.MaxEmptyLinesToKeep = 3;
     GoogleStyle.NamespaceIndentation = FormatStyle::NI_All;
     GoogleStyle.SpacesInContainerLiterals = false;

clang/lib/Format/TokenAnnotator.cpp

if (Style.Language == FormatStyle::LK_JavaScript) {
     if (Left.is(tok::l_brace) && Line.Level == 0 &&
         (Line.startsWith(tok::kw_enum) ||
          Line.startsWith(tok::kw_const, tok::kw_enum) ||
          Line.startsWith(tok::kw_export, tok::kw_enum) ||
          Line.startsWith(tok::kw_export, tok::kw_const, tok::kw_enum)))
       // JavaScript top-level enum key/value pairs are put on separate lines
       // instead of bin-packing.
       return true;
+    if (Right.is(tok::r_brace) && Left.is(tok::l_brace) && Left.Previous &&
+        Left.Previous->is(TT_JsFatArrow)) {
+      // JS arrow function (=> {...}).
+      switch (Style.AllowShortLambdasOnASingleLine) {
+      case FormatStyle::SLS_All:
+        return false;
+      case FormatStyle::SLS_None:
+        return true;
+      case FormatStyle::SLS_Empty:
+        return !Left.Children.empty();
+      case FormatStyle::SLS_Inline:
+        // allow one-lining inline (e.g. in function call args) and empty arrow
+        // functions.
+        return (Left.NestingLevel == 0 && Line.Level == 0) &&
+               !Left.Children.empty();
+      default:
+        break;
+      }
+    }
+
     if (Right.is(tok::r_brace) && Left.is(tok::l_brace) &&
         !Left.Children.empty())
       // Support AllowShortFunctionsOnASingleLine for JavaScript.
       return Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_None ||
              Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Empty ||
              (Left.NestingLevel == 0 && Line.Level == 0 &&
               Style.AllowShortFunctionsOnASingleLine &
                   FormatStyle::SFS_InlineOnly);

clang/unittests/Format/FormatTestJS.cpp

TEST_F(FormatTestJS, ContainerLiterals) {
   verifyFormat("function x() {\n"
                "  y = {z: 1};\n"
                "}");
   verifyFormat("x = foo && {a: 123};");
 
   // Arrow functions in object literals.
   verifyFormat("var x = {\n"
                "  y: (a) => {\n"
+               "    x();\n"
                "    return a;\n"
-               "  }\n"
+               "  },\n"
                "};");
   verifyFormat("var x = {y: (a) => a};");
 
   // Methods in object literals.
   verifyFormat("var x = {\n"
                "  y(a: string): number {\n"
                "    return a;\n"
                "  }\n"
TEST_F(FormatTestJS, ContainerLiterals) {
   verifyFormat("var x = {\n"
                "  [a]: 1,\n"
                "  b: 2,\n"
                "  [c]: 3,\n"
                "};");
 
   // Object literals can leave out labels.
   verifyFormat("f({a}, () => {\n"
-               "  g();  //\n"
+               "  x;\n"
+               "  g();\n"
                "});");
 
   // Keys can be quoted.
   verifyFormat("var x = {\n"
                "  a: a,\n"
                "  b: b,\n"
                "  'c': c,\n"
                "};");
verifyFormat("return [aaaaaaaaaaaaaaaaaaaaaa]\n"
                "    .aaaaaaa(function() {\n"
                "      //\n"
                "    })\n"
                "    .bbbbbb();");
 }
 
 TEST_F(FormatTestJS, ArrowFunctions) {
   verifyFormat("var x = (a) => {\n"
+               "  x;\n"
                "  return a;\n"
-               "};");
+               "};\n");
   verifyFormat("var x = (a) => {\n"
                "  function y() {\n"
                "    return 42;\n"
                "  }\n"
                "  return a;\n"
                "};");
   verifyFormat("var x = (a: type): {some: type} => {\n"
+               "  y;\n"
                "  return a;\n"
                "};");
   verifyFormat("var x = (a) => a;");
   verifyFormat("return () => [];");
   verifyFormat("var aaaaaaaaaaaaaaaaaaaa = {\n"
                "  aaaaaaaaaaaaaaaaaaaaaaaaaaaa:\n"
                "      (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
                "       aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) =>\n"
TEST_F(FormatTestJS, ArrowFunctions) {
   // FIXME: This is bad, we should be wrapping before "() => {".
   verifyFormat("someFunction(() => {\n"
                "  doSomething();  // break\n"
                "})\n"
                "    .doSomethingElse(\n"
                "        // break\n"
                "    );");
   verifyFormat("const f = (x: string|null): string|null => {\n"
+               "  y;\n"
                "  return x;\n"
                "}\n");
 }
 
+TEST_F(FormatTestJS, ArrowFunctionStyle) {
+  FormatStyle Style = getGoogleStyle(FormatStyle::LK_JavaScript);
+  Style.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
+  verifyFormat("const arr = () => { x; };", Style);
+  verifyFormat("const arrInlineAll = () => {};", Style);
+  Style.AllowShortLambdasOnASingleLine = FormatStyle::SLS_None;
+  verifyFormat("const arr = () => {\n"
+               "  x;\n"
+               "};",
+               Style);
+  verifyFormat("const arrInlineNone = () => {\n"
+               "};",
+               Style);
+  Style.AllowShortLambdasOnASingleLine = FormatStyle::SLS_Empty;
+  verifyFormat("const arr = () => {\n"
+               "  x;\n"
+               "};",
+               Style);
+  verifyFormat("const arrInlineEmpty = () => {};",
+               Style);
+  Style.AllowShortLambdasOnASingleLine = FormatStyle::SLS_Inline;
+  verifyFormat("const arr = () => {\n"
+               "  x;\n"
+               "};",
+               Style);
+  verifyFormat("foo(() => {});",
+               Style);
+  verifyFormat("const arrInlineInline = () => {};", Style);
+}
+
 TEST_F(FormatTestJS, ReturnStatements) {
   verifyFormat("function() {\n"
                "  return [hello, world];\n"
                "}");
 }
 
 TEST_F(FormatTestJS, ForLoops) {
   verifyFormat("for (var i in [2, 3]) {\n"
verifyFormat(
       "}\n",
       "interface AbstractStrategyFactoryProvider { a: number }\n",
       Style);
 }
 
 TEST_F(FormatTestJS, RemoveEmptyLinesInArrowFunctions) {
   verifyFormat("x = () => {\n"
                "  foo();\n"
+               "  bar();\n"
                "};\n",
                "x = () => {\n"
                "\n"
                "  foo();\n"
+               "  bar();\n"
                "\n"
                "};\n");
 }
 
 TEST_F(FormatTestJS, Modules) {
   verifyFormat("import SomeThing from 'some/module.js';");
   verifyFormat("import {X, Y} from 'some/module.js';");
   verifyFormat("import a, {X, Y} from 'some/module.js';");
verifyFormat("export enum Foo {\n"
                "  // adsdasd\n"
                "  BAZ\n"
                "}");
   verifyFormat("export default [\n"
                "  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
                "  bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\n"
                "];");
   verifyFormat("export default [];");
   verifyFormat("export default () => {};");
+  verifyFormat("export default () => {\n"
+               "  x;\n"
+               "  x;\n"
+               "};");
   verifyFormat("export interface Foo {\n"
                "  foo: number;\n"
                "}\n"
                "export class Bar {\n"
                "  blah(): string {\n"
                "    return this.blah;\n"
                "  };\n"
                "}");