parens: Emit parens where needed throughout Flow types; add tests

There were a couple of particular cases that were already covered,
but many others that were not.

I ran into one of them in practice (`A & (B | C)` was getting printed
as `A & B | C`, which means something different) and looked into it,
and decided to go about just fixing this whole class of issues.

I started by scanning through all the different kinds of FlowType
node, looking for any that could need parens around them; then I
wrote up a comprehensive list of test cases.

Once that was done, the whole thing turned out to be doable with a
pretty reasonable amount of code!  And given the tests, I'm hopeful
that this logic is pretty much complete.

Some of the added tests are skipped for now, because they involve
function types and those currently have other bugs that cause those
test cases to break.  They start passing when this branch is merged
with benjamn#1089, which fixes those other bugs.

Author: gnprice

lib/fast-path.ts

@@ -532,11 +532,81 @@ FPp.needsParens = function (assumeExpressionContext) {
     // parentheses explicitly in the AST, with TSParenthesizedType.)
 
     case "OptionalIndexedAccessType":
-      return parent.type === "IndexedAccessType";
+      switch (parent.type) {
+        case "IndexedAccessType":
+          // `(O?.['x'])['y']` is distinct from `O?.['x']['y']`.
+          return name === "objectType" && parent.objectType === node;
+        default:
+          return false;
+      }
+
+    case "IndexedAccessType":
+    case "ArrayTypeAnnotation":
+      return false;
+
+    case "NullableTypeAnnotation":
+      switch (parent.type) {
+        case "OptionalIndexedAccessType":
+        case "IndexedAccessType":
+          return name === "objectType" && parent.objectType === node;
+        case "ArrayTypeAnnotation":
+          return true;
+        default:
+          return false;
+      }
 
     case "IntersectionTypeAnnotation":
+      switch (parent.type) {
+        case "OptionalIndexedAccessType":
+        case "IndexedAccessType":
+          return name === "objectType" && parent.objectType === node;
+        case "ArrayTypeAnnotation":
+        case "NullableTypeAnnotation":
+          return true;
+        default:
+          return false;
+      }
+
     case "UnionTypeAnnotation":
-      return parent.type === "NullableTypeAnnotation";
+      switch (parent.type) {
+        case "OptionalIndexedAccessType":
+        case "IndexedAccessType":
+          return name === "objectType" && parent.objectType === node;
+        case "ArrayTypeAnnotation":
+        case "NullableTypeAnnotation":
+        case "IntersectionTypeAnnotation":
+          return true;
+        default:
+          return false;
+      }
+
+    case "FunctionTypeAnnotation":
+      switch (parent.type) {
+        case "OptionalIndexedAccessType":
+        case "IndexedAccessType":
+          return name === "objectType" && parent.objectType === node;
+
+        case "ArrayTypeAnnotation":
+        // We need parens.
+
+        // fallthrough
+        case "NullableTypeAnnotation":
+        // We don't *need* any parens here… unless some ancestor
+        // means we do, by putting a `&` or `|` on the right.
+        // Just use parens; probably more readable that way anyway.
+        // (FWIW, this agrees with Prettier's behavior.)
+
+        // fallthrough
+        case "IntersectionTypeAnnotation":
+        case "UnionTypeAnnotation":
+          // We need parens if there's another `&` or `|` after this node.
+          // For consistency, just always use parens.
+          // (FWIW, this agrees with Prettier's behavior.)
+          return true;
+
+        default:
+          return false;
+      }
   }
 
   if (

test/flow.ts

@@ -14,6 +14,12 @@ describe("type syntax", function () {
     parser: require("flow-parser"),
   };
 
+  function checkEquiv(a: string, b: string) {
+    const aAst = parse(a, flowParserParseOptions);
+    const bAst = parse(b, flowParserParseOptions);
+    types.astNodesAreEquivalent.assert(aAst, bAst);
+  }
+
   function check(source: string, parseOptions?: any) {
     parseOptions = parseOptions || flowParserParseOptions;
     const ast1 = parse(source, parseOptions);
@@ -212,12 +218,6 @@ describe("type syntax", function () {
     check("type H = (Obj?.['bar'])[string][];");
     check("type I = Obj?.['bar']?.[string][];");
 
-    function checkEquiv(a: string, b: string) {
-      const aAst = parse(a, flowParserParseOptions);
-      const bAst = parse(b, flowParserParseOptions);
-      types.astNodesAreEquivalent.assert(aAst, bAst);
-    }
-
     // Since FastPath#needsParens does not currently add any parentheses to
     // these expressions, make sure they do not matter for parsing the AST.
 
@@ -231,4 +231,108 @@ describe("type syntax", function () {
       "type F = Obj['bar']?.[string][];",
     );
   });
+
+  it("parenthesizes correctly", () => {
+    // The basic binary operators `&` and `|`.
+    // `&` binds tighter than `|`
+    check("type Num = number & (empty | mixed);"); // parens needed
+    check("type Num = number | empty & mixed;"); // equivalent to `…|(…&…)`
+
+    // Unary suffix `[]`, with the above.
+    // `[]` binds tighter than `&` or `|`
+    check("type T = (number | string)[];");
+    check("type T = number | string[];"); // a union
+    check("type T = (number & mixed)[];");
+    check("type T = number & mixed[];"); // an intersection
+
+    // Unary prefix `?`, with the above.
+    // `?` binds tighter than `&` or `|`
+    check("type T = ?(A & B);");
+    check("type T = ?(A | B);");
+    // `?` binds less tightly than `[]`
+    check("type T = (?number)[];"); // array of nullable
+    check("type T = ?number[];"); // nullable of array
+
+    // (Optional) indexed-access types, with the above.
+    // `[…]` and `?.[…]` bind (their left) tighter than either `&` or `|`
+    check("type T = (O & P)['x'];");
+    check("type T = (O | P)['x'];");
+    check("type T = (O & P)?.['x'];");
+    check("type T = (O | P)?.['x'];");
+    // `[…]` and `?.[…]` bind (their left) tighter than `?`
+    check("type T = (?O)['x'];"); // indexed-access of nullable
+    check("type T = ?O['x'];"); // nullable of indexed-access
+    check("type T = (?O)?.['x'];"); // optional-indexed-access of nullable
+    check("type T = ?O?.['x'];"); // nullable of optional-indexed-access
+    // `[…]` and `?.[…]` provide brackets on their right, so skip parens:
+    check("type T = A[B & C];");
+    check("type T = A[B | C];");
+    check("type T = A[?B];");
+    check("type T = A[B[]];");
+    check("type T = A[B[C]];");
+    check("type T = A[B?.[C]];");
+    check("type T = A?.[B & C];");
+    check("type T = A?.[B | C];");
+    check("type T = A?.[?B];");
+    check("type T = A?.[B[]];");
+    check("type T = A?.[B[C]];");
+    check("type T = A?.[B?.[C]];");
+    // `[…]` and `?.[…]` interact in a nonobvious way:
+    // OptionalIndexedAccessType inside IndexedAccessType.
+    check("type T = (O?.['x']['y'])['z'];"); // indexed of optional-indexed
+    check("type T = O?.['x']['y']['z'];"); // optional-indexed throughout
+
+    return;
+    // Skip test cases involving function types, because those are currently
+    // broken in other ways.  Those will be fixed by:
+    //   https://github.com/benjamn/recast/pull/1089
+
+    // Function types.
+    // Function binds less tightly than binary operators at right:
+    check("type T = (() => number) & O;"); // an intersection
+    check("type T = (() => number) | void;"); // a union
+    check("type T = () => number | void;"); // a function
+    check("type T = (() => void)['x'];");
+    check("type T = () => void['x'];"); // a function
+    check("type T = (() => void)?.['x'];");
+    check("type T = () => void?.['x'];"); // a function
+    // … and less tightly than suffix operator:
+    check("type T = (() => void)[];"); // an array
+    check("type T = () => void[];"); // a function
+
+    // Function does bind tighter than prefix operator (how could it not?)
+    checkEquiv("type T = ?() => void;", "type T = ?(() => void);");
+    // … and tighter than `&` or `|` at left (ditto):
+    checkEquiv("type T = A | () => void;", "type T = A | (() => void);");
+    checkEquiv("type T = A & () => void;", "type T = A & (() => void);");
+    // … but we choose to insert parens anyway:
+    check("type T = ?(() => void);");
+    check("type T = A | (() => void);");
+    check("type T = A & (() => void);");
+    // We don't insert parens for the *right* operand of indexed access,
+    // though, that'd be silly (sillier than writing such a type at all?):
+    check("type T = A[() => void];");
+    check("type T = A?.[() => void];");
+
+    // Here's one reason we insert those parens we don't strictly have to:
+    // Even when the parent is something at left so that function binds
+    // tighter than it, *its* parent (or further ancestor) might be
+    // something at right that binds tighter than function.
+    // E.g., union of nullable of function:
+    check("type T = ?(() => void) | A;");
+    checkEquiv("type T = ?() => void | A;", "type T = ?() => (void | A);");
+    // … or intersection of nullable of function:
+    check("type T = ?(() => void) & A;");
+    checkEquiv("type T = ?() => void & A;", "type T = ?() => (void & A);");
+    // … or array or (optional-)indexed-access of nullable of function:
+    check("type T = ?(() => void)[];");
+    check("type T = ?(() => void)['x'];");
+    check("type T = ?(() => void)?.['x'];");
+    // … or union of intersection:
+    check("type T = A & (() => void) | B;");
+    // Or for an example beyond the grandparent: union of cubic nullable:
+    check("type T = ???(() => void) | B;");
+    // … or union of intersection of nullable:
+    check("type T = A & ?(() => void) | B;");
+  });
 });