[clang-format] Add new option to support adding a space between Javascript Union and Intersection types

[MyDeveloperDay]

Summary

https://github.com/llvm/llvm-project/issues/49858

The following issue highlights a problem where we cannot add a space between A|B when detecting a JSTypeOperator

This patch allows clang-format to be configured to support that (which seems to match most references I can find)

https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#union-types
https://flow.org/en/docs/types/intersections/

Fixes: #49858

[mprobst]

clang/lib/Format/TokenAnnotator.cpp

3520–3524

shouldn't you change this line here?

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

3520–3524

You know I thought the same and this was where I first put the code change in, but this code doesn't seem to have any effect and I've been caught out by this before... (anyone else seen that?)

I'm not sure if something has been changed, but I'm finding that often I add code into spaceRequiredBetween() and despite it being executed correctly it doesn't have the desired effect, which is why the code is in spaceRequiredBefore()

[mprobst]

clang/lib/Format/TokenAnnotator.cpp

3520–3524

Generally we put space between two operators. So this line must have some effect, as otherwise we'd always emit A | B. Given that, I think we need more debugging here to make this change - working around by some more code somewhere else doesn't seem like a good long term approach.

Also, note that this is all in spaceRequiredBefore, right?

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

3475

Be explicit on the binding.

3520–3524

It's on my plan to refactor these 2 methods, because I think they can and should be made easier to understand and reason about.

So from my point of view one can accept this patch.

[curdeius]

LGTM apart from nits.
I agree though that spaceRequired(Before|Between) need some refactoring love.

clang/docs/ReleaseNotes.rst

327

Also, why capital letters on union and intersection type?

clang/include/clang/Format/Format.h

3618

[curdeius]

clang/unittests/Format/FormatTestJS.cpp

1708–1726

Oh, you might need to add a test for this case too.

[curdeius]

clang/lib/Format/TokenAnnotator.cpp

3520–3526

I modified it like this and it passes all the tests. Please recheck.

[mprobst]

clang/lib/Format/TokenAnnotator.cpp

3520–3524

I appreciate the intention to refactor this in the future.

I still believe we really have to understand how this code works though, and why changing this line doesn't have the effect we'd expect, before we land the fix. Chances are there is some substantial misunderstanding how this all fits together, which usually leads to bugs.

[curdeius]

clang/unittests/Format/FormatTestJS.cpp

1705–1726

Suggestion.

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

3520–3524

@mprobst I do agree, let me take another look.

[MyDeveloperDay]

Make suggested improvements
Add additional test case

[mprobst]

clang/lib/Format/TokenAnnotator.cpp

3522

Why do we need this further qualification here? I'd have expect that you can simply "return Style.SpacesInJavaScriptUnion;"? identifier || template closer also sounds oddly specific, why exactly those?

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

3522

Left = template closer, Right =JsTypeOperator is for this case

type Foo = Bar<X> | Baz;
               ^^^^^

Left = type or identifier, Right =JsTypeOperator is for this case

let x: A | B = A | B;
    ^^^^^^^^^

[curdeius]

Looks ok from my side.

[mprobst]

clang/lib/Format/TokenAnnotator.cpp

3522

What about:

let x: {foo: string}&{bar: number};

I'm not sure including curlies would be comprehensive. It might be worth checking the TS grammar.

But coming back, did you try this:

if (Left.is(TT_JsTypeOperator) || Right.is(TT_JsTypeOperator)) {
  return Style.SpacesInJavaScriptUnion;

(We might also need to do something in spaceRequiredBefore, maybe that fires earlier?)

[shafik]

Is this change still relevant or can we close this?

clang/docs/ClangFormatStyleOptions.rst

**SpacesInContainerLiterals** (``Boolean``) :versionbadge:`clang-format 3.7`
   ObjC and Javascript array and dict literals).
 
   .. code-block:: js
 
      true:                                  false:
      var arr = [ 1, 2, 3 ];         vs.     var arr = [1, 2, 3];
      f({a : 1, b : 2, c : 3});              f({a: 1, b: 2, c: 3});
 
+**SpacesInJavaScriptUnion** (``Boolean``) :versionbadge:`clang-format 14`
+  .. code-block:: c++
+
+     true:                                  false:
+     let x: A | B;   vs.                    let x: A|B;
+
 **SpacesInLineCommentPrefix** (``SpacesInLineComment``) :versionbadge:`clang-format 14`
   How many spaces are allowed at the start of a line comment. To disable the
   maximum set it to ``-1``, apart from that the maximum takes precedence
   over the minimum.
 
   .. code-block:: c++
 
     Minimum = 1

clang/docs/ReleaseNotes.rst

- The command line argument `-style=<string>` has been extended so that a specific
   format file at location <format_file_path> can be selected. This is supported
   via the syntax: `-style=file:<format_file_path>`.
 
 - Improved C++20 Modules and Coroutines support.
 
 - Option ``AfterOverloadedOperator`` has been added in ``SpaceBeforeParensOptions``
   to allow space between overloaded operator and opening parentheses.
 
