clang-format: [JS] recognize more type locations.

[mprobst]

Summary

Includes parenthesized type expressions and type aliases.

[mprobst]

This seems to be getting a bit tricky with the declaration contexts. If you have a better suggestion, please come forward.

[djasper]

lib/Format/TokenAnnotator.cpp

480 ↗

(On Diff #61503)

Why is this necessary?

[mprobst]

• Remove parseJSTypeDefinition

lib/Format/TokenAnnotator.cpp

480 ↗

(On Diff #61503)

It is not, actually. I was chasing a wrong path here before understanding it was because of the assignment. Removed.

[djasper]

Basically looks good.

lib/Format/TokenAnnotator.cpp

155 ↗

(On Diff #61548)

I'd merge this one in with the previous.. And maybe even the one from above so that we end up with:

} else if (Style.Language == FormatStyle::LK_JavaScript && Left->Previous &&
           (Line.First->is(Keywords.kw_type) ||
            Left->Previous->isOneOf(Keywords.kw_function, TT_JsTypeColon) ||
            (Left->Previous->endsSequence(tok::identifier,
                                          Keywords.kw_function)))) {

(in order for Line.First to be "type", Left->Previous cannot be nullptr, so this should be equivalent)

664 ↗

(On Diff #61548)

nit: undo

926 ↗

(On Diff #61548)

I'd move the ! into the parentheses, but doesn't matter much.

[mprobst]

lib/Format/TokenAnnotator.cpp

155 ↗

(On Diff #61548)

I think I'd prefer keeping this as is. The way it is now formulated is more verbose, but that's to communicate and keep apart three very different situations (type alias, (async)? function, variable declaration).

Merging the boolean conditions in some clever way will make it very hard to reverse-decipher which partially overlapping boolean clause is for which of these situations, and it's usually non-obvious if test coverage is good enough to make sure nothing breaks if you refactor.

The could would also be harder to read, e.g. the is(kw_function) part is logically related to the endsSeqeunce call, but they end up in different parts of the clause.

So, in the interest of readability and maintainability, I'll keep it like this, and assume by accepting the diff you're cool with that :-) If not, please do push back.

926 ↗

(On Diff #61548)

I've formulated it like this intentionally – it follows my brain waves, as in "mark this as an expression if not this is javascript and it's a type def". Changing it to "mark this as an expression if language is not javascript or line is not a typedef" is less readable to me.

[mprobst]

• progress

cfe/trunk/lib/Format/FormatToken.h

AdditionalKeywords(IdentifierTable &IdentTable) {
     kw_async = &IdentTable.get("async");
     kw_await = &IdentTable.get("await");
     kw_finally = &IdentTable.get("finally");
     kw_from = &IdentTable.get("from");
     kw_function = &IdentTable.get("function");
     kw_import = &IdentTable.get("import");
     kw_is = &IdentTable.get("is");
     kw_let = &IdentTable.get("let");
+    kw_type = &IdentTable.get("type");
     kw_var = &IdentTable.get("var");
     kw_yield = &IdentTable.get("yield");
 
     kw_abstract = &IdentTable.get("abstract");
     kw_assert = &IdentTable.get("assert");
     kw_extends = &IdentTable.get("extends");
     kw_implements = &IdentTable.get("implements");
     kw_instanceof = &IdentTable.get("instanceof");
struct AdditionalKeywords {
   IdentifierInfo *kw_async;
   IdentifierInfo *kw_await;
   IdentifierInfo *kw_finally;
   IdentifierInfo *kw_from;
   IdentifierInfo *kw_function;
   IdentifierInfo *kw_import;
   IdentifierInfo *kw_is;
   IdentifierInfo *kw_let;
+  IdentifierInfo *kw_type;
   IdentifierInfo *kw_var;
   IdentifierInfo *kw_yield;
 
   // Java keywords.
   IdentifierInfo *kw_abstract;
   IdentifierInfo *kw_assert;
   IdentifierInfo *kw_extends;
   IdentifierInfo *kw_implements;

cfe/trunk/lib/Format/TokenAnnotator.cpp

if (CurrentToken->is(tok::caret)) {
       if (MaybeSel->isObjCAtKeyword(tok::objc_selector) && MaybeSel->Previous &&
           MaybeSel->Previous->is(tok::at)) {
         StartsObjCMethodExpr = true;
       }
     }
 
     if (Left->is(TT_OverloadedOperatorLParen)) {
       Contexts.back().IsExpression = false;
+    } else if (Style.Language == FormatStyle::LK_JavaScript &&
+               Line.startsWith(Keywords.kw_type, tok::identifier)) {
+      // type X = (...);
+      Contexts.back().IsExpression = false;
     } else if (Left->Previous &&
         (Left->Previous->isOneOf(tok::kw_static_assert, tok::kw_decltype,
                                  tok::kw_if, tok::kw_while, tok::l_paren,
                                  tok::comma) ||
          Left->Previous->is(TT_BinaryOperator))) {
       // static_assert, if and while usually contain expressions.
       Contexts.back().IsExpression = true;
     } else if (Style.Language == FormatStyle::LK_JavaScript && Left->Previous &&
                (Left->Previous->is(Keywords.kw_function) ||
                 (Left->Previous->endsSequence(tok::identifier,
                                               Keywords.kw_function)))) {
       // function(...) or function f(...)
       Contexts.back().IsExpression = false;
+    } else if (Style.Language == FormatStyle::LK_JavaScript && Left->Previous &&
+               Left->Previous->is(TT_JsTypeColon)) {
+      // let x: (SomeType);
+      Contexts.back().IsExpression = false;
     } else if (Left->Previous && Left->Previous->is(tok::r_square) &&
                Left->Previous->MatchingParen &&
                Left->Previous->MatchingParen->is(TT_LambdaLSquare)) {
       // This is a parameter list of a lambda expression.
       Contexts.back().IsExpression = false;
     } else if (Line.InPPDirective &&
                (!Left->Previous || !Left->Previous->is(tok::identifier))) {
       Contexts.back().IsExpression = true;
struct ScopedContextCreator {
     }
 
     ~ScopedContextCreator() { P.Contexts.pop_back(); }
   };
 
   void modifyContext(const FormatToken &Current) {
     if (Current.getPrecedence() == prec::Assignment &&
         !Line.First->isOneOf(tok::kw_template, tok::kw_using, tok::kw_return) &&
+        // Type aliases use `type X = ...;` in TypeScript.
+        !(Style.Language == FormatStyle::LK_JavaScript &&
+          Line.startsWith(Keywords.kw_type, tok::identifier)) &&
         (!Current.Previous || Current.Previous->isNot(tok::kw_operator))) {
       Contexts.back().IsExpression = true;
       if (!Line.startsWith(TT_UnaryOperator)) {
         for (FormatToken *Previous = Current.Previous;
              Previous && Previous->Previous &&
              !Previous->Previous->isOneOf(tok::comma, tok::semi);
              Previous = Previous->Previous) {
           if (Previous->isOneOf(tok::r_square, tok::r_paren)) {

cfe/trunk/unittests/Format/FormatTestJS.cpp

 TEST_F(FormatTestJS, UnionIntersectionTypes) {
   verifyFormat("let x: A|B = A | B;");
   verifyFormat("let x: A&B|C = A & B;");
   verifyFormat("let x: Foo<A|B> = new Foo<A|B>();");
   verifyFormat("function(x: A|B): C&D {}");
   verifyFormat("function(x: A|B = A | B): C&D {}");
   verifyFormat("function x(path: number|string) {}");
   verifyFormat("function x(): string|number {}");
+  verifyFormat("type Foo = Bar|Baz;");
+  verifyFormat("type Foo = Bar<X>|Baz;");
+  verifyFormat("type Foo = (Bar<X>|Baz);");
+  verifyFormat("let x: Bar|Baz;");
+  verifyFormat("let x: Bar<X>|Baz;");
+  verifyFormat("let x: (Foo|Bar)[];");
 }
 
 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  private x: string = 12;\n}");
   verifyFormat("class C {\n  private static x: string = 12;\n}");