Merge remote-tracking branch 'me/pr-parens-flow'

I.e., this PR branch:
  benjamn#1127

Author: gnprice

lib/fast-path.ts

@@ -329,13 +329,13 @@ FPp.needsParens = function (assumeExpressionContext) {
   const name = this.getName();
 
   // If the value of this path is some child of a Node and not a Node
-  // itself, then it doesn't need parentheses. Only Node objects (in fact,
-  // only Expression nodes) need parentheses.
+  // itself, then it doesn't need parentheses. Only Node objects
+  // need parentheses.
   if (this.getValue() !== node) {
     return false;
   }
 
-  // Only statements don't need parentheses.
+  // Statements don't need parentheses.
   if (n.Statement.check(node)) {
     return false;
   }
@@ -424,13 +424,6 @@ FPp.needsParens = function (assumeExpressionContext) {
           return true;
       }
 
-    case "OptionalIndexedAccessType":
-      return node.optional && parent.type === "IndexedAccessType";
-
-    case "IntersectionTypeAnnotation":
-    case "UnionTypeAnnotation":
-      return parent.type === "NullableTypeAnnotation";
-
     case "Literal":
       return (
         parent.type === "MemberExpression" &&
@@ -531,6 +524,89 @@ FPp.needsParens = function (assumeExpressionContext) {
       ) {
         return true;
       }
+      break;
+
+    // Flow type nodes.
+    //
+    // (TS type nodes don't need any logic here, because they represent
+    // parentheses explicitly in the AST, with TSParenthesizedType.)
+
+    case "OptionalIndexedAccessType":
+      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":
+      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,14 @@ describe("Flow type syntax", function () {
     parser: require("flow-parser"),
   };
 
+  function checkEquiv(a: string, b: string) {
+    it(`handles equivalently \`${a}\` vs. \`${b}\``, () => {
+      const aAst = parse(a, flowParserParseOptions);
+      const bAst = parse(b, flowParserParseOptions);
+      types.astNodesAreEquivalent.assert(aAst, bAst);
+    });
+  }
+
   function check(source: string, parseOptions?: any) {
     it(`handles: ${source}`, () => {
       parseOptions = parseOptions || flowParserParseOptions;
@@ -25,14 +33,6 @@ describe("Flow type syntax", function () {
     });
   }
 
-  function checkEquiv(a: string, b: string) {
-    it(`handles equivalently \`${a}\` vs. \`${b}\``, () => {
-      const aAst = parse(a, flowParserParseOptions);
-      const bAst = parse(b, flowParserParseOptions);
-      types.astNodesAreEquivalent.assert(aAst, bAst);
-    });
-  }
-
   describe("should parse and print type annotations correctly", function () {
     // Import type annotations
     check("import type foo from 'foo';");
@@ -276,6 +276,8 @@ describe("Flow type syntax", function () {
     check("type B = Array<string>?.[number];");
     check("type C = Obj?.['bar']['baz'];");
     check("type D = (Obj?.['bar'])['baz'];");
+    check("type C3 = Obj?.['foo']['bar']['baz'];");
+    check("type D3 = (Obj?.['foo']['bar'])['baz'];");
     check("type E = Obj?.['bar'][];");
     check("type F = Obj?.['bar'][boolean][];");
     check("type G = Obj['bar']?.[boolean][];");
@@ -284,15 +286,112 @@ describe("Flow type syntax", function () {
 
     // Since FastPath#needsParens does not currently add any parentheses to
     // these expressions, make sure they do not matter for parsing the AST.
-
     checkEquiv(
       "type F = (Obj?.['bar'])?.[string][];",
       "type F = Obj?.['bar']?.[string][];",
     );
-
     checkEquiv(
       "type F = (Obj['bar'])?.[string][];",
       "type F = Obj['bar']?.[string][];",
     );
   });
+
+  describe("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
+
+    // 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;");
+  });
 });