+- Option ``SpacesInJavaScriptUnion`` has been added to allow spaces around
+  JavaScript union and intersection type operators.
+
 libclang
 --------
 
 - ...
 
 Static Analyzer
 ---------------
 

clang/include/clang/Format/Format.h

///    NS_TABLE_.*_END$"
   ///   Maximum: 0
   ///   /// List:                                 ///List:
   ///   ///  - Foo                                /// - Foo
   ///   ///    - Bar                              ///   - Bar
   /// \endcode
   /// \version 14
   SpacesInLineComment SpacesInLineCommentPrefix;
 
+  // If ``true``, spaces will be inserted before and after the ``|`` of
+  // a JavaScript/TypeScript union.
+  /// \code
+  ///    true:                                  false:
+  ///    let x: A | B;   vs.                    let x: A|B;
+  /// \endcode
+  /// \version 14
+  bool SpacesInJavaScriptUnion;
+
   /// If ``true``, spaces will be inserted after ``(`` and before ``)``.
   /// \code
   ///    true:                                  false:
   ///    t f( Deleted & ) & = delete;   vs.     t f(Deleted &) & = delete;
   /// \endcode
   /// \version 3.7
   bool SpacesInParentheses;
 

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("SpacesInConditionalStatement",
                    Style.SpacesInConditionalStatement);
     IO.mapOptional("SpacesInContainerLiterals",
                    Style.SpacesInContainerLiterals);
     IO.mapOptional("SpacesInCStyleCastParentheses",
                    Style.SpacesInCStyleCastParentheses);
     IO.mapOptional("SpacesInLineCommentPrefix",
                    Style.SpacesInLineCommentPrefix);
+    IO.mapOptional("SpacesInJavaScriptUnion", Style.SpacesInJavaScriptUnion);
     IO.mapOptional("SpacesInParentheses", Style.SpacesInParentheses);
     IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
     IO.mapOptional("SpaceBeforeSquareBrackets",
                    Style.SpaceBeforeSquareBrackets);
     IO.mapOptional("BitFieldColonSpacing", Style.BitFieldColonSpacing);
     IO.mapOptional("Standard", Style.Standard);
     IO.mapOptional("StatementAttributeLikeMacros",
                    Style.StatementAttributeLikeMacros);
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.ReflowComments = true;
   LLVMStyle.SpacesInParentheses = false;
   LLVMStyle.SpacesInSquareBrackets = false;
   LLVMStyle.SpaceInEmptyBlock = false;
   LLVMStyle.SpaceInEmptyParentheses = false;
   LLVMStyle.SpacesInContainerLiterals = true;
   LLVMStyle.SpacesInCStyleCastParentheses = false;
   LLVMStyle.SpacesInLineCommentPrefix = {/*Minimum=*/1, /*Maximum=*/-1u};
+  LLVMStyle.SpacesInJavaScriptUnion = false;
   LLVMStyle.SpaceAfterCStyleCast = false;
   LLVMStyle.SpaceAfterLogicalNot = false;
   LLVMStyle.SpaceAfterTemplateKeyword = true;
   LLVMStyle.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Default;
   LLVMStyle.SpaceBeforeCaseColon = false;
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
   LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;

clang/lib/Format/TokenAnnotator.cpp

