diff --git a/lib/fast-path.ts b/lib/fast-path.ts index 925cbaac..bf3b7200 100644 --- a/lib/fast-path.ts +++ b/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 ( diff --git a/test/flow.ts b/test/flow.ts index efddec1e..141f0c74 100644 --- a/test/flow.ts +++ b/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?.[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;"); + }); });