if (Style.isCpp()) {
     // the tag identifier and the template string.
     if (Keywords.IsJavaScriptIdentifier(Left,
                                         /* AcceptIdentifierName= */ false) &&
         Right.is(TT_TemplateString))
       return false;
     if (Right.is(tok::star) &&
         Left.isOneOf(Keywords.kw_function, Keywords.kw_yield))
       return false;
     if (Right.isOneOf(tok::l_brace, tok::l_square) &&
         Left.isOneOf(Keywords.kw_function, Keywords.kw_yield,
                      Keywords.kw_extends, Keywords.kw_implements))
       return true;
     if (Right.is(tok::l_paren)) {
       // JS methods can use some keywords as names (e.g. `delete()`).
       if (Line.MustBeDeclaration && Left.Tok.getIdentifierInfo())
         return false;
       // Valid JS method names can include keywords, e.g. `foo.delete()` or
if (Left.is(Keywords.kw_as) &&
       return true;
     if (Left.is(tok::kw_default) && Left.Previous &&
         Left.Previous->is(tok::kw_export))
       return true;
     if (Left.is(Keywords.kw_is) && Right.is(tok::l_brace))
       return true;
     if (Right.isOneOf(TT_JsTypeColon, TT_JsTypeOptionalQuestion))
       return false;
-    if (Left.is(TT_JsTypeOperator) || Right.is(TT_JsTypeOperator))
+    if (Left.is(TT_JsTypeOperator) || Right.is(TT_JsTypeOperator)) {
+      if ((Left.is(TT_JsTypeOperator) && Right.isTypeOrIdentifier()) ||
+          (Left.isTypeOrIdentifier() || Left.is(TT_TemplateCloser)) &&
+              Right.is(TT_JsTypeOperator))
+        return Style.SpacesInJavaScriptUnion;
       return false;
+    }
     if ((Left.is(tok::l_brace) || Right.is(tok::r_brace)) &&
         Line.First->isOneOf(Keywords.kw_import, tok::kw_export))
       return false;
     if (Left.is(tok::ellipsis))
       return false;
     if (Left.is(TT_TemplateCloser) &&
         !Right.isOneOf(tok::equal, tok::l_brace, tok::comma, tok::l_square,
                        Keywords.kw_implements, Keywords.kw_extends))

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(SortUsingDeclarations);
   CHECK_PARSE_BOOL(SpacesInParentheses);
   CHECK_PARSE_BOOL(SpacesInSquareBrackets);
   CHECK_PARSE_BOOL(SpacesInConditionalStatement);
   CHECK_PARSE_BOOL(SpaceInEmptyBlock);
   CHECK_PARSE_BOOL(SpaceInEmptyParentheses);
   CHECK_PARSE_BOOL(SpacesInContainerLiterals);
   CHECK_PARSE_BOOL(SpacesInCStyleCastParentheses);
+  CHECK_PARSE_BOOL(SpacesInJavaScriptUnion);
   CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
   CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
   CHECK_PARSE_BOOL(SpaceAfterLogicalNot);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
   CHECK_PARSE_BOOL(SpaceBeforeCaseColon);
   CHECK_PARSE_BOOL(SpaceBeforeCpp11BracedList);
   CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
   CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon);

clang/unittests/Format/FormatTestJS.cpp

TEST_F(FormatTestJS, UnionIntersectionTypes) {
   verifyFormat("let x: Bar<X>|Baz;");
   verifyFormat("let x: (Foo|Bar)[];");
   verifyFormat("type X = {\n"
                "  a: Foo|Bar;\n"
                "};");
   verifyFormat("export type X = {\n"
                "  a: Foo|Bar;\n"
                "};");
+
+  auto Style = getGoogleJSStyleWithColumns(60);
+  Style.SpacesInJavaScriptUnion = true;
+  verifyFormat("let x: A | B = A | B;", Style);
+  verifyFormat("let x?: A | B = A | B;", Style);
+  verifyFormat("let x: A & B | C = A & B;", Style);
+  verifyFormat("let x: Foo<A | B> = new Foo<A | B>();", Style);
+  verifyFormat("function(x: A | B): C & D {}", Style);
+  verifyFormat("function(x: A | B = A | B): C & D {}", Style);
+  verifyFormat("function x(path: number | string) {}", Style);
+  verifyFormat("function x(): string | number {}", Style);
+  verifyFormat("type Foo = Bar | Baz;", Style);
+  verifyFormat("type Foo = Bar<X> | Baz;", Style);
+  verifyFormat("type Foo = (Bar<X> | Baz);", Style);
+  verifyFormat("let x: Bar | Baz;", Style);
+  verifyFormat("let x: Bar<X> | Baz;", Style);
+  verifyFormat("let x: (Foo | Bar)[];", Style);
+  verifyFormat("type X = {\n"
+               "  a: Foo | Bar;\n"
+               "};",
+               Style);
+  verifyFormat("export type X = {\n"
+               "  a: Foo | Bar;\n"
+               "};",
+               Style);
 }
 
 TEST_F(FormatTestJS, UnionIntersectionTypesInObjectType) {
   verifyFormat("let x: {x: number|null} = {x: number | null};");
   verifyFormat("let nested: {x: {y: number|null}};");
   verifyFormat("let mixed: {x: [number|null, {w: number}]};");
   verifyFormat("class X {\n"
                "  contructor(x: {\n"
                "    a: a|null,\n"
                "    b: b|null,\n"
                "  }) {}\n"
                "}");
+
+  auto Style = getGoogleJSStyleWithColumns(60);
+  Style.SpacesInJavaScriptUnion = true;
+  verifyFormat("let x: {x: number | null} = {x: number | null};", Style);
+  verifyFormat("let nested: {x: {y: number | null}};", Style);
+  verifyFormat("let mixed: {x: [number | null, {w: number}]};", Style);
+  verifyFormat("class X {\n"
+               "  contructor(x: {\n"
+               "    a: a | null,\n"
+               "    b: b | null,\n"
+               "  }) {}\n"
+               "}",
+               Style);
 }
 
 TEST_F(FormatTestJS, ClassDeclarations) {
   verifyFormat("class C {\n  x: string = 12;\n}");
   verifyFormat("class C {\n  x(): string => 12;\n}");
   verifyFormat("class C {\n  ['x' + 2]: string = 12;\n}");
   verifyFormat("class C {\n"
                "  foo() {}\